From: Kaartic Sivaraam Date: Tue, 21 Nov 2017 19:12:34 GMT Subject: Re: [PATCH v2 4/4] builtin/branch: strip refs/heads/ using skip_prefix Message-ID: <60caf5c8-4dc3-481a-80d6-37a697e31b52@gmail.com> In-Reply-To: On Wednesday 22 November 2017 12:08 AM, Eric Sunshine wrote: > On Tue, Nov 21, 2017 at 9:18 AM, Kaartic Sivaraam > 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@gmail.com/ --- Kaartic