Re: [PATCH] MyFirstContribution: Document --range-diff option when writing v2

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 13, 2021, 21:39 UTC
Message-ID: <xmqqwnnkcfz8.fsf@gitster.g>
In-Reply-To: <20210913194816.51182-1-chooglen@google.com>

Glen Choo <chooglen@google.com> writes:

Show 6 quoted lines

> +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.

Show 7 quoted lines

> +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.

Show 9 quoted lines

> +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.