On Thu, Aug 31, 2023 at 09:33:22AM -0700, Junio C Hamano wrote:
> > -static int option_parse_stage(const struct option *opt,
> > +static int option_parse_stage(const struct option *opt UNUSED,
> > const char *arg, int unset)
> > {
> > BUG_ON_OPT_NEG(unset);
>
> I suspect that the original is buggy; when given
>
> $ git checkout-index --stage=all --stage=1 path
>
> the first option turns --temp on, but the second one does not turn
> it off. For now I think being bug-to-bug compatible and annotating
> the opt as UNUSED is good, but as a follow-up, we could make the
> caller:
>
> (1) point &checkout_stage with opt->value;
>
> (2) make to_tempfile to tristate <unspecified, false, true> by
> initializing it to -1;
>
> (3) adjust to_tempfile that is still <unspecified> after
> parse_options() returns, according to the value in
> checkout_stage.
>
> and then this can follow the "opt->value points at the variable that
> is affected" pattern.
Yeah, I think that would work, with one extra bit:
(4) complain when (!to_tempfile && checkout_stage == CHECKOUT_ALL)
I do think it would be better to fix separately, but maybe if I'm
re-rolling I can do it as an early patch in the series (it is not much
different than the "xopts" fix in scope).
> > -static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
> > +static int parse_refmap_arg(const struct option *opt UNUSED,
> > + const char *arg, int unset)
> > {
> > BUG_ON_OPT_NEG(unset);
>
> Can't this just point opt->value at the global &refmap? Obviously
> not a huge deal, as we could have taken the "annotate as UNUSED"
> approach for all the functions in [3/8].
Hmm, yeah. I think I looked at the abstract refspec_append() here and
assumed that it might be touching other variables. But it's not. It's
operating purely on the &refspec we pass it (and even though it uses
ALLOC_GROW, the "nr" and "alloc" are both contained in the struct). So
yeah, it really should have been converted in patch 3.
I think that is probably worth a re-roll.
-Peff
