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

From: Philippe Blain <levraiphilippeblain@gmail.com>
Date: Jun 6, 2022, 20:33 UTC
Message-ID: <ed907ed2-ee9c-789f-0f25-0a1f5b58714f@gmail.com>
In-Reply-To: <xmqqwndznf0z.fsf@gitster.g>

Hi Junio,

Le 2022-06-02 à 13:36, Junio C Hamano a écrit :

Show 25 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
>>> 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.
> 
> IOW, "I wrote it for the purpose of doing X, I do not care those who
> have been using it for doing Y, I am OK with changing behaviour on
> them".
> 
> Philippe, do you have a good guess on other users and workflows that
> may benefit from the current behaviour?  I suspect in the longer term
> this might have to become configurable, and I am having a hard time
> judging if (1) a temporary regression (to them) is acceptable or (2)
> the new feature to also show submodule changes is not urgent enough
> that it may be better to make it configurable from day one, instead
> of using a different hardcoded and only setting like this patch does.
>

Just to be clear: the "out of the box" behaviour (i.e. nothing in the config) is correct, i.e. submodule changes are detected and shown by 'git range-diff'.

It's only if someone has 'diff.submodule={log,diff}' in their config that submodule changes are quietly ignored (log) or might crash 'git range-diff' (diff). So I do not think of any user or workflow that benefit from the current behaviour, no. If you have diff.submodule={log,diff} set in your config, it's most probably because you work in projects that involve submodules and you do care about submodule changes. So having these changes "hidden" by range-diff (or having range-diff crash) just because the output format of 'git -c diff.submodule={log,diff} log' does not use a 'diff --git' header for submodules is really not expected. So I do not think we need to make that configurable. I think hardcoding '--submodule=short' is an easy fix and a good first step in making 'git range-diff' more useful for submodule users.

Thanks,

Philippe.