Re: [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.

From: Junio C Hamano <gitster@pobox.com>
Date: May 1, 2017, 01:01 UTC
Message-ID: <xmqqpoftjspa.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170428080620.cbnreh5invwpasxr@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 21 quoted lines

> On Thu, Apr 27, 2017 at 04:50:37PM -0400, Marc Branchaud wrote:
>
>> Subject: [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.
>>
>> This makes the commands respect diff configuration options, such as
>> indentHeuristic.
>> 
>> Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
>
> I think it would be helpful to future readers to explain what is going
> on here. I.e., the bit about calling diff_setup_done(), which copies the
> globals into the diff struct.
>
>...
>
>> +	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
>>  	init_revisions(&rev, prefix);
>>  	gitmodules_config();
>> -	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
>>  	rev.abbrev = 0;
>>  	precompose_argv(argc, argv);

This somehow made me worried at the first glance, but the matches what is done by the Porcelain "git diff". In other words, it was a bug that the original called init_revisions() before doing the diff configuration, and it does not have much to do with "now let's have them honor indentHeuristic". Even if we wanted to keep the indent heuristic in the ui-config (not basic) section of the diff tweak knobs, we should be applying this patch as a bugfix.

Show 9 quoted lines

> It's somewhat odd to me that diff_files uses a rev_info struct at all.
> It doesn't traverse at all, and doesn't respect most of the options.
> There's a half-hearted attempt to reject some obviously bogus cases, but
> most of the options are just silently ignored.
>
> I think it's mostly a historical wart (especially around the fact that
> some diff options like combine_merges are in rev_info, which they
> probably should not be). Anyway, none of that is your problem, and is
> way outside the scope of this patch.

Yeah. The underlying diff machinery was built to be easily usable by revision traversal and that is probably the reason why we have this entanglement that we probably could (and maybe we would want to) detangle. Surely not a theme of this topic.

Thanks.