Re: [RFC PATCH 1/1] for-each-ref: add '--no-symbolic' option

[Jeff King <peff@peff.net>]

On Sat, Sep 07, 2019 at 07:28:21PM -0400, Taylor Blau wrote:

> > diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> > index 465153e853..b71ab2f135 100644
> > --- a/builtin/for-each-ref.c
> > +++ b/builtin/for-each-ref.c
> > @@ -18,7 +18,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
> >  {
> >  	int i;
> >  	struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
> > -	int maxcount = 0, icase = 0;
> > +	int maxcount = 0, icase = 0, nosym = 0;
> 
> I'm a little timid around a single-bit value prefixed with 'no'. Maybe
> it would be clearer as:
> 
>   int sym = 1;
> 
> ...instead of the negated form. Of course, the rest of the readers of
> this variable would have to be updated, too, but involving fewer
> negations seems like it would only improve the clarity.

In general, it is nice to avoid negations in the variable names, so you
don't end up with double negations like "if (!no_symrefs)". However, it
can be tricky because of zero-initialization. Ultimately this flag ends
up in "struct ref_filter", and the default initialization of that struct
would set it to false, which is what we want.

So either we end up flipping the variable when we assign to the struct,
or we have to start providing a more detailed initializer for the
struct (and switch all callsites to start using it).

> Applying your patch shows that I can write the following:
> 
>   $ git for-each-ref --no-no-symbolic
> 
> Which is likely unintended. There are two ways that you can go about
> this:
> 
>   - write this as 'OPT_BOOL(0, "symbolic", ...)', to make sure that the
>     option you _actually_ want is the one generated by the complement,
>     not the complement's complement.
> 
>   - or, pass 'PARSE_OPT_NONEG' to tell the parse-options API not to
>     generate the complement in the first place.
> 
> I'd lean towards the former, at the peril of having a meaningless
> default option (i.e., passing '--symbolic' is wasteful, since
> '--symbolic' is implied by its default value). But, there are certainly
> counter-examples, which you can find with
> 
>   $ git grep 'OPT_BOOL(.*\"no-'
> 
> So, I'd be curious to hear about the thoughts of others.

This has been discussed off and on over the years, which is why you'll
find both types of solution in the code base. :) Less important than
making sure "--no-no-symbolic" does not work is making sure that
"--symbolic" _does_ work. You're right that it's usually pointless, but
it can be used to countermand an earlier --no-symbolic.

And in fact this _does_ work due to 0f1930c587 (parse-options: allow
positivation of options starting, with no-, 2012-02-25).

Another advantage of using the "no-" form is that the "-h" usage message
will show it rather than its positive counterpart.

The "no-no" form is a weird artifact of the parsing. It's probably not
_hurting_ anybody, since you don't see it unless you try to use it.
There was patch a while ago to disallow these:

  https://public-inbox.org/git/20170419090820.20279-1-jacob.e.keller@intel.com/

but ultimately we never did anything. If we do want to disable these, I
think I'd rather do it centrally like that, rather than having to
specify NONEG in the individual options.

> > +test_expect_success 'filtering with --no-symbolic' '
> > +	git symbolic-ref refs/symbolic refs/heads/master &&
> > +	git for-each-ref --format="%(refname)" --no-symbolic >actual &&
> > +	test_must_fail grep refs/symbolic actual
> 
> This style is uncommon, and instead it is preferred to write:
> 
>   ! grep refs/symbolic actual
> 
> Since 'test_must_fail' also catches segfaults, whereas '!' does not.
> Since we'd like this test to fail if/when grep segfaults, use of the
> later is preferred here.

Your suggestion is correct, but I'm not sure I follow the reasoning.
Using "!" would cause us _not_ to notice a segfault, whereas
test_must_fail would. For non-git tools we prefer to use the simpler
"!", because should be able to safely assume they do not segfault or die
by signal.

-Peff