{"thread":{"id":"53207","subject":"[PATCH] merge: use skip_prefix to parse config key","startedAt":"2020-04-10T15:11:02Z","lastAt":"2020-04-11T07:13:09Z","messageCount":6,"participants":["Martin Ågren","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"395192","messageId":"20200410151032.23198-1-martin.agren@gmail.com","threadId":"53207","inReplyTo":null,"subject":"[PATCH] merge: use skip_prefix to parse config key","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-10T15:10:32Z","receivedAt":"2020-04-10T15:11:02Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Instead of using `starts_with()`, the magic number 7, `strlen()` and a\nfair number of additions to verify the three parts of the config key\n\"branch..mergeoptions\", use `skip_prefix()` to jump through them\nmore explicitly.\n\nWe need to introduce a new variable for this (we certainly can't modify\n`k` just because we see \"branch.\"!). With `skip_prefix()` we often use\nquite bland names like `p` or `str`. Let's do the same. If and when this\nfunction needs to do more prefix-skipping, we'll have a generic variable\nready for this.\n\nAdjust the indentation while we're here.\n\nSigned-off-by: Martin Ågren \n---\n\n worktree.c also matches \"strcmp.*strlen\" and could see a similar patch.\n I'm working on a longer series for that file and this fell out of that\n work. This feels independent enough that I'm posting it on its own.\n\n builtin/merge.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex d127d2225f..bde5f14f05 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -597,10 +597,10 @@ static void parse_branch_merge_options(char *bmo)\n static int git_merge_config(const char *k, const char *v, void *cb)\n {\n \tint status;\n+\tconst char *str;\n \n-\tif (branch && starts_with(k, \"branch.\") &&\n-\t\tstarts_with(k + 7, branch) &&\n-\t\t!strcmp(k + 7 + strlen(branch), \".mergeoptions\")) {\n+\tif (branch && skip_prefix(k, \"branch.\", &str) &&\n+\t skip_prefix(str, branch, &str) && !strcmp(str, \".mergeoptions\")) {\n \t\tfree(branch_mergeoptions);\n \t\tbranch_mergeoptions = xstrdup(v);\n \t\treturn 0;\n-- \n2.26.0\n\n"},{"id":"395197","messageId":"20200410155827.GA71011@coredump.intra.peff.net","threadId":"53207","inReplyTo":"20200410151032.23198-1-martin.agren@gmail.com","subject":"Re: [PATCH] merge: use skip_prefix to parse config key","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-10T15:58:27Z","receivedAt":"2020-04-10T15:58:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 10, 2020 at 05:10:32PM +0200, Martin Ågren wrote:\n\n> Instead of using `starts_with()`, the magic number 7, `strlen()` and a\n> fair number of additions to verify the three parts of the config key\n> \"branch..mergeoptions\", use `skip_prefix()` to jump through them\n> more explicitly.\n\nThe conversion looks correct to me and is certainly an improvement.\n\n> We need to introduce a new variable for this (we certainly can't modify\n> `k` just because we see \"branch.\"!). With `skip_prefix()` we often use\n> quite bland names like `p` or `str`. Let's do the same. If and when this\n> function needs to do more prefix-skipping, we'll have a generic variable\n> ready for this.\n\nI was about to comment on this in the patch, but your explanation\npreempted me. :) The logic here makes sense.\n\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index d127d2225f..bde5f14f05 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -597,10 +597,10 @@ static void parse_branch_merge_options(char *bmo)\n> static int git_merge_config(const char *k, const char *v, void *cb)\n> {\n> \tint status;\n> +\tconst char *str;\n> \n> -\tif (branch && starts_with(k, \"branch.\") &&\n> -\t\tstarts_with(k + 7, branch) &&\n> -\t\t!strcmp(k + 7 + strlen(branch), \".mergeoptions\")) {\n> +\tif (branch && skip_prefix(k, \"branch.\", &str) &&\n> +\t skip_prefix(str, branch, &str) && !strcmp(str, \".mergeoptions\")) {\n> \t\tfree(branch_mergeoptions);\n> \t\tbranch_mergeoptions = xstrdup(v);\n> \t\treturn 0;\n\nIn general, parsing subsections accurately involves looking from both\nthe start and the end of the string, pulling out the section and key and\nleaving the rest in the middle. But I think we can get away with this\nleft-to-right parsing because we're only interested in matching a\n_specific_ subsection name, and a specific key. So there are no cases it\nwill handle incorrectly.\n\nThe more general form would be:\n\n const char *subsection, *key;\n int subsection_len;\n\n if (!parse_config_key(\"branch\", &subsection, &subsection_len, &key) &&\n subsection_len == strlen(branch) && !strncmp(subsection, branch) &&\n !strcmp(key, \"mergeoptions\"))\n ...\n\nbut that's a bit more awkward (it would be less so if we had a helper\nfunction for comparing a NUL-terminated string against a ptr/len pair).\n\n-Peff\n"},{"id":"395201","messageId":"xmqqo8rzjnnb.fsf@gitster.c.googlers.com","threadId":"53207","inReplyTo":"20200410155827.GA71011@coredump.intra.peff.net","subject":"Re: [PATCH] merge: use skip_prefix to parse config key","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-10T16:44:56Z","receivedAt":"2020-04-10T16:45:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> In general, parsing subsections accurately involves looking from both\n> the start and the end of the string, pulling out the section and key and\n> leaving the rest in the middle. But I think we can get away with this\n> left-to-right parsing because we're only interested in matching a\n> _specific_ subsection name, and a specific key. So there are no cases it\n> will handle incorrectly.\n\nIn other words, if k were \"branch.A.B.mergeoptions\", it can only be\nthe 'branch.*.mergeoptions' variable attached to branch \"A.B\", but\nwhen checking for branch==\"A\", the first two skip_prefix() would\npass and the only thing that protects us from misparsing is that\n\"B.mergeoptions\" is not what we are looking for.\n\n> The more general form would be:\n>\n> const char *subsection, *key;\n> int subsection_len;\n>\n> if (!parse_config_key(\"branch\", &subsection, &subsection_len, &key) &&\n> subsection_len == strlen(branch) && !strncmp(subsection, branch) &&\n> !strcmp(key, \"mergeoptions\"))\n> ...\n>\n> but that's a bit more awkward (it would be less so if we had a helper\n> function for comparing a NUL-terminated string against a ptr/len pair).\n\nYes, but even with such a helper, i.e.\n\n\tif (branch &&\n\t !parse_config_key(\"branch\", &sub, &sublen, &key) &&\n\t !spanstrcmp(sub, sublen, branch) &&\n\t !strcmp(key, \"mergeoptions\"))\n\nwhat Martin wrote, especially if it is reflowed to match the above, i.e.\n\n\tif (branch &&\n\t skip_prefix(key, \"branch.\", &sub) &&\n\t skip_prefix(sub, branch, &sub) &&\n\t !strcmp(sub, \".mergeoptions\")\n\nI find it just as, if not more, easy to read.\n\nWhere the parse_config_key() helper shines, I think, is when we do\nnot have the middle level to compare against, and in that case, we\nmust work only from the given key, scanning from both ends for dot.\n\nThanks.\n\n\n"},{"id":"395202","messageId":"20200410165644.GA79836@coredump.intra.peff.net","threadId":"53207","inReplyTo":"xmqqo8rzjnnb.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] merge: use skip_prefix to parse config key","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-10T16:56:44Z","receivedAt":"2020-04-10T16:56:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 10, 2020 at 09:44:56AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > In general, parsing subsections accurately involves looking from both\n> > the start and the end of the string, pulling out the section and key and\n> > leaving the rest in the middle. But I think we can get away with this\n> > left-to-right parsing because we're only interested in matching a\n> > _specific_ subsection name, and a specific key. So there are no cases it\n> > will handle incorrectly.\n> \n> In other words, if k were \"branch.A.B.mergeoptions\", it can only be\n> the 'branch.*.mergeoptions' variable attached to branch \"A.B\", but\n> when checking for branch==\"A\", the first two skip_prefix() would\n> pass and the only thing that protects us from misparsing is that\n> \"B.mergeoptions\" is not what we are looking for.\n\nYes, exactly.\n\n> > The more general form would be:\n> [...]\n> Yes, but even with such a helper, i.e.\n> [...]\n> what Martin wrote, especially if it is reflowed to match the above, i.e.\n> [...]\n> I find it just as, if not more, easy to read.\n\nYeah, sorry if I was unclear; I think the left-to-right is perfectly\nfine for this case.\n\n> Where the parse_config_key() helper shines, I think, is when we do\n> not have the middle level to compare against, and in that case, we\n> must work only from the given key, scanning from both ends for dot.\n\nAgreed.\n\nAnother option for known-value cases like this is to do it outside of\nthe callback handler, like:\n\n char *key = xstrfmt(\"branch.%s.mergeoptions\");\n const char *value;\n if (!git_config_get_string_const(key, &value))\n ...\n free(key);\n\nThe allocation is a bit awkward, though we could hide that with a clever\nhelper.\n\nShifting from \"iterate over and store config keys\" to \"look up config\nkeys on demand\" is a much bigger change, though. I would only do it if\nit made the rest of the code flow easier.\n\n-Peff\n"},{"id":"395203","messageId":"xmqqk12njmdm.fsf@gitster.c.googlers.com","threadId":"53207","inReplyTo":"20200410165644.GA79836@coredump.intra.peff.net","subject":"Re: [PATCH] merge: use skip_prefix to parse config key","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-10T17:12:21Z","receivedAt":"2020-04-10T17:12:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Shifting from \"iterate over and store config keys\" to \"look up config\n> keys on demand\" is a much bigger change, though. I would only do it if\n> it made the rest of the code flow easier.\n\nYeah, absolutely.\n\nThanks.\n\n"},{"id":"395271","messageId":"20200411071145.6839-1-martin.agren@gmail.com","threadId":"53207","inReplyTo":"20200410165644.GA79836@coredump.intra.peff.net","subject":"[PATCH v2] merge: use skip_prefix to parse config key","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-11T07:11:45Z","receivedAt":"2020-04-11T07:13:09Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Fri, 10 Apr 2020 at 18:56, Jeff King wrote:\n>\n> On Fri, Apr 10, 2020 at 09:44:56AM -0700, Junio C Hamano wrote:\n>\n> > Jeff King writes:\n> >\n> > In other words, if k were \"branch.A.B.mergeoptions\", it can only be\n> > the 'branch.*.mergeoptions' variable attached to branch \"A.B\", but\n> > when checking for branch==\"A\", the first two skip_prefix() would\n> > pass and the only thing that protects us from misparsing is that\n> > \"B.mergeoptions\" is not what we are looking for.\n>\n> Yes, exactly.\n>\n> > > The more general form would be:\n> > [...]\n> > Yes, but even with such a helper, i.e.\n> > [...]\n> > what Martin wrote, especially if it is reflowed to match the above, i.e.\n> > [...]\n> > I find it just as, if not more, easy to read.\n>\n> Yeah, sorry if I was unclear; I think the left-to-right is perfectly\n> fine for this case.\n\nThanks both for an interesting discussion. Junio left a remark about\n\"[easy to read] especially if it is reflowed\" which I recognized from my\nown thinking yesterday. Then I went for fewer lines in what arguably\nlooks a bit crammed. Since I was not the only one to find readability in\nthe larger number of lines, here it is as v2.\n\nJunio, if you go \"meh, what I've already picked up will do just as\nfine\", feel free to act on that, i.e., to not act on this v2.\n\n> Another option for known-value cases like this is to do it outside of\n> the callback handler, like:\n>\n> char *key = xstrfmt(\"branch.%s.mergeoptions\");\n> const char *value;\n> if (!git_config_get_string_const(key, &value))\n> ...\n> free(key);\n>\n> The allocation is a bit awkward [...]\n\nI very briefly thought about such an approach here, but still in the\ncallback handler, where the allocation would be quite a bit more\nawkward, since we'd do it over and over.\n\nSomething I considered a bit more seriously was a helper that takes\nthree strings (\"branch\", branch and \"mergeoptions\") and does the check.\nBut as you mention in this thread, it's a bit special that we're looking\nfor a specific subsection (that is not a compile-time constant). So it\nfelt like the kind of \"nice-to-have\" helper that you'd end up using just\nonce or twice. (That said, I didn't look to see if I could find other\ninstances.)\n\nMartin\n\n-- >8 --\nInstead of using `starts_with()`, the magic number 7, `strlen()` and a\nfair number of additions to verify the three parts of the config key\n\"branch..mergeoptions\", use `skip_prefix()` to jump through them\nmore explicitly.\n\nWe need to introduce a new variable for this (we certainly can't modify\n`k` just because we see \"branch.\"!). With `skip_prefix()` we often use\nquite bland names like `p` or `str`. Let's do the same. If and when this\nfunction needs to do more prefix-skipping, we'll have a generic variable\nready for this.\n\nSigned-off-by: Martin Ågren \n---\n builtin/merge.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex d127d2225f..df83ba2a80 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -597,10 +597,12 @@ static void parse_branch_merge_options(char *bmo)\n static int git_merge_config(const char *k, const char *v, void *cb)\n {\n \tint status;\n+\tconst char *str;\n \n-\tif (branch && starts_with(k, \"branch.\") &&\n-\t\tstarts_with(k + 7, branch) &&\n-\t\t!strcmp(k + 7 + strlen(branch), \".mergeoptions\")) {\n+\tif (branch &&\n+\t skip_prefix(k, \"branch.\", &str) &&\n+\t skip_prefix(str, branch, &str) &&\n+\t !strcmp(str, \".mergeoptions\")) {\n \t\tfree(branch_mergeoptions);\n \t\tbranch_mergeoptions = xstrdup(v);\n \t\treturn 0;\n-- \n2.26.0\n\n"}]}