Re: [PATCH v2] stash: setup default diff output format if necessary

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 22, 2019, 03:25 UTC
Message-ID: <xmqqtvfvr3oi.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190321095103.GB2894@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 10 quoted lines

>> I think this also deserves some explanation of what didn't change,
>> especially after what I said in [*1*].  We're still not using the
>> 'diff_opt_parse()' option parser, as it doesn't understand '-v' for
>> example.  'setup_revisions()' understands that, but 'diff_opt_parse()'
>> doesn't, so we'd still have a change in behaviour at least there.
>> After discovering that I gave up on that approach.
>
> Yeah, I think this would get into the "why are we even looking at
> revision options" thing, which really is a separate topic. Let's do the
> minimal fix here.

I tend to agree. Limiting ourselves to diff options would be a good direction going forward in the longer term, but let's fix the regression so that we can push the base topic to 'master' before we need to know the shape of the next release.

Show 6 quoted lines

> There is a "set_default" callback that was added by 6c374008b1
> (diff_opt: track whether flags have been set explicitly, 2013-05-10),
> but it looks like it was never actually used. I think the theory is that
> cases like this could do their tweaking in such a callback. But I think
> it makes sense to follow the lead of builtin/diff.c for the immediate
> fix, and we can look at using set_default as a separate topic.

I agree with the conclusion. I wouldn't be surprised if this is one of the things that was once used but left behind when the last caller disappeared, though.