patch metadata - to use or not to use?
Hi, I recently got some patches accepted for inclusion in 3.3, and each time, the patch metadata (such as my name and my commit comment) were stripped by applying the patch manually, instead of hg importing it. This makes it clear in the history who eventually reviewed and applied the patch, but less visible who wrote it (except for the entry in Misc/NEWS). I didn't see this mentioned in the dev-guide. Is it being considered the Right Way To Do It? Stefan
On Sat, 19 Nov 2011 09:42:49 +0100 Stefan Behnel <stefan_ml@behnel.de> wrote:
Hi,
I recently got some patches accepted for inclusion in 3.3, and each time, the patch metadata (such as my name and my commit comment) were stripped by applying the patch manually, instead of hg importing it. This makes it clear in the history who eventually reviewed and applied the patch, but less visible who wrote it (except for the entry in Misc/NEWS).
I didn't see this mentioned in the dev-guide. Is it being considered the Right Way To Do It?
It is common to add minor things to the patch when committing (such as a redacted NEWS entry, or a ACKS entry), in which cases you can't import it anyway. Often the name of the contributor is added to NEWS *and* to the commit message. Regards Antoine.
On Sat, 19 Nov 2011 09:42:49 +0100 Stefan Behnel <stefan_ml@behnel.de> wrote:
I didn't see this mentioned in the dev-guide. Is it being considered the Right Way To Do It?
That said, to answer your question more generally, I think it's simply how we worked with SVN, and we haven't found any compelling reason to change. Regards Antoine.
On Sat, Nov 19, 2011 at 6:42 PM, Stefan Behnel <stefan_ml@behnel.de> wrote:
Hi,
I recently got some patches accepted for inclusion in 3.3, and each time, the patch metadata (such as my name and my commit comment) were stripped by applying the patch manually, instead of hg importing it. This makes it clear in the history who eventually reviewed and applied the patch, but less visible who wrote it (except for the entry in Misc/NEWS).
I didn't see this mentioned in the dev-guide. Is it being considered the Right Way To Do It?
Generally speaking, it's more useful for the checkin metadata to reflect who actually did the checkin, since that's the most useful information for the tracker and buildbot integration. The question of did the original patch does matter in terms of giving appropriate credit (which is covered by NEWS and the commit message), but who did the checkin matters for immediate workflow reasons (i.e. who is responsible for dealing with any buildbot breakage, objections on python-dev, objections on the tracker, etc). One of the key aspects of having push rights is that we're the ones that take responsibility for the state of the central repo - if we stuff it up and break the build (either because we missed something on review, or due to cross-platform issues), that's *our* problem, not usually something the original patch contributor needs to worry about. We do have a guideline that says to always use the "--no-commit" flag with "hg import" and then run the tests before committing, so that may answer your question about whether or not it's an official policy. (That said, I don't know if the devguide actually says that explicitly anywhere - it's just reflected in the various workflow examples, as well as in the mailing list discussions that helped craft those examples) Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On Sat, Nov 19, 2011 at 2:52 AM, Nick Coghlan <ncoghlan@gmail.com> wrote:
On Sat, Nov 19, 2011 at 6:42 PM, Stefan Behnel <stefan_ml@behnel.de> wrote:
Hi,
I recently got some patches accepted for inclusion in 3.3, and each time, the patch metadata (such as my name and my commit comment) were stripped by applying the patch manually, instead of hg importing it. This makes it clear in the history who eventually reviewed and applied the patch, but less visible who wrote it (except for the entry in Misc/NEWS).
I didn't see this mentioned in the dev-guide. Is it being considered the Right Way To Do It?
Generally speaking, it's more useful for the checkin metadata to reflect who actually did the checkin, since that's the most useful information for the tracker and buildbot integration. The question of did the original patch does matter in terms of giving appropriate credit (which is covered by NEWS and the commit message), but who did the checkin matters for immediate workflow reasons (i.e. who is responsible for dealing with any buildbot breakage, objections on python-dev, objections on the tracker, etc).
One of the key aspects of having push rights is that we're the ones that take responsibility for the state of the central repo - if we stuff it up and break the build (either because we missed something on review, or due to cross-platform issues), that's *our* problem, not usually something the original patch contributor needs to worry about.
Well, it doesn't hurt to keep the patch author in the loop about those -- they may know their patch best and they may even learn something new, which might make their future patches better! Of course if they *don't* know how to fix an issue (e.g. if it's a platform-specific thing) then they shouldn't be blamed.
We do have a guideline that says to always use the "--no-commit" flag with "hg import" and then run the tests before committing, so that may answer your question about whether or not it's an official policy. (That said, I don't know if the devguide actually says that explicitly anywhere - it's just reflected in the various workflow examples, as well as in the mailing list discussions that helped craft those examples)
I agree with this, but I also want to make sure the author of the patch always gets proper recognition (after all that's what motivates people to contribute!). I think that their name should always be in the description if it's not in the committer field. Use your best judgment or qualifying terms for patches that are co-productions of committer and original author. -- --Guido van Rossum (python.org/~guido)
Nick Coghlan wrote:
Generally speaking, it's more useful for the checkin metadata to reflect who actually did the checkin, since that's the most useful information for the tracker and buildbot integration.
At least in git, the commit metadata contains both author and committer (at least if they differ). Maybe mercurial has this too?
On Sat, Nov 19, 2011 at 20:41, Petri Lehtinen <petri@digip.org> wrote:
Generally speaking, it's more useful for the checkin metadata to reflect who actually did the checkin, since that's the most useful information for the tracker and buildbot integration.
At least in git, the commit metadata contains both author and committer (at least if they differ). Maybe mercurial has this too?
It does not. Personally, I find it more appropriate to have the original patch author in the "official" metadata, mostly because I personally find it very satisfying to see my name in the changelog on hgweb and the like. My own experience with that makes me think that it's probably helpful in engaging contributors. Cheers, Dirkjan
In article <CAKmKYaB6DcW=CMtbXWxHFLVfwZSQQHCFd8OS0v2TPc=pwfXB-Q@mail.gmail.com>, Dirkjan Ochtman <dirkjan@ochtman.nl> wrote:
On Sat, Nov 19, 2011 at 20:41, Petri Lehtinen <petri@digip.org> wrote:
Generally speaking, it's more useful for the checkin metadata to reflect who actually did the checkin, since that's the most useful information for the tracker and buildbot integration. At least in git, the commit metadata contains both author and committer (at least if they differ). Maybe mercurial has this too? It does not.
Personally, I find it more appropriate to have the original patch author in the "official" metadata, mostly because I personally find it very satisfying to see my name in the changelog on hgweb and the like. My own experience with that makes me think that it's probably helpful in engaging contributors.
As Nick pointed out, it's important that who did the checkin is recorded for python-dev workflow reasons. Ensuring that the original patch submitter is mentioned in the commit message and, as appropriate, in any Misc/NEWS item seems to me an appropriate and sufficient way to give that recognition. The NEWS file will eventually get installed on countless systems around the world: hard to beat that! WRT the original commit message, a more flexible approach to applying patches is to use "hg qimport" rather than "hg import". It is then possible to edit the patch, make the necessary changes to Misc/NEWS, edit the original patch commit comment using "hg qrefresh -e" and then commit the patch with "hg qfinish". -- Ned Deily, nad@acm.org
Hi,
I recently got some patches accepted for inclusion in 3.3, and each time, the patch metadata (such as my name and my commit comment) were stripped by applying the patch manually, instead of hg importing it. This makes it clear in the history who eventually reviewed and applied the patch, but less visible who wrote it (except for the entry in Misc/NEWS).
We had a similar discussion on python-committers a while back, and the gist of the replies was that there is no such thing as a patch ready for commit, i.e. the core dev always edits something. As Antoine said, we’ve switched to Mercurial to ease contributions, but we still work with patches, not directly with changesets. That said, I remember that once I got a patch that was complete, and I just used hg import and hg push since it was so easy. I share the opinion that putting contributors’ names in the spotlight is a good way to encourage them. Cheers
participants (8)
-
Antoine Pitrou -
Dirkjan Ochtman -
Guido van Rossum -
Ned Deily -
Nick Coghlan -
Petri Lehtinen -
Stefan Behnel -
Éric Araujo