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

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

[Kaartic Sivaraam]

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.

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---

Eric Sunshine <sunshine@sunshineco.com> writes:
> Perhaps call them 'oldref_bare' and 'newref_bare' or something.

Nice suggestion but I opted for the more communicative (atleast to
me) 'interpreted_*name'. Let me know if they have any issues.

>  It's probably not worth a re-roll,
> though...

It's definitely not worth a re-roll of the series but deserves a re-roll
of this patch. (I fixed a comment in this re-roll, thanks!)

Thanks,
Kaartic

 builtin/branch.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index ca9d8abd0..d3751ca70 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -462,6 +462,8 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 {
 	struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;
 	struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
+	const char *interpreted_oldname = NULL;
+	const char *interpreted_newname = NULL;
 	int recovery = 0;
 	int clobber_head_ok;
 
@@ -493,13 +495,17 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 
 	reject_rebase_or_bisect_branch(oldref.buf);
 
+	/* 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);
+
 	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))
 		die(_("Branch rename failed"));
 	if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
@@ -508,10 +514,10 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	if (recovery) {
 		if (copy)
 			warning(_("Created a copy of a misnamed branch '%s'"),
-				oldref.buf + 11);
+				interpreted_oldname);
 		else
 			warning(_("Renamed a misnamed branch '%s' away"),
-				oldref.buf + 11);
+				interpreted_oldname);
 	}
 
 	if (!copy &&
@@ -520,9 +526,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 
 	strbuf_release(&logmsg);
 
-	strbuf_addf(&oldsection, "branch.%s", oldref.buf + 11);
+	strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
 	strbuf_release(&oldref);
-	strbuf_addf(&newsection, "branch.%s", newref.buf + 11);
+	strbuf_addf(&newsection, "branch.%s", interpreted_newname);
 	strbuf_release(&newref);
 	if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
 		die(_("Branch is renamed, but update of config-file failed"));
-- 
2.15.0.345.gf926f18f3

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

[Eric Sunshine]

On Tue, Nov 21, 2017 at 9:18 AM, Kaartic Sivaraam
<kaartic.sivaraam@gmail.com> 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@gmail.com>
> ---
> 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))

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

[Kaartic Sivaraam]

On Wednesday 22 November 2017 12:08 AM, Eric Sunshine wrote:
> 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.


>> 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

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

[Eric Sunshine]

On Tue, Nov 21, 2017 at 2:12 PM, Kaartic Sivaraam
<kaartic.sivaraam@gmail.com> wrote:
> On Wednesday 22 November 2017 12:08 AM, Eric Sunshine wrote:
>> The original code unconditionally uses "+ 11", which says that the
>> prefix is _always_ present. This commit message muddies the waters [...]
>
> 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.
>
> [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/

Thanks for the explanation and history reference.

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Tue, Nov 21, 2017 at 2:12 PM, Kaartic Sivaraam
> <kaartic.sivaraam@gmail.com> wrote:
>> On Wednesday 22 November 2017 12:08 AM, Eric Sunshine wrote:
>>> The original code unconditionally uses "+ 11", which says that the
>>> prefix is _always_ present. This commit message muddies the waters [...]
>>
>> 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.
>>
>> [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/
>
> Thanks for the explanation and history reference.

I have a suspicion that it wasn't the case.  The ugly uninitialized
byte sequence came from an error codepath of a function that is asked
to fill a strbuf with "refs/heads/$something" returning early when it
found an error in the input, without realizing that the caller still
looks at the strbuf even when it got an error.  That caller-callee pair
was updated.

It is just a bug and +11 is always correct; passing the data that
does not begin with "refs/heads/" there, including the case where an
uninitialized buffer is passed, is simply a bug.

In other words, skip_prefix() is a good change, but if we are to use
it, we should also check the result and error out if we do not see
"refs/heads/" there--you already bothered to spend extra cycles for
that.

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

[Kaartic Sivaraam]

On Wednesday 22 November 2017 07:39 AM, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
>> On Tue, Nov 21, 2017 at 2:12 PM, Kaartic Sivaraam
>> <kaartic.sivaraam@gmail.com> wrote:
>>> On Wednesday 22 November 2017 12:08 AM, Eric Sunshine wrote:
>>>> The original code unconditionally uses "+ 11", which says that the
>>>> prefix is _always_ present. This commit message muddies the waters [...]
>>>
>>> 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.
>>>
>>> [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/
>>
>> Thanks for the explanation and history reference.
> 
> I have a suspicion that it wasn't the case.  The ugly uninitialized
> byte sequence came from an error codepath of a function that is asked
> to fill a strbuf with "refs/heads/$something" returning early when it
> found an error in the input, without realizing that the caller still
> looks at the strbuf even when it got an error.  That caller-callee pair
> was updated.
> 

You seem to be right when viewing from the perspective of the callee 
(strbuf_check_branch_ref). IOW, you *seem* to be telling that the 
"callee" should have known that the caller was expecting the 'prefix' 
even in case of an error and should have "inserted" it regardless of the 
error (I thought the strbuf was initialized and contained the result of 
strbuf_branchname even in the case of an error) ?

I thought that the 'caller' should have known that the 'callee' would 
not insert the prefix when there was an error in the branch name thus it 
should have anticipated that there would be a case in which the prefix 
(refs/heads/) doesn't exist in the buf (the assumption).


> It is just a bug and +11 is always correct; passing the data that
> does not begin with "refs/heads/" there, including the case where an
> uninitialized buffer is passed, is simply a bug.

Let me see if I could correlate your statement with that error. AFAIK, I 
don't think the buffer was uninitialized in the case of that error. It 
had in it the result of interpretation of 'strbuf_branchname', didn't 
it? So that clears one case and leads me to the interpretation that,

"Passing the data (at an offset of 11 bytes from the beginning of the 
buf) that does not begin with "refs/heads/" is a bug"

Which seems to be in line with my statement that it was wrong (in the 
part of the "caller") to assume that "strbuf_check_branch_ref" always 
returns a buf with the prefix "refs/heads/" ?


> 
> In other words, skip_prefix() is a good change, but if we are to use
> it, we should also check the result and error out if we do not see
> "refs/heads/" there--you already bothered to spend extra cycles for
> that.
> 

Makes sense. I should have better done this initially instead of giving 
a muddy justification for not doing it.


Thanks,
Kaartic

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

[Kaartic Sivaraam]

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.

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 Changes in v3:

  - Update commit message

  - Removed superfluous comment

 builtin/branch.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index ca9d8abd0..4ad3b228b 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -462,6 +462,8 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 {
 	struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;
 	struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
+	const char *interpreted_oldname = NULL;
+	const char *interpreted_newname = NULL;
 	int recovery = 0;
 	int clobber_head_ok;
 
@@ -493,6 +495,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 
 	reject_rebase_or_bisect_branch(oldref.buf);
 
+	skip_prefix(oldref.buf, "refs/heads/", &interpreted_oldname);
+	skip_prefix(newref.buf, "refs/heads/", &interpreted_newname);
+
 	if (copy)
 		strbuf_addf(&logmsg, "Branch: copied %s to %s",
 			    oldref.buf, newref.buf);
@@ -508,10 +513,10 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	if (recovery) {
 		if (copy)
 			warning(_("Created a copy of a misnamed branch '%s'"),
-				oldref.buf + 11);
+				interpreted_oldname);
 		else
 			warning(_("Renamed a misnamed branch '%s' away"),
-				oldref.buf + 11);
+				interpreted_oldname);
 	}
 
 	if (!copy &&
@@ -520,9 +525,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 
 	strbuf_release(&logmsg);
 
-	strbuf_addf(&oldsection, "branch.%s", oldref.buf + 11);
+	strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
 	strbuf_release(&oldref);
-	strbuf_addf(&newsection, "branch.%s", newref.buf + 11);
+	strbuf_addf(&newsection, "branch.%s", interpreted_newname);
 	strbuf_release(&newref);
 	if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
 		die(_("Branch is renamed, but update of config-file failed"));
-- 
2.15.0.345.gf926f18f3