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

[Taylor Blau <me@ttaylorr.com>]

Hi Eric,

On Sat, Sep 07, 2019 at 03:36:46PM -0600, Eric Freese wrote:
> Using the new flag will omit symbolic refs from the output.
>
> Without this flag, it is possible to get this behavior by using the
> `%(symref)` formatting field name and piping output through grep to
> include only those refs that do not output a value for `%(symref)`, but
> having this flag is more elegant and intention revealing.

Please include a 'Signed-off-by' trailer from yourself in this commit.
You can write one yourself, but instead use 'git commit -s' when
committing.
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.

>       struct ref_array array;
>       struct ref_filter filter;
>       struct ref_format format = REF_FORMAT_INIT;
> @@ -46,6 +46,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>               OPT_CONTAINS(&filter.with_commit, N_("print only refs which contain the commit")),
>               OPT_NO_CONTAINS(&filter.no_commit, N_("print only refs which don't contain the commit")),
>               OPT_BOOL(0, "ignore-case", &icase, N_("sorting and filtering are case insensitive")),
> +             OPT_BOOL(0, "no-symbolic", &nosym, N_("exclude symbolic refs")),

I'm a little bit weary of this "no-" prefixing, but this time for a
different reason. The parse-options API has a built-in "negative" option
to pair with each 'OPT_BOOL'. So, for example, in the line above yours,
it is actually the case that you can run 'git for-each-ref
--ignore-case' just as much as you can run 'git for-each-ref
--no-ignore-case'.

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.

>               OPT_END(),
>       };
>
> @@ -72,6 +73,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>               sorting = ref_default_sorting();
>       sorting->ignore_case = icase;
>       filter.ignore_case = icase;
> +     filter.no_symbolic = nosym;
>
>       filter.name_patterns = argv;
>       filter.match_as_path = 1;
> diff --git a/ref-filter.c b/ref-filter.c
> index f27cfc8c3e..01beb279dc 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -2093,6 +2093,10 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>               return 0;
>       }
>
> +     if (filter->no_symbolic && flag & REF_ISSYMREF) {
> +             return 0;
> +     }
> +

In Documentation/CodingGuidelines, we avoid braces around single-line
if-statements.
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.

> +'
> +
>  test_done
> --
> 2.23.0

Thanks,
Taylor