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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, May 30 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.
>
> 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.
>
> Note that the test must use '--creation-factor=100' to force the second
> commit in the range not to be considered a complete rewrite.
>
> 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(-)

Thanks for picking this up again, and nice to have a test on this
iteration!

> 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",
>  		     /*
>  		      * 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" &&
> +	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 &&
> +	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
> +'
> +

I'd find this much easier to follow if this were a two-part where we do
most of this test code in the 1st commit, and assert the current
(failing) behavior with a test_expect_failure.

Then this commit would narrowly be the bugfix itself.

I also see that the --creation-factor=100 isn't necessary and seems
somewhat orthagonal, i.e. we'd like to test this *without* that option
and see how we behave, i.e. we'll emit the "full replacement".

Why not compare the output without --creation-factor=100, and then just
have another --creation-factor=100 test to show what we emit if we "look
into" those commits and diff their contents?