On Wed, Mar 20, 2019 at 10:49:55PM +0000, Thomas Gummerer wrote: > 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. > 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
