| Message ID | 20210913194816.51182-1-chooglen@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MyFirstContribution: Document --range-diff option when writing v2 | expand |
On Mon, Sep 13, 2021 at 3:48 PM Glen Choo <chooglen@google.com> wrote: > In the "Sending V2" section, readers are directed to create v2 patches > without using --range-diff. However, it is custom to include a range > diff against the v1 patches as a reviewer aid. > > Update the "Sending V2" section to include the --range-diff option. Also > include some explanation for -v2 and --range-diff to help the reader > understand the importance. Makes sense. A few minor comments below... > Signed-off-by: Glen Choo <chooglen@google.com> > --- > diff --git a/Documentation/MyFirstContribution.txt b/Documentation/MyFirstContribution.txt > @@ -1033,18 +1033,33 @@ Skip ahead to <<reviewing,Responding to Reviews>> for information on how to > -When you're ready with the next iteration of your patch, the process is fairly > -similar. > +Let's write v2 as its own topic branch, because this will make some things more > +convenient later on. Create the `psuh-v2` branch like so: > > -First, generate your v2 patches again: > +---- > +$ git checkout -b psuh-v2 psuh > +---- These days, we're generally promote `git switch -c psuh-v2 psuh` rather than `git branch -b`. However, since the document already uses `git branch -b` elsewhere, the use here is probably acceptable. (An alternative would be to make this a two-patch series in which the first patch changes `git branch -b` over to `git switch -c`.) > +When you're ready with the next iteration of your patch, the process is fairly > +similar to before. Generate your patches again, but with some new flags: > > ---- > -$ git format-patch -v2 --cover-letter -o psuh/ master..psuh > +$ git format-patch -v2 --range-diff psuh..psuh-v2 --cover-letter -o psuh/ master..psuh > ---- As long as both versions have the same base, it's generally easier to say merely `--range-diff=psuh` -- that is, you want a range-diff against `psuh` -- than to spell out the range explicitly. However, perhaps spelling out the range here has some pedagogical value, so maybe this is okay as-is. > +The `--range-diff psuh..psuh-v2` parameter tells `format-patch` to include a > +range diff between `psuh` and `psuh-v2`. This helps tell reviewers about the > +differences between your v1 and v2 patches. I think we usually spell it as "range-diff", not "range diff". Also, it might be a good idea to give some hint as to what a range-diff is, even if that hint is just a link to the `git range-diff` manual page. Maybe: ...to include a range-diff (see linkgit:range-diff[1]) between...
On Mon, Sep 13, 2021 at 4:00 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > I think we usually spell it as "range-diff", not "range diff". Also, > it might be a good idea to give some hint as to what a range-diff is, > even if that hint is just a link to the `git range-diff` manual page. > Maybe: > > ...to include a range-diff (see linkgit:range-diff[1]) between... Of course, I meant: ...to include a range-diff (see linkgit:git-range-diff[1]) between...
Glen Choo <chooglen@google.com> writes: > +Let's write v2 as its own topic branch, because this will make some things more > +convenient later on. Create the `psuh-v2` branch like so: > > +---- > +$ git checkout -b psuh-v2 psuh > +---- What's missing here is on which branch this new description expects the user to work on. From its name, I am assuming that psuh-v2 will be modified while leaving psuh untouched, but spell your expectation out. The following review is based on the assumption that the user is expected to futz with psuh-v2, leaving psuh intact. If that is not the case, it is a strong sign that you caused confusion on one reader by not spelling out your expectation. I do not think it is a good suggestion at all to use a new topic branch, especially a one that forked from the tip of the original submission, and work on that branch to produce the new round. It would be much better to create a topic branch or a lightweight tag "psuh-v1" that points at the old tip and keep working on the same branch. But that is a separate story. > +When you're ready with the next iteration of your patch, the process is fairly > +similar to before. Generate your patches again, but with some new flags: > > ---- > -$ git format-patch -v2 --cover-letter -o psuh/ master..psuh > +$ git format-patch -v2 --range-diff psuh..psuh-v2 --cover-letter -o psuh/ master..psuh > ---- Before the "Generate your patches again", there would have been "rebase -i" of the original commits that went into "psuh" (v1). But you do not necessarily have to touch all the commits during "rebase -i" session. What happens when the first few commits did not need to be touched? Since the --range-diff says psuh..psuh-v2, these early and unmodified commits are excluded from the range, no? That would mean what appears to be commit #1 in the range-diff on the new side would not be the [PATCH 1/n] of the output, no? And the command line says to format master..psuh, which is puzzling. Shouldn't it format the updated psuh-v2 branch? > +The `--range-diff psuh..psuh-v2` parameter tells `format-patch` to include a > +range diff between `psuh` and `psuh-v2`. This helps tell reviewers about the > +differences between your v1 and v2 patches. See above. The range-diff may fail to tell the fact that there are a few bottommost commits that are the same by omitting them. Perhaps it would make it easier to manage if we used psuh-v1 as the anchoring point to represent where the tip of the last round was? With something like: $ git checkout psuh $ git branch psuh-v1 ;# optional -- "git tag" is also OK. ... work work work with "rebase -i" ... $ git format-patch -v2 --cover-letter -o psuh/ \ --range-diff master..psuh-v1 master.. # ..psuh-v1 can be ..@{yesterday} or whatever reflog reference we do not have to worry if "rebase -i" left the bottommost commits untouched or silly things like that. > +The `-v2` parameter tells `format-patch` to output "v2" patches. For instance, > +you may notice that your v2 patches, are all named like > +`v2-000n-my-commit-subject.patch`. `-v2` will also format your patches by > +prefixing them with "[PATCH V2]" instead of "[PATCH]", and your range-diff will > +be prefaced with "Range-diff against v1". > + > +Afer you run this command, `format-patch` will output the patches to the `psuh/` > +directory, alongside the v1 patches. That's fine, but be careful when you are > +ready to send them. It is unclear what "That's fine, but" is trying to convey. I'd replace it with something like: You can refer to the old v1 patches while giving the final proofreading on the v2 patches, but you now need to say "git send-email psuh/v2-*.patch" to send them out ("*.patch" would match both v1 and v2 patches). Thanks.
On 14/09/21 02.48, Glen Choo wrote: > In the "Sending V2" section, readers are directed to create v2 patches > without using --range-diff. However, it is custom to include a range > diff against the v1 patches as a reviewer aid. > > Update the "Sending V2" section to include the --range-diff option. Also > include some explanation for -v2 and --range-diff to help the reader > understand the importance. I think plain "Changes since v1 [link]" is sufficient if you can describe such changes well without resorting to range-diff.
On Mon, Sep 13, 2021 at 10:43 PM Bagas Sanjaya <bagasdotme@gmail.com> wrote: > On 14/09/21 02.48, Glen Choo wrote: > > In the "Sending V2" section, readers are directed to create v2 patches > > without using --range-diff. However, it is custom to include a range > > diff against the v1 patches as a reviewer aid. > > > > Update the "Sending V2" section to include the --range-diff option. Also > > include some explanation for -v2 and --range-diff to help the reader > > understand the importance. > > I think plain "Changes since v1 [link]" is sufficient if you can > describe such changes well without resorting to range-diff. Not so. Anyone who does any serious amount of review on this project finds it tremendously helpful to have both a prose description of the changes ("Changes since v1..." plus a link to the previous submission) and a mechanical range-diff or interdiff. A range-diff (or interdiff) is especially important to provide reviewers with context which they might have forgotten since the previous version of a patch series was posted, which can matter since it's so easy to forget specifics even about one's own review if enough time has passed or if reviewing a large number of unrelated submissions. A range-diff or interdiff also helps a reviewer determine at-a-glance whether or not earlier review comments have been addressed without having to laboriously re-read each and every patch.
On 14/09/21 10.46, Eric Sunshine wrote: > Not so. Anyone who does any serious amount of review on this project > finds it tremendously helpful to have both a prose description of the > changes ("Changes since v1..." plus a link to the previous submission) > and a mechanical range-diff or interdiff. A range-diff (or interdiff) > is especially important to provide reviewers with context which they > might have forgotten since the previous version of a patch series was > posted, which can matter since it's so easy to forget specifics even > about one's own review if enough time has passed or if reviewing a > large number of unrelated submissions. A range-diff or interdiff also > helps a reviewer determine at-a-glance whether or not earlier review > comments have been addressed without having to laboriously re-read > each and every patch. > So will range-diff be mandatory or optional?
On Mon, Sep 13, 2021 at 02:39:39PM -0700, Junio C Hamano wrote: > What's missing here is on which branch this new description expects > the user to work on. From its name, I am assuming that psuh-v2 will > be modified while leaving psuh untouched, but spell your expectation > out. > > The following review is based on the assumption that the user is > expected to futz with psuh-v2, leaving psuh intact. If that is not > the case, it is a strong sign that you caused confusion on one > reader by not spelling out your expectation. That's fair, I was suggesting a specific workflow and should have been more explicit. > I do not think it is a good suggestion at all to use a new topic > branch, especially a one that forked from the tip of the original > submission, and work on that branch to produce the new round. It > would be much better to create a topic branch or a lightweight tag > "psuh-v1" that points at the old tip and keep working on the same > branch. But that is a separate story. Given that this is a "my first contribution" guide, I think it would be beneficial to be at least mildly opinionated on the workflow. Since the specifics of range-diff would depend on the workflow we choose to promote, it would be nice to make it as helpful as possible the first time around. That is to say, I'd really appreciate your thoughts on your recommended workflow and I'd like to structure my additions around your recommendation :) What I've documented is just my own workflow, but I'm sure you have more insight into this area. > But you do not necessarily have to touch all the commits during > "rebase -i" session. What happens when the first few commits did > not need to be touched? > > Since the --range-diff says psuh..psuh-v2, these early and > unmodified commits are excluded from the range, no? That would mean > what appears to be commit #1 in the range-diff on the new side would > not be the [PATCH 1/n] of the output, no? Yes, good catch. This was an oversight on my part. > Perhaps it would make it easier to manage if we used psuh-v1 as the > anchoring point to represent where the tip of the last round was? > > With something like: > > $ git checkout psuh > $ git branch psuh-v1 ;# optional -- "git tag" is also OK. > ... work work work with "rebase -i" ... > $ git format-patch -v2 --cover-letter -o psuh/ \ > --range-diff master..psuh-v1 master.. Having an explicit psuh-v1 is a good idea. > # ..psuh-v1 can be ..@{yesterday} or whatever reflog reference If we make it clear that psuh-v1 is just the tip of the last round, perhaps the readers who would do this can already infer this from context, and we can omit the comment for brevity? > > +Afer you run this command, `format-patch` will output the patches to the `psuh/` > > +directory, alongside the v1 patches. That's fine, but be careful when you are > > +ready to send them. > > It is unclear what "That's fine, but" is trying to convey. Very valid. > I'd replace it with something like: > > You can refer to the old v1 patches while giving the final > proofreading on the v2 patches, but you now need to say "git > send-email psuh/v2-*.patch" to send them out ("*.patch" would > match both v1 and v2 patches). Here, I have to confess that I'm not sure why the guide reuses the psuh/ directory. When I was first going through this, having v1 and v2 patches in the same directory seemed like a risky default because of the inherent risk of sending both v1 and v2 together. I believe the unspoken intent is that having v1 patches in the same directory as v2 patches makes it easy to refer to both versions, and in turn, this promotes better quality discussion between v1 and v2. There might be other strong reasons to reuse the directory, but these are not obvious to me from reading this guide alone. It would be helpful to make these reasons explicit to the reader.
Glen Choo <chooglen@google.com> writes: >> $ git format-patch -v2 --cover-letter -o psuh/ \ >> --range-diff master..psuh-v1 master.. > > Having an explicit psuh-v1 is a good idea. I think what's good idea is not explicit psuh-v1 but taking the range from the upstream (i.e. "master"), not "psuh" (which assumes that symmetric difference will cover everything, which may not necessarily be the case). >> # ..psuh-v1 can be ..@{yesterday} or whatever reflog reference > > If we make it clear that psuh-v1 is just the tip of the last round, perhaps the > readers who would do this can already infer this from context, and we can omit > the comment for brevity? Oh absolutely. These #comments were meant for your consumption and not as a suggestion to be placed in the final text given to the end users. > I believe the unspoken intent is that having v1 patches in the same directory as > v2 patches makes it easy to refer to both versions, and in turn, this promotes > better quality discussion between v1 and v2. Separate directories may make it easier to do: diff -u psuh-v1/ pshu-v2/ as cd psuh && diff -u v1* v2* would NOT work, but comparing patch e-mails textually, especially after the order of the patches or title of them got updated, won't be so useful an operation anyway. When you have multiple topics in flight, you want your psuh topic in a single directory (perhaps differentiating the iterations with v$n prefix, or you can choose to have a subdirectory v1, v2, etc. in that directory) that is different from the directory for your plul topic, instead of flat collection of directories for psuh-v1, psuh-v2 and plul-v1 topic-iterations.
diff against the v1 patches as a reviewer aid. Update the "Sending V2" section to include the --range-diff option. Also include some explanation for -v2 and --range-diff to help the reader understand the importance. Signed-off-by: Glen Choo <chooglen@google.com> --- Documentation/MyFirstContribution.txt | 29 ++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/Documentation/MyFirstContribution.txt b/Documentation/MyFirstContribution.txt index 015cf24631..add1c2bba9 100644 --- a/Documentation/MyFirstContribution.txt +++ b/Documentation/MyFirstContribution.txt @@ -1033,18 +1033,33 @@ Skip ahead to <<reviewing,Responding to Reviews>> for information on how to handle comments from reviewers. Continue this section when your topic branch is shaped the way you want it to look for your patchset v2. -When you're ready with the next iteration of your patch, the process is fairly -similar. +Let's write v2 as its own topic branch, because this will make some things more +convenient later on. Create the `psuh-v2` branch like so: -First, generate your v2 patches again: +---- +$ git checkout -b psuh-v2 psuh +---- + +When you're ready with the next iteration of your patch, the process is fairly +similar to before. Generate your patches again, but with some new flags: ---- -$ git format-patch -v2 --cover-letter -o psuh/ master..psuh +$ git format-patch -v2 --range-diff psuh..psuh-v2 --cover-letter -o psuh/ master..psuh ---- -This will add your v2 patches, all named like `v2-000n-my-commit-subject.patch`, -to the `psuh/` directory. You may notice that they are sitting alongside the v1 -patches; that's fine, but be careful when you are ready to send them. +The `--range-diff psuh..psuh-v2` parameter tells `format-patch` to include a +range diff between `psuh` and `psuh-v2`. This helps tell reviewers about the +differences between your v1 and v2 patches. + +The `-v2` parameter tells `format-patch` to output "v2" patches. For instance, +you may notice that your v2 patches, are all named like +`v2-000n-my-commit-subject.patch`. `-v2` will also format your patches by +prefixing them with "[PATCH V2]" instead of "[PATCH]", and your range-diff will +be prefaced with "Range-diff against v1". + +Afer you run this command, `format-patch` will output the patches to the `psuh/` +directory, alongside the v1 patches. That's fine, but be careful when you are +ready to send them. Edit your cover letter again. Now is a good time to mention what's different between your last version and now, if it's something significant. You do not