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

From: Jeff King <peff@peff.net>
Date: Mar 21, 2019, 09:51 UTC
Message-ID: <20190321095103.GB2894@sigill.intra.peff.net>
In-Reply-To: <20190320224955.GE32487@hank.intra.tgummerer.com>

On Wed, Mar 20, 2019 at 10:49:55PM +0000, Thomas Gummerer wrote:

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

Show 5 quoted lines

> The other thing that was pointed out is the 'diff_setup_done()' call
> here.  'diff_setup_done()' is already called inside of
> 'setup_revisions()', so we don't need to do it again, unless we change
> the output format, which is what we are doing here.  In fact this is
> the same way it's implemented in 'builtin/diff.c'.

That makes me wonder if any part of diff_setup_done() could be surprised at being called twice. I guess not, if nobody has reported a bug in git-diff. :)

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.

-Peff