Re: [PATCH] range-diff: show submodule changes irrespective of diff.submodule

[Philippe Blain <levraiphilippeblain@gmail.com>]

Hi Dscho,

Le 2022-06-02 à 11:36, Johannes Schindelin a écrit :
> Hi Philippe,
> 
> On Mon, 30 May 2022, Philippe Blain via GitGitGadget wrote:
> 
>> From: Philippe Blain <levraiphilippeblain@gmail.com>
>>
>> After generating diffs for each range to be compared using a 'git log'
>> invocation, range-diff.c::read_patches looks for the "diff --git" header
>> in those diffs to recognize the beginning of a new change.
>>
>> In a project with submodules, and with 'diff.submodule=log' set in the
>> config, this header is missing for the diff of a changed submodule, so
>> any submodule changes are quietly ignored in the range-diff.
> 
> This means that we can go two ways here: either we explicitly disable
> `diff.submodule` for the invocation that is spawned from `range-diff`, or
> we allow it but then handle the diff header as expected.
> 
>>
>> When 'diff.submodule=diff' is set in the config, the "diff --git" header
>> is also missing for the submodule itself, but is shown for submodule
>> content changes, which can easily confuse 'git range-diff' and lead to
>> errors such as:
>>
>>     error: git apply: bad git-diff - inconsistent old filename on line 1
>>     error: could not parse git header 'diff --git path/to/submodule/and/some/file/within
>>     '
>>     error: could not parse log for '@{u}..@{1}'
>>
>> Force the submodule diff format to its default ("short") when invoking
>> 'git log' to generate the patches for each range, such that submodule
>> changes are always shown.
> 
> Full disclosure: I do not see much value in range-diffs in the presence of
> submodules. Nothing in the design of range-diffs is prepared for
> submodules.
> 
> But since `--submodules=short` does not change anything when running
> `range-diff` in repositories without submodules, I don't mind this change.
> 
>>
>> Note that the test must use '--creation-factor=100' to force the second
>> commit in the range not to be considered a complete rewrite.
> 
> Thank you for this considerate note!
> 
>>
>> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
>> ---
>>     range-diff: show submodule changes irrespective of diff.submodule
>>
>>     This fixes a bug that I reported last summer [1].
>>
>>     [1]
>>     https://lore.kernel.org/git/e469038c-d78c-cd4b-0214-7094746b9281@gmail.com/
>>
>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1244%2Fphil-blain%2Frange-diff-submodule-diff-v1
>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1244/phil-blain/range-diff-submodule-diff-v1
>> Pull-Request: https://github.com/gitgitgadget/git/pull/1244
>>
>>  range-diff.c          |  2 +-
>>  t/t3206-range-diff.sh | 44 +++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 45 insertions(+), 1 deletion(-)
>>
>> diff --git a/range-diff.c b/range-diff.c
>> index b72eb9fdbee..068bf214544 100644
>> --- a/range-diff.c
>> +++ b/range-diff.c
>> @@ -44,7 +44,7 @@ static int read_patches(const char *range, struct string_list *list,
>>
>>  	strvec_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges",
>>  		     "--reverse", "--date-order", "--decorate=no",
>> -		     "--no-prefix",
>> +		     "--no-prefix", "--submodule=short",
> 
> As I mentioned above, since this does not change anything in the intended
> scenarios (i.e. without submodules), I am fine with it.
> 
>>  		     /*
>>  		      * Choose indicators that are not used anywhere
>>  		      * else in diffs, but still look reasonable
>> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
>> index e30bc48a290..ac848c42536 100755
>> --- a/t/t3206-range-diff.sh
>> +++ b/t/t3206-range-diff.sh
>> @@ -772,4 +772,48 @@ test_expect_success '--left-only/--right-only' '
>>  	test_cmp expect actual
>>  '
>>
>> +test_expect_success 'submodule changes are shown irrespective of diff.submodule' '
>> +	git init sub-repo &&
>> +	test_commit -C sub-repo sub-first &&
>> +	sub_oid1=$(git -C sub-repo rev-parse HEAD) &&
>> +	test_commit -C sub-repo sub-second &&
>> +	sub_oid2=$(git -C sub-repo rev-parse HEAD) &&
>> +	test_commit -C sub-repo sub-third &&
>> +	sub_oid3=$(git -C sub-repo rev-parse HEAD) &&
>> +
>> +	git checkout -b main-sub topic &&
>> +	git submodule add ./sub-repo sub &&
>> +	git -C sub checkout --detach sub-first &&
>> +	git add sub &&
>> +	git commit -m "add sub" &&
> 
> Just a suggestion: use `git commit -m sub-first sub` instead (one `git`
> invocation instead of two).

OK, good idea. I'll tweak that.

> 
>> +	sup_oid1=$(git rev-parse --short HEAD) &&
>> +	git checkout -b topic-sub &&
>> +	git -C sub checkout sub-second &&
>> +	git add sub &&
>> +	git commit -m "change sub" &&
>> +	sup_oid2=$(git rev-parse --short HEAD) &&
>> +	git checkout -b modified-sub main-sub &&
> 
> Another suggestion: instead of naming the branches, use the `sup_oid*`
> variables directly.
> 

I think I like the branch names, they make the test closer to a 
"real-life" scenario (in my opinion). So I think I'll keep them,
since you write later in your reply that you do not mind that much.

>> +	git -C sub checkout sub-third &&
>> +	git add sub &&
>> +	git commit -m "change sub" &&
>> +	sup_oid3=$(git rev-parse --short HEAD) &&
>> +
>> +	test_config diff.submodule log &&
>> +	git range-diff --creation-factor=100 topic topic-sub modified-sub >actual &&
>> +	cat >expect <<-EOF &&
>> +	1:  $sup_oid1 = 1:  $sup_oid1 add sub
>> +	2:  $sup_oid2 ! 2:  $sup_oid3 change sub
>> +	    @@ Commit message
>> +	      ## sub ##
>> +	     @@
>> +	     -Subproject commit $sub_oid1
>> +	    -+Subproject commit $sub_oid2
>> +	    ++Subproject commit $sub_oid3
>> +	EOF
>> +	test_cmp expect actual &&
>> +	test_config diff.submodule diff &&
>> +	git range-diff --creation-factor=100 topic topic-sub modified-sub >actual &&
>> +	test_cmp expect actual
>> +'
> 
> This test case is very clear and concise, even without my suggested
> changes. Therefore, if you want to keep the patch as-is, I am fine with
> that, too.
> 
> Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> Thank you,

Thanks,

Philippe.