Re: [PATCH 4/8] parse-options: mark unused "opt" parameter in callbacks
- From
Jeff King <peff@peff.net>
- Date
- Aug 31, 2023, 17:50 UTC
- Message-ID
- <20230831175018.GB3208283@coredump.intra.peff.net>
- In-Reply-To
- <xmqqil8vxjcd.fsf@gitster.g>
On Thu, Aug 31, 2023 at 09:33:22AM -0700, Junio C Hamano wrote:
Show 26 quoted lines
> > -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).
Show 9 quoted lines
> > -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