[RFC PATCH 0/1] for-each-ref: Add '--no-symbolic' option

[RFC PATCH 0/1] for-each-ref: Add '--no-symbolic' option

[Eric Freese]

Hi,

I was recently using git-for-each-ref in a script to get a list of
remote refs that pointed at a particular commit so that they could
automatically be updated and pushed back to that remote. This fails when
it comes across refs/remotes/origin/HEAD, which is a symbolic link.

I was able to solve the problem with:

```
git for-each-ref ... --format="%(refname) %(symref)" | grep " $"
```

But that feels a little clumsy to me. I would have expected there to be
a flag like `--no-symbolic` that would exclude symbolic refs from the
output. So I went ahead and added it :)

I could forsee this option also being added to git-branch and git-tag,
but decided to keep it to git-for-each-ref to test the waters before
investing any further time into it.

Cheers

Eric Freese (1):
  for-each-ref: add '--no-symbolic' option

 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(-)

-- 
2.23.0

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

[Eric Freese]

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.
---
 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;
 	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")),
 		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;
+	}
+
 	/* 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
+'
+
 test_done
-- 
2.23.0

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

[Taylor Blau]

Hi Eric,

On Sat, Sep 07, 2019 at 03:36:45PM -0600, Eric Freese wrote:
> Hi,
>
> I was recently using git-for-each-ref in a script to get a list of
> remote refs that pointed at a particular commit so that they could
> automatically be updated and pushed back to that remote. This fails when
> it comes across refs/remotes/origin/HEAD, which is a symbolic link.
>
> I was able to solve the problem with:
>
> ```
> git for-each-ref ... --format="%(refname) %(symref)" | grep " $"
> ```
>
> But that feels a little clumsy to me. I would have expected there to be
> a flag like `--no-symbolic` that would exclude symbolic refs from the
> output. So I went ahead and added it :)

Indeed, that does seem a little clumsy to me, too. It's kind of a great
hack, but I could foresee functionality like '--no-symbolic' to be
useful to avoid hackery like this.

> I could forsee this option also being added to git-branch and git-tag,
> but decided to keep it to git-for-each-ref to test the waters before
> investing any further time into it.

Yep. It's good that your patches are mainly in the 'ref-filter.[ch]'
code, so it would be easy to wire up new flags in both the branch+tag
builtins, too, since they use the same ref-filter API.

> Cheers
>
> Eric Freese (1):
>   for-each-ref: add '--no-symbolic' option
>
>  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(-)
>
> --
> 2.23.0

Thanks,
Taylor

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

[Taylor Blau]

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

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

[Taylor Blau]

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

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

[Taylor Blau]

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

Oh, great. I was pretty sure that vger accidentally ate my last mail,
but I guess not. Sorry for the re-send.

Thanks,
Taylor

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

[Jeff King]

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

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

[Jeff King]

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.

This seems like a reasonable addition to me. As you note, it can be done
by post-processing the result, but it can get a little clumsy.

Just letting my mind wander for a moment, a more general solution would
be providing some mechanism by which you could ask for-each-ref to omit
results based on their expansions. E.g., you might want to ask for only
refs for which %(taggerdate) is non-empty (i.e., just the annotated
tags).

But that opens a can of worms. How do we negate it? You want to omit
non-empty %(symref) here, but my tag example would only show non-empty
%(taggerdate). Howe do we combine options ("non-symrefs that point to
commits")? How do we express more complex logic, like string matching or
numeric comparison?

It's a slippery slope that ends in embedding a Turing complete language. :)
And you can already do these complex things in the language of your
choice by post-processing the output. The one advantage to doing it
inside for-each-ref is that we may save some work by filtering early.
This probably matters more for other tools like cat-file (where I might
want to say "print all the blobs", but I can't do so without a
multi-process pipeline that accesses each object twice), but we'd
eventually like to unify the formatting languages of all tools.

So in my mind there's an endgame we'd like to eventually reach where
the option added by your patch isn't needed anymore. But we're a long
way from that. And it's not entirely clear where we'd draw the line
anyway. So in the meantime, this seems like a useful thing, and it
wouldn't be a burden to carry it even if we eventually added
"--omit=%(symref)" or something.

> 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.
> +

I wonder if "symbolic" might be too vague here. Would "--no-symref" be a
better name?

I responded separately to Taylor's questions about negation, but one
thing I didn't bring up there: another option to avoid it is to specify
the action positively. E.g., "--ignore-symrefs" or similar. I could go
either way on that.

> 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;

Likewise, maybe worth writing out "symref" here (and in the struct
option)? But...

> @@ -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 think you could just write directly to &filter.no_symbolic here,
dropping nosym entirely. But I guess ignore-case directly above set a
bad example. ;)

> 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;
> +	}
> +

Ooh, here we avoid the double negation of "if (!filter->no_symbolic)"
with an early return. :) (Nothing wrong with that, just an amusing
outcome given the discussion elsewhere in the thread).

> [...]

The rest of the patch looked OK, aside from other review comments. The
whole thing looks cleanly done. I don't have a strong opinion on adding
the feature to branch/tag. We've only ever promised that HEAD and
refs/remotes/.../HEAD work as symrefs, though of course they do work
anywhere in the namespace, and I imagine people have taken advantage of
that. So I don't know how useful the option would be in other contexts.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So in my mind there's an endgame we'd like to eventually reach where
> the option added by your patch isn't needed anymore. But we're a long
> way from that. And it's not entirely clear where we'd draw the line
> anyway.

All true and very good "thinking out loud".

> So in the meantime, this seems like a useful thing, and it
> wouldn't be a burden to carry it even if we eventually added
> "--omit=%(symref)" or something.

I would draw the line above this particular change, though.

>> +--no-symbolic::
>> +	Only list refs that are not symbolic.
>> +
>
> I wonder if "symbolic" might be too vague here. Would "--no-symref" be a
> better name?

Definitely.  Another disturbing thing is the design mistake that
made this a bool.  If it is useful to filter out symrefs, it would
equally be useful to only show symrefs.  --[no-]symbolic-refs does
not capture the tristate-ness, and that is why I do not think this
is good enough in the meantime, without causing us trouble carrying
forward.

THanks.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> So in my mind there's an endgame we'd like to eventually reach where
>> the option added by your patch isn't needed anymore. But we're a long
>> way from that. And it's not entirely clear where we'd draw the line
>> anyway.
>
> All true and very good "thinking out loud".
>
>> So in the meantime, this seems like a useful thing, and it
>> wouldn't be a burden to carry it even if we eventually added
>> "--omit=%(symref)" or something.

We can introduce two new options, e.g. --(include|exclude)=<format>
where

 * Without either, there is no filtering based on the placeholder
   expansion;

 * With only --include, only the refs for which <format> expands to
   non-empty are included.

 * With only --exclude, the refs for which <format> expands to
   non-empty are excluded (and everything else included).

 * With both --include and --exclude, only the refs for which the
   <format> for --include expands to non-empty are eligible to be
   included, but among them, the ones for which the <format> for
   --exclude expands to non-empty are discarded.

Then "--exclude=%(symref)" would be Eric's --no-symref,
"--include=%(symref)" would be the opposite (i.e. "show only
symbolic refs"), etc.

I guess with "%(if)...%(then)...%(else)...%(end)" you might be able
to do either one of --include/--exclude without supporting the
other, e.g. "--include='%(if)%(symref)%(then)%(else)not a
symref%(end)" would be usable as "I do not want to see symrefs" in a
system that supports only "--include" without "--exclude".

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

[Eric Freese]

On Sun, Sep 9, 2019 at 4:34 PM Junio C Hamano <gitster@pobox.com> wrote:

> I guess with "%(if)...%(then)...%(else)...%(end)" you might be able
> to do either one of --include/--exclude without supporting the
> other, e.g. "--include='%(if)%(symref)%(then)%(else)not a
> symref%(end)" would be usable as "I do not want to see symrefs" in a
> system that supports only "--include" without "--exclude".

This has made me realize that I can get the behavior I need by using `%(if)`.

Exclude symrefs:
  "%(if)%(symref)%(then)%(else)%(refname)%(end)"

Only symrefs:
  "%(if)%(symref)%(then)%(refname)%(end)"

However, this still prints an empty line for each ref that does not match the
condition. This can be cleaned up by piping through `grep .`, but what would
you think of adding a new optional flag to git-for-each-ref to prevent it from
printing empty expanded format strings?

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

[Jeff King]

On Sun, Sep 08, 2019 at 10:01:33PM -0600, Eric Freese wrote:

> On Sun, Sep 9, 2019 at 4:34 PM Junio C Hamano <gitster@pobox.com> wrote:
> 
> > I guess with "%(if)...%(then)...%(else)...%(end)" you might be able
> > to do either one of --include/--exclude without supporting the
> > other, e.g. "--include='%(if)%(symref)%(then)%(else)not a
> > symref%(end)" would be usable as "I do not want to see symrefs" in a
> > system that supports only "--include" without "--exclude".
> 
> This has made me realize that I can get the behavior I need by using `%(if)`.
> 
> Exclude symrefs:
>   "%(if)%(symref)%(then)%(else)%(refname)%(end)"
> 
> Only symrefs:
>   "%(if)%(symref)%(then)%(refname)%(end)"
> 
> However, this still prints an empty line for each ref that does not match the
> condition. This can be cleaned up by piping through `grep .`, but what would
> you think of adding a new optional flag to git-for-each-ref to prevent it from
> printing empty expanded format strings?

Yeah, I think that is a much nicer direction, in that it covers many
more cases. I'm tempted to suggest it should even be the default, since
the blank lines are mostly useless (by definition they carry no
information except the single bit of "there was a ref that didn't have
any output for your format").

My only reservation is that for-each-ref is plumbing, and it's
_possible_ somebody is counting up the blank lines as part of some
workflow (say, counting up tags and non-tags). It seems kind of
unlikely, though. Much more likely is having an output format that has
some non-blank elements and some blank, but the proposal wouldn't change
the behavior there at all.

-Peff

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

[Junio C Hamano]

Eric Freese <ericdfreese@gmail.com> writes:

> However, this still prints an empty line for each ref that does not match the
> condition. This can be cleaned up by piping through `grep .`, but what would
> you think of adding a new optional flag to git-for-each-ref to prevent it from
> printing empty expanded format strings?

Offhand I do not think of a reason why anybody wants to give a
format that results in a total blank, so it might not even need to
be an optional behaviour, but certainly a new option that triggers
such a behaviour would be a safe thing to add.