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.
> ---
> Documentation/git-for-each-ref.txt | 3 +++
> builtin/for-each-ref.c | 4 +++-
> ref-filter.c | 4 ++++
> ref-filter.h | 3 ++-
> t/t6302-for-each-ref-filter.sh | 6 ++++++
> 5 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 6dcd39f6f6..be19111510 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -95,6 +95,9 @@ OPTIONS
> --ignore-case::
> Sorting and filtering refs are case insensitive.
>
> +--no-symbolic::
> + Only list refs that are not symbolic.
> +
> FIELD NAMES
> -----------
>
> 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.
> 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.
> /* Obtain the current ref kind from filter_ref_kind() and ignore unwanted refs. */
> kind = filter_ref_kind(filter, refname);
> if (!(kind & filter->kind))
> diff --git a/ref-filter.h b/ref-filter.h
> index f1dcff4c6e..23e0d01a33 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -65,7 +65,8 @@ struct ref_filter {
> unsigned int with_commit_tag_algo : 1,
> match_as_path : 1,
> ignore_case : 1,
> - detached : 1;
> + detached : 1,
> + no_symbolic : 1;
> unsigned int kind,
> lines;
> int abbrev,
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index 35408d53fd..ab9c00fff4 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -454,4 +454,10 @@ test_expect_success 'validate worktree atom' '
> test_cmp expect actual
> '
>
> +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.
> +'
> +
> test_done
> --
> 2.23.0
Thanks,
Taylor
