On Tue, Nov 21, 2017 at 9:18 AM, Kaartic Sivaraam
<kaartic.sivaraam@xxxxxxxxx> wrote:
> Instead of hard-coding the offset strlen("refs/heads/") to skip
> the prefix "refs/heads/" use the skip_prefix() function which
> is more communicative and verifies that the string actually
> starts with that prefix.
>
> 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.
I'm guessing that the code is correct, which means the commit message
should be revised. The motivation for using skip_prefix() over
hard-coded magic values should be pretty obvious, thus doesn't require
a long (and potentially confusing) explanation. Perhaps take a hint
from de3ce210ed (merge: use skip_prefix(), 2017-08-10):
merge: use skip_prefix()
Get rid of a magic string length constant by using skip_prefix() instead
of starts_with().
and pattern your commit message after it.
> Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx>
> ---
> 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.
> 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?
> die(_("Branch rename failed"));
> if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
