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
