Re: [REGRESSION ps/stash-in-c] git stash show -v

From: Jeff King <peff@peff.net>
Date: Mar 20, 2019, 05:04 UTC
Message-ID: <20190320050449.GA6401@sigill.intra.peff.net>
In-Reply-To: <20190319231826.GB32487@hank.intra.tgummerer.com>

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.

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)

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

-Peff