On 03/20, Jeff King wrote: > On Tue, Mar 19, 2019 at 11:18:26PM +0000, Thomas Gummerer wrote: > > > From a quick search I couldn't find where 'git diff' actually parses > > the '-v' argument, but I wonder if we should actually disallow it, in > > case we want to use it for something else in the future? It's not > > documented anywhere in the docs either. > > It's a bit interesting, actually. git-diff uses setup_revisions() to > parse its arguments, which picks up any diff options, as well as parsing > the revs and pathspecs. > > But it also means we accept any random revision options. So nonsense > like: > > git diff --ancestry-path HEAD^ HEAD > > is accepted, even though nobody ever looks at the flags set by parsing > that option. > > And "-v" is mostly in the same boat. It works more or less like --pretty > (try rev-list with and without it), and does nothing for an endpoint > diff. What's a little interesting, though, is that it was originally > added as a diff-tree option in the very early days, via cee99d2257 > (diff-tree: add "verbose header" mode, 2005-05-06). And the reason there > is that "diff-tree --stdin" filled a "log"-like role; it didn't traverse > the commits itself, but it was responsible for showing them. Thanks for the explanation! > Most of that is historical curiosity, but I think the takeaways are: > > - we probably should use a less bizarre option to demonstrate this bug > (Junio suggested --patience, which makes perfect sense to me) Yep, that should make the test a bit easier to understand. Will do that in v2. > - we may want to teach the "diff" porcelain not to accept useless > revision options. I suspect it may be a bit tricky, just because of > the way the code takes advantage of setup_revisions. It would also > be nice if "diff-tree" in non-stdin mode could do the same, but I > suspect that is even trickier (we do not even know whether we are in > --stdin mode or not until we've fed the options to setup_revisions). > So I'd guess this is not really worth the effort it would take. > > - "-v" is a real thing; we should consider either documenting it or > deprecating it. Makes sense, I don't have a strong opinion on which way we should go here, but I'll leave that separately from the bug fix either way.
