Re: [PATCH] range-diff: show submodule changes irrespective of diff.submodule
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Jun 2, 2022, 15:36 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2206021724240.349@tvgsbejvaqbjf.bet>
- In-Reply-To
- <pull.1244.git.1653916145441.gitgitgadget@gmail.com>
Hi Philippe,
On Mon, 30 May 2022, Philippe Blain via GitGitGadget wrote:
Show 9 quoted lines
> 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.
Show 14 quoted lines
>
> 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!
Show 28 quoted lines
> > 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.
Show 25 quoted lines
> /* > * 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).
Show 7 quoted lines
> + 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.
Show 22 quoted lines
> + 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, Dscho
Show 7 quoted lines
> + > test_done > > base-commit: 7a3eb286977746bc09a5de7682df0e5a7085e17c > -- > gitgitgadget >