* [PATCH 0/3] Update SubmittingPatches @ 2013-01-01 23:24 Junio C Hamano 2013-01-01 23:24 ` [PATCH 1/3] SubmittingPatches: who am I and who cares? Junio C Hamano ` (3 more replies) 0 siblings, 4 replies; 12+ messages in thread From: Junio C Hamano @ 2013-01-01 23:24 UTC (permalink / raw) To: git; +Cc: Jason Holden The main thing this series wants to do is the second one, but I wanted to reduce the clutter in this document while at it. Junio C Hamano (3): SubmittingPatches: who am I and who cares? SubmittingPatches: mention subsystems with dedicated repositories SubmittingPatches: remove overlong checklist Documentation/SubmittingPatches | 160 ++++++++++++++++++++-------------------- 1 file changed, 79 insertions(+), 81 deletions(-) -- 1.8.1.209.gc32ab23 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] SubmittingPatches: who am I and who cares? 2013-01-01 23:24 [PATCH 0/3] Update SubmittingPatches Junio C Hamano @ 2013-01-01 23:24 ` Junio C Hamano 2013-01-01 23:24 ` [PATCH 2/3] SubmittingPatches: mention subsystems with dedicated repositories Junio C Hamano ` (2 subsequent siblings) 3 siblings, 0 replies; 12+ messages in thread From: Junio C Hamano @ 2013-01-01 23:24 UTC (permalink / raw) To: git; +Cc: Jason Holden The introductory text in the "long version" talks about the origin of this document with "I started ...", but it is unclear who that I is, and more importantly, it is not interesting how it was started. Just state the purpose of the document to help readers decide if it is releavant to them. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Documentation/SubmittingPatches | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index c34c9d1..b9fd182 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -60,14 +60,8 @@ Checklist (and a short version for the impatient): Long version: -I started reading over the SubmittingPatches document for Linux -kernel, primarily because I wanted to have a document similar to -it for the core GIT to make sure people understand what they are -doing when they write "Signed-off-by" line. - -But the patch submission requirements are a lot more relaxed -here on the technical/contents front, because the core GIT is -thousand times smaller ;-). So here is only the relevant bits. +Here are some guidelines for people who want to contribute their code +to this software. (0) Decide what to base your work on. -- 1.8.1.209.gc32ab23 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] SubmittingPatches: mention subsystems with dedicated repositories 2013-01-01 23:24 [PATCH 0/3] Update SubmittingPatches Junio C Hamano 2013-01-01 23:24 ` [PATCH 1/3] SubmittingPatches: who am I and who cares? Junio C Hamano @ 2013-01-01 23:24 ` Junio C Hamano 2013-01-02 1:52 ` Jason Holden 2013-01-01 23:24 ` [PATCH 3/3] SubmittingPatches: remove overlong checklist Junio C Hamano 2013-01-02 9:08 ` [PATCH 0/3] Update SubmittingPatches Jeff King 3 siblings, 1 reply; 12+ messages in thread From: Junio C Hamano @ 2013-01-01 23:24 UTC (permalink / raw) To: git; +Cc: Jason Holden These were only mentioned in periodical "A note from the maintainer" posting and not in the documentation suite. SubmittingPatches has a section to help contributors decide on what commit to base their changes, which is the most suitable place for this information. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Documentation/SubmittingPatches | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index b9fd182..e810263 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -88,6 +88,10 @@ change is relevant to. wait until some of the dependent topics graduate to 'master', and rebase your work. + - Some parts of the system have dedicated maintainers with their own + repositories (see the section "Subsystems" below). Changes to + these parts should be based on their trees. + To find the tip of a topic branch, run "git log --first-parent master..pu" and look for the merge commit. The second parent of this commit is the tip of the topic branch. @@ -279,6 +283,26 @@ You can also create your own tag or use one that's in common usage such as "Thanks-to:", "Based-on-patch-by:", or "Mentored-by:". ------------------------------------------------ +Subsystems with dedicated maintainers + +Some parts of the system have dedicated maintainers with their own +repositories. + + - git-gui/ comes from git-gui project, maintained by Pat Thoyts: + + git://repo.or.cz/git-gui.git + + - gitk-git/ comes from Paul Mackerras's gitk project: + + git://ozlabs.org/~paulus/gitk + + - po/ comes from the localization coordinator, Jiang Xin: + + https://github.com/git-l10n/git-po/ + +Patches to these parts should be based on their trees. + +------------------------------------------------ An ideal patch flow Here is an ideal patch flow for this project the current maintainer -- 1.8.1.209.gc32ab23 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] SubmittingPatches: mention subsystems with dedicated repositories 2013-01-01 23:24 ` [PATCH 2/3] SubmittingPatches: mention subsystems with dedicated repositories Junio C Hamano @ 2013-01-02 1:52 ` Jason Holden 2013-01-02 2:14 ` Junio C Hamano 0 siblings, 1 reply; 12+ messages in thread From: Jason Holden @ 2013-01-02 1:52 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Tue, Jan 01, 2013 at 03:24:54PM -0800, Junio C Hamano wrote: > > ------------------------------------------------ > +Subsystems with dedicated maintainers > + > +Some parts of the system have dedicated maintainers with their own > +repositories. > + > + - git-gui/ comes from git-gui project, maintained by Pat Thoyts: > + > + git://repo.or.cz/git-gui.git > + > + - gitk-git/ comes from Paul Mackerras's gitk project: > + > + git://ozlabs.org/~paulus/gitk > + > + - po/ comes from the localization coordinator, Jiang Xin: > + > + https://github.com/git-l10n/git-po/ > + > +Patches to these parts should be based on their trees. > + > +------------------------------------------------ > An ideal patch flow > Any reason to leave out the maintainers email addresses? If we add that, all the content of the MAINTAINERS file we had been discussing would then be in this file. My only other suggestion for this series might be to augment the file with a patch submittal example(s). I found the best example to be in git-send-email's man page, but maybe enumerate the example out for the most common workflows. Maybe something like: ----------------------------------------------------------- Example of sending patches for a new feature: Create the patches: $ git format-patch --cover-letter -M origin/master -o outgoing/ $ edit outgoing/0000-* To send your completed patches for initial consideration: $ git send-email outgoing/* -to git@vger.kernel.org -cc gitster@pobox.com To send your reviewed patches for inclusion: $ git send-email outgoing/* -to gitster@pobox.com -cc git@vger.kernel.org -Jason ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] SubmittingPatches: mention subsystems with dedicated repositories 2013-01-02 1:52 ` Jason Holden @ 2013-01-02 2:14 ` Junio C Hamano 2013-01-02 17:22 ` Junio C Hamano 0 siblings, 1 reply; 12+ messages in thread From: Junio C Hamano @ 2013-01-02 2:14 UTC (permalink / raw) To: Jason Holden; +Cc: git Jason Holden <jason.k.holden.swdev@gmail.com> writes: > Any reason to leave out the maintainers email addresses? Nothing particular, other than that I did not find anywhere in the file that does not break the flow. > My only other suggestion for this series might be to augment the file with > a patch submittal example(s). I found the best example to be in > git-send-email's man page,... I'd feel better to avoid copying and pasting. If send-email has good examples, shouldn't it be sufficient to refer to them? > ----------------------------------------------------------- > Example of sending patches for a new feature: > > Create the patches: > $ git format-patch --cover-letter -M origin/master -o outgoing/ > $ edit outgoing/0000-* > > To send your completed patches for initial consideration: > $ git send-email outgoing/* -to git@vger.kernel.org -cc gitster@pobox.com This is ambiguous; it makes it look as if you are CC'ing the maintainer, but for the initial round, it is likely that you are sending it to me only because I have been involved in the area the patch touches. > To send your reviewed patches for inclusion: > $ git send-email outgoing/* -to gitster@pobox.com -cc git@vger.kernel.org This is fine, but we would probably want to see it CC'ed to people who reviewed the initial submission, too. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] SubmittingPatches: mention subsystems with dedicated repositories 2013-01-02 2:14 ` Junio C Hamano @ 2013-01-02 17:22 ` Junio C Hamano 2013-01-04 20:58 ` [PATCH] SubmittingPatches: Document how to request a patch review tag Jason Holden 0 siblings, 1 reply; 12+ messages in thread From: Junio C Hamano @ 2013-01-02 17:22 UTC (permalink / raw) To: Jason Holden; +Cc: git, Jeff King Junio C Hamano <gitster@pobox.com> writes: > Jason Holden <jason.k.holden.swdev@gmail.com> writes: > >> Any reason to leave out the maintainers email addresses? > > Nothing particular, other than that I did not find anywhere in the > file that does not break the flow. I've prepared this on top of the previous three. The second hunk that updates "(4) Sending your patches." is the logical place to have this information. The other hunks are correcting minor mistakes that are not related. I think I'll squash them to the patch 3/3. Documentation/SubmittingPatches | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index a7daad3..9e113d0 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -31,8 +31,9 @@ change is relevant to. these parts should be based on their trees. To find the tip of a topic branch, run "git log --first-parent -master..pu" and look for the merge commit. The second parent of this -commit is the tip of the topic branch. +master..pu" and look for merge commits. The second parent of these +commits are the tips of topic branches. + (1) Make separate commits for logically separate changes. @@ -70,6 +71,7 @@ changes do not trigger errors with the sample pre-commit hook shipped in templates/hooks--pre-commit. To help ensure this does not happen, run git diff --check on your changes before you commit. + (2) Describe your changes well. The first line of the commit message should be a short description (50 @@ -185,14 +187,20 @@ not a text/plain, it's something else. Send your patch with "To:" set to the mailing list, with "cc:" listing people who are involved in the area you are touching (the output from "git blame $path" and "git shortlog --no-merges $path" would help to -identify them), to solicit comments and reviews. After the list -reached a consensus that it is a good idea to apply the patch, re-send -it with "To:" set to the maintainer and "cc:" the list for inclusion. -Do not forget to add trailers such as "Acked-by:", "Reviewed-by:" and -"Tested-by:" after your "Signed-off-by:" line as necessary. +identify them), to solicit comments and reviews. + +After the list reached a consensus that it is a good idea to apply the +patch, re-send it with "To:" set to the maintainer [*1*] and "cc:" the +list [*2*] for inclusion. Do not forget to add trailers such as +"Acked-by:", "Reviewed-by:" and "Tested-by:" after your +"Signed-off-by:" line as necessary. + + [Addresses] + *1* The current maintainer: gitster@pobox.com + *2* The mailing list: git@vger.kernel.org -(4) Sign your work +(5) Sign your work To improve tracking of who did what, we've borrowed the "sign-off" procedure from the Linux kernel project on patches @@ -304,7 +312,8 @@ suggests to the contributors: even get them in a "on top of your change" patch form. (3) Polish, refine, and re-send to the list and the people who - spend their time to improve your patch. Go back to step (2). + spent their time to help improving your patch. Go back to + step (2) until step (4) happens. (4) The list forms consensus that the last round of your patch is good. Send it to the list and cc the maintainer. ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH] SubmittingPatches: Document how to request a patch review tag 2013-01-02 17:22 ` Junio C Hamano @ 2013-01-04 20:58 ` Jason Holden 2013-01-04 21:47 ` Junio C Hamano 2013-01-06 7:32 ` Michael Haggerty 0 siblings, 2 replies; 12+ messages in thread From: Jason Holden @ 2013-01-04 20:58 UTC (permalink / raw) To: gitster; +Cc: git, Jason Holden Document the preferred way a developer should request to have their Acked-by/Tested-by/Reviewed-by tag to a patch series under discussion Signed-off-by: Jason Holden <jason.k.holden.swdev@gmail.com> --- Junio, I was ready to add my Reviewed-by to this patch series, but I wasn't sure if I should email just you the patch author (to cut down on overall list traffic) or both you and the list. If all reviewed-by/acked-by/tested-by traffic should go via the email list I think this patch would be helpful, as I wasn't quite sure how wide of a distribution list to use for my "Reviewed-by" email. A very similiar question was asked previously in: http://thread.gmane.org/gmane.comp.version-control.git/185564/focus=185570 This will apply on top of your last tweak to SubmittingPatches Please add my reviewed-by to the rest of the patches in this series. -Jason Documentation/SubmittingPatches | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index f6276ff..80001c9 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -268,6 +268,11 @@ If you like, you can put extra tags at the end: 4. "Tested-by:" is used to indicate that the person applied the patch and found it to have the desired effect. +If you are a reviewer and wish to add your Acked-by/Reviewed-by/Tested-by tag +to a patch series under discussion (after having reviewed it or tested it +of course!), reply to the author of the patch series, cc'ing the git mailing +list. + You can also create your own tag or use one that's in common usage such as "Thanks-to:", "Based-on-patch-by:", or "Mentored-by:". -- 1.8.1.rc3.28.g0ab5d1f ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] SubmittingPatches: Document how to request a patch review tag 2013-01-04 20:58 ` [PATCH] SubmittingPatches: Document how to request a patch review tag Jason Holden @ 2013-01-04 21:47 ` Junio C Hamano 2013-01-06 7:32 ` Michael Haggerty 1 sibling, 0 replies; 12+ messages in thread From: Junio C Hamano @ 2013-01-04 21:47 UTC (permalink / raw) To: Jason Holden; +Cc: git Jason Holden <jason.k.holden.swdev@gmail.com> writes: > A very similiar question was asked previously in: > http://thread.gmane.org/gmane.comp.version-control.git/185564/focus=185570 "Reviewed-by" is for those who are familiar with the part of the system being touched to say "I reviewed this patch, it looks good", and Michael indeed was involved in recent updates to the refs.c infrastructure, so as he said in his message "it looks like I should", it was the right thing to do. I do not think Michael was asking if that was the standard _thing_ to do; I think the question was if there was a standard _way_ (perhaps a tool) to send such a "Reviewed-by:" line. > This will apply on top of your last tweak to SubmittingPatches > > Please add my reviewed-by to the rest of the patches in this series. I do not think you "own" anyting in SubmittingPatches document, though; at least not yet. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] SubmittingPatches: Document how to request a patch review tag 2013-01-04 20:58 ` [PATCH] SubmittingPatches: Document how to request a patch review tag Jason Holden 2013-01-04 21:47 ` Junio C Hamano @ 2013-01-06 7:32 ` Michael Haggerty 2013-01-06 9:01 ` Junio C Hamano 1 sibling, 1 reply; 12+ messages in thread From: Michael Haggerty @ 2013-01-06 7:32 UTC (permalink / raw) To: Jason Holden; +Cc: gitster, git On 01/04/2013 09:58 PM, Jason Holden wrote: > Document the preferred way a developer should request to have their > Acked-by/Tested-by/Reviewed-by tag to a patch series under discussion > > Signed-off-by: Jason Holden <jason.k.holden.swdev@gmail.com> > --- > Junio, > I was ready to add my Reviewed-by to this patch series, but I wasn't sure if > I should email just you the patch author (to cut down on overall list traffic) > or both you and the list. If all reviewed-by/acked-by/tested-by traffic > should go via the email list I think this patch would be helpful, as I > wasn't quite sure how wide of a distribution list to use for my > "Reviewed-by" email. > > A very similiar question was asked previously in: > http://thread.gmane.org/gmane.comp.version-control.git/185564/focus=185570 On 01/04/2013 10:47 PM, Junio C Hamano wrote: > "Reviewed-by" is for those who are familiar with the part of the > system being touched to say "I reviewed this patch, it looks good", > and Michael indeed was involved in recent updates to the refs.c > infrastructure, so as he said in his message "it looks like I should", > it was the right thing to do. > > I do not think Michael was asking if that was the standard _thing_ > to do; I think the question was if there was a standard _way_ > (perhaps a tool) to send such a "Reviewed-by:" line. Junio is correct; I was just asking whether there was a particular email convention for adding a "Reviewed-by:" line. It would be nice for this to be mentioned in the documentation. > Documentation/SubmittingPatches | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches > index f6276ff..80001c9 100644 > --- a/Documentation/SubmittingPatches > +++ b/Documentation/SubmittingPatches > @@ -268,6 +268,11 @@ If you like, you can put extra tags at the end: > 4. "Tested-by:" is used to indicate that the person applied the patch > and found it to have the desired effect. > > +If you are a reviewer and wish to add your Acked-by/Reviewed-by/Tested-by tag > +to a patch series under discussion (after having reviewed it or tested it > +of course!), reply to the author of the patch series, cc'ing the git mailing > +list. > + > You can also create your own tag or use one that's in common usage > such as "Thanks-to:", "Based-on-patch-by:", or "Mentored-by:". I don't think this is quite correct. Such emails should be "reply-to-all" people who have participated in the thread, which might include more than just the patch author and the git mailing list. Michael -- Michael Haggerty mhagger@alum.mit.edu http://softwareswirl.blogspot.com/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] SubmittingPatches: Document how to request a patch review tag 2013-01-06 7:32 ` Michael Haggerty @ 2013-01-06 9:01 ` Junio C Hamano 0 siblings, 0 replies; 12+ messages in thread From: Junio C Hamano @ 2013-01-06 9:01 UTC (permalink / raw) To: Michael Haggerty; +Cc: Jason Holden, git Michael Haggerty <mhagger@alum.mit.edu> writes: > On 01/04/2013 10:47 PM, Junio C Hamano wrote: >> "Reviewed-by" is for those who are familiar with the part of the >> system being touched to say "I reviewed this patch, it looks good", >> and Michael indeed was involved in recent updates to the refs.c >> infrastructure, so as he said in his message "it looks like I should", >> it was the right thing to do. >> >> I do not think Michael was asking if that was the standard _thing_ >> to do; I think the question was if there was a standard _way_ >> (perhaps a tool) to send such a "Reviewed-by:" line. > > Junio is correct; I was just asking whether there was a particular email > convention for adding a "Reviewed-by:" line. It would be nice for this > to be mentioned in the documentation. Yeah, I wasn't exactly correct in that I was talking more about Acked-by than Reviewed-by, but they are morally very similar and the same argument applies to both. In the particular case of your "refs.c" review, because you are not just familiar with the code, but essentially are the current owner of the code, Acked-by might have been even more appropriate than Reviewed-by, by the way. >> +If you are a reviewer and wish to add your Acked-by/Reviewed-by/Tested-by tag >> +to a patch series under discussion (after having reviewed it or tested it >> +of course!), reply to the author of the patch series, cc'ing the git mailing >> +list. >> + >> You can also create your own tag or use one that's in common usage >> such as "Thanks-to:", "Based-on-patch-by:", or "Mentored-by:". > > I don't think this is quite correct. Such emails should be > "reply-to-all" people who have participated in the thread, which might > include more than just the patch author and the git mailing list. That would be more helpful. In practice, I can pick these up either way, but Cc'ing everybody would be better as a common courtesy. When the author resubmits an already reviewed patch with these Acks and Reviews for final application, these reviewers should be Cc'ed so that they can say "Huh? that is not the exact patch I reviewed. What is going on?". Thanks for a review. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] SubmittingPatches: remove overlong checklist 2013-01-01 23:24 [PATCH 0/3] Update SubmittingPatches Junio C Hamano 2013-01-01 23:24 ` [PATCH 1/3] SubmittingPatches: who am I and who cares? Junio C Hamano 2013-01-01 23:24 ` [PATCH 2/3] SubmittingPatches: mention subsystems with dedicated repositories Junio C Hamano @ 2013-01-01 23:24 ` Junio C Hamano 2013-01-02 9:08 ` [PATCH 0/3] Update SubmittingPatches Jeff King 3 siblings, 0 replies; 12+ messages in thread From: Junio C Hamano @ 2013-01-01 23:24 UTC (permalink / raw) To: git; +Cc: Jason Holden The section is no longer a concise checklist. It also talks about things that are not covered in the "Long version" text, which means people need to read both, covering more or less the same thing in different phrasing. Fold the details into the main text and remove the section. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Documentation/SubmittingPatches | 126 +++++++++++++++++----------------------- 1 file changed, 53 insertions(+), 73 deletions(-) diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index e810263..a7daad3 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -1,65 +1,3 @@ -Checklist (and a short version for the impatient): - - Commits: - - - make commits of logical units - - check for unnecessary whitespace with "git diff --check" - before committing - - do not check in commented out code or unneeded files - - the first line of the commit message should be a short - description (50 characters is the soft limit, see DISCUSSION - in git-commit(1)), and should skip the full stop - - it is also conventional in most cases to prefix the - first line with "area: " where the area is a filename - or identifier for the general area of the code being - modified, e.g. - . archive: ustar header checksum is computed unsigned - . git-cherry-pick.txt: clarify the use of revision range notation - (if in doubt which identifier to use, run "git log --no-merges" - on the files you are modifying to see the current conventions) - - the body should provide a meaningful commit message, which: - . explains the problem the change tries to solve, iow, what - is wrong with the current code without the change. - . justifies the way the change solves the problem, iow, why - the result with the change is better. - . alternate solutions considered but discarded, if any. - - describe changes in imperative mood, e.g. "make xyzzy do frotz" - instead of "[This patch] makes xyzzy do frotz" or "[I] changed - xyzzy to do frotz", as if you are giving orders to the codebase - to change its behaviour. - - try to make sure your explanation can be understood without - external resources. Instead of giving a URL to a mailing list - archive, summarize the relevant points of the discussion. - - add a "Signed-off-by: Your Name <you@example.com>" line to the - commit message (or just use the option "-s" when committing) - to confirm that you agree to the Developer's Certificate of Origin - - make sure that you have tests for the bug you are fixing - - make sure that the test suite passes after your commit - - Patch: - - - use "git format-patch -M" to create the patch - - do not PGP sign your patch - - do not attach your patch, but read in the mail - body, unless you cannot teach your mailer to - leave the formatting of the patch alone. - - be careful doing cut & paste into your mailer, not to - corrupt whitespaces. - - provide additional information (which is unsuitable for - the commit message) between the "---" and the diffstat - - if you change, add, or remove a command line option or - make some other user interface change, the associated - documentation should be updated as well. - - if your name is not writable in ASCII, make sure that - you send off a message in the correct encoding. - - send the patch to the list (git@vger.kernel.org) and the - maintainer (gitster@pobox.com) if (and only if) the patch - is ready for inclusion. If you use git-send-email(1), - please test it first by sending email to yourself. - - see below for instructions specific to your mailer - -Long version: - Here are some guidelines for people who want to contribute their code to this software. @@ -119,13 +57,52 @@ change, the approach taken by the change, and if relevant how this differs substantially from the prior version, are all good things to have. +Make sure that you have tests for the bug you are fixing. + +When adding a new feature, make sure that you have new tests to show +the feature triggers the new behaviour when it should, and to show the +feature does not trigger when it shouldn't. Also make sure that the +test suite passes after your commit. Do not forget to update the +documentation to describe the updated behaviour. + Oh, another thing. I am picky about whitespaces. Make sure your changes do not trigger errors with the sample pre-commit hook shipped in templates/hooks--pre-commit. To help ensure this does not happen, run git diff --check on your changes before you commit. +(2) Describe your changes well. + +The first line of the commit message should be a short description (50 +characters is the soft limit, see DISCUSSION in git-commit(1)), and +should skip the full stop. It is also conventional in most cases to +prefix the first line with "area: " where the area is a filename or +identifier for the general area of the code being modified, e.g. + + . archive: ustar header checksum is computed unsigned + . git-cherry-pick.txt: clarify the use of revision range notation + +If in doubt which identifier to use, run "git log --no-merges" on the +files you are modifying to see the current conventions. + +The body should provide a meaningful commit message, which: -(2) Generate your patch using git tools out of your commits. + . explains the problem the change tries to solve, iow, what is wrong + with the current code without the change. + + . justifies the way the change solves the problem, iow, why the + result with the change is better. + + . alternate solutions considered but discarded, if any. + +Describe your changes in imperative mood, e.g. "make xyzzy do frotz" +instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy +to do frotz", as if you are giving orders to the codebase to change +its behaviour. Try to make sure your explanation can be understood +without external resources. Instead of giving a URL to a mailing list +archive, summarize the relevant points of the discussion. + + +(3) Generate your patch using git tools out of your commits. git based diff tools generate unidiff which is the preferred format. @@ -133,22 +110,27 @@ You do not have to be afraid to use -M option to "git diff" or "git format-patch", if your patch involves file renames. The receiving end can handle them just fine. -Please make sure your patch does not include any extra files -which do not belong in a patch submission. Make sure to review +Please make sure your patch does not add commented out debugging code, +or include any extra files which do not relate to what your patch +is trying to achieve. Make sure to review your patch after generating it, to ensure accuracy. Before sending out, please make sure it cleanly applies to the "master" branch head. If you are preparing a work based on "next" branch, that is fine, but please mark it as such. -(3) Sending your patches. +(4) Sending your patches. People on the git mailing list need to be able to read and comment on the changes you are submitting. It is important for a developer to be able to "quote" your changes, using standard e-mail tools, so that they may comment on specific portions of your code. For this reason, all patches should be submitted -"inline". WARNING: Be wary of your MUAs word-wrap +"inline". If your log message (including your name on the +Signed-off-by line) is not writable in ASCII, make sure that +you send off a message in the correct encoding. + +WARNING: Be wary of your MUAs word-wrap corrupting your patch. Do not cut-n-paste your patch; you can lose tabs that way if you are not careful. @@ -200,16 +182,14 @@ patch, format it as "multipart/signed", not a text/plain message that starts with '-----BEGIN PGP SIGNED MESSAGE-----'. That is not a text/plain, it's something else. -Unless your patch is a very trivial and an obviously correct one, -first send it with "To:" set to the mailing list, with "cc:" listing +Send your patch with "To:" set to the mailing list, with "cc:" listing people who are involved in the area you are touching (the output from "git blame $path" and "git shortlog --no-merges $path" would help to identify them), to solicit comments and reviews. After the list reached a consensus that it is a good idea to apply the patch, re-send -it with "To:" set to the maintainer and optionally "cc:" the list for -inclusion. Do not forget to add trailers such as "Acked-by:", -"Reviewed-by:" and "Tested-by:" after your "Signed-off-by:" line as -necessary. +it with "To:" set to the maintainer and "cc:" the list for inclusion. +Do not forget to add trailers such as "Acked-by:", "Reviewed-by:" and +"Tested-by:" after your "Signed-off-by:" line as necessary. (4) Sign your work -- 1.8.1.209.gc32ab23 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] Update SubmittingPatches 2013-01-01 23:24 [PATCH 0/3] Update SubmittingPatches Junio C Hamano ` (2 preceding siblings ...) 2013-01-01 23:24 ` [PATCH 3/3] SubmittingPatches: remove overlong checklist Junio C Hamano @ 2013-01-02 9:08 ` Jeff King 3 siblings, 0 replies; 12+ messages in thread From: Jeff King @ 2013-01-02 9:08 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jason Holden On Tue, Jan 01, 2013 at 03:24:52PM -0800, Junio C Hamano wrote: > The main thing this series wants to do is the second one, but I > wanted to reduce the clutter in this document while at it. > > Junio C Hamano (3): > SubmittingPatches: who am I and who cares? > SubmittingPatches: mention subsystems with dedicated repositories > SubmittingPatches: remove overlong checklist FWIW, all look reasonable to me. I did not pay close attention to the earlier discussion on MAINTAINERS, but my first thought on reading it was "why isn't this stuff just in the SubmittingPatches doc? That's where I would look for it". I guess some people might balk at the third one, as the checklist is supposed to help with a quick and easy start. But I agree with your commit log message that it isn't short or quick anymore. Of course it has been a long time since I have needed to read that document. Maybe people who have used it more recently would have more relevant thoughts. -Peff ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2013-01-06 9:02 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-01-01 23:24 [PATCH 0/3] Update SubmittingPatches Junio C Hamano 2013-01-01 23:24 ` [PATCH 1/3] SubmittingPatches: who am I and who cares? Junio C Hamano 2013-01-01 23:24 ` [PATCH 2/3] SubmittingPatches: mention subsystems with dedicated repositories Junio C Hamano 2013-01-02 1:52 ` Jason Holden 2013-01-02 2:14 ` Junio C Hamano 2013-01-02 17:22 ` Junio C Hamano 2013-01-04 20:58 ` [PATCH] SubmittingPatches: Document how to request a patch review tag Jason Holden 2013-01-04 21:47 ` Junio C Hamano 2013-01-06 7:32 ` Michael Haggerty 2013-01-06 9:01 ` Junio C Hamano 2013-01-01 23:24 ` [PATCH 3/3] SubmittingPatches: remove overlong checklist Junio C Hamano 2013-01-02 9:08 ` [PATCH 0/3] Update SubmittingPatches Jeff King
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).