{"thread":{"id":"46877","subject":"[PATCH 0/1] ref-filter.c: pass empty-string as NULL to atom parsers","startedAt":"2017-10-02T05:51:01Z","lastAt":"2017-10-05T02:11:55Z","messageCount":12,"participants":["Taylor Blau","Jeff King","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"329406","messageId":"20171002055052.GB10729@D-10-157-251-166.dhcp4.washington.edu","threadId":"46877","inReplyTo":null,"subject":"[PATCH 0/1] ref-filter.c: pass empty-string as NULL to atom parsers","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2017-10-02T05:50:52Z","receivedAt":"2017-10-02T05:51:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nAttached is a one-long patch series to un-distinguish between atoms\nwithout sub-arguments (\"%(refname)\") and atoms with empty sub-argument\nlists (\"%(refname:)\").\n\nThis addresses a user-experience issue that Peff points out:\n\n> Doh, that string_list behavior is what I was missing in my earlier\n> comments. I agree this is probably the best way of doing it. I'm tempted\n> to say that parse_ref_filter_atom() should do a similar thing. Right now\n> we've got:\n>\n> $ git for-each-ref --format='%(refname)' | wc\n> 2206 2206 79929\n> $ git for-each-ref --format='%(refname:short)' | wc\n> 2206 2206 53622\n> $ git for-each-ref --format='%(refname:)' | wc\n> fatal: unrecognized %(refname:) argument:\n> 0 0 0\n\nBy treating %(refname) and %(refname:) as the same thing. Peff has\nconvinced me that these _are_ indeed the same thing, as the first is a\n%(refname) atom without any sub-arguments, and the later is a %(refname)\n%atom with empty sub-arguments.\n\nThe reasoning is highlighted in the comment this patch adds, which makes\nmore ergonomic the use of string_list_split in atom parser\nimplementations.\n\nThank you in advance :-).\n\n\n--\n- Taylor\n"},{"id":"329408","messageId":"20171002055311.29681-1-me@ttaylorr.com","threadId":"46877","inReplyTo":"20171002055052.GB10729@D-10-157-251-166.dhcp4.washington.edu","subject":"[PATCH] ref-filter.c: pass empty-string as NULL to atom parsers","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2017-10-02T05:53:11Z","receivedAt":"2017-10-02T05:53:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Peff points out that different atom parsers handle the empty\n\"sub-argument\" list differently. An example of this is the format\n\"%(refname:)\".\n\nSince callers often use `string_list_split` (which splits the empty\nstring with any delimiter as a 1-ary string_list containing the empty\nstring), this makes handling empty sub-argument strings non-ergonomic.\n\nLet's fix this by assuming that atom parser implementations don't care\nabout distinguishing between the empty string \"%(refname:)\" and no\nsub-arguments \"%(refname)\".\n\nSigned-off-by: Taylor Blau \n---\n ref-filter.c | 17 ++++++++++++++++-\n t/t6300-for-each-ref.sh | 1 +\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex bc591f4f3..22be4097a 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -415,8 +415,23 @@ static int parse_ref_filter_atom(const struct ref_format *format,\n \tREALLOC_ARRAY(used_atom, used_atom_cnt);\n \tused_atom[at].name = xmemdupz(atom, ep - atom);\n \tused_atom[at].type = valid_atom[i].cmp_type;\n-\tif (arg)\n+\tif (arg) {\n \t\targ = used_atom[at].name + (arg - atom) + 1;\n+\t\tif (!*arg) {\n+\t\t\t/*\n+\t\t\t * string_list_split is often use by atom parsers to\n+\t\t\t * split multiple sub-arguments for inspection.\n+\t\t\t *\n+\t\t\t * Given a string that does not contain a delimiter\n+\t\t\t * (example: \"\"), string_list_split returns a 1-ary\n+\t\t\t * string_list that requires adding special cases to\n+\t\t\t * atom parsers.\n+\t\t\t *\n+\t\t\t * Thus, treat the empty argument string as NULL.\n+\t\t\t */\n+\t\t\targ = NULL;\n+\t\t}\n+\t}\n \tmemset(&used_atom[at].u, 0, sizeof(used_atom[at].u));\n \tif (valid_atom[i].parser)\n \t\tvalid_atom[i].parser(format, &used_atom[at], arg);\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 2274a4b73..edc1bd8ea 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -51,6 +51,7 @@ test_atom() {\n }\n \n test_atom head refname refs/heads/master\n+test_atom head refname: refs/heads/master\n test_atom head refname:short master\n test_atom head refname:lstrip=1 heads/master\n test_atom head refname:lstrip=2 master\n-- \n2.14.1.145.gb3622a4ee\n\n"},{"id":"329416","messageId":"20171002064335.vzm3j2dgax4q5bxd@sigill.intra.peff.net","threadId":"46877","inReplyTo":"20171002055311.29681-1-me@ttaylorr.com","subject":"Re: [PATCH] ref-filter.c: pass empty-string as NULL to atom parsers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-02T06:43:35Z","receivedAt":"2017-10-02T06:43:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 01, 2017 at 10:53:11PM -0700, Taylor Blau wrote:\n\n> Peff points out that different atom parsers handle the empty\n> \"sub-argument\" list differently. An example of this is the format\n> \"%(refname:)\".\n> \n> Since callers often use `string_list_split` (which splits the empty\n> string with any delimiter as a 1-ary string_list containing the empty\n> string), this makes handling empty sub-argument strings non-ergonomic.\n> \n> Let's fix this by assuming that atom parser implementations don't care\n> about distinguishing between the empty string \"%(refname:)\" and no\n> sub-arguments \"%(refname)\".\n\nThis looks good to me (both the explanation and the function of the\ncode).\n\nBut let me assume for a moment that your \"please let me know\" from the\nearlier series is still in effect, and you wish to be showered with\npedantry and subjective advice. ;)\n\nI see a lot of newer contributors sending single patches as a 1-patch\nseries with a cover letter. As a reviewer, I think this is mostly just a\nhassle. The cover letter ends up mostly repeating the same content from\nthe single commit, so readers end up having to go over it twice (and you\nended up having to write it twice).\n\nSometimes there _is_ useful information to be conveyed that doesn't\nbelong in the commit message, but that can easily go after the \"---\" (or\nbefore a \"-- >8 --\" if you really feel it should be read before the\ncommit message.\n\nIn general, if you find yourself writing a really long cover letter, and\nespecially one that isn't mostly \"meta\" information (like where this\nshould be applied, or what's changed since the last version), you should\nconsider whether that information ought to go into the commit message\ninstead. The one exception is if you _do_ have a long series and you\nneed to sketch out the approach to help the reader see the big picture\n(in which case your cover letter should be summarizing what's already in\nthe commit messages).\n\nAnd before anybody digs in the list to find my novel-length cover\nletters to throw back in my face, I know that I'm very guilty of this.\nI'm trying to get better at it, and passing it on so you can learn from\nmy mistakes. :)\n\n> -\tif (arg)\n> +\tif (arg) {\n> \t\targ = used_atom[at].name + (arg - atom) + 1;\n> +\t\tif (!*arg) {\n> +\t\t\t/*\n> +\t\t\t * string_list_split is often use by atom parsers to\n> +\t\t\t * split multiple sub-arguments for inspection.\n> +\t\t\t *\n> +\t\t\t * Given a string that does not contain a delimiter\n> +\t\t\t * (example: \"\"), string_list_split returns a 1-ary\n> +\t\t\t * string_list that requires adding special cases to\n> +\t\t\t * atom parsers.\n> +\t\t\t *\n> +\t\t\t * Thus, treat the empty argument string as NULL.\n> +\t\t\t */\n> +\t\t\targ = NULL;\n> +\t\t}\n> +\t}\n\nI know this is getting _really_ subjective, but IMHO this is a lot more\nreasoning than the comment needs. The commit message goes into the\ndetails of the \"why\", but here I'd have just written something like:\n\n /* treat \"%(foo:)\" the same as \"%(foo)\"; i.e., no arguments */\n\n-Peff\n"},{"id":"329459","messageId":"20171002161034.44867-1-me@ttaylorr.com","threadId":"46877","inReplyTo":"20171002055052.GB10729@D-10-157-251-166.dhcp4.washington.edu","subject":"[PATCH v2] ref-filter.c: pass empty-string as NULL to atom parsers","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2017-10-02T16:10:34Z","receivedAt":"2017-10-02T16:10:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Peff points out that different atom parsers handle the empty\n\"sub-argument\" list differently. An example of this is the format\n\"%(refname:)\".\n\nSince callers often use `string_list_split` (which splits the empty\nstring with any delimiter as a 1-ary string_list containing the empty\nstring), this makes handling empty sub-argument strings non-ergonomic.\n\nLet's fix this by assuming that atom parser implementations don't care\nabout distinguishing between the empty string \"%(refname:)\" and no\nsub-arguments \"%(refname)\".\n\nSigned-off-by: Taylor Blau \n---\n ref-filter.c | 10 +++++++++-\n t/t6300-for-each-ref.sh | 1 +\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex bc591f4f3..f3e53d444 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -415,8 +415,16 @@ static int parse_ref_filter_atom(const struct ref_format *format,\n \tREALLOC_ARRAY(used_atom, used_atom_cnt);\n \tused_atom[at].name = xmemdupz(atom, ep - atom);\n \tused_atom[at].type = valid_atom[i].cmp_type;\n-\tif (arg)\n+\tif (arg) {\n \t\targ = used_atom[at].name + (arg - atom) + 1;\n+\t\tif (!*arg) {\n+\t\t\t/*\n+\t\t\t * Treat empty sub-arguments list as NULL (i.e.,\n+\t\t\t * \"%(atom:)\" is equivalent to \"%(atom)\").\n+\t\t\t */\n+\t\t\targ = NULL;\n+\t\t}\n+\t}\n \tmemset(&used_atom[at].u, 0, sizeof(used_atom[at].u));\n \tif (valid_atom[i].parser)\n \t\tvalid_atom[i].parser(format, &used_atom[at], arg);\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 2274a4b73..edc1bd8ea 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -51,6 +51,7 @@ test_atom() {\n }\n \n test_atom head refname refs/heads/master\n+test_atom head refname: refs/heads/master\n test_atom head refname:short master\n test_atom head refname:lstrip=1 heads/master\n test_atom head refname:lstrip=2 master\n-- \n2.14.1.145.gb3622a4ee\n\n"},{"id":"329460","messageId":"20171002161258.GC39723@D-10-157-251-166.dhcp4.washington.edu","threadId":"46877","inReplyTo":"20171002064335.vzm3j2dgax4q5bxd@sigill.intra.peff.net","subject":"Re: [PATCH] ref-filter.c: pass empty-string as NULL to atom parsers","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2017-10-02T16:12:58Z","receivedAt":"2017-10-02T16:13:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 02, 2017 at 02:43:35AM -0400, Jeff King wrote:\n> On Sun, Oct 01, 2017 at 10:53:11PM -0700, Taylor Blau wrote:\n>\n> > Peff points out that different atom parsers handle the empty\n> > \"sub-argument\" list differently. An example of this is the format\n> > \"%(refname:)\".\n> >\n> > Since callers often use `string_list_split` (which splits the empty\n> > string with any delimiter as a 1-ary string_list containing the empty\n> > string), this makes handling empty sub-argument strings non-ergonomic.\n> >\n> > Let's fix this by assuming that atom parser implementations don't care\n> > about distinguishing between the empty string \"%(refname:)\" and no\n> > sub-arguments \"%(refname)\".\n>\n> This looks good to me (both the explanation and the function of the\n> code).\n\nThanks :-).\n\n> But let me assume for a moment that your \"please let me know\" from the\n> earlier series is still in effect, and you wish to be showered with\n> pedantry and subjective advice. ;)\n>\n> I see a lot of newer contributors sending single patches as a 1-patch\n> series with a cover letter. As a reviewer, I think this is mostly just a\n> hassle. The cover letter ends up mostly repeating the same content from\n> the single commit, so readers end up having to go over it twice (and you\n> ended up having to write it twice).\n>\n> Sometimes there _is_ useful information to be conveyed that doesn't\n> belong in the commit message, but that can easily go after the \"---\" (or\n> before a \"-- >8 --\" if you really feel it should be read before the\n> commit message.\n>\n> In general, if you find yourself writing a really long cover letter, and\n> especially one that isn't mostly \"meta\" information (like where this\n> should be applied, or what's changed since the last version), you should\n> consider whether that information ought to go into the commit message\n> instead. The one exception is if you _do_ have a long series and you\n> need to sketch out the approach to help the reader see the big picture\n> (in which case your cover letter should be summarizing what's already in\n> the commit messages).\n\nThank you for this advice. I was worried when writing my cover letter\nlast night that it would be considered repetitive, but I wasn't sure how\nmuch brevity/detail would be desired in a patch series of this length.\n\nI'll keep this in mind for the future.\n\n> And before anybody digs in the list to find my novel-length cover\n> letters to throw back in my face, I know that I'm very guilty of this.\n> I'm trying to get better at it, and passing it on so you can learn from\n> my mistakes. :)\n\nI appreciate your humility ;-).\n\n> > -\tif (arg)\n> > +\tif (arg) {\n> > \t\targ = used_atom[at].name + (arg - atom) + 1;\n> > +\t\tif (!*arg) {\n> > +\t\t\t/*\n> > +\t\t\t * string_list_split is often use by atom parsers to\n> > +\t\t\t * split multiple sub-arguments for inspection.\n> > +\t\t\t *\n> > +\t\t\t * Given a string that does not contain a delimiter\n> > +\t\t\t * (example: \"\"), string_list_split returns a 1-ary\n> > +\t\t\t * string_list that requires adding special cases to\n> > +\t\t\t * atom parsers.\n> > +\t\t\t *\n> > +\t\t\t * Thus, treat the empty argument string as NULL.\n> > +\t\t\t */\n> > +\t\t\targ = NULL;\n> > +\t\t}\n> > +\t}\n>\n> I know this is getting _really_ subjective, but IMHO this is a lot more\n> reasoning than the comment needs. The commit message goes into the\n> details of the \"why\", but here I'd have just written something like:\n>\n> /* treat \"%(foo:)\" the same as \"%(foo)\"; i.e., no arguments */\n\nI sent an updated v2 of this \"series\" (without a cover-letter) that\nshortens this comment to more or less what you suggested. I've kept the\ncommit message longer, since I think that that information is useful\nwithin \"git blame\".\n\n\n--\n- Taylor\n"},{"id":"329479","messageId":"20171002194208.sfjepr3vsidobb6b@sigill.intra.peff.net","threadId":"46877","inReplyTo":"20171002161034.44867-1-me@ttaylorr.com","subject":"Re: [PATCH v2] ref-filter.c: pass empty-string as NULL to atom parsers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-02T19:42:08Z","receivedAt":"2017-10-02T19:42:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 02, 2017 at 09:10:34AM -0700, Taylor Blau wrote:\n\n> Peff points out that different atom parsers handle the empty\n> \"sub-argument\" list differently. An example of this is the format\n> \"%(refname:)\".\n> \n> Since callers often use `string_list_split` (which splits the empty\n> string with any delimiter as a 1-ary string_list containing the empty\n> string), this makes handling empty sub-argument strings non-ergonomic.\n> \n> Let's fix this by assuming that atom parser implementations don't care\n> about distinguishing between the empty string \"%(refname:)\" and no\n> sub-arguments \"%(refname)\".\n> \n> Signed-off-by: Taylor Blau \n> ---\n> ref-filter.c | 10 +++++++++-\n> t/t6300-for-each-ref.sh | 1 +\n> 2 files changed, 10 insertions(+), 1 deletion(-)\n\nThis looks good to me, thanks.\n\n-Peff\n"},{"id":"329480","messageId":"20171002194241.sdv7p3wgqok7sapi@sigill.intra.peff.net","threadId":"46877","inReplyTo":"20171002161258.GC39723@D-10-157-251-166.dhcp4.washington.edu","subject":"Re: [PATCH] ref-filter.c: pass empty-string as NULL to atom parsers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-02T19:42:42Z","receivedAt":"2017-10-02T19:42:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 02, 2017 at 09:12:58AM -0700, Taylor Blau wrote:\n\n> > I know this is getting _really_ subjective, but IMHO this is a lot more\n> > reasoning than the comment needs. The commit message goes into the\n> > details of the \"why\", but here I'd have just written something like:\n> >\n> > /* treat \"%(foo:)\" the same as \"%(foo)\"; i.e., no arguments */\n> \n> I sent an updated v2 of this \"series\" (without a cover-letter) that\n> shortens this comment to more or less what you suggested. I've kept the\n> commit message longer, since I think that that information is useful\n> within \"git blame\".\n\nYeah, sorry if I wasn't clear: definitely the commit message is fine and\nis the place to go into the detail and rationale.\n\n-Peff\n"},{"id":"329485","messageId":"20171002224052.GR19555@aiede.mtv.corp.google.com","threadId":"46877","inReplyTo":"20171002161034.44867-1-me@ttaylorr.com","subject":"Re: [PATCH v2] ref-filter.c: pass empty-string as NULL to atom parsers","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-02T22:40:52Z","receivedAt":"2017-10-02T22:40:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nTaylor Blau wrote:\n\n> Peff points out that different atom parsers handle the empty\n> \"sub-argument\" list differently. An example of this is the format\n> \"%(refname:)\".\n>\n> Since callers often use `string_list_split` (which splits the empty\n> string with any delimiter as a 1-ary string_list containing the empty\n> string), this makes handling empty sub-argument strings non-ergonomic.\n>\n> Let's fix this by assuming that atom parser implementations don't care\n> about distinguishing between the empty string \"%(refname:)\" and no\n> sub-arguments \"%(refname)\".\n>\n> Signed-off-by: Taylor Blau \n> ---\n> ref-filter.c | 10 +++++++++-\n> t/t6300-for-each-ref.sh | 1 +\n> 2 files changed, 10 insertions(+), 1 deletion(-)\n\nThe above does a nice job of explaining\n\n - what this change is going to do\n - how it's good for the internal code structure / maintainability\n\nWhat it doesn't tell me about is why the user-facing effect won't\ncause problems. Is there no atom where %(atom:) was previously\naccepted and did something meaningful that this may break?\n\nLooking at the manpage and code, I don't see any, so for what it's\nworth, this is\n\nReviewed-by: Jonathan Nieder \n\nbut for next time, please remember to discuss regression risk in\nthe commit message, too.\n\nThanks,\nJonathan\n"},{"id":"329500","messageId":"xmqqmv59m7fu.fsf@gitster.mtv.corp.google.com","threadId":"46877","inReplyTo":"20171002224052.GR19555@aiede.mtv.corp.google.com","subject":"Re: [PATCH v2] ref-filter.c: pass empty-string as NULL to atom parsers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-02T23:55:01Z","receivedAt":"2017-10-02T23:55:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> The above does a nice job of explaining\n>\n> - what this change is going to do\n> - how it's good for the internal code structure / maintainability\n>\n> What it doesn't tell me about is why the user-facing effect won't\n> cause problems. Is there no atom where %(atom:) was previously\n> accepted and did something meaningful that this may break?\n\nThat is, was there any situation where %(atom) and %(atom:) did two\ndiffernt things and their differences made sense?\n\n> Looking at the manpage and code, I don't see any, so for what it's\n> worth, this is\n>\n> Reviewed-by: Jonathan Nieder \n>\n> but for next time, please remember to discuss regression risk in\n> the commit message, too.\n\nYes, I agree that it is necessary to make sure somebody looked at\nthe issue _and_ record the fact that it happened. Thanks for doing\nthat already ;-)\n\nI also took a look at the code and currently we seem to abort,\neither with \"unrecognised arg\" (e.g. \"refname:\") or \"does not take\nargs\" (e.g. \"body\"), so we should be OK, I'd think.\n"},{"id":"329517","messageId":"20171003033726.GA84817@D-10-157-251-166.dhcp4.washington.edu","threadId":"46877","inReplyTo":"xmqqmv59m7fu.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] ref-filter.c: pass empty-string as NULL to atom parsers","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2017-10-03T03:37:26Z","receivedAt":"2017-10-03T03:37:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 03, 2017 at 08:55:01AM +0900, Junio C Hamano wrote:\n> Jonathan Nieder writes:\n>\n> > The above does a nice job of explaining\n> >\n> > - what this change is going to do\n> > - how it's good for the internal code structure / maintainability\n> >\n> > What it doesn't tell me about is why the user-facing effect won't\n> > cause problems. Is there no atom where %(atom:) was previously\n> > accepted and did something meaningful that this may break?\n>\n> That is, was there any situation where %(atom) and %(atom:) did two\n> differnt things and their differences made sense?\n>\n> > Looking at the manpage and code, I don't see any, so for what it's\n> > worth, this is\n> >\n> > Reviewed-by: Jonathan Nieder \n> >\n> > but for next time, please remember to discuss regression risk in\n> > the commit message, too.\n>\n> Yes, I agree that it is necessary to make sure somebody looked at\n> the issue _and_ record the fact that it happened. Thanks for doing\n> that already ;-)\n>\n> I also took a look at the code and currently we seem to abort,\n> either with \"unrecognised arg\" (e.g. \"refname:\") or \"does not take\n> args\" (e.g. \"body\"), so we should be OK, I'd think.\n\nThank you both for the helpful pointers. I will make sure to include a\nmore thorough review of potential breakage in existing code given a\nparticular change.\n\nWith respect to this particular patch, I agree with Jonathan and Junio\nthat there are no places in ref-filter.c that would be affected by this\nchange, and that it should be able to be applied cleanly without\nbreakage.\n\n--\n- Taylor\n"},{"id":"329716","messageId":"xmqq376y9xf7.fsf@gitster.mtv.corp.google.com","threadId":"46877","inReplyTo":"20171003033726.GA84817@D-10-157-251-166.dhcp4.washington.edu","subject":"Re: [PATCH v2] ref-filter.c: pass empty-string as NULL to atom parsers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-05T01:49:00Z","receivedAt":"2017-10-05T01:49:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Tue, Oct 03, 2017 at 08:55:01AM +0900, Junio C Hamano wrote:\n>> Jonathan Nieder writes:\n>>\n>> > The above does a nice job of explaining\n>> >\n>> > - what this change is going to do\n>> > - how it's good for the internal code structure / maintainability\n>> >\n>> > What it doesn't tell me about is why the user-facing effect won't\n>> > cause problems. Is there no atom where %(atom:) was previously\n>> > accepted and did something meaningful that this may break?\n\nThis loose end needs to be tied. \n\nI replaced \"let's assume that doing this change is OK\" from the\noriginal log message with something a bit stronger, as with this\nchange, we are saying that it is forbidden to treat %(atom) and\n%(atom:) differently. I also recorded the result of due-diligence\nsurvey of the current code to suggest that the change would be OK\nfor current users.\n\n-- >8 --\nFrom: Taylor Blau \nDate: Mon, 2 Oct 2017 09:10:34 -0700\nSubject: [PATCH] ref-filter.c: pass empty-string as NULL to atom parsers\n\nPeff points out that different atom parsers handle the empty\n\"sub-argument\" list differently. An example of this is the format\n\"%(refname:)\".\n\nSince callers often use `string_list_split` (which splits the empty\nstring with any delimiter as a 1-ary string_list containing the empty\nstring), this makes handling empty sub-argument strings non-ergonomic.\n\nLet's fix this by declaring that atom parser implementations must\nnot care about distinguishing between the empty string \"%(refname:)\"\nand no sub-arguments \"%(refname)\". Current code aborts, either with\n\"unrecognised arg\" (e.g. \"refname:\") or \"does not take args\"\n(e.g. \"body:\") as an error message.\n\nSigned-off-by: Taylor Blau \nReviewed-by: Jeff King \nReviewed-by: Jonathan Nieder \nSigned-off-by: Junio C Hamano \n---\n ref-filter.c | 10 +++++++++-\n t/t6300-for-each-ref.sh | 1 +\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex bc591f4f3d..f3e53d4448 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -415,8 +415,16 @@ static int parse_ref_filter_atom(const struct ref_format *format,\n \tREALLOC_ARRAY(used_atom, used_atom_cnt);\n \tused_atom[at].name = xmemdupz(atom, ep - atom);\n \tused_atom[at].type = valid_atom[i].cmp_type;\n-\tif (arg)\n+\tif (arg) {\n \t\targ = used_atom[at].name + (arg - atom) + 1;\n+\t\tif (!*arg) {\n+\t\t\t/*\n+\t\t\t * Treat empty sub-arguments list as NULL (i.e.,\n+\t\t\t * \"%(atom:)\" is equivalent to \"%(atom)\").\n+\t\t\t */\n+\t\t\targ = NULL;\n+\t\t}\n+\t}\n \tmemset(&used_atom[at].u, 0, sizeof(used_atom[at].u));\n \tif (valid_atom[i].parser)\n \t\tvalid_atom[i].parser(format, &used_atom[at], arg);\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 2274a4b733..edc1bd8eab 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -51,6 +51,7 @@ test_atom() {\n }\n \n test_atom head refname refs/heads/master\n+test_atom head refname: refs/heads/master\n test_atom head refname:short master\n test_atom head refname:lstrip=1 heads/master\n test_atom head refname:lstrip=2 master\n-- \n2.14.2-921-g20a440a8ba\n\n"},{"id":"329718","messageId":"20171005021148.GU19555@aiede.mtv.corp.google.com","threadId":"46877","inReplyTo":"xmqq376y9xf7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] ref-filter.c: pass empty-string as NULL to atom parsers","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-05T02:11:48Z","receivedAt":"2017-10-05T02:11:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> From: Taylor Blau \n> Date: Mon, 2 Oct 2017 09:10:34 -0700\n> Subject: [PATCH] ref-filter.c: pass empty-string as NULL to atom parsers\n>\n> Peff points out that different atom parsers handle the empty\n> \"sub-argument\" list differently. An example of this is the format\n> \"%(refname:)\".\n>\n> Since callers often use `string_list_split` (which splits the empty\n> string with any delimiter as a 1-ary string_list containing the empty\n> string), this makes handling empty sub-argument strings non-ergonomic.\n>\n> Let's fix this by declaring that atom parser implementations must\n> not care about distinguishing between the empty string \"%(refname:)\"\n> and no sub-arguments \"%(refname)\". Current code aborts, either with\n> \"unrecognised arg\" (e.g. \"refname:\") or \"does not take args\"\n> (e.g. \"body:\") as an error message.\n>\n> Signed-off-by: Taylor Blau \n> Reviewed-by: Jeff King \n> Reviewed-by: Jonathan Nieder \n> Signed-off-by: Junio C Hamano \n> ---\n> ref-filter.c | 10 +++++++++-\n> t/t6300-for-each-ref.sh | 1 +\n> 2 files changed, 10 insertions(+), 1 deletion(-)\n\nThanks for taking care of it. This is indeed still\nReviewed-by: Jonathan Nieder \n"}]}