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.
