Re: [PATCH v2 4/4] builtin/branch: strip refs/heads/ using skip_prefix

From: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Date: Nov 21, 2017, 19:12 UTC
Message-ID: <60caf5c8-4dc3-481a-80d6-37a697e31b52@gmail.com>
In-Reply-To: <CAPig+cSGu-+6Bw3YBzMwzxO8vKt7jdf9xgtr9FUdOAQpXzi2rQ@mail.gmail.com>

On Wednesday 22 November 2017 12:08 AM, Eric Sunshine wrote:

Show 15 quoted lines

> On Tue, Nov 21, 2017 at 9:18 AM, Kaartic Sivaraam
> <kaartic.sivaraam@gmail.com> wrote:
>>
>> Though we don't check for the result of verification here as
>> it's (almost) always the case that the string does start
>> with "refs/heads", it's just better to avoid hard-coding and
>> be more communicative.
> 
> The original code unconditionally uses "+ 11", which says that the
> prefix is _always_ present. This commit message muddies the waters by
> saying the prefix might or might not be present. Which is correct? If
> the code is correct, then the commit message is misleading; if the
> message is correct, then the code is buggy and the commit message
> should say that it is fixing a bug.
>

That muddiness of that statement is a consequence of my recent encounter[1] in which the assumption (that the prefix(refs/heads/ always exists) of that code failed. I had a little suspicion, when I wrote that commit message, that there might be other cases in which assumption might fail. The issue has been resolved only in 3/4 of jc/branch-name-sanity but that was only after I wrote the commit message initially. So, it does make sense to remove that muddiness now. Thanks for noting that.

Show 19 quoted lines

>> diff --git a/builtin/branch.c b/builtin/branch.c
>> @@ -493,13 +495,17 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>> +       /* At this point it should be safe to believe that the refs have the
>> +          prefix "refs/heads/" */
>> +       skip_prefix(oldref.buf, "refs/heads/", &interpreted_oldname);
>> +       skip_prefix(newref.buf, "refs/heads/", &interpreted_newname);
> 
>      /*
>       * Format mult-line comments
>       * like this.
>       */
> 
> However, this in-code comment shares the same problem as the commit
> message. It muddies the waters by saying that the prefix may or may
> not be present, whereas the original code unconditionally stated that
> it was present. Moreover, the comment adds very little or any value
> since it's pretty much repeating what the code itself already says.
> Consequently, it probably would be best to drop the comment
> altogether.

Makes sense. Of course, only if there aren't any code paths that lead to a state where the expected prefix doesn't exist. Hope, there's none!

Show 11 quoted lines

> 
>>          if (copy)
>>                  strbuf_addf(&logmsg, "Branch: copied %s to %s",
>>                              oldref.buf, newref.buf);
>>          else
>>                  strbuf_addf(&logmsg, "Branch: renamed %s to %s",
>>                              oldref.buf, newref.buf);
>> -
>>          if (!copy && rename_ref(oldref.buf, newref.buf, logmsg.buf))
> 
> Was the blank line removal intentional?

Nope.

Footnotes: [1]: Note the 'warning: ' message in the following mail. That ugly '|�?' was a consequence of the assumption that the 'prefix' always existed!

https://public-inbox.org/git/1509209933.2256.4.camel@gmail.com/

--- Kaartic