| Message ID | cover.1603155607.git.bkuhn@sfconservancy.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | clarify meaning of --signoff & related doc improvements in describing Signed-off-by | expand |
Hi Bradley, On Mon, Oct 19, 2020 at 06:03:51PM -0700, Bradley M. Kuhn wrote: > [ Note that there were competing v2's of this patch series, one from me and > one from Junio. Sorry about that; I had missed Junio's from Sunday. ] > > I believe this patch series now addresses all the issues raised in the > discussion. > > * [1/4] is unchanged from Junio's v2 and is the preparatory cleanup. > > * [2/4] remains unchanged textually since I originally posted it and Pfeff > signed off. > > * [3/4] is unchanged from Junio's v2 and is his clarification for what > Git's --signoff means. > > * [4/4] is a rework of a patch from *my* v2. It takes into account Junio's > comments about preferring the word "trailer" and leaving off the ':' > whenever possible when discussing Signed-off-by. Thanks; I think some of our emails crossed over one another, but this version looks good to me. I'd be happy to discard what's currently in seen (integrated as 1b98087e0f (Merge branch 'bk/sob-dco' into jch, 2020-10-19 at the time of writing) in favor of what's here. Thanks, Taylor
Taylor Blau wrote: > Thanks; I think some of our emails crossed over one another, but this > version looks good to me. Yes, I was preparing the patch when you wrote that you disagreed with Junio and preferred the ":". FWIW, I left the ":" anywhere headers were being discussed and those headers were described with ":"s on them. I only changed places where "Signed-off-by:" stood alone. Before my v3 patchset, usage was inconsistent about (roughly half/half), so the decision is mostly a coin toss. I didn't have a strong opinion when I was first writing the v3 patchset, but having thought about it overnight, I now think leaving the ":" *out* is better because a reader new to Git is more likely to think a ":" is punctuation, rather than being part of a moniker. Thus, IMO, leaving out the ":" in most cases probably improves readability. The remainder of this email is purely an edification question that may help serve to improve Documentation/SubmittingPatches: > I'd be happy to discard what's currently in seen (integrated as 1b98087e0f > (Merge branch 'bk/sob-dco' into jch, 2020-10-19 at the time of writing) in > favor of what's here. I wasn't sure what I should be doing with the patch set once it was already in 'seen'. The only two references in SubmittingPatches I could find were: From Documentation/SubmittingPatches: >> In any time between the (2)-(3) cycle, the maintainer may pick it up from >> the list and queue it to `seen`, in order to make it easier for people >> play with it without having to pick up and apply the patch to their trees >> themselves. and >> `git pull --rebase` will automatically skip already-applied patches, and >> will let you know. This works only if you rebase on top of the branch in >> which your patch has been merged (i.e. it will not tell you if your patch >> is merged in `seen` if you rebase on top of master). The former hints that you *shouldn't* change the workflow if some of your patchset is in `seen`, and the latter hints that maybe you should, but neither section tells you what to do differently, if anything, once your patches are in `seen`. I'm curious to know if I went wrong somewhere and the workflow and would be glad to propose another patch to improve SubmittingPatches with a section of what to do when patches show up in `seen`, but since I'm a n00b (at least as an upstream Git contributor :), I'd need to know how to DTRT in this case to do that. -- Bradley M. Kuhn - he/him Policy Fellow & Hacker-in-Residence at Software Freedom Conservancy ======================================================================== Become a Conservancy Supporter today: https://sfconservancy.org/supporter
Hi Bradley, On Tue, Oct 20, 2020 at 02:28:20PM -0700, Bradley M. Kuhn wrote: > The remainder of this email is purely an edification question that may help > serve to improve Documentation/SubmittingPatches: > > > I'd be happy to discard what's currently in seen (integrated as 1b98087e0f > > (Merge branch 'bk/sob-dco' into jch, 2020-10-19 at the time of writing) in > > favor of what's here. > > I wasn't sure what I should be doing with the patch set once it was already > in 'seen'. The only two references in SubmittingPatches I could find were: The conclusive answer is that you can do anything you want when the patches are in 'seen', but when it is in 'next', things have solidified and the series is not meant to be changed. The one exception to that rule is immediately after a release, in which case 'next' is rewound (and thus some topics can be ejected from next). > From Documentation/SubmittingPatches: > >> In any time between the (2)-(3) cycle, the maintainer may pick it up from > >> the list and queue it to `seen`, in order to make it easier for people > >> play with it without having to pick up and apply the patch to their trees > >> themselves. > > and > > >> `git pull --rebase` will automatically skip already-applied patches, and > >> will let you know. This works only if you rebase on top of the branch in > >> which your patch has been merged (i.e. it will not tell you if your patch > >> is merged in `seen` if you rebase on top of master). > > The former hints that you *shouldn't* change the workflow if some of your > patchset is in `seen`, and the latter hints that maybe you should, but > neither section tells you what to do differently, if anything, once your > patches are in `seen`. I think the latter is really only talking about branches based on 'master' (of course, the same thing is true of branches based on any branch, but it's uncommon to base topics off of 'seen'). The former is saying that 'seen' may change zero, one, or multiple times during the lifetime of a topic. The latter says if you track upstream's 'master' and then 'git pull --rebase', 'git rebase' will tell you if your patches are already applied upstream (in which case you can drop them). > I'm curious to know if I went wrong somewhere and the workflow and would be > glad to propose another patch to improve SubmittingPatches with a section of > what to do when patches show up in `seen`, but since I'm a n00b (at least as > an upstream Git contributor :), I'd need to know how to DTRT in this case to > do that. It couldn't hurt to add something to the effect of: Since 'seen' is a convenience branch that contains the current state of the in-flight topics, it is subject to be changed and rebuilt multiple times (c.f., link:howto/maintain-git) so the presence of your patches in 'seen' (but not 'next' or 'master') should not affect your workflow. Thanks, Taylor
"Bradley M. Kuhn" <bkuhn@sfconservancy.org> writes: > I wasn't sure what I should be doing with the patch set once it was already > in 'seen'. The only two references in SubmittingPatches I could find were: Being 'seen' is an indication that it has been seen and does not mean anything more than that. It is appreciated that a topic in such a state is improved by replacing. > From Documentation/SubmittingPatches: >>> In any time between the (2)-(3) cycle, the maintainer may pick it up from >>> the list and queue it to `seen`, in order to make it easier for people >>> play with it without having to pick up and apply the patch to their trees >>> themselves. Yes. Other people then can "git fetch" from me and follow the first parent chain "git log --first-parent origin/master..origin/seen" to find the tip of your topic, instead of finding your message in the list archive and running "git am" themselves. The original submitter/owner of the topic can also find the tip of the topic _in_ my tree the same way as others and reset their branch to what is queued in 'seen' if they wanted to keep minor fixes I made based on review comments while applying the e-mailed patches. Then they can further work on polishing the topic with the usual means, e.g. using "rebase -i", and finally "format-patch" to send out a new round. Being or not being in 'seen' does not change the workflow that much. >>> `git pull --rebase` will automatically skip already-applied patches, and >>> will let you know. This works only if you rebase on top of the branch in >>> which your patch has been merged (i.e. it will not tell you if your patch >>> is merged in `seen` if you rebase on top of master). This is talking about a fairly mature topic that has already been in 'next' and was on the course to graduate to 'master'. The topic would eventually be in 'master', and at that point "pull --rebase" would notice that the patches are no longer needed (or were merged in a different form). But that does not apply to topics that are not in 'master' yet. Where the workflow changes is when the topic hits 'next'. After that, we request you to give incremental updates to refine what is queued already. The reasoning behind this is simple and arbitrary. It often is the case that keeping mistakes in early iterations, and fixes to these mistakes, recorded in history is not worth the attention of future readers of "git log" who need to study the history, assuming that trivial mistakes are caught early. Once earlier rounds of review is done and everybody is more or less happy, the topic gets merged to 'next', and after that point, a new issue that gets noticed and fixed _are_ worth recording in history, because both the original contributor and reviewers failed to catch such glitches. > I'm curious to know if I went wrong somewhere and the workflow and would be > glad to propose another patch to improve SubmittingPatches with a section of > what to do when patches show up in `seen`, but since I'm a n00b (at least as > an upstream Git contributor :), I'd need to know how to DTRT in this case to > do that. I thought your v3 did things perfectly. Thanks.
> "Bradley M. Kuhn" <bkuhn@sfconservancy.org> writes: > > I wasn't sure what I should be doing with the patch set once it was already > > in 'seen'. Junio C Hamano wrote: > Being 'seen' is an indication that it has been seen and does not mean > anything more than that. Documentation/SubmittingPatches says: > >>> `git pull --rebase` will automatically skip already-applied patches, and > >>> will let you know. This works only if you rebase on top of the branch in > >>> which your patch has been merged (i.e. it will not tell you if your patch > >>> is merged in `seen` if you rebase on top of master). > This is talking about a fairly mature topic that has already been in 'next' > and was on the course to graduate to 'master'. The topic would eventually > be in 'master', and at that point "pull --rebase" would notice that the > patches are no longer needed (or were merged in a different form). But > that does not apply to topics that are not in 'master' yet. Junio, thanks for your detailed reply. I have a couple ideas of some minor changes to SubmittingPatches that would have made what you said clearer to me when I was doing my first patch. I'll think about it some and send something along, but probably won't get to it until next month (but it doesn't seem urgent). Taylor wrote: >>> Since 'seen' is a convenience branch that contains the current state >>> of the in-flight topics, it is subject to be changed and rebuilt >>> multiple times (c.f., link:howto/maintain-git) so the presence of your >>> patches in 'seen' (but not 'next' or 'master') should not affect your >>> workflow. I'll hold on to this text as a possibility, to start from when I dig into the proposals above. Thanks! -- Bradley M. Kuhn - he/him Policy Fellow & Hacker-in-Residence at Software Freedom Conservancy ======================================================================== Become a Conservancy Supporter today: https://sfconservancy.org/supporter
[ Note that there were competing v2's of this patch series, one from me and one from Junio. Sorry about that; I had missed Junio's from Sunday. ] I believe this patch series now addresses all the issues raised in the discussion. * [1/4] is unchanged from Junio's v2 and is the preparatory cleanup. * [2/4] remains unchanged textually since I originally posted it and Pfeff signed off. * [3/4] is unchanged from Junio's v2 and is his clarification for what Git's --signoff means. * [4/4] is a rework of a patch from *my* v2. It takes into account Junio's comments about preferring the word "trailer" and leaving off the ':' whenever possible when discussing Signed-off-by. For reference: v1: https://lore.kernel.org/git/20201015215933.96425-1-bkuhn@sfconservancy.org/ Junio's v2: https://lore.kernel.org/git/20201018194912.2716372-1-gitster@pobox.com/ my v2: https://lore.kernel.org/git/cover.1603142543.git.bkuhn@sfconservancy.org/ Bradley M. Kuhn (2): Documentation: clarify and expand description of --signoff Documentation: stylistically normalize references to Signed-off-by: Junio C Hamano (2): doc: preparatory clean-up of description on the sign-off option SubmittingPatches: clarify DCO is our --signoff rule Documentation/MyFirstContribution.txt | 2 +- Documentation/SubmittingPatches | 35 +++++++++++++++------------ Documentation/config/format.txt | 2 +- Documentation/git-am.txt | 2 +- Documentation/git-cherry-pick.txt | 2 +- Documentation/git-commit.txt | 10 ++------ Documentation/git-format-patch.txt | 2 +- Documentation/git-rebase.txt | 2 +- Documentation/git-revert.txt | 2 +- Documentation/git-send-email.txt | 4 +-- Documentation/git-svn.txt | 4 +-- Documentation/githooks.txt | 2 +- Documentation/merge-options.txt | 11 +-------- Documentation/signoff-option.txt | 18 ++++++++++++++ builtin/am.c | 2 +- builtin/commit.c | 2 +- builtin/log.c | 2 +- builtin/merge.c | 2 +- builtin/pull.c | 2 +- builtin/rebase.c | 2 +- builtin/revert.c | 2 +- commit.c | 2 +- 22 files changed, 60 insertions(+), 54 deletions(-) create mode 100644 Documentation/signoff-option.txt