mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Hao Jin <>
Subject Re: Squash/Merge PRs
Date Thu, 12 Jul 2018 08:11:46 GMT
+1 for Marco's proposal on using the co-author field. IMHO the usage of
co-author field should not be that often, consider:
1) If a PR is so big that it needs multiple people to contribute a
substantial part of it, why can't that PR be broken down into smaller PRs
each submitted by single one of them?
2) If a PR is small enough (for example, < 300 lines), why can't a single
person complete it?
3) If a majority of PR is done by someone, and there's some minor issue
he/she needs help with(a small bug, incomplete doc), why can't that be done
through code reviews?
>From the above 3 situations we can see that collaborations on individual
PRs should not be quite often, but I do agree that it could still be
necessary when someone lacks the related expertise/knowledge on some
necessary part of that PR given that the required knowledge cannot be
picked up in a short period of time.

I do agree that keeping the commit histories of PRs clean is very important
as it could be confusing when reviewing PRs, but that really depends on
personal preferences (For my case, I usually do "git commit --amend" on
trivial changes and get a new commit for bigger changes such as a
checkpoint of my whole PR). With growing the community and attracting more
contributors as a high priority for our project, I would prefer that we do
not put even more burden on the contributors by asking them to manage and
squash the commits themselves just for the not-so-often cases of having
multiple contributors on a single PR.
Best regards,

On Thu, Jul 12, 2018 at 12:35 AM, Marco de Abreu <> wrote:

> Hi Naveen,
> I'm in favour of the squashing, considering the number of commits in some
> PRs and especially because of some people making commit messages a la "fix"
> "fix" "fix" all the time. Additionally, it gets hard (not impossible, just
> more inconvenient) to determine the atomic states of master - aka, which
> commits are separate from each other. You should consider that intermediary
> commits are unstable (fail CI) and thus it could be very hard to bisect
> failures in future - and the commit history gets cluttered.
> As alternative, I'd like to suggest the co-author field for these cases.
> Further documentation is available at
> I definitely agree with the second part. We should all lead by example and
> maintain a high quality by keeping our commit messages clean and
> meaningful. When I receive an email notification that a new commit has been
> added and it only contains "fix" as title, it's not that helpful and also
> it's hard to track the development of a PR overtime. E.g., why has
> something been changed? Was there maybe a bug that we didn't cover with
> tests but the author just hacked something to get it to work but the
> problem still lays somewhere? We won't know that way and it makes it harder
> for us to review.
> Best regards,
> Marco
> Naveen Swamy <> schrieb am Do., 12. Juli 2018, 10:09:
> > Hi All,
> >
> > I am seeing that maintainers merge PRs into the repo, they are squashing
> > the commits in the PR, which I understand and agree is to keep a sane
> > commit history, however this is causing problem when there are multiple
> > contributors involved on a PR(by contributing to a fork of the repo) this
> > effectively removes credit for multiple contributors involved and shows
> all
> > code as authored by the contributor who created the PR.
> >
> > Can I request maintainers to not squash PRs if there are multiple
> > contributors involved on the PR.
> >
> > Also on the same note, I request contributors(regardless of multiple
> > contributors or not) to keep a clean commit history by squashing the
> > commits and not push all your WIP commits to the PR. this will help us
> keep
> > our commit history clean and meaningful.
> >
> > Let me know your thoughts/better approach or If I have misunderstood how
> > this works.
> >
> > Thanks, Naveen
> >

  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message