git format-patch --range-diff bug?

git format-patch --range-diff bug?

[SZEDER Gábor]

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
-- 
2.18.0.584.g40ce41604d

Re: git format-patch --range-diff bug?

[Junio C Hamano]

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

> 	: 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 &&

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

Re: git format-patch --range-diff bug?

[Eric Sunshine]

On Thu, Nov 12, 2020 at 12:57 PM Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> >       git range-diff v1...v2 &&
> >       git format-patch -1 --range-diff=v1...v2 v2 &&
>
> 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...

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

Re: git format-patch --range-diff bug?

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

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

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.

Re: git format-patch --range-diff bug?

[Eric Sunshine]

On Thu, Nov 12, 2020 at 2:00 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > [...] the best "fix" likely would be for `git format-patch --range-diff` to
> > error out when it sees a 3-dot range.
>
> 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.

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