On Thu, Mar 09, 2023 at 08:31:37AM -0800, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> 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).
> > +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
