{"thread":{"id":"47267","subject":"[PATCH v2 4/4] builtin/branch: strip refs/heads/ using skip_prefix","startedAt":"2017-11-21T14:19:16Z","lastAt":"2017-11-22T17:46:03Z","messageCount":7,"participants":["Kaartic Sivaraam","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"333095","messageId":"20171121141852.551-1-kaartic.sivaraam@gmail.com","threadId":"47267","inReplyTo":null,"subject":"[PATCH v2 4/4] builtin/branch: strip refs/heads/ using skip_prefix","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-11-21T14:18:52Z","receivedAt":"2017-11-21T14:19:16Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"Instead of hard-coding the offset strlen(\"refs/heads/\") to skip\nthe prefix \"refs/heads/\" use the skip_prefix() function which\nis more communicative and verifies that the string actually\nstarts with that prefix.\n\nThough we don't check for the result of verification here as\nit's (almost) always the case that the string does start\nwith \"refs/heads\", it's just better to avoid hard-coding and\nbe more communicative.\n\nSigned-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n---\n\nEric Sunshine <sunshine@sunshineco.com> writes:\n> Perhaps call them 'oldref_bare' and 'newref_bare' or something.\n\nNice suggestion but I opted for the more communicative (atleast to\nme) 'interpreted_*name'. Let me know if they have any issues.\n\n>  It's probably not worth a re-roll,\n> though...\n\nIt's definitely not worth a re-roll of the series but deserves a re-roll\nof this patch. (I fixed a comment in this re-roll, thanks!)\n\nThanks,\nKaartic\n\n builtin/branch.c | 16 +++++++++++-----\n 1 file changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex ca9d8abd0..d3751ca70 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -462,6 +462,8 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n {\n \tstruct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;\n \tstruct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;\n+\tconst char *interpreted_oldname = NULL;\n+\tconst char *interpreted_newname = NULL;\n \tint recovery = 0;\n \tint clobber_head_ok;\n \n@@ -493,13 +495,17 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \n \treject_rebase_or_bisect_branch(oldref.buf);\n \n+\t/* At this point it should be safe to believe that the refs have the\n+\t   prefix \"refs/heads/\" */\n+\tskip_prefix(oldref.buf, \"refs/heads/\", &interpreted_oldname);\n+\tskip_prefix(newref.buf, \"refs/heads/\", &interpreted_newname);\n+\n \tif (copy)\n \t\tstrbuf_addf(&logmsg, \"Branch: copied %s to %s\",\n \t\t\t    oldref.buf, newref.buf);\n \telse\n \t\tstrbuf_addf(&logmsg, \"Branch: renamed %s to %s\",\n \t\t\t    oldref.buf, newref.buf);\n-\n \tif (!copy && rename_ref(oldref.buf, newref.buf, logmsg.buf))\n \t\tdie(_(\"Branch rename failed\"));\n \tif (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))\n@@ -508,10 +514,10 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \tif (recovery) {\n \t\tif (copy)\n \t\t\twarning(_(\"Created a copy of a misnamed branch '%s'\"),\n-\t\t\t\toldref.buf + 11);\n+\t\t\t\tinterpreted_oldname);\n \t\telse\n \t\t\twarning(_(\"Renamed a misnamed branch '%s' away\"),\n-\t\t\t\toldref.buf + 11);\n+\t\t\t\tinterpreted_oldname);\n \t}\n \n \tif (!copy &&\n@@ -520,9 +526,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \n \tstrbuf_release(&logmsg);\n \n-\tstrbuf_addf(&oldsection, \"branch.%s\", oldref.buf + 11);\n+\tstrbuf_addf(&oldsection, \"branch.%s\", interpreted_oldname);\n \tstrbuf_release(&oldref);\n-\tstrbuf_addf(&newsection, \"branch.%s\", newref.buf + 11);\n+\tstrbuf_addf(&newsection, \"branch.%s\", interpreted_newname);\n \tstrbuf_release(&newref);\n \tif (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)\n \t\tdie(_(\"Branch is renamed, but update of config-file failed\"));\n-- \n2.15.0.345.gf926f18f3\n\n"},{"id":"333115","messageId":"CAPig+cSGu-+6Bw3YBzMwzxO8vKt7jdf9xgtr9FUdOAQpXzi2rQ@mail.gmail.com","threadId":"47267","inReplyTo":"20171121141852.551-1-kaartic.sivaraam@gmail.com","subject":"Re: [PATCH v2 4/4] builtin/branch: strip refs/heads/ using skip_prefix","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-21T18:38:19Z","receivedAt":"2017-11-21T18:38:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 21, 2017 at 9:18 AM, Kaartic Sivaraam\n<kaartic.sivaraam@gmail.com> wrote:\n> Instead of hard-coding the offset strlen(\"refs/heads/\") to skip\n> the prefix \"refs/heads/\" use the skip_prefix() function which\n> is more communicative and verifies that the string actually\n> starts with that prefix.\n>\n> Though we don't check for the result of verification here as\n> it's (almost) always the case that the string does start\n> with \"refs/heads\", it's just better to avoid hard-coding and\n> be more communicative.\n\nThe original code unconditionally uses \"+ 11\", which says that the\nprefix is _always_ present. This commit message muddies the waters by\nsaying the prefix might or might not be present. Which is correct? If\nthe code is correct, then the commit message is misleading; if the\nmessage is correct, then the code is buggy and the commit message\nshould say that it is fixing a bug.\n\nI'm guessing that the code is correct, which means the commit message\nshould be revised. The motivation for using skip_prefix() over\nhard-coded magic values should be pretty obvious, thus doesn't require\na long (and potentially confusing) explanation. Perhaps take a hint\nfrom de3ce210ed (merge: use skip_prefix(), 2017-08-10):\n\n    merge: use skip_prefix()\n\n    Get rid of a magic string length constant by using skip_prefix() instead\n    of starts_with().\n\nand pattern your commit message after it.\n\n> Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n> ---\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> @@ -493,13 +495,17 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n> +       /* At this point it should be safe to believe that the refs have the\n> +          prefix \"refs/heads/\" */\n> +       skip_prefix(oldref.buf, \"refs/heads/\", &interpreted_oldname);\n> +       skip_prefix(newref.buf, \"refs/heads/\", &interpreted_newname);\n\n    /*\n     * Format mult-line comments\n     * like this.\n     */\n\nHowever, this in-code comment shares the same problem as the commit\nmessage. It muddies the waters by saying that the prefix may or may\nnot be present, whereas the original code unconditionally stated that\nit was present. Moreover, the comment adds very little or any value\nsince it's pretty much repeating what the code itself already says.\nConsequently, it probably would be best to drop the comment\naltogether.\n\n>         if (copy)\n>                 strbuf_addf(&logmsg, \"Branch: copied %s to %s\",\n>                             oldref.buf, newref.buf);\n>         else\n>                 strbuf_addf(&logmsg, \"Branch: renamed %s to %s\",\n>                             oldref.buf, newref.buf);\n> -\n>         if (!copy && rename_ref(oldref.buf, newref.buf, logmsg.buf))\n\nWas the blank line removal intentional?\n\n>                 die(_(\"Branch rename failed\"));\n>         if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))\n"},{"id":"333117","messageId":"60caf5c8-4dc3-481a-80d6-37a697e31b52@gmail.com","threadId":"47267","inReplyTo":"CAPig+cSGu-+6Bw3YBzMwzxO8vKt7jdf9xgtr9FUdOAQpXzi2rQ@mail.gmail.com","subject":"Re: [PATCH v2 4/4] builtin/branch: strip refs/heads/ using skip_prefix","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-11-21T19:12:34Z","receivedAt":"2017-11-21T19:12:52Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Wednesday 22 November 2017 12:08 AM, Eric Sunshine wrote:\n> On Tue, Nov 21, 2017 at 9:18 AM, Kaartic Sivaraam\n> <kaartic.sivaraam@gmail.com> wrote:\n>>\n>> Though we don't check for the result of verification here as\n>> it's (almost) always the case that the string does start\n>> with \"refs/heads\", it's just better to avoid hard-coding and\n>> be more communicative.\n> \n> The original code unconditionally uses \"+ 11\", which says that the\n> prefix is _always_ present. This commit message muddies the waters by\n> saying the prefix might or might not be present. Which is correct? If\n> the code is correct, then the commit message is misleading; if the\n> message is correct, then the code is buggy and the commit message\n> should say that it is fixing a bug.\n> \n\nThat muddiness of that statement is a consequence of my recent \nencounter[1] in which the assumption (that the prefix(refs/heads/ always \nexists) of that code failed. I had a little suspicion, when I wrote that \ncommit message, that there might be other cases in which assumption \nmight fail. The issue has been resolved only in 3/4 of \njc/branch-name-sanity but that was only after I wrote the commit message \ninitially.  So, it does make sense to remove that muddiness now. Thanks \nfor noting that.\n\n\n>> diff --git a/builtin/branch.c b/builtin/branch.c\n>> @@ -493,13 +495,17 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n>> +       /* At this point it should be safe to believe that the refs have the\n>> +          prefix \"refs/heads/\" */\n>> +       skip_prefix(oldref.buf, \"refs/heads/\", &interpreted_oldname);\n>> +       skip_prefix(newref.buf, \"refs/heads/\", &interpreted_newname);\n> \n>      /*\n>       * Format mult-line comments\n>       * like this.\n>       */\n> \n> However, this in-code comment shares the same problem as the commit\n> message. It muddies the waters by saying that the prefix may or may\n> not be present, whereas the original code unconditionally stated that\n> it was present. Moreover, the comment adds very little or any value\n> since it's pretty much repeating what the code itself already says.\n> Consequently, it probably would be best to drop the comment\n> altogether.\n\nMakes sense. Of course, only if there aren't any code paths that lead to \na state where the expected prefix doesn't exist. Hope, there's none!\n\n\n> \n>>          if (copy)\n>>                  strbuf_addf(&logmsg, \"Branch: copied %s to %s\",\n>>                              oldref.buf, newref.buf);\n>>          else\n>>                  strbuf_addf(&logmsg, \"Branch: renamed %s to %s\",\n>>                              oldref.buf, newref.buf);\n>> -\n>>          if (!copy && rename_ref(oldref.buf, newref.buf, logmsg.buf))\n> \n> Was the blank line removal intentional?\n\nNope.\n\n\nFootnotes:\n[1]: Note the 'warning: ' message in the following mail. That ugly '|�?' \nwas a consequence of the assumption that the 'prefix' always existed!\n\nhttps://public-inbox.org/git/1509209933.2256.4.camel@gmail.com/\n\n\n---\nKaartic\n"},{"id":"333118","messageId":"20171121191708.7662-1-kaartic.sivaraam@gmail.com","threadId":"47267","inReplyTo":"CAPig+cSGu-+6Bw3YBzMwzxO8vKt7jdf9xgtr9FUdOAQpXzi2rQ@mail.gmail.com","subject":"[PATCH v3 4/4] builtin/branch: strip refs/heads/ using skip_prefix","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-11-21T19:17:08Z","receivedAt":"2017-11-21T19:17:58Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"Instead of hard-coding the offset strlen(\"refs/heads/\") to skip\nthe prefix \"refs/heads/\" use the skip_prefix() function which\nis more communicative and verifies that the string actually\nstarts with that prefix.\n\nSigned-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n---\n Changes in v3:\n\n  - Update commit message\n\n  - Removed superfluous comment\n\n builtin/branch.c | 13 +++++++++----\n 1 file changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex ca9d8abd0..4ad3b228b 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -462,6 +462,8 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n {\n \tstruct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;\n \tstruct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;\n+\tconst char *interpreted_oldname = NULL;\n+\tconst char *interpreted_newname = NULL;\n \tint recovery = 0;\n \tint clobber_head_ok;\n \n@@ -493,6 +495,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \n \treject_rebase_or_bisect_branch(oldref.buf);\n \n+\tskip_prefix(oldref.buf, \"refs/heads/\", &interpreted_oldname);\n+\tskip_prefix(newref.buf, \"refs/heads/\", &interpreted_newname);\n+\n \tif (copy)\n \t\tstrbuf_addf(&logmsg, \"Branch: copied %s to %s\",\n \t\t\t    oldref.buf, newref.buf);\n@@ -508,10 +513,10 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \tif (recovery) {\n \t\tif (copy)\n \t\t\twarning(_(\"Created a copy of a misnamed branch '%s'\"),\n-\t\t\t\toldref.buf + 11);\n+\t\t\t\tinterpreted_oldname);\n \t\telse\n \t\t\twarning(_(\"Renamed a misnamed branch '%s' away\"),\n-\t\t\t\toldref.buf + 11);\n+\t\t\t\tinterpreted_oldname);\n \t}\n \n \tif (!copy &&\n@@ -520,9 +525,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \n \tstrbuf_release(&logmsg);\n \n-\tstrbuf_addf(&oldsection, \"branch.%s\", oldref.buf + 11);\n+\tstrbuf_addf(&oldsection, \"branch.%s\", interpreted_oldname);\n \tstrbuf_release(&oldref);\n-\tstrbuf_addf(&newsection, \"branch.%s\", newref.buf + 11);\n+\tstrbuf_addf(&newsection, \"branch.%s\", interpreted_newname);\n \tstrbuf_release(&newref);\n \tif (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)\n \t\tdie(_(\"Branch is renamed, but update of config-file failed\"));\n-- \n2.15.0.345.gf926f18f3\n\n"},{"id":"333121","messageId":"CAPig+cRK8+pzf3EWuoG+36CRNaaGv6CQk27d4CrzTv=8beoQGg@mail.gmail.com","threadId":"47267","inReplyTo":"60caf5c8-4dc3-481a-80d6-37a697e31b52@gmail.com","subject":"Re: [PATCH v2 4/4] builtin/branch: strip refs/heads/ using skip_prefix","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-21T19:22:12Z","receivedAt":"2017-11-21T19:22:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 21, 2017 at 2:12 PM, Kaartic Sivaraam\n<kaartic.sivaraam@gmail.com> wrote:\n> On Wednesday 22 November 2017 12:08 AM, Eric Sunshine wrote:\n>> The original code unconditionally uses \"+ 11\", which says that the\n>> prefix is _always_ present. This commit message muddies the waters [...]\n>\n> That muddiness of that statement is a consequence of my recent encounter[1]\n> in which the assumption (that the prefix(refs/heads/ always exists) of that\n> code failed. I had a little suspicion, when I wrote that commit message,\n> that there might be other cases in which assumption might fail. The issue\n> has been resolved only in 3/4 of jc/branch-name-sanity but that was only\n> after I wrote the commit message initially.  So, it does make sense to\n> remove that muddiness now. Thanks for noting that.\n>\n> [1]: Note the 'warning: ' message in the following mail. That ugly '|�?' was\n> a consequence of the assumption that the 'prefix' always existed!\n> https://public-inbox.org/git/1509209933.2256.4.camel@gmail.com/\n\nThanks for the explanation and history reference.\n"},{"id":"333220","messageId":"xmqqeforxdx6.fsf@gitster.mtv.corp.google.com","threadId":"47267","inReplyTo":"CAPig+cRK8+pzf3EWuoG+36CRNaaGv6CQk27d4CrzTv=8beoQGg@mail.gmail.com","subject":"Re: [PATCH v2 4/4] builtin/branch: strip refs/heads/ using skip_prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-22T02:09:09Z","receivedAt":"2017-11-22T02:09:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Tue, Nov 21, 2017 at 2:12 PM, Kaartic Sivaraam\n> <kaartic.sivaraam@gmail.com> wrote:\n>> On Wednesday 22 November 2017 12:08 AM, Eric Sunshine wrote:\n>>> The original code unconditionally uses \"+ 11\", which says that the\n>>> prefix is _always_ present. This commit message muddies the waters [...]\n>>\n>> That muddiness of that statement is a consequence of my recent encounter[1]\n>> in which the assumption (that the prefix(refs/heads/ always exists) of that\n>> code failed. I had a little suspicion, when I wrote that commit message,\n>> that there might be other cases in which assumption might fail. The issue\n>> has been resolved only in 3/4 of jc/branch-name-sanity but that was only\n>> after I wrote the commit message initially.  So, it does make sense to\n>> remove that muddiness now. Thanks for noting that.\n>>\n>> [1]: Note the 'warning: ' message in the following mail. That ugly '|�?' was\n>> a consequence of the assumption that the 'prefix' always existed!\n>> https://public-inbox.org/git/1509209933.2256.4.camel@gmail.com/\n>\n> Thanks for the explanation and history reference.\n\nI have a suspicion that it wasn't the case.  The ugly uninitialized\nbyte sequence came from an error codepath of a function that is asked\nto fill a strbuf with \"refs/heads/$something\" returning early when it\nfound an error in the input, without realizing that the caller still\nlooks at the strbuf even when it got an error.  That caller-callee pair\nwas updated.\n\nIt is just a bug and +11 is always correct; passing the data that\ndoes not begin with \"refs/heads/\" there, including the case where an\nuninitialized buffer is passed, is simply a bug.\n\nIn other words, skip_prefix() is a good change, but if we are to use\nit, we should also check the result and error out if we do not see\n\"refs/heads/\" there--you already bothered to spend extra cycles for\nthat.\n"},{"id":"333305","messageId":"b5083b36-c91b-f3c7-2a22-237a1e08f993@gmail.com","threadId":"47267","inReplyTo":"xmqqeforxdx6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 4/4] builtin/branch: strip refs/heads/ using skip_prefix","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2017-11-22T17:45:47Z","receivedAt":"2017-11-22T17:46:03Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Wednesday 22 November 2017 07:39 AM, Junio C Hamano wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> \n>> On Tue, Nov 21, 2017 at 2:12 PM, Kaartic Sivaraam\n>> <kaartic.sivaraam@gmail.com> wrote:\n>>> On Wednesday 22 November 2017 12:08 AM, Eric Sunshine wrote:\n>>>> The original code unconditionally uses \"+ 11\", which says that the\n>>>> prefix is _always_ present. This commit message muddies the waters [...]\n>>>\n>>> That muddiness of that statement is a consequence of my recent encounter[1]\n>>> in which the assumption (that the prefix(refs/heads/ always exists) of that\n>>> code failed. I had a little suspicion, when I wrote that commit message,\n>>> that there might be other cases in which assumption might fail. The issue\n>>> has been resolved only in 3/4 of jc/branch-name-sanity but that was only\n>>> after I wrote the commit message initially.  So, it does make sense to\n>>> remove that muddiness now. Thanks for noting that.\n>>>\n>>> [1]: Note the 'warning: ' message in the following mail. That ugly '|�?' was\n>>> a consequence of the assumption that the 'prefix' always existed!\n>>> https://public-inbox.org/git/1509209933.2256.4.camel@gmail.com/\n>>\n>> Thanks for the explanation and history reference.\n> \n> I have a suspicion that it wasn't the case.  The ugly uninitialized\n> byte sequence came from an error codepath of a function that is asked\n> to fill a strbuf with \"refs/heads/$something\" returning early when it\n> found an error in the input, without realizing that the caller still\n> looks at the strbuf even when it got an error.  That caller-callee pair\n> was updated.\n> \n\nYou seem to be right when viewing from the perspective of the callee \n(strbuf_check_branch_ref). IOW, you *seem* to be telling that the \n\"callee\" should have known that the caller was expecting the 'prefix' \neven in case of an error and should have \"inserted\" it regardless of the \nerror (I thought the strbuf was initialized and contained the result of \nstrbuf_branchname even in the case of an error) ?\n\nI thought that the 'caller' should have known that the 'callee' would \nnot insert the prefix when there was an error in the branch name thus it \nshould have anticipated that there would be a case in which the prefix \n(refs/heads/) doesn't exist in the buf (the assumption).\n\n\n> It is just a bug and +11 is always correct; passing the data that\n> does not begin with \"refs/heads/\" there, including the case where an\n> uninitialized buffer is passed, is simply a bug.\n\nLet me see if I could correlate your statement with that error. AFAIK, I \ndon't think the buffer was uninitialized in the case of that error. It \nhad in it the result of interpretation of 'strbuf_branchname', didn't \nit? So that clears one case and leads me to the interpretation that,\n\n\"Passing the data (at an offset of 11 bytes from the beginning of the \nbuf) that does not begin with \"refs/heads/\" is a bug\"\n\nWhich seems to be in line with my statement that it was wrong (in the \npart of the \"caller\") to assume that \"strbuf_check_branch_ref\" always \nreturns a buf with the prefix \"refs/heads/\" ?\n\n\n> \n> In other words, skip_prefix() is a good change, but if we are to use\n> it, we should also check the result and error out if we do not see\n> \"refs/heads/\" there--you already bothered to spend extra cycles for\n> that.\n> \n\nMakes sense. I should have better done this initially instead of giving \na muddy justification for not doing it.\n\n\nThanks,\nKaartic\n"}]}