{"thread":{"id":"62455","subject":"[PATCH] t6300: values containing ')' are broken in ref formats","startedAt":"2024-11-05T19:02:53Z","lastAt":"2024-11-08T18:12:32Z","messageCount":14,"participants":["Kousik Sanagavarapu","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"506673","messageId":"20241105190235.13502-1-five231003@gmail.com","threadId":"62455","inReplyTo":null,"subject":"[PATCH] t6300: values containing ')' are broken in ref formats","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2024-11-05T18:41:34Z","receivedAt":"2024-11-05T19:02:53Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"Document that values containing ')' in formats are not parsed correctly\nin ref-filter.\n\nThe problem here is that ref-filter, while parsing the format string,\nlooks for the first occurence of ')' and marks it to be the end of _that_\nparticular atom - which is obviously wrong in cases where the format is\nof type\n\n\tatom:key=value\n\nwhere \"value\" has ')' somewhere in it.\n\nHowever formats having a '(' instead in \"value\" will parse correctly\nbecause in a general format string we also mark start of the format by\nmaking note of '%(' instead of just '('.\n\nFor example, in\n\n\t%(if:equals=somere)f)%(refname:short)...\n\nthe string that ref-filter should compare against is \"somere)f\", although\nsince the parsing behavior in these cases is broken, we instead compare\nagainst \"somere\".\n\nWhile in\n\n\t%(if:equals=somere(f)%(refname:short)...\n\nref-filter rightly compares against \"somere(f\" as expected.\n\nAs a side note it should be mentioned that values containing ')' are\nlegit in %(refname) (and other atoms like %(upstream)).  Meaning\nthe parser wouldn't err out such values as they are legal, which is also\nconfirmed by - say the refname coming from\n\n\t$ git branch '1)branch'\n\nor a remote name coming from\n\n\t$ git remote add 'up)stream' 'some.url'\n\t$ git push --set-upstream 'up)stream' '1)branch'\n\nsince none of these fail.\n\nSigned-off-by: Kousik Sanagavarapu <five231003@gmail.com>\n---\n\nThis raises the question of what can be done to parse ')' in values of\nthe format correctly.  It seems to me like a clean solution would\ninvolve a huge refactoring involving a large portion of ref-filter but I\nmaybe wrong.\n\nSo this patch also hopes to open up discussion on not only solving this\nbug but also how in general ref-filter currently parses and formats\natoms and if it is the way in which we would like to do things in the\nfuture, which would in turn also be helpful in the long term goal of\nmerging both pretty and ref-filter.\n\nHere is also a simple script to demonstrate the difference between '('\nand ')' in values in formats - as described in the commit msg -\n\n#!/bin/sh\n\nrm -rf /tmp/atom-test-dir &&\n\n# create env\ngit init /tmp/atom-test-dir 1>/tmp/init-dump 2>>/tmp/init-dump &&\ncd /tmp/atom-test-dir &&\necho \"smtg\" >file &&\ngit add file &&\ngit commit -s -m \"initial revision\" >/tmp/commit-dump &&\n\n# using \"(\" in refname works good\necho \"test with refname as \\\"bran(ch\\\"\" &&\ngit branch \"bran(ch\" &&\nprintf \"bran(ch\\n\\n\" >expect &&\ngit for-each-ref --format=\"%(if:equals=bran(ch)%(refname:short)%(then)%(refname:short)%(end)\" refs/heads/ >actual &&\nif ! diff -u expect actual; then\n\techo \"\t\tactual is different from expect\"\nelse\n\techo \"\t\tactual is the same as expect\"\nfi\n\necho \"\" &&\n\n# using \")\" in refname will parse wrong in ref-filter code\necho \"test with refname as \\\"bran()ch\\\"\" &&\ngit branch \"bran()ch\" &&\nprintf \"bran()ch\\n\\n\\n\" >expect &&\ngit for-each-ref --format=\"%(if:equals=bran()ch)%(refname:short)%(then)%(refname:short)%(end)\" refs/heads/ >actual &&\nif ! diff -u expect actual; then\n\techo \"\t\tactual is different from expect\"\nelse\n\techo \"\t\tactual is the same as expect\"\nfi\n\n t/t6300-for-each-ref.sh | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex c39d4e7e9c..ce5c607193 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -2141,4 +2141,19 @@ test_expect_success GPG 'show lack of signature with custom format' '\n \ttest_cmp expect actual\n '\n \n+test_expect_failure 'format having values containing ) parse correctly' '\n+\tgit branch \"1)feat\" &&\n+\tcat >expect <<-\\EOF &&\n+\trefs/heads/1)feat\n+\tnot equals\n+\tnot equals\n+\tnot equals\n+\tnot equals\n+\tnot equals\n+\tEOF\n+\tgit for-each-ref --format=\"%(if:equals=1)feat)%(refname:short)%(then)%(refname)%(else)not equals%(end)\" \\\n+\t\trefs/heads/ >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.47.0.230.g0cf584699a\n\n"},{"id":"506691","messageId":"xmqqikt1qhwt.fsf@gitster.g","threadId":"62455","inReplyTo":"20241105190235.13502-1-five231003@gmail.com","subject":"Re: [PATCH] t6300: values containing ')' are broken in ref formats","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-06T01:18:10Z","receivedAt":"2024-11-06T01:18:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kousik Sanagavarapu <five231003@gmail.com> writes:\n\n> Document that values containing ')' in formats are not parsed correctly\n> in ref-filter.\n\nThe problem is probably lack of a way to quote such a closing\nparenthesis.\n\n> However formats having a '(' instead in \"value\" will parse correctly\n> because in a general format string we also mark start of the format by\n> making note of '%(' instead of just '('.\n\nSo if you wanted to have a two-char sequence '%(' in value, you'd\nsee a similar problem?  If so, it is not quite a \"bug\" or \"not\nparsed correctly\"---it is \"because there is no way to include\nclosing ')' in the value (e.g., by quoting), you cannot write such a\nstring in the value part\".\n\n> This raises the question of what can be done to parse ')' in values of\n> the format correctly.  It seems to me like a clean solution would\n> involve a huge refactoring involving a large portion of ref-filter but I\n> maybe wrong.\n\nYes, so I wouldn't even call the current behaviour \"bug\".  The\nlanguage is merely \"limited\" and the user cannot express certain\nvalues with it at all.\n\n\nHaving said that, I just tried this\n\n    $ git for-each-ref --format='%28%(refname)%29' refs/heads/master\n    (refs/heads/master)\n\nSo, if there is anything that needs \"fixing\", wouldn't it be\ndocumentation?\n\nIf I knew (or easily find out from \"git for-each-ref --help\") that\nhex escapes %XX can be used, I wouldn't have written any of what I\nsaid before \"Having said that\" in this response.\n\n"},{"id":"506693","messageId":"20241106022552.GA816908@coredump.intra.peff.net","threadId":"62455","inReplyTo":"xmqqikt1qhwt.fsf@gitster.g","subject":"Re: [PATCH] t6300: values containing ')' are broken in ref formats","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-06T02:25:52Z","receivedAt":"2024-11-06T02:25:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 05, 2024 at 05:18:10PM -0800, Junio C Hamano wrote:\n\n> > This raises the question of what can be done to parse ')' in values of\n> > the format correctly.  It seems to me like a clean solution would\n> > involve a huge refactoring involving a large portion of ref-filter but I\n> > maybe wrong.\n> \n> Yes, so I wouldn't even call the current behaviour \"bug\".  The\n> language is merely \"limited\" and the user cannot express certain\n> values with it at all.\n\nAgreed. I think we may have discussed this quoting problem before, but\nit's not usually a big deal because the set of likely values is quite\nlimited. Most of them are just keywords or numeric values. I _think_\nthat the equals/notequals parameters of %(if) are the only ones.\n\nWhich isn't to say we shouldn't make things better if we can. Just that\nI am not too surprised nobody has run into it before.\n\n> Having said that, I just tried this\n> \n>     $ git for-each-ref --format='%28%(refname)%29' refs/heads/master\n>     (refs/heads/master)\n> \n> So, if there is anything that needs \"fixing\", wouldn't it be\n> documentation?\n> \n> If I knew (or easily find out from \"git for-each-ref --help\") that\n> hex escapes %XX can be used, I wouldn't have written any of what I\n> said before \"Having said that\" in this response.\n\nI tried something similar, but I don't think it quite works for the case\nin question. Within %(if:equals=<foo>) we do not further expand the\n<foo> value (at least from my limited tests). And so something like:\n\n  git for-each-ref --format='%(if:equals=ref-with-%29)%(refname:short)...etc'\n\nwould never match \"ref-with-(\", but only a literal \"ref-with-%29\".\n\nI am tempted to say the solution is to expand that \"equals\" value, and\npossibly add some less-arcane version of the character (maybe \"%)\"?).\nBut it be a break in backwards compatibility if somebody is trying to\nmatch literal %-chars in their \"if\" block.\n\nAnother option: in the rest of the \"if\" design we tried to keep\narbitrary text outside of the parentheses. So you could imagine a syntax\nlike:\n\n  %(if:equals)ref-with-)%(foo)%(refname:short)%(then)...%(end)\n\nwhere %(foo) is some placeholder that separate the two arguments to the\n\"equals\". In sane languages that is a space or a comma, but I'm not sure\nthat works here. We have %(end) which would otherwise be a syntax error\nhere, but it feels word. I dunno. The whole language is kind of\nhideously verbose. I feel sorry for anybody trying to write non-trivial\nformats. :)\n\n-Peff\n"},{"id":"506694","messageId":"ZyrXIKU8Qn46Z0LF@five231003","threadId":"62455","inReplyTo":"xmqqikt1qhwt.fsf@gitster.g","subject":"Re: [PATCH] t6300: values containing ')' are broken in ref formats","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2024-11-06T02:40:32Z","receivedAt":"2024-11-06T02:40:37Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Tue, Nov 05, 2024 at 05:18:10PM -0800, Junio C Hamano wrote:\n> Kousik Sanagavarapu <five231003@gmail.com> writes:\n> \n> > Document that values containing ')' in formats are not parsed correctly\n> > in ref-filter.\n> \n> The problem is probably lack of a way to quote such a closing\n> parenthesis.\n\nYes correct.  We currently don't have a way to quote such strings in\n\"value\" of \"%(atom:someparam=value)\"\n\n> > However formats having a '(' instead in \"value\" will parse correctly\n> > because in a general format string we also mark start of the format by\n> > making note of '%(' instead of just '('.\n> \n> So if you wanted to have a two-char sequence '%(' in value, you'd\n> see a similar problem?  If so, it is not quite a \"bug\" or \"not\n> parsed correctly\"---it is \"because there is no way to include\n> closing ')' in the value (e.g., by quoting), you cannot write such a\n> string in the value part\".\n> \n> > This raises the question of what can be done to parse ')' in values of\n> > the format correctly.  It seems to me like a clean solution would\n> > involve a huge refactoring involving a large portion of ref-filter but I\n> > maybe wrong.\n> \n> Yes, so I wouldn't even call the current behaviour \"bug\".  The\n> language is merely \"limited\" and the user cannot express certain\n> values with it at all.\n> \n> \n> Having said that, I just tried this\n> \n>     $ git for-each-ref --format='%28%(refname)%29' refs/heads/master\n>     (refs/heads/master)\n> \n> So, if there is anything that needs \"fixing\", wouldn't it be\n> documentation?\n> \n> If I knew (or easily find out from \"git for-each-ref --help\") that\n> hex escapes %XX can be used, I wouldn't have written any of what I\n> said before \"Having said that\" in this response.\n\nHmm, but hex escapes do work as intended and the problem here is not the\n')' outside the atom but within it.  To be more clear, let's take\n\n\t$ git for-each-ref --format=\"%(if:equals=refs/heads/step-1)start)%(refname)%(then)%(objectname:short)%(end)\" refs/heads/\n\n(Sorry for not wrapping the line above x<)\n\nFirst let's notice the difference between what these two commands are\ntrying to do.  Your command asks to print all the refs matching\n\"refs/heads/master\" in the format of (%(refname)) and since we support\nescaping literals in the form of %xx, where xx is the hexcode of the\nliteral to be escaped during the parsing of the format, this would\nobviously work as intended.\n\nNow let's come to my command.  My command asks to print all the\nabbreviated commit ids of the refs which compare equal to\n\"refs/heads/step-1)start\" from all of \"refs/heads/\".  Now here, since\nref-filter parses the format string by making note of '%(' and ')', it\naccidentally thinks that I want to compare equality with\n\"refs/heads/step-1\" instead of \"refs/heads/step-1)start\", which I\nactually wanted.  If my local repo contained both the \"refs/heads/step1\"\nand \"refs/heads/step1)start\", wouldn't this be a bug?\n\nSo I do agree that it is a lack of quoting when entering the \"value\"\npart of \"%(atom:someparam=value)\", but another part of me also thinks\nthat ref-filter should be intelligent enough while parsing the format\nstring to acknowledge where exactly the atom ends and which is the last\nclosing ')' and hence follows whatever I wrote below the \"---\" line,\ntill the script.\n\nAlso, I'm thinking the commit msg was not clear as it lead you (perhaps\nsomeone else too when they visit this topic) to think about escape\nliterals while that not exactly is the problem I'm trying to get at.\n\nAlso if you think a change to the documentation would be more proper\nthan reflecting this with a test breakage, I'll do that.  My intention\nwith the test was that - in the future if we parse the \"value\" correctly\nthen that commit would also include a\n\ns/test_expect_failure/test_expect_success\n\nchange.\n\nThanks!\n"},{"id":"506695","messageId":"xmqq8qtxqcye.fsf@gitster.g","threadId":"62455","inReplyTo":"20241106022552.GA816908@coredump.intra.peff.net","subject":"Re: [PATCH] t6300: values containing ')' are broken in ref formats","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-06T03:05:13Z","receivedAt":"2024-11-06T03:05:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I am tempted to say the solution is to expand that \"equals\" value, and\n> possibly add some less-arcane version of the character (maybe \"%)\"?).\n> But it be a break in backwards compatibility if somebody is trying to\n> match literal %-chars in their \"if\" block.\n\nIf they were trying to write a literal %, wouldn't they be writing\n%% already, not because % followed by a byte without any special\nmeaning happens to be passed intact by the implementation, but\nbecause that is _the_ right thing to do, when % is used as an\nintroducer for escape sequences?  So I do agree it would be a change\nthat breaks backward compatibility but I do not think we want to\nstay bug to bug compatible with the current behaviour here.  I am\nnot sure with the wisdom of %) though.  Wouldn't \"%(foo %)\" look as\nif %( opens and %) closes a group in our language?\n\nSo I am very much in favor of this \"if condition should be expanded\nbefore comparison\" solution.\n"},{"id":"506697","messageId":"ZyroYBwtQtgc6NoR@five231003","threadId":"62455","inReplyTo":"xmqq8qtxqcye.fsf@gitster.g","subject":"Re: [PATCH] t6300: values containing ')' are broken in ref formats","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2024-11-06T03:54:08Z","receivedAt":"2024-11-06T03:54:14Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Tue, Nov 05, 2024 at 07:05:13PM -0800, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n> > I am tempted to say the solution is to expand that \"equals\" value, and\n> > possibly add some less-arcane version of the character (maybe \"%)\"?).\n> > But it be a break in backwards compatibility if somebody is trying to\n> > match literal %-chars in their \"if\" block.\n> \n> If they were trying to write a literal %, wouldn't they be writing\n> %% already, not because % followed by a byte without any special\n> meaning happens to be passed intact by the implementation, but\n> because that is _the_ right thing to do, when % is used as an\n> introducer for escape sequences?  So I do agree it would be a change\n> that breaks backward compatibility but I do not think we want to\n> stay bug to bug compatible with the current behaviour here.  I am\n> not sure with the wisdom of %) though.  Wouldn't \"%(foo %)\" look as\n> if %( opens and %) closes a group in our language?\n> \n> So I am very much in favor of this \"if condition should be expanded\n> before comparison\" solution.\n\nI had worked on this \"if condition should be expanded before comparison\"\nsolution but Christian and I agreed that it would be better to open up\ndiscussion incase there would be other possible and better solutions\nwhich would also be viable in the long term.\n\nThis solution relies on how we parse out literals in ref-filter, so '%%'\nwould work as intended in the comparision string - ie if we want to\ncompare against \"some%ref\", we would do \"%(if:equals=some%%ref)...\".\n\nAlso it is obscure that someone will use this in practice as I myself\nhad discovered this when working on a corner case of some other\nimplementation related to parsing but way lower in the call-chain of\nref-filter ;) but here goes\n\n(this applies on top of the current patch)\n\n------------------------ >8 ------------------------\nSubject: [PATCH] ref-filter: parse parentheses correctly in %(if) atoms\n\nHaving a ')' in \"<string>\" in \":equals=<string>\" or\n\":notequals=<string>\" wouldn't parse correctly in ref-filter as\ndocumented in the previous commit.\n\nOne way to fix this is refactoring the way in which we parse our format\nstring.  Although this would mean we would have to do a huge refactoring\nas this step happens very high up in the call chain.\n\nTherefore, support including parenthesis characters in \"<string>\" by\ninstead giving their hexcode equivalents - as a for-now hack.\n\nDo this by further abstracting \"append_literal()\" to \"parse_literal()\"\nwhere the output is no longer stored into a ref formatting stack's\nstrbuf but a standard standalone strbuf.  append_literal would then\nhence wrap appropriately around \"parse_literal()\".\n\nAlso introduce \"convert_hexcode()\" which also wraps around\n\"parse_literal()\" and must be used to convert hexcode in a given string\nand be silent when such a string doesn't contain hexcode or doesn't\nexist (ie is NULL).\n\nUsing \"convert_hexcode()\" would mean that we now have an alloced string -\nhence free() it once we are done with it to prevent any memory leaks.\n\nSigned-off-by: Kousik Sanagavarapu <five231003@gmail.com>\n---\n Documentation/git-for-each-ref.txt |  4 ++\n ref-filter.c                       | 76 +++++++++++++++++++++++-------\n t/t6300-for-each-ref.sh            |  6 ++-\n 3 files changed, 66 insertions(+), 20 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex d3764401a2..ce12400040 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -221,6 +221,10 @@ if::\n \tthe value between the %(if:...) and %(then) atoms with the\n \tgiven string.\n \n+\tAdditionally, if `<string>` must contain parenthesis, then these\n+\tparentheses are spelled out as hexcode.  For e.g., `1)someref`\n+\twould need to be `1%29someref`.\n+\n symref::\n \tThe ref which the given symbolic ref refers to. If not a\n \tsymbolic ref, nothing is printed. Respects the `:short`,\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 84c6036107..ebdb2daeb7 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1234,13 +1234,64 @@ static void if_then_else_handler(struct ref_formatting_stack **stack)\n \t*stack = cur;\n }\n \n+/*\n+ * Parse out literals in the string pointed to by \"cp\" and store them in\n+ * a strbuf - this would go on until we hit NUL or \"ep\".\n+ *\n+ * While at it, if they're of the form \"%xx\", where xx represents the\n+ * hexcode of some character, then convert them into their equivalent\n+ * characters.\n+ */\n+static void parse_literal(const char *cp, const char *ep,\n+\t\t\t  struct strbuf *s)\n+{\n+\twhile (*cp && (!ep || cp < ep)) {\n+\t\tif (*cp == '%') {\n+\t\t\tif (cp[1] == '%')\n+\t\t\t\tcp++;\n+\t\t\telse {\n+\t\t\t\tint ch = hex2chr(cp + 1);\n+\t\t\t\tif (0 <= ch) {\n+\t\t\t\t\tstrbuf_addch(s, ch);\n+\t\t\t\t\tcp += 3;\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t}\n+\t\tstrbuf_addch(s, *cp);\n+\t\tcp++;\n+\t}\n+}\n+\n+/*\n+ * Convert the string, pointed to by \"cp\", which might or might not\n+ * contain hexcode (in the format of \"%xx\" where xx is the hexcode) to\n+ * its character-equivalent string and return it.\n+ *\n+ * If the string does not contain any hexcode - then it is returned as\n+ * is.\n+ */\n+static const char *convert_hexcode(const char *cp)\n+{\n+\tstruct strbuf s = STRBUF_INIT;\n+\n+\tif (!cp)\n+\t\treturn NULL;\n+\t/*\n+\t * This has the effect of an in-place translation but\n+\t * implementation-wise it is not.\n+\t */\n+\tparse_literal(cp, NULL, &s);\n+\treturn strbuf_detach(&s, NULL);\n+}\n+\n static int if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,\n \t\t\t   struct strbuf *err UNUSED)\n {\n \tstruct ref_formatting_stack *new_stack;\n \tstruct if_then_else *if_then_else = xcalloc(1, sizeof(*if_then_else));\n \n-\tif_then_else->str = atomv->atom->u.if_then_else.str;\n+\tif_then_else->str = convert_hexcode(atomv->atom->u.if_then_else.str);\n \tif_then_else->cmp_status = atomv->atom->u.if_then_else.cmp_status;\n \n \tpush_stack_element(&state->stack);\n@@ -1296,6 +1347,9 @@ static int then_atom_handler(struct atom_value *atomv UNUSED,\n \t\t\tif_then_else->condition_satisfied = 1;\n \t} else if (cur->output.len && !is_empty(&cur->output))\n \t\tif_then_else->condition_satisfied = 1;\n+\n+\tif (if_then_else->str)\n+\t\tfree((char *)if_then_else->str);\n \tstrbuf_reset(&cur->output);\n \treturn 0;\n }\n@@ -3425,26 +3479,12 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)\n \t\tQSORT_S(array->items, array->nr, compare_refs, sorting);\n }\n \n-static void append_literal(const char *cp, const char *ep, struct ref_formatting_state *state)\n+static void append_literal(const char *cp, const char *ep,\n+\t\t\t   struct ref_formatting_state *state)\n {\n \tstruct strbuf *s = &state->stack->output;\n \n-\twhile (*cp && (!ep || cp < ep)) {\n-\t\tif (*cp == '%') {\n-\t\t\tif (cp[1] == '%')\n-\t\t\t\tcp++;\n-\t\t\telse {\n-\t\t\t\tint ch = hex2chr(cp + 1);\n-\t\t\t\tif (0 <= ch) {\n-\t\t\t\t\tstrbuf_addch(s, ch);\n-\t\t\t\t\tcp += 3;\n-\t\t\t\t\tcontinue;\n-\t\t\t\t}\n-\t\t\t}\n-\t\t}\n-\t\tstrbuf_addch(s, *cp);\n-\t\tcp++;\n-\t}\n+\tparse_literal(cp, ep, s);\n }\n \n int format_ref_array_item(struct ref_array_item *info,\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex ce5c607193..c9383d23a4 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -2141,7 +2141,7 @@ test_expect_success GPG 'show lack of signature with custom format' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'format having values containing ) parse correctly' '\n+test_expect_success 'format having values containing ) parse correctly' '\n \tgit branch \"1)feat\" &&\n \tcat >expect <<-\\EOF &&\n \trefs/heads/1)feat\n@@ -2151,7 +2151,9 @@ test_expect_failure 'format having values containing ) parse correctly' '\n \tnot equals\n \tnot equals\n \tEOF\n-\tgit for-each-ref --format=\"%(if:equals=1)feat)%(refname:short)%(then)%(refname)%(else)not equals%(end)\" \\\n+\n+\t# 29 is the hexcode of )\n+\tgit for-each-ref --format=\"%(if:equals=1%29feat)%(refname:short)%(then)%(refname)%(else)not equals%(end)\" \\\n \t\trefs/heads/ >actual &&\n \ttest_cmp expect actual\n '\n-- \n2.47.0.230.g0cf584699a\n\n"},{"id":"506749","messageId":"20241106185102.GA880133@coredump.intra.peff.net","threadId":"62455","inReplyTo":"xmqq8qtxqcye.fsf@gitster.g","subject":"Re: [PATCH] t6300: values containing ')' are broken in ref formats","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-06T18:51:02Z","receivedAt":"2024-11-06T18:51:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 05, 2024 at 07:05:13PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I am tempted to say the solution is to expand that \"equals\" value, and\n> > possibly add some less-arcane version of the character (maybe \"%)\"?).\n> > But it be a break in backwards compatibility if somebody is trying to\n> > match literal %-chars in their \"if\" block.\n> \n> If they were trying to write a literal %, wouldn't they be writing\n> %% already, not because % followed by a byte without any special\n> meaning happens to be passed intact by the implementation, but\n> because that is _the_ right thing to do, when % is used as an\n> introducer for escape sequences?  So I do agree it would be a change\n> that breaks backward compatibility but I do not think we want to\n> stay bug to bug compatible with the current behaviour here.\n\nI think \"because that is the right thing to do\" is what is in question.\nIt is not like we happen to allow \"%\", but you should be writing \"%%\" in\nan if:equals value already. They mean two different things, and anybody\nwho is doing:\n\n  %(if:equals=%%foo)\n\nto match the literal \"%%foo\" will be broken if we change that. They are\nnot doing anything wrong; that is the only way to make it work now.\n\nI wouldn't go so far as to call the current behavior a bug. It's\njust...not very flexible. I also think it is unlikely that anybody would\ncare in practice (though I find matching refs with \")\" in them already a\nbit far-fetched).\n\nIf we wanted to be extra careful, we could introduce a variant of\n\"equals\" that indicates that it will be expanded before comparison.  Or\neven an extra tag, like:\n\n  %(if:expand:equals=%%foo)\n\n> I am not sure with the wisdom of %) though.  Wouldn't \"%(foo %)\" look\n> as if %( opens and %) closes a group in our language?\n\nYeah, I agree it is ugly and possibly confusing. Normally I'd suggest\n\"\\\" for escaping, but it isn't otherwise syntactically important within\nthese formats (I don't think, anyway). The magic character is \"%\" so\nthat is what we have to work with.\n\n-Peff\n"},{"id":"506750","messageId":"20241106185511.GB880133@coredump.intra.peff.net","threadId":"62455","inReplyTo":"ZyroYBwtQtgc6NoR@five231003","subject":"Re: [PATCH] t6300: values containing ')' are broken in ref formats","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-06T18:55:11Z","receivedAt":"2024-11-06T18:55:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 06, 2024 at 09:24:08AM +0530, Kousik Sanagavarapu wrote:\n\n> One way to fix this is refactoring the way in which we parse our format\n> string.  Although this would mean we would have to do a huge refactoring\n> as this step happens very high up in the call chain.\n> \n> Therefore, support including parenthesis characters in \"<string>\" by\n> instead giving their hexcode equivalents - as a for-now hack.\n\nSo if I understand this is just expanding %<hex> and nothing else? That\nseems like the worst of both worlds. Now \"%\" is magic in these value\nstrings, breaking compatibility, but we didn't buy ourselves the\nflexibility to do arbitrary comparisons like:\n\n  %(if:equals=%(upstream:lstrip=3))%(refname:short)%(then)...\n\n-Peff\n"},{"id":"506773","messageId":"ZywmElhgd1om2Y3E@five231003","threadId":"62455","inReplyTo":"20241106185102.GA880133@coredump.intra.peff.net","subject":"Re: [PATCH] t6300: values containing ')' are broken in ref formats","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2024-11-07T02:29:38Z","receivedAt":"2024-11-07T02:29:43Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Wed, Nov 06, 2024 at 01:51:02PM -0500, Jeff King wrote:\n> On Tue, Nov 05, 2024 at 07:05:13PM -0800, Junio C Hamano wrote:\n> \n> > Jeff King <peff@peff.net> writes:\n> > \n> > > I am tempted to say the solution is to expand that \"equals\" value, and\n> > > possibly add some less-arcane version of the character (maybe \"%)\"?).\n> > > But it be a break in backwards compatibility if somebody is trying to\n> > > match literal %-chars in their \"if\" block.\n> > \n> > If they were trying to write a literal %, wouldn't they be writing\n> > %% already, not because % followed by a byte without any special\n> > meaning happens to be passed intact by the implementation, but\n> > because that is _the_ right thing to do, when % is used as an\n> > introducer for escape sequences?  So I do agree it would be a change\n> > that breaks backward compatibility but I do not think we want to\n> > stay bug to bug compatible with the current behaviour here.\n> \n> I think \"because that is the right thing to do\" is what is in question.\n> It is not like we happen to allow \"%\", but you should be writing \"%%\" in\n> an if:equals value already. They mean two different things, and anybody\n> who is doing:\n> \n>   %(if:equals=%%foo)\n> \n> to match the literal \"%%foo\" will be broken if we change that. They are\n> not doing anything wrong; that is the only way to make it work now.\n\nTrue.\n\n> I wouldn't go so far as to call the current behavior a bug. It's\n> just...not very flexible. I also think it is unlikely that anybody would\n> care in practice (though I find matching refs with \")\" in them already a\n> bit far-fetched).\n\nYeah.  I really don't think anyone in practice will hit upon this case.\nAs I mentioned already before, I was just trying to pick out a corner\ncase for another implementation in ref-filter and stumbled upon this.\n\n> If we wanted to be extra careful, we could introduce a variant of\n> \"equals\" that indicates that it will be expanded before comparison.  Or\n> even an extra tag, like:\n> \n>   %(if:expand:equals=%%foo)\n\nThis seems like a nice idea, if we are thinking about not breaking\nbackwards compatibility but then there is also this discussion about the\nformats being too verbose but I dunno.\n"},{"id":"506774","messageId":"ZywnS4j7gxn53N+G@five231003","threadId":"62455","inReplyTo":"20241106185511.GB880133@coredump.intra.peff.net","subject":"Re: [PATCH] t6300: values containing ')' are broken in ref formats","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2024-11-07T02:34:51Z","receivedAt":"2024-11-07T02:34:56Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Wed, Nov 06, 2024 at 01:55:11PM -0500, Jeff King wrote:\n> On Wed, Nov 06, 2024 at 09:24:08AM +0530, Kousik Sanagavarapu wrote:\n> \n> > One way to fix this is refactoring the way in which we parse our format\n> > string.  Although this would mean we would have to do a huge refactoring\n> > as this step happens very high up in the call chain.\n> > \n> > Therefore, support including parenthesis characters in \"<string>\" by\n> > instead giving their hexcode equivalents - as a for-now hack.\n> \n> So if I understand this is just expanding %<hex> and nothing else? That\n> seems like the worst of both worlds. Now \"%\" is magic in these value\n> strings, breaking compatibility, but\n\nYeah, I agree that this might be the worst of both worlds after I read\nyour reply to Junio.  It indeed is a hack - just trying to fix the\nparenthesis case and not taking into account\n\n- backwards compatibility with regards to '%'.\n- not being able to do\n\n> we didn't buy ourselves the flexibility to do arbitrary comparisons like:\n> \n>   %(if:equals=%(upstream:lstrip=3))%(refname:short)%(then)...\n"},{"id":"506775","messageId":"xmqqo72rvjqk.fsf@gitster.g","threadId":"62455","inReplyTo":"20241106185102.GA880133@coredump.intra.peff.net","subject":"Re: [PATCH] t6300: values containing ')' are broken in ref formats","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-07T02:52:03Z","receivedAt":"2024-11-07T02:52:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> who is doing:\n>\n>   %(if:equals=%%foo)\n>\n> to match the literal \"%%foo\" will be broken if we change that. They are\n> not doing anything wrong; that is the only way to make it work now.\n\nAh, you're absolutely right.  Unescaping would start breaking them.\n\n> I wouldn't go so far as to call the current behavior a bug. It's\n> just...not very flexible. I also think it is unlikely that anybody would\n> care in practice (though I find matching refs with \")\" in them already a\n> bit far-fetched).\n\n100% agreed.  For that matter, I find \"if:equals=%%foo\" equally\nimplausible.\n\n> If we wanted to be extra careful, we could introduce a variant of\n> \"equals\" that indicates that it will be expanded before comparison.  Or\n> even an extra tag, like:\n>\n>   %(if:expand:equals=%%foo)\n\nSurely, but if nobody screams, I am tempted to suggest fixing the\nequals/notequals---we do not have to be bug-to-bug compatible with a\nbuggy old implementation. After all, we do expand the string being\ninspected that appears between %(if) and %(then).  I do not think of\na good excuse for us to limit the string that it gets compared with\nto literals.\n\nThe implementation may be a bit involved, but shouldn't be too bad.\n\nWhen .str is an empty string in if_atom_handler(), we can follow\nwhat the current code does.  If .str is not empty, allocate a new\nstack element in order to parse the .str to its end by pointing\n.at_end of the new stack element to a new handler (call it\nif_cond_handler()), and pass the if_then_else structure it allocated\nas .at_end_data to it.\n\nAnd in the if_cond_handler(), grab the cur->output and overwrite the\n.str member with it (while being careful to avoid leaks).  At the\nend of the if_cond_handler(), pass control to if_then_else_handler()\nby arranging the if_then_else_handler is called, imitating the way\nhow if_atom_handler() passes control to if_then_else_handler() in\nthe current code.\n\nThen things like\n\n  %(if:equals=%(upstream:lstrip=3))%(refname:short)%(then)...\n\nwould work as expected ;-)\n"},{"id":"506824","messageId":"Zy2PV+yywkS64D1p@five231003","threadId":"62455","inReplyTo":"xmqqo72rvjqk.fsf@gitster.g","subject":"Re: [PATCH] t6300: values containing ')' are broken in ref formats","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2024-11-08T04:11:03Z","receivedAt":"2024-11-08T04:11:08Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Thu, Nov 07, 2024 at 11:52:03AM +0900, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n> [...]\n> \n> The implementation may be a bit involved, but shouldn't be too bad.\n> \n> When .str is an empty string in if_atom_handler(), we can follow\n> what the current code does.  If .str is not empty, allocate a new\n> stack element in order to parse the .str to its end by pointing\n> .at_end of the new stack element to a new handler (call it\n> if_cond_handler()), and pass the if_then_else structure it allocated\n> as .at_end_data to it.\n> \n> And in the if_cond_handler(), grab the cur->output and overwrite the\n> .str member with it (while being careful to avoid leaks).  At the\n> end of the if_cond_handler(), pass control to if_then_else_handler()\n> by arranging the if_then_else_handler is called, imitating the way\n> how if_atom_handler() passes control to if_then_else_handler() in\n> the current code.\n> \n> Then things like\n> \n>   %(if:equals=%(upstream:lstrip=3))%(refname:short)%(then)...\n\nSo if I understand correctly, we grab the .str and operate on it so that\nwe expand the atom within it and then do the comparision.\n\nThis seems nice, but there is a problem.  Since we always look for the\nfirst occurring ')' in our format string to indicate the end of the atom,\nwe end up with\n\n\t.str = %(upstream:lstrip=3\n\n(the call chain is\n\n\tverify_ref_format() -> parse_ref_filter_atom() -> if_atom_parser()\n)\n\nSince we have now left out a ')', this ')' gets appended to our output\nbuf, which would also show up in cur->output when we do the comparision\nin then_atom_handler().  For example, in this case our cur->output would\nbe \")master\" instead of \"master\" after we get the value of\n%(refname:short), meaning our comparision always fails.\n"},{"id":"506876","messageId":"20241108171637.GA548990@coredump.intra.peff.net","threadId":"62455","inReplyTo":"Zy2PV+yywkS64D1p@five231003","subject":"Re: [PATCH] t6300: values containing ')' are broken in ref formats","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-08T17:16:37Z","receivedAt":"2024-11-08T17:16:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 08, 2024 at 09:41:03AM +0530, Kousik Sanagavarapu wrote:\n\n> > Then things like\n> > \n> >   %(if:equals=%(upstream:lstrip=3))%(refname:short)%(then)...\n> \n> So if I understand correctly, we grab the .str and operate on it so that\n> we expand the atom within it and then do the comparision.\n> \n> This seems nice, but there is a problem.  Since we always look for the\n> first occurring ')' in our format string to indicate the end of the atom,\n> we end up with\n> \n> \t.str = %(upstream:lstrip=3\n> \n> (the call chain is\n> \n> \tverify_ref_format() -> parse_ref_filter_atom() -> if_atom_parser()\n> )\n> \n> Since we have now left out a ')', this ')' gets appended to our output\n> buf, which would also show up in cur->output when we do the comparision\n> in then_atom_handler().  For example, in this case our cur->output would\n> be \")master\" instead of \"master\" after we get the value of\n> %(refname:short), meaning our comparision always fails.\n\nYes, though I think the parser _could_ be improved here. This is\ndifferent the earlier case of matching \"ref-with-)\". In that case it is\nsyntactically ambiguous. For example given:\n\n  %(if:equals=ref-with-))foo\n\nyou cannot tell the difference between:\n\n  - matching \"ref-with-)\", followed by \"foo\"\n\n  - matching \"ref-with-\", followed by \")foo\"\n\nBut if the parenthesis in question is closing a %() item, like:\n\n  %(if:equals=%(refname))foo\n\nthen we know that the inner \")\" is closing %(refname), since parsing it\nas \"%(refname\" followed by \")foo\" would leave an unbalanced pair. But\nfinding that would require a real recursive descent parser, rather than\na blind strchr() for the closing \")\"[1].\n\nIn the meantime yeah, you'd have to spell it as:\n\n  %(if:equals=%(refname%29)\n\nwhich is...deeply unsatisfying.\n\nI have long dreamed of throwing out all of this format code in favor of\na recursive parser which generates an actual tree of nodes, and\nimplements all of the ref-filter/pretty.c/cat-file format placeholders.\nBut I think it's a non-trivial task.\n\n-Peff\n\n[1] Incidentally, the \"%)\" I proposed earlier would also fall afoul of\n    this problem. The search for the closing \")\" is done blindly without\n    regard to possible quoting.\n"},{"id":"506885","messageId":"Zy5Ui0tHtKL1vYpw@five231003","threadId":"62455","inReplyTo":"20241108171637.GA548990@coredump.intra.peff.net","subject":"Re: [PATCH] t6300: values containing ')' are broken in ref formats","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2024-11-08T18:12:27Z","receivedAt":"2024-11-08T18:12:32Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Fri, Nov 08, 2024 at 12:16:37PM -0500, Jeff King wrote:\n> On Fri, Nov 08, 2024 at 09:41:03AM +0530, Kousik Sanagavarapu wrote:\n> \n> [...]\n> \n> In the meantime yeah, you'd have to spell it as:\n> \n>   %(if:equals=%(refname%29)\n> \n> which is...deeply unsatisfying.\n> \n> I have long dreamed of throwing out all of this format code in favor of\n> a recursive parser which generates an actual tree of nodes, and\n> implements all of the ref-filter/pretty.c/cat-file format placeholders.\n\nOh!  I remember this, let me search up the thread...\n\nQuoting you from\n\n\thttps://lore.kernel.org/git/20230901191639.GA1955435@coredump.intra.peff.net/\n\n    IMHO the code would be a lot easier to work with if the atoms were\n    structured as a parse tree with child pointers (especially when you get\n    into things like \"if\" that have sub-expressions). I think one of the\n    reasons that used_atom is an array is to de-duplicate repeated mentions\n    (so if you formatted \"%(foo) %(foo)\" it would only have to store the\n    computed value once).\n\n    But I think that is the wrong way to optimize it. We shouldn't be\n    storing any strings per-atom, but rather walking the parse tree to\n    produce a single output buffer. And the values should be cheap to fill\n    in, because we should parse the object as necessary up front. This is\n    more or less the way the pretty.c parser does it.\n\n> But I think it's a non-trivial task.\n\nTrue.\n"}]}