git format-patch --range-diff bug?

I've just run into an unexpected looking range-diff generated by 'git format-patch -1 --range-diff=...'. While the 'git range-diff' command correctly pairs up the old and new versions of the commit, 'git format-patch --range-diff' somehow doesn't and reports it in a weird way.

The test below demonstrates the issue, though it doesn't fail but only prints the outputs of those commands:

  ---  >8  ---

#!/bin/sh

test_description='test'

. ./test-lib.sh

test_expect_success 'test' '
	test_tick &&
	printf "%s\n" 0 1 2 3 4 5 6 7 8 9 >file &&
	git add file &&
	git commit -m initial &&

	git checkout -b v1 &&
	echo foo >>file &&
	git commit -m change file &&

	git checkout -b v2 master &&
	echo bar >>file &&
	git commit -m change file &&

	: The output of the range-diff command is what I would expect to be included in the generated patch &&
	git range-diff v1...v2 &&

	: But that is not what we get embedded in the generated patch &&
	git format-patch -1 --range-diff=v1...v2 v2 &&
	sed -n -e "/^Range-diff:$/,/^$/ p" 0001-change.patch &&

	: Adding the range-diff to the cover letter doesnt seem to help &&
	git format-patch -1 --range-diff=v1...v2 --cover-letter v2 &&
	sed -n -e "/^Range-diff:$/,$ p" 0000-cover-letter.patch
'
test_done

  ---  8<  ---

And here is the relevant part of the output:

+ : The output of the range-diff command is what I would expect to be
included in the generated patch
+ git range-diff v1...v2
1:  f8c2d0b ! 1:  83c34a8 change
    @@ -9,4 +9,4 @@
      7
      8
      9
    -+foo
    ++bar
+ : But that is not what we get embedded in the generated patch
+ git format-patch -1 --range-diff=v1...v2 v2
0001-change.patch
+ sed -n -e /^Range-diff:$/,/^$/ p 0001-change.patch
Range-diff:
1:  83c34a8 = 1:  83c34a8 change
2:  f8c2d0b < -:  ------- change

+ : Adding the range-diff to the cover letter doesnt seem to help
+ git format-patch -1 --range-diff=v1...v2 --cover-letter v2
0000-cover-letter.patch
0001-change.patch
+ sed -n -e /^Range-diff:$/,$ p 0000-cover-letter.patch
Range-diff:
1:  83c34a8 = 1:  83c34a8 change
2:  f8c2d0b < -:  ------- change

SZEDER Gábor <szeder.dev@gmail.com> writes:

The other day I did something similar and ended up with

	git format-patch --range-diff=v1 v1..v2

Would it help not to use the three-dot form? From my reading of "git format-patch --range-diff=<previous>" description, it only needs to give a single range (i.e. previous side of series of commits) as the other range to be compared are by definition the patches you are producing, while v1...v2 syntax is to give two ranges with one option. So...

On Thu, Nov 12, 2020 at 12:57 PM Junio C Hamano <gitster@pobox.com> wrote:

Indeed. It's not clear (at least to me) how git-format-patch's --range-diff option should interpret a 3-dot range, thus the implementation explicitly supports only a single revision or a 2-dot range, and the documentation reflects this.

It turns out, as you discovered, that the implementation is a bit deficient in that it doesn't outright reject a 3-dot range, and instead gets tripped up by:

     int prev_is_range = !!strstr(prev, "..");

which accidentally also matches a 3-dot range, with the result that the 3-dot range gets passed into the lower-level range-diff machinery which then forwards the 3-dot range to git-log. The git-range-diff command, on the other hand, interprets a 3-dot range manually so such a range doesn't make it down to the lower-level range-diff machinery.

I haven't fully thought it through yet, but at this point, the best "fix" likely would be for `git format-patch --range-diff` to error out when it sees a 3-dot range. (Unless there is some other intuitive interpretation of a 3-dot range which escapes me.)

Eric Sunshine <sunshine@sunshineco.com> writes:

And possibly rename the option. Giving "--range-diff=<prev>" is not an instruction to run "git range-diff <prev>", so it is clear that the option is misnamed.

It probably should have been "--[no-]range-diff" boolean that controls if we add the range-diff from the previous, whose default may be affected by the user of the "-v$n" option, plus another option that gives where to find the "previous series", whose presence probably trigger "--range-diff" implicitly, or something like that.

And the option whose value we are having problem with is exactly that "--previous-series=<prev>" option.

On Thu, Nov 12, 2020 at 2:00 PM Junio C Hamano <gitster@pobox.com> wrote:

You may be right that, due to the name similarity, some people misinterpret --range-diff as providing one-to-one parity with the `git range-diff` command, but that was never the intention (as was discussed during review). Instead, the intention all along was only to make it easy to embed range-diffs in `git format-patch` output with a simple and concise invocation, and only for common cases. For instance, it is quite common for v2 to share a common base with v1, in which case `git format-patch -v2 --range-diff=v1 <base>` would be sufficient (when v2 is the checked-out branch). Anything more complex can be achieved by utilizing `git range-diff` directly; for instance: `git range-diff {complexity} >>0000-cover-letter.patch`

As discussed during review, I had considered more complex invocation possibilities for `git format-patch` but ended up rejecting them and opting instead for simplicity since `git range-diff` itself is a suitable escape hatch for accomplishing complex cases not handled by the intentionally simplistic --range-diff option. Thus, I'm hesitant to go in a direction which adds more complexity to the `git format-patch` invocation, thus making it more difficult to use range-diff with `git format-patch`.