{"thread":{"id":"51815","subject":"[RFC PATCH 0/1] for-each-ref: Add '--no-symbolic' option","startedAt":"2019-09-07T21:36:15Z","lastAt":"2019-09-09T18:08:45Z","messageCount":13,"participants":["Eric Freese","Taylor Blau","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"382027","messageId":"20190907213646.21231-1-ericdfreese@gmail.com","threadId":"51815","inReplyTo":null,"subject":"[RFC PATCH 0/1] for-each-ref: Add '--no-symbolic' option","fromName":"Eric Freese","fromEmail":"ericdfreese@gmail.com","sentAt":"2019-09-07T21:36:45Z","receivedAt":"2019-09-07T21:36:15Z","isPatch":true,"sender":{"key":"ericdfreese@gmail.com","avatar":"https://gravatar.com/avatar/9a57c69e58170c5f4b54051c9df2c2fdb51ac99e53f847868317ba042302bfda?d=mp&s=160"},"body":"Hi,\n\nI was recently using git-for-each-ref in a script to get a list of\nremote refs that pointed at a particular commit so that they could\nautomatically be updated and pushed back to that remote. This fails when\nit comes across refs/remotes/origin/HEAD, which is a symbolic link.\n\nI was able to solve the problem with:\n\n```\ngit for-each-ref ... --format=\"%(refname) %(symref)\" | grep \" $\"\n```\n\nBut that feels a little clumsy to me. I would have expected there to be\na flag like `--no-symbolic` that would exclude symbolic refs from the\noutput. So I went ahead and added it :)\n\nI could forsee this option also being added to git-branch and git-tag,\nbut decided to keep it to git-for-each-ref to test the waters before\ninvesting any further time into it.\n\nCheers\n\nEric Freese (1):\n for-each-ref: add '--no-symbolic' option\n\n Documentation/git-for-each-ref.txt | 3 +++\n builtin/for-each-ref.c | 4 +++-\n ref-filter.c | 4 ++++\n ref-filter.h | 3 ++-\n t/t6302-for-each-ref-filter.sh | 6 ++++++\n 5 files changed, 18 insertions(+), 2 deletions(-)\n\n-- \n2.23.0\n\n"},{"id":"382028","messageId":"20190907213646.21231-2-ericdfreese@gmail.com","threadId":"51815","inReplyTo":"20190907213646.21231-1-ericdfreese@gmail.com","subject":"[RFC PATCH 1/1] for-each-ref: add '--no-symbolic' option","fromName":"Eric Freese","fromEmail":"ericdfreese@gmail.com","sentAt":"2019-09-07T21:36:46Z","receivedAt":"2019-09-07T21:36:22Z","isPatch":true,"sender":{"key":"ericdfreese@gmail.com","avatar":"https://gravatar.com/avatar/9a57c69e58170c5f4b54051c9df2c2fdb51ac99e53f847868317ba042302bfda?d=mp&s=160"},"body":"Using the new flag will omit symbolic refs from the output.\n\nWithout this flag, it is possible to get this behavior by using the\n`%(symref)` formatting field name and piping output through grep to\ninclude only those refs that do not output a value for `%(symref)`, but\nhaving this flag is more elegant and intention revealing.\n---\n Documentation/git-for-each-ref.txt | 3 +++\n builtin/for-each-ref.c | 4 +++-\n ref-filter.c | 4 ++++\n ref-filter.h | 3 ++-\n t/t6302-for-each-ref-filter.sh | 6 ++++++\n 5 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 6dcd39f6f6..be19111510 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -95,6 +95,9 @@ OPTIONS\n --ignore-case::\n \tSorting and filtering refs are case insensitive.\n \n+--no-symbolic::\n+\tOnly list refs that are not symbolic.\n+\n FIELD NAMES\n -----------\n \ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 465153e853..b71ab2f135 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -18,7 +18,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n \tstruct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n-\tint maxcount = 0, icase = 0;\n+\tint maxcount = 0, icase = 0, nosym = 0;\n \tstruct ref_array array;\n \tstruct ref_filter filter;\n \tstruct ref_format format = REF_FORMAT_INIT;\n@@ -46,6 +46,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \t\tOPT_CONTAINS(&filter.with_commit, N_(\"print only refs which contain the commit\")),\n \t\tOPT_NO_CONTAINS(&filter.no_commit, N_(\"print only refs which don't contain the commit\")),\n \t\tOPT_BOOL(0, \"ignore-case\", &icase, N_(\"sorting and filtering are case insensitive\")),\n+\t\tOPT_BOOL(0, \"no-symbolic\", &nosym, N_(\"exclude symbolic refs\")),\n \t\tOPT_END(),\n \t};\n \n@@ -72,6 +73,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \t\tsorting = ref_default_sorting();\n \tsorting->ignore_case = icase;\n \tfilter.ignore_case = icase;\n+\tfilter.no_symbolic = nosym;\n \n \tfilter.name_patterns = argv;\n \tfilter.match_as_path = 1;\ndiff --git a/ref-filter.c b/ref-filter.c\nindex f27cfc8c3e..01beb279dc 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2093,6 +2093,10 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n \t\treturn 0;\n \t}\n \n+\tif (filter->no_symbolic && flag & REF_ISSYMREF) {\n+\t\treturn 0;\n+\t}\n+\n \t/* Obtain the current ref kind from filter_ref_kind() and ignore unwanted refs. */\n \tkind = filter_ref_kind(filter, refname);\n \tif (!(kind & filter->kind))\ndiff --git a/ref-filter.h b/ref-filter.h\nindex f1dcff4c6e..23e0d01a33 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -65,7 +65,8 @@ struct ref_filter {\n \tunsigned int with_commit_tag_algo : 1,\n \t\tmatch_as_path : 1,\n \t\tignore_case : 1,\n-\t\tdetached : 1;\n+\t\tdetached : 1,\n+\t\tno_symbolic : 1;\n \tunsigned int kind,\n \t\tlines;\n \tint abbrev,\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 35408d53fd..ab9c00fff4 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -454,4 +454,10 @@ test_expect_success 'validate worktree atom' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'filtering with --no-symbolic' '\n+\tgit symbolic-ref refs/symbolic refs/heads/master &&\n+\tgit for-each-ref --format=\"%(refname)\" --no-symbolic >actual &&\n+\ttest_must_fail grep refs/symbolic actual\n+'\n+\n test_done\n-- \n2.23.0\n\n"},{"id":"382029","messageId":"20190907231617.GA36491@syl.local","threadId":"51815","inReplyTo":"20190907213646.21231-1-ericdfreese@gmail.com","subject":"Re: [RFC PATCH 0/1] for-each-ref: Add '--no-symbolic' option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-09-07T23:16:17Z","receivedAt":"2019-09-07T23:16:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Eric,\n\nOn Sat, Sep 07, 2019 at 03:36:45PM -0600, Eric Freese wrote:\n> Hi,\n>\n> I was recently using git-for-each-ref in a script to get a list of\n> remote refs that pointed at a particular commit so that they could\n> automatically be updated and pushed back to that remote. This fails when\n> it comes across refs/remotes/origin/HEAD, which is a symbolic link.\n>\n> I was able to solve the problem with:\n>\n> ```\n> git for-each-ref ... --format=\"%(refname) %(symref)\" | grep \" $\"\n> ```\n>\n> But that feels a little clumsy to me. I would have expected there to be\n> a flag like `--no-symbolic` that would exclude symbolic refs from the\n> output. So I went ahead and added it :)\n\nIndeed, that does seem a little clumsy to me, too. It's kind of a great\nhack, but I could foresee functionality like '--no-symbolic' to be\nuseful to avoid hackery like this.\n\n> I could forsee this option also being added to git-branch and git-tag,\n> but decided to keep it to git-for-each-ref to test the waters before\n> investing any further time into it.\n\nYep. It's good that your patches are mainly in the 'ref-filter.[ch]'\ncode, so it would be easy to wire up new flags in both the branch+tag\nbuiltins, too, since they use the same ref-filter API.\n\n> Cheers\n>\n> Eric Freese (1):\n> for-each-ref: add '--no-symbolic' option\n>\n> Documentation/git-for-each-ref.txt | 3 +++\n> builtin/for-each-ref.c | 4 +++-\n> ref-filter.c | 4 ++++\n> ref-filter.h | 3 ++-\n> t/t6302-for-each-ref-filter.sh | 6 ++++++\n> 5 files changed, 18 insertions(+), 2 deletions(-)\n>\n> --\n> 2.23.0\n\nThanks,\nTaylor\n"},{"id":"382030","messageId":"20190907232821.GA42449@syl.local","threadId":"51815","inReplyTo":"20190907213646.21231-2-ericdfreese@gmail.com","subject":"Re: [RFC PATCH 1/1] for-each-ref: add '--no-symbolic' option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-09-07T23:28:21Z","receivedAt":"2019-09-07T23:38:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Eric,\n\nOn Sat, Sep 07, 2019 at 03:36:46PM -0600, Eric Freese wrote:\n> Using the new flag will omit symbolic refs from the output.\n>\n> Without this flag, it is possible to get this behavior by using the\n> `%(symref)` formatting field name and piping output through grep to\n> include only those refs that do not output a value for `%(symref)`, but\n> having this flag is more elegant and intention revealing.\n\nPlease include a 'Signed-off-by' trailer from yourself in this commit.\nYou can write one yourself, but instead use 'git commit -s' when\ncommitting.\n\n> ---\n> Documentation/git-for-each-ref.txt | 3 +++\n> builtin/for-each-ref.c | 4 +++-\n> ref-filter.c | 4 ++++\n> ref-filter.h | 3 ++-\n> t/t6302-for-each-ref-filter.sh | 6 ++++++\n> 5 files changed, 18 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n> index 6dcd39f6f6..be19111510 100644\n> --- a/Documentation/git-for-each-ref.txt\n> +++ b/Documentation/git-for-each-ref.txt\n> @@ -95,6 +95,9 @@ OPTIONS\n> --ignore-case::\n> \tSorting and filtering refs are case insensitive.\n>\n> +--no-symbolic::\n> +\tOnly list refs that are not symbolic.\n> +\n> FIELD NAMES\n> -----------\n>\n> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> index 465153e853..b71ab2f135 100644\n> --- a/builtin/for-each-ref.c\n> +++ b/builtin/for-each-ref.c\n> @@ -18,7 +18,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> {\n> \tint i;\n> \tstruct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n> -\tint maxcount = 0, icase = 0;\n> +\tint maxcount = 0, icase = 0, nosym = 0;\n\nI'm a little timid around a single-bit value prefixed with 'no'. Maybe\nit would be clearer as:\n\n int sym = 1;\n\n...instead of the negated form. Of course, the rest of the readers of\nthis variable would have to be updated, too, but involving fewer\nnegations seems like it would only improve the clarity.\n\n> \tstruct ref_array array;\n> \tstruct ref_filter filter;\n> \tstruct ref_format format = REF_FORMAT_INIT;\n> @@ -46,6 +46,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> \t\tOPT_CONTAINS(&filter.with_commit, N_(\"print only refs which contain the commit\")),\n> \t\tOPT_NO_CONTAINS(&filter.no_commit, N_(\"print only refs which don't contain the commit\")),\n> \t\tOPT_BOOL(0, \"ignore-case\", &icase, N_(\"sorting and filtering are case insensitive\")),\n> +\t\tOPT_BOOL(0, \"no-symbolic\", &nosym, N_(\"exclude symbolic refs\")),\n\nI'm a little bit weary of this \"no-\" prefixing, but this time for a\ndifferent reason. The parse-options API has a built-in \"negative\" option\nto pair with each 'OPT_BOOL'. So, for example, in the line above yours,\nit is actually the case that you can run 'git for-each-ref\n--ignore-case' just as much as you can run 'git for-each-ref\n--no-ignore-case'.\n\nApplying your patch shows that I can write the following:\n\n $ git for-each-ref --no-no-symbolic\n\nWhich is likely unintended. There are two ways that you can go about\nthis:\n\n - write this as 'OPT_BOOL(0, \"symbolic\", ...)', to make sure that the\n option you _actually_ want is the one generated by the complement,\n not the complement's complement.\n\n - or, pass 'PARSE_OPT_NONEG' to tell the parse-options API not to\n generate the complement in the first place.\n\nI'd lean towards the former, at the peril of having a meaningless\ndefault option (i.e., passing '--symbolic' is wasteful, since\n'--symbolic' is implied by its default value). But, there are certainly\ncounter-examples, which you can find with\n\n $ git grep 'OPT_BOOL(.*\\\"no-'\n\nSo, I'd be curious to hear about the thoughts of others.\n\n> \t\tOPT_END(),\n> \t};\n>\n> @@ -72,6 +73,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> \t\tsorting = ref_default_sorting();\n> \tsorting->ignore_case = icase;\n> \tfilter.ignore_case = icase;\n> +\tfilter.no_symbolic = nosym;\n>\n> \tfilter.name_patterns = argv;\n> \tfilter.match_as_path = 1;\n> diff --git a/ref-filter.c b/ref-filter.c\n> index f27cfc8c3e..01beb279dc 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -2093,6 +2093,10 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n> \t\treturn 0;\n> \t}\n>\n> +\tif (filter->no_symbolic && flag & REF_ISSYMREF) {\n> +\t\treturn 0;\n> +\t}\n> +\n\nIn Documentation/CodingGuidelines, we avoid braces around single-line\nif-statements.\n\n> \t/* Obtain the current ref kind from filter_ref_kind() and ignore unwanted refs. */\n> \tkind = filter_ref_kind(filter, refname);\n> \tif (!(kind & filter->kind))\n> diff --git a/ref-filter.h b/ref-filter.h\n> index f1dcff4c6e..23e0d01a33 100644\n> --- a/ref-filter.h\n> +++ b/ref-filter.h\n> @@ -65,7 +65,8 @@ struct ref_filter {\n> \tunsigned int with_commit_tag_algo : 1,\n> \t\tmatch_as_path : 1,\n> \t\tignore_case : 1,\n> -\t\tdetached : 1;\n> +\t\tdetached : 1,\n> +\t\tno_symbolic : 1;\n> \tunsigned int kind,\n> \t\tlines;\n> \tint abbrev,\n> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n> index 35408d53fd..ab9c00fff4 100755\n> --- a/t/t6302-for-each-ref-filter.sh\n> +++ b/t/t6302-for-each-ref-filter.sh\n> @@ -454,4 +454,10 @@ test_expect_success 'validate worktree atom' '\n> \ttest_cmp expect actual\n> '\n>\n> +test_expect_success 'filtering with --no-symbolic' '\n> +\tgit symbolic-ref refs/symbolic refs/heads/master &&\n> +\tgit for-each-ref --format=\"%(refname)\" --no-symbolic >actual &&\n> +\ttest_must_fail grep refs/symbolic actual\n\nThis style is uncommon, and instead it is preferred to write:\n\n ! grep refs/symbolic actual\n\nSince 'test_must_fail' also catches segfaults, whereas '!' does not.\nSince we'd like this test to fail if/when grep segfaults, use of the\nlater is preferred here.\n\n> +'\n> +\n> test_done\n> --\n> 2.23.0\n\nThanks,\nTaylor\n"},{"id":"382031","messageId":"20190907233730.GA43328@syl.local","threadId":"51815","inReplyTo":"20190907213646.21231-2-ericdfreese@gmail.com","subject":"Re: [RFC PATCH 1/1] for-each-ref: add '--no-symbolic' option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-09-07T23:37:30Z","receivedAt":"2019-09-07T23:38:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Eric,\n\nOn Sat, Sep 07, 2019 at 03:36:46PM -0600, Eric Freese wrote:\n> Using the new flag will omit symbolic refs from the output.\n>\n> Without this flag, it is possible to get this behavior by using the\n> `%(symref)` formatting field name and piping output through grep to\n> include only those refs that do not output a value for `%(symref)`, but\n> having this flag is more elegant and intention revealing.\n\nPlease include a 'Signed-off-by' trailer from yourself in this commit.\nYou can write one yourself, but instead use 'git commit -s' when\ncommitting.\nI'm a little timid around a single-bit value prefixed with 'no'. Maybe\nit would be clearer as:\n\n int sym = 1;\n\n...instead of the negated form. Of course, the rest of the readers of\nthis variable would have to be updated, too, but involving fewer\nnegations seems like it would only improve the clarity.\n\n> struct ref_array array;\n> struct ref_filter filter;\n> struct ref_format format = REF_FORMAT_INIT;\n> @@ -46,6 +46,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> OPT_CONTAINS(&filter.with_commit, N_(\"print only refs which contain the commit\")),\n> OPT_NO_CONTAINS(&filter.no_commit, N_(\"print only refs which don't contain the commit\")),\n> OPT_BOOL(0, \"ignore-case\", &icase, N_(\"sorting and filtering are case insensitive\")),\n> + OPT_BOOL(0, \"no-symbolic\", &nosym, N_(\"exclude symbolic refs\")),\n\nI'm a little bit weary of this \"no-\" prefixing, but this time for a\ndifferent reason. The parse-options API has a built-in \"negative\" option\nto pair with each 'OPT_BOOL'. So, for example, in the line above yours,\nit is actually the case that you can run 'git for-each-ref\n--ignore-case' just as much as you can run 'git for-each-ref\n--no-ignore-case'.\n\nApplying your patch shows that I can write the following:\n\n $ git for-each-ref --no-no-symbolic\n\nWhich is likely unintended. There are two ways that you can go about\nthis:\n\n - write this as 'OPT_BOOL(0, \"symbolic\", ...)', to make sure that the\n option you _actually_ want is the one generated by the complement,\n not the complement's complement.\n\n - or, pass 'PARSE_OPT_NONEG' to tell the parse-options API not to\n generate the complement in the first place.\n\nI'd lean towards the former, at the peril of having a meaningless\ndefault option (i.e., passing '--symbolic' is wasteful, since\n'--symbolic' is implied by its default value). But, there are certainly\ncounter-examples, which you can find with\n\n $ git grep 'OPT_BOOL(.*\\\"no-'\n\nSo, I'd be curious to hear about the thoughts of others.\n\n> OPT_END(),\n> };\n>\n> @@ -72,6 +73,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> sorting = ref_default_sorting();\n> sorting->ignore_case = icase;\n> filter.ignore_case = icase;\n> + filter.no_symbolic = nosym;\n>\n> filter.name_patterns = argv;\n> filter.match_as_path = 1;\n> diff --git a/ref-filter.c b/ref-filter.c\n> index f27cfc8c3e..01beb279dc 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -2093,6 +2093,10 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n> return 0;\n> }\n>\n> + if (filter->no_symbolic && flag & REF_ISSYMREF) {\n> + return 0;\n> + }\n> +\n\nIn Documentation/CodingGuidelines, we avoid braces around single-line\nif-statements.\nThis style is uncommon, and instead it is preferred to write:\n\n ! grep refs/symbolic actual\n\nSince 'test_must_fail' also catches segfaults, whereas '!' does not.\nSince we'd like this test to fail if/when grep segfaults, use of the\nlater is preferred here.\n\n> +'\n> +\n> test_done\n> --\n> 2.23.0\n\nThanks,\nTaylor\n"},{"id":"382032","messageId":"20190907233949.GA43535@syl.local","threadId":"51815","inReplyTo":"20190907232821.GA42449@syl.local","subject":"Re: [RFC PATCH 1/1] for-each-ref: add '--no-symbolic' option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-09-07T23:39:49Z","receivedAt":"2019-09-07T23:39:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Sep 07, 2019 at 07:28:21PM -0400, Taylor Blau wrote:\n> ...\n\nOh, great. I was pretty sure that vger accidentally ate my last mail,\nbut I guess not. Sorry for the re-send.\n\nThanks,\nTaylor\n"},{"id":"382039","messageId":"20190908094427.GA15641@sigill.intra.peff.net","threadId":"51815","inReplyTo":"20190907232821.GA42449@syl.local","subject":"Re: [RFC PATCH 1/1] for-each-ref: add '--no-symbolic' option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-08T09:44:27Z","receivedAt":"2019-09-08T09:46:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 07, 2019 at 07:28:21PM -0400, Taylor Blau wrote:\n\n> > diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> > index 465153e853..b71ab2f135 100644\n> > --- a/builtin/for-each-ref.c\n> > +++ b/builtin/for-each-ref.c\n> > @@ -18,7 +18,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> > {\n> > \tint i;\n> > \tstruct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n> > -\tint maxcount = 0, icase = 0;\n> > +\tint maxcount = 0, icase = 0, nosym = 0;\n> \n> I'm a little timid around a single-bit value prefixed with 'no'. Maybe\n> it would be clearer as:\n> \n> int sym = 1;\n> \n> ...instead of the negated form. Of course, the rest of the readers of\n> this variable would have to be updated, too, but involving fewer\n> negations seems like it would only improve the clarity.\n\nIn general, it is nice to avoid negations in the variable names, so you\ndon't end up with double negations like \"if (!no_symrefs)\". However, it\ncan be tricky because of zero-initialization. Ultimately this flag ends\nup in \"struct ref_filter\", and the default initialization of that struct\nwould set it to false, which is what we want.\n\nSo either we end up flipping the variable when we assign to the struct,\nor we have to start providing a more detailed initializer for the\nstruct (and switch all callsites to start using it).\n\n> Applying your patch shows that I can write the following:\n> \n> $ git for-each-ref --no-no-symbolic\n> \n> Which is likely unintended. There are two ways that you can go about\n> this:\n> \n> - write this as 'OPT_BOOL(0, \"symbolic\", ...)', to make sure that the\n> option you _actually_ want is the one generated by the complement,\n> not the complement's complement.\n> \n> - or, pass 'PARSE_OPT_NONEG' to tell the parse-options API not to\n> generate the complement in the first place.\n> \n> I'd lean towards the former, at the peril of having a meaningless\n> default option (i.e., passing '--symbolic' is wasteful, since\n> '--symbolic' is implied by its default value). But, there are certainly\n> counter-examples, which you can find with\n> \n> $ git grep 'OPT_BOOL(.*\\\"no-'\n> \n> So, I'd be curious to hear about the thoughts of others.\n\nThis has been discussed off and on over the years, which is why you'll\nfind both types of solution in the code base. :) Less important than\nmaking sure \"--no-no-symbolic\" does not work is making sure that\n\"--symbolic\" _does_ work. You're right that it's usually pointless, but\nit can be used to countermand an earlier --no-symbolic.\n\nAnd in fact this _does_ work due to 0f1930c587 (parse-options: allow\npositivation of options starting, with no-, 2012-02-25).\n\nAnother advantage of using the \"no-\" form is that the \"-h\" usage message\nwill show it rather than its positive counterpart.\n\nThe \"no-no\" form is a weird artifact of the parsing. It's probably not\n_hurting_ anybody, since you don't see it unless you try to use it.\nThere was patch a while ago to disallow these:\n\n https://public-inbox.org/git/20170419090820.20279-1-jacob.e.keller@intel.com/\n\nbut ultimately we never did anything. If we do want to disable these, I\nthink I'd rather do it centrally like that, rather than having to\nspecify NONEG in the individual options.\n\n> > +test_expect_success 'filtering with --no-symbolic' '\n> > +\tgit symbolic-ref refs/symbolic refs/heads/master &&\n> > +\tgit for-each-ref --format=\"%(refname)\" --no-symbolic >actual &&\n> > +\ttest_must_fail grep refs/symbolic actual\n> \n> This style is uncommon, and instead it is preferred to write:\n> \n> ! grep refs/symbolic actual\n> \n> Since 'test_must_fail' also catches segfaults, whereas '!' does not.\n> Since we'd like this test to fail if/when grep segfaults, use of the\n> later is preferred here.\n\nYour suggestion is correct, but I'm not sure I follow the reasoning.\nUsing \"!\" would cause us _not_ to notice a segfault, whereas\ntest_must_fail would. For non-git tools we prefer to use the simpler\n\"!\", because should be able to safely assume they do not segfault or die\nby signal.\n\n-Peff\n"},{"id":"382040","messageId":"20190908100543.GB15641@sigill.intra.peff.net","threadId":"51815","inReplyTo":"20190907213646.21231-2-ericdfreese@gmail.com","subject":"Re: [RFC PATCH 1/1] for-each-ref: add '--no-symbolic' option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-08T10:05:44Z","receivedAt":"2019-09-08T10:05:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 07, 2019 at 03:36:46PM -0600, Eric Freese wrote:\n\n> Using the new flag will omit symbolic refs from the output.\n> \n> Without this flag, it is possible to get this behavior by using the\n> `%(symref)` formatting field name and piping output through grep to\n> include only those refs that do not output a value for `%(symref)`, but\n> having this flag is more elegant and intention revealing.\n\nThis seems like a reasonable addition to me. As you note, it can be done\nby post-processing the result, but it can get a little clumsy.\n\nJust letting my mind wander for a moment, a more general solution would\nbe providing some mechanism by which you could ask for-each-ref to omit\nresults based on their expansions. E.g., you might want to ask for only\nrefs for which %(taggerdate) is non-empty (i.e., just the annotated\ntags).\n\nBut that opens a can of worms. How do we negate it? You want to omit\nnon-empty %(symref) here, but my tag example would only show non-empty\n%(taggerdate). Howe do we combine options (\"non-symrefs that point to\ncommits\")? How do we express more complex logic, like string matching or\nnumeric comparison?\n\nIt's a slippery slope that ends in embedding a Turing complete language. :)\nAnd you can already do these complex things in the language of your\nchoice by post-processing the output. The one advantage to doing it\ninside for-each-ref is that we may save some work by filtering early.\nThis probably matters more for other tools like cat-file (where I might\nwant to say \"print all the blobs\", but I can't do so without a\nmulti-process pipeline that accesses each object twice), but we'd\neventually like to unify the formatting languages of all tools.\n\nSo in my mind there's an endgame we'd like to eventually reach where\nthe option added by your patch isn't needed anymore. But we're a long\nway from that. And it's not entirely clear where we'd draw the line\nanyway. So in the meantime, this seems like a useful thing, and it\nwouldn't be a burden to carry it even if we eventually added\n\"--omit=%(symref)\" or something.\n\n> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n> index 6dcd39f6f6..be19111510 100644\n> --- a/Documentation/git-for-each-ref.txt\n> +++ b/Documentation/git-for-each-ref.txt\n> @@ -95,6 +95,9 @@ OPTIONS\n> --ignore-case::\n> \tSorting and filtering refs are case insensitive.\n> \n> +--no-symbolic::\n> +\tOnly list refs that are not symbolic.\n> +\n\nI wonder if \"symbolic\" might be too vague here. Would \"--no-symref\" be a\nbetter name?\n\nI responded separately to Taylor's questions about negation, but one\nthing I didn't bring up there: another option to avoid it is to specify\nthe action positively. E.g., \"--ignore-symrefs\" or similar. I could go\neither way on that.\n\n> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> index 465153e853..b71ab2f135 100644\n> --- a/builtin/for-each-ref.c\n> +++ b/builtin/for-each-ref.c\n> @@ -18,7 +18,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> {\n> \tint i;\n> \tstruct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n> -\tint maxcount = 0, icase = 0;\n> +\tint maxcount = 0, icase = 0, nosym = 0;\n\nLikewise, maybe worth writing out \"symref\" here (and in the struct\noption)? But...\n\n> @@ -46,6 +46,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> \t\tOPT_CONTAINS(&filter.with_commit, N_(\"print only refs which contain the commit\")),\n> \t\tOPT_NO_CONTAINS(&filter.no_commit, N_(\"print only refs which don't contain the commit\")),\n> \t\tOPT_BOOL(0, \"ignore-case\", &icase, N_(\"sorting and filtering are case insensitive\")),\n> +\t\tOPT_BOOL(0, \"no-symbolic\", &nosym, N_(\"exclude symbolic refs\")),\n\nI think you could just write directly to &filter.no_symbolic here,\ndropping nosym entirely. But I guess ignore-case directly above set a\nbad example. ;)\n\n> diff --git a/ref-filter.c b/ref-filter.c\n> index f27cfc8c3e..01beb279dc 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -2093,6 +2093,10 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n> \t\treturn 0;\n> \t}\n> \n> +\tif (filter->no_symbolic && flag & REF_ISSYMREF) {\n> +\t\treturn 0;\n> +\t}\n> +\n\nOoh, here we avoid the double negation of \"if (!filter->no_symbolic)\"\nwith an early return. :) (Nothing wrong with that, just an amusing\noutcome given the discussion elsewhere in the thread).\n\n> [...]\n\nThe rest of the patch looked OK, aside from other review comments. The\nwhole thing looks cleanly done. I don't have a strong opinion on adding\nthe feature to branch/tag. We've only ever promised that HEAD and\nrefs/remotes/.../HEAD work as symrefs, though of course they do work\nanywhere in the namespace, and I imagine people have taken advantage of\nthat. So I don't know how useful the option would be in other contexts.\n\n-Peff\n"},{"id":"382047","messageId":"xmqq8sqyn6ek.fsf@gitster-ct.c.googlers.com","threadId":"51815","inReplyTo":"20190908100543.GB15641@sigill.intra.peff.net","subject":"Re: [RFC PATCH 1/1] for-each-ref: add '--no-symbolic' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-08T15:40:03Z","receivedAt":"2019-09-08T15:40:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So in my mind there's an endgame we'd like to eventually reach where\n> the option added by your patch isn't needed anymore. But we're a long\n> way from that. And it's not entirely clear where we'd draw the line\n> anyway.\n\nAll true and very good \"thinking out loud\".\n\n> So in the meantime, this seems like a useful thing, and it\n> wouldn't be a burden to carry it even if we eventually added\n> \"--omit=%(symref)\" or something.\n\nI would draw the line above this particular change, though.\n\n>> +--no-symbolic::\n>> +\tOnly list refs that are not symbolic.\n>> +\n>\n> I wonder if \"symbolic\" might be too vague here. Would \"--no-symref\" be a\n> better name?\n\nDefinitely. Another disturbing thing is the design mistake that\nmade this a bool. If it is useful to filter out symrefs, it would\nequally be useful to only show symrefs. --[no-]symbolic-refs does\nnot capture the tristate-ness, and that is why I do not think this\nis good enough in the meantime, without causing us trouble carrying\nforward.\n\nTHanks.\n"},{"id":"382056","messageId":"xmqq4l1mmn8i.fsf@gitster-ct.c.googlers.com","threadId":"51815","inReplyTo":"xmqq8sqyn6ek.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH 1/1] for-each-ref: add '--no-symbolic' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-08T22:34:05Z","receivedAt":"2019-09-08T22:46:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> So in my mind there's an endgame we'd like to eventually reach where\n>> the option added by your patch isn't needed anymore. But we're a long\n>> way from that. And it's not entirely clear where we'd draw the line\n>> anyway.\n>\n> All true and very good \"thinking out loud\".\n>\n>> So in the meantime, this seems like a useful thing, and it\n>> wouldn't be a burden to carry it even if we eventually added\n>> \"--omit=%(symref)\" or something.\n\nWe can introduce two new options, e.g. --(include|exclude)=\nwhere\n\n * Without either, there is no filtering based on the placeholder\n expansion;\n\n * With only --include, only the refs for which expands to\n non-empty are included.\n\n * With only --exclude, the refs for which expands to\n non-empty are excluded (and everything else included).\n\n * With both --include and --exclude, only the refs for which the\n for --include expands to non-empty are eligible to be\n included, but among them, the ones for which the for\n --exclude expands to non-empty are discarded.\n\nThen \"--exclude=%(symref)\" would be Eric's --no-symref,\n\"--include=%(symref)\" would be the opposite (i.e. \"show only\nsymbolic refs\"), etc.\n\nI guess with \"%(if)...%(then)...%(else)...%(end)\" you might be able\nto do either one of --include/--exclude without supporting the\nother, e.g. \"--include='%(if)%(symref)%(then)%(else)not a\nsymref%(end)\" would be usable as \"I do not want to see symrefs\" in a\nsystem that supports only \"--include\" without \"--exclude\".\n\n"},{"id":"382060","messageId":"CAAikoA+MvORDJ6y=VSk_1s6v7110UeOfU2X2G7=9ChV+m+COxQ@mail.gmail.com","threadId":"51815","inReplyTo":"xmqq4l1mmn8i.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH 1/1] for-each-ref: add '--no-symbolic' option","fromName":"Eric Freese","fromEmail":"ericdfreese@gmail.com","sentAt":"2019-09-09T04:01:33Z","receivedAt":"2019-09-09T04:01:02Z","isPatch":true,"sender":{"key":"ericdfreese@gmail.com","avatar":"https://gravatar.com/avatar/9a57c69e58170c5f4b54051c9df2c2fdb51ac99e53f847868317ba042302bfda?d=mp&s=160"},"body":"On Sun, Sep 9, 2019 at 4:34 PM Junio C Hamano wrote:\n\n> I guess with \"%(if)...%(then)...%(else)...%(end)\" you might be able\n> to do either one of --include/--exclude without supporting the\n> other, e.g. \"--include='%(if)%(symref)%(then)%(else)not a\n> symref%(end)\" would be usable as \"I do not want to see symrefs\" in a\n> system that supports only \"--include\" without \"--exclude\".\n\nThis has made me realize that I can get the behavior I need by using `%(if)`.\n\nExclude symrefs:\n \"%(if)%(symref)%(then)%(else)%(refname)%(end)\"\n\nOnly symrefs:\n \"%(if)%(symref)%(then)%(refname)%(end)\"\n\nHowever, this still prints an empty line for each ref that does not match the\ncondition. This can be cleaned up by piping through `grep .`, but what would\nyou think of adding a new optional flag to git-for-each-ref to prevent it from\nprinting empty expanded format strings?\n"},{"id":"382062","messageId":"20190909125753.GA8684@sigill.intra.peff.net","threadId":"51815","inReplyTo":"CAAikoA+MvORDJ6y=VSk_1s6v7110UeOfU2X2G7=9ChV+m+COxQ@mail.gmail.com","subject":"Re: [RFC PATCH 1/1] for-each-ref: add '--no-symbolic' option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-09T12:57:53Z","receivedAt":"2019-09-09T12:57:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 08, 2019 at 10:01:33PM -0600, Eric Freese wrote:\n\n> On Sun, Sep 9, 2019 at 4:34 PM Junio C Hamano wrote:\n> \n> > I guess with \"%(if)...%(then)...%(else)...%(end)\" you might be able\n> > to do either one of --include/--exclude without supporting the\n> > other, e.g. \"--include='%(if)%(symref)%(then)%(else)not a\n> > symref%(end)\" would be usable as \"I do not want to see symrefs\" in a\n> > system that supports only \"--include\" without \"--exclude\".\n> \n> This has made me realize that I can get the behavior I need by using `%(if)`.\n> \n> Exclude symrefs:\n> \"%(if)%(symref)%(then)%(else)%(refname)%(end)\"\n> \n> Only symrefs:\n> \"%(if)%(symref)%(then)%(refname)%(end)\"\n> \n> However, this still prints an empty line for each ref that does not match the\n> condition. This can be cleaned up by piping through `grep .`, but what would\n> you think of adding a new optional flag to git-for-each-ref to prevent it from\n> printing empty expanded format strings?\n\nYeah, I think that is a much nicer direction, in that it covers many\nmore cases. I'm tempted to suggest it should even be the default, since\nthe blank lines are mostly useless (by definition they carry no\ninformation except the single bit of \"there was a ref that didn't have\nany output for your format\").\n\nMy only reservation is that for-each-ref is plumbing, and it's\n_possible_ somebody is counting up the blank lines as part of some\nworkflow (say, counting up tags and non-tags). It seems kind of\nunlikely, though. Much more likely is having an output format that has\nsome non-blank elements and some blank, but the proposal wouldn't change\nthe behavior there at all.\n\n-Peff\n"},{"id":"382081","messageId":"xmqq1rwpl4uz.fsf@gitster-ct.c.googlers.com","threadId":"51815","inReplyTo":"CAAikoA+MvORDJ6y=VSk_1s6v7110UeOfU2X2G7=9ChV+m+COxQ@mail.gmail.com","subject":"Re: [RFC PATCH 1/1] for-each-ref: add '--no-symbolic' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-09T18:08:36Z","receivedAt":"2019-09-09T18:08:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Freese writes:\n\n> However, this still prints an empty line for each ref that does not match the\n> condition. This can be cleaned up by piping through `grep .`, but what would\n> you think of adding a new optional flag to git-for-each-ref to prevent it from\n> printing empty expanded format strings?\n\nOffhand I do not think of a reason why anybody wants to give a\nformat that results in a total blank, so it might not even need to\nbe an optional behaviour, but certainly a new option that triggers\nsuch a behaviour would be a safe thing to add.\n"}]}