| Message ID | e469038c-d78c-cd4b-0214-7094746b9281@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | 'git range-diff' does not detect submodule changes if 'diff.submodule=log' | expand |
Hi Philippe, On Mon, 19 Jul 2021, Philippe Blain wrote: > I noticed that 'git range-diff' silently "drops" submodule changes if > 'diff.submodule=log' is set in the config. This is because the 'diff --git' > header is not shown in that case, and the code in range-diff.c::read_patches > expects that header to be found to detect changes. > > If 'diff.submodule' is instead set to 'diff', the range-diff outright errors > with > (at least in my case): > > error: git apply: bad git-diff - inconsistent old filename on line 1 > error: could not parse git header 'diff --git > error: path/to/submodule/and/some/file/within > ' > error: could not parse log for '@{u}..@{1}' > > > I think it would make sense to force '--submodule=short' for range-diff, > something like: > > diff --git a/range-diff.c b/range-diff.c > index 1a4471fe4c..d74b9c7a55 100644 > --- a/range-diff.c > +++ b/range-diff.c > @@ -54,7 +54,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 > > > What do you think ? Sure. I never thought that `range-diff` would be useful in the context of submodules. But then, I am an anti-fan of submodules anyway, so don't put too much stock into my opinion about anything submodule-related. > P.S. As an aside, > I'm not sure why you chose to skip the 'diff --git' header with > '--submodule=log' > (I did not search the list), but I think that in general 'git diff' and > friends, > it would be nice to be able to still see this header even with > --submodule=log... I don't remember the details. It might be that `git-tbdiff` did it that way, and I copied it without questioning. *clicketyclick* Yep, that seems plausible: https://github.com/trast/tbdiff/blob/047d1c79dfada57522a42f307cd4b0ddcb098934/git-tbdiff.py#L48 Ciao, Dscho
Hi Dscho, Le 2021-07-22 à 09:19, Johannes Schindelin a écrit : > Hi Philippe, > > On Mon, 19 Jul 2021, Philippe Blain wrote: > >> I noticed that 'git range-diff' silently "drops" submodule changes if >> 'diff.submodule=log' is set in the config. This is because the 'diff --git' >> header is not shown in that case, and the code in range-diff.c::read_patches >> expects that header to be found to detect changes. >> >> If 'diff.submodule' is instead set to 'diff', the range-diff outright errors >> with >> (at least in my case): >> >> error: git apply: bad git-diff - inconsistent old filename on line 1 >> error: could not parse git header 'diff --git >> error: path/to/submodule/and/some/file/within >> ' >> error: could not parse log for '@{u}..@{1}' >> >> >> I think it would make sense to force '--submodule=short' for range-diff, >> something like: >> >> diff --git a/range-diff.c b/range-diff.c >> index 1a4471fe4c..d74b9c7a55 100644 >> --- a/range-diff.c >> +++ b/range-diff.c >> @@ -54,7 +54,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 >> >> >> What do you think ? > > Sure. I never thought that `range-diff` would be useful in the context of > submodules. But then, I am an anti-fan of submodules anyway, so don't put > too much stock into my opinion about anything submodule-related. OK. I'll try to find the time to send a patch for that. > >> P.S. As an aside, >> I'm not sure why you chose to skip the 'diff --git' header with >> '--submodule=log' >> (I did not search the list), but I think that in general 'git diff' and >> friends, >> it would be nice to be able to still see this header even with >> --submodule=log... > > I don't remember the details. It might be that `git-tbdiff` did it that > way, and I copied it without questioning. > > *clicketyclick* > > Yep, that seems plausible: > https://github.com/trast/tbdiff/blob/047d1c79dfada57522a42f307cd4b0ddcb098934/git-tbdiff.py#L48 I meant when you implemented '--submodule=log' back in the days, why did you choose to skip the 'diff --git' header. In fact I dig up the thread and keeping that header is something Jens Lehmann actually suggested [1] (near the end of the message). I for one would find it useful; it's easy to miss submodules changes among a sea of other changes if the submodule is updated with just a few commits... yet another thing to add to my list :) Cheers, Philippe. [1] https://lore.kernel.org/git/4AC9D6EB.8090002@web.de/
diff --git a/range-diff.c b/range-diff.c index 1a4471fe4c..d74b9c7a55 100644 --- a/range-diff.c +++ b/range-diff.c @@ -54,7 +54,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
Hi Johannes, I noticed that 'git range-diff' silently "drops" submodule changes if 'diff.submodule=log' is set in the config. This is because the 'diff --git' header is not shown in that case, and the code in range-diff.c::read_patches expects that header to be found to detect changes. If 'diff.submodule' is instead set to 'diff', the range-diff outright errors with (at least in my case): 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}' I think it would make sense to force '--submodule=short' for range-diff, something like: What do you think ? Thanks, Philippe. P.S. As an aside, I'm not sure why you chose to skip the 'diff --git' header with '--submodule=log' (I did not search the list), but I think that in general 'git diff' and friends, it would be nice to be able to still see this header even with --submodule=log...