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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wednesday 22 November 2017 12:08 AM, Eric Sunshine wrote:
On Tue, Nov 21, 2017 at 9:18 AM, Kaartic Sivaraam
<kaartic.sivaraam@xxxxxxxxx> 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.


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!



         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@xxxxxxxxx/


---
Kaartic



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux