Re: [PATCH 3/5] diff: add --default-prefix option

From: Jeff King <peff@peff.net>
Date: Mar 10, 2023, 09:44 UTC
Message-ID: <ZAr7+zW+pkOXoIfL@coredump.intra.peff.net>
In-Reply-To: <xmqq5yb9q42e.fsf@gitster.g>

On Thu, Mar 09, 2023 at 08:31:37AM -0800, Junio C Hamano wrote:

Show 10 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > This isn't strictly necessary for the series, but it seemed like a gap.
> > You can always do:
> >
> >   git -c diff.noprefix=false -c diff.mnemonicprefix=false ...
> >
> > but that's rather a mouthful.
> 
> or "git diff --src-prefix=a/ --dst-prefix=b/"

Doh. How did I write this whole patch series without remembering the existence of those options?

While it is not _quite_ the same thing to say "use prefixes a/ and b/" versus "countermand any config and use the default", it is close enough that I am tempted to say this patch should be scrapped. I mostly just wanted to have a way to counter format.noprefix, if we are going to endorse it as a concept (whether by adding it, or saying "no, respecting diff.noprefix is not a bug").

(If we do scrap it, I'd probably fold the extra tests into the previous commit, but using --src-prefix, etc).

Show 12 quoted lines

> > +static int diff_opt_default_prefix(const struct option *opt,
> > +				   const char *optarg, int unset)
> > +{
> > +	struct diff_options *options = opt->value;
> > +
> > +	BUG_ON_OPT_NEG(unset);
> > +	BUG_ON_OPT_ARG(optarg);
> 
> OK.  It is a bit unsatisfactory that we already said this does not
> take negative form or any argument in the option[] array, and still
> have to do this, but that is completely outside the topic of this
> series.

We don't strictly have to do it. It's a cross-check that the correct flags were set in the options struct, and serves as documentation both for the human and the compiler (via -Wunused-parameter) that yes, it really is correct to take "unset" and not look at it. We could just as easily mark unset with "UNUSED", but I consider the extra run-time check a bonus.

I do admit that in a one-off callback like this, it is not accomplishing much. It's much more useful for generic ones like parse_opt_commit(), that may be triggered from many places. I do wish there was a better way to make sure they matched at compile-time, but I can't think of one.

-Peff