git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* 'git range-diff' does not detect submodule changes if 'diff.submodule=log'
@ 2021-07-19 20:43 Philippe Blain
  2021-07-22 13:19 ` Johannes Schindelin
  0 siblings, 1 reply; 3+ messages in thread
From: Philippe Blain @ 2021-07-19 20:43 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Git mailing list

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:

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 ?

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: 'git range-diff' does not detect submodule changes if 'diff.submodule=log'
  2021-07-19 20:43 'git range-diff' does not detect submodule changes if 'diff.submodule=log' Philippe Blain
@ 2021-07-22 13:19 ` Johannes Schindelin
  2021-07-22 22:42   ` Philippe Blain
  0 siblings, 1 reply; 3+ messages in thread
From: Johannes Schindelin @ 2021-07-22 13:19 UTC (permalink / raw)
  To: Philippe Blain; +Cc: Git mailing list

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: 'git range-diff' does not detect submodule changes if 'diff.submodule=log'
  2021-07-22 13:19 ` Johannes Schindelin
@ 2021-07-22 22:42   ` Philippe Blain
  0 siblings, 0 replies; 3+ messages in thread
From: Philippe Blain @ 2021-07-22 22:42 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Git mailing list

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/

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-07-22 22:42 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-19 20:43 'git range-diff' does not detect submodule changes if 'diff.submodule=log' Philippe Blain
2021-07-22 13:19 ` Johannes Schindelin
2021-07-22 22:42   ` Philippe Blain

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git