[BUG] builtin/checkout.c:1098: should be able to skip past 'refs/heads/'

[BUG] builtin/checkout.c:1098: should be able to skip past 'refs/heads/'

[Todd Zullinger]

Hi,

A bug was filed in the Fedora/Red Hat bugzilla today for the
git 2.35.0 rc (rc1, but it's the same in rc2).  Petr (Cc'd),
ran the following

    git clone https://github.com/psss/fmf /tmp/fmf
    cd /tmp/fmf
    cp .git/refs/remotes/origin/HEAD .git/refs/heads/__DEFAULT__
    git checkout -f __DEFAULT__
    git checkout -f __DEFAULT__

The second git checkout call runs into the BUG() call added
in 9081a421a6 (checkout: fix "branch info" memory leaks,
2021-11-16):

    BUG: builtin/checkout.c:1098: should be able to skip
    past 'refs/heads/' in 'refs/remotes/origin/master'!
    Aborted (core dumped)

This worked in 2.34.1, so it's new to 2.35.0.  Should this
work or does the manual copy to setup a branch fall into a
new category of "don't do that"?

(It's novel to get a bug report from rc testing of a distro
build -- that doesn't happen often.)

-- 
Todd

Re: [BUG] builtin/checkout.c:1098: should be able to skip past 'refs/heads/'

[Todd Zullinger]

I wrote:
> A bug was filed in the Fedora/Red Hat bugzilla today for the
> git 2.35.0 rc (rc1, but it's the same in rc2).

For completeness, https://bugzilla.redhat.com/2042920 is the
ticket.

(It's apparently too early for me, despite the clock's claim
of noon).

-- 
Todd

[PATCH] checkout: fix BUG() case in 9081a421a6

[Ævar Arnfjörð Bjarmason]

Fix a regression in my 9081a421a6d (checkout: fix "branch info" memory
leaks, 2021-11-16) where I'd assumed that the old_branch_info.path
would have to start with refs/heads/*, but as has been reported[1]
that's not the case.

As a test case[2] to reproduce this shows the second "git checkout"
here runs into the BUG() in the pre-image. The test being added is
amended from[2] and will pass both with this change, and before
9081a421a6. I.e. our behavior now is again the same as before that
commit.

1. https://bugzilla.redhat.com/show_bug.cgi?id=2042920
2. https://lore.kernel.org/git/YemTGQZ97vAPUPY0@pobox.com/

Reported-by: Petr Šplíchal <psplicha@redhat.com>
Reported-by: Todd Zullinger <tmz@pobox.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Thu, Jan 20 2022, Todd Zullinger wrote:

> Hi,
>
> A bug was filed in the Fedora/Red Hat bugzilla today for the
> git 2.35.0 rc (rc1, but it's the same in rc2).  Petr (Cc'd),
> ran the following
>
>     git clone https://github.com/psss/fmf /tmp/fmf
>     cd /tmp/fmf
>     cp .git/refs/remotes/origin/HEAD .git/refs/heads/__DEFAULT__
>     git checkout -f __DEFAULT__
>     git checkout -f __DEFAULT__
>
> The second git checkout call runs into the BUG() call added
> in 9081a421a6 (checkout: fix "branch info" memory leaks,
> 2021-11-16):
>
>     BUG: builtin/checkout.c:1098: should be able to skip
>     past 'refs/heads/' in 'refs/remotes/origin/master'!
>     Aborted (core dumped)
>
> This worked in 2.34.1, so it's new to 2.35.0.  Should this
> work or does the manual copy to setup a branch fall into a
> new category of "don't do that"?
>
> (It's novel to get a bug report from rc testing of a distro
> build -- that doesn't happen often.)

Thanks to both you and Petr for the report and easy to reproduce case,
and sorry about causing it.

In retrospec it's a rather obvious thinko. Here's a minimal fix for
it, along with a derived test case that I made more exhaustive to
check the state of the repo before, after, and in-between the two "git
checkout" commands. As noted it'll also pass with 9081a421a6d
reverted, showing that our behavior is the same as before that commit.

 builtin/checkout.c         | 11 ++++-------
 t/t2018-checkout-branch.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 6a5dd2a2a22..52a47ef40e1 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1090,13 +1090,10 @@ static int switch_branches(const struct checkout_opts *opts,
 		FREE_AND_NULL(old_branch_info.path);
 
 	if (old_branch_info.path) {
-		const char *const prefix = "refs/heads/";
-		const char *p;
-		if (skip_prefix(old_branch_info.path, prefix, &p))
-			old_branch_info.name = xstrdup(p);
-		else
-			BUG("should be able to skip past '%s' in '%s'!",
-			    prefix, old_branch_info.path);
+		const char *p = old_branch_info.path;
+
+		skip_prefix(old_branch_info.path, "refs/heads/", &p);
+		old_branch_info.name = xstrdup(p);
 	}
 
 	if (opts->new_orphan_branch && opts->orphan_from_empty_tree) {
diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 3e93506c045..82df9b8bf64 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -85,6 +85,39 @@ test_expect_success 'setup' '
 	git branch -m branch1
 '
 
+test_expect_success REFFILES 'checkout a branch without refs/heads/* prefix' '
+	git clone --no-tags . repo-odd-prefix &&
+	(
+		cd repo-odd-prefix &&
+
+		cp .git/refs/remotes/origin/HEAD .git/refs/heads/a-branch &&
+
+		echo branch1 >expect.ref &&
+		git rev-parse --abbrev-ref HEAD >actual.ref &&
+		test_cmp expect.ref actual.ref &&
+
+		git checkout -f a-branch &&
+
+		echo origin/branch1 >expect.ref &&
+		git rev-parse --abbrev-ref HEAD >actual.ref &&
+		test_cmp expect.ref actual.ref &&
+
+		git checkout -f a-branch &&
+
+		cat >expect <<-EOF &&
+		$(git rev-parse HEAD) commit	refs/heads/a-branch
+		$(git rev-parse HEAD) commit	refs/heads/branch1
+		$(git rev-parse HEAD) commit	refs/remotes/origin/HEAD
+		$(git rev-parse HEAD) commit	refs/remotes/origin/branch1
+		EOF
+		git for-each-ref >actual &&
+		test_cmp expect actual &&
+
+		git rev-parse --abbrev-ref HEAD >actual &&
+		test_cmp expect.ref actual.ref
+	)
+'
+
 test_expect_success 'checkout -b to a new branch, set to HEAD' '
 	test_when_finished "
 		git checkout branch1 &&
-- 
2.35.0.rc1.864.g57621b115b6

Re: [PATCH] checkout: fix BUG() case in 9081a421a6

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Fix a regression in my 9081a421a6d (checkout: fix "branch info" memory
> leaks, 2021-11-16) where I'd assumed that the old_branch_info.path
> would have to start with refs/heads/*, but as has been reported[1]
> that's not the case.
>
> As a test case[2] to reproduce this shows the second "git checkout"
> here runs into the BUG() in the pre-image. The test being added is
> amended from[2] and will pass both with this change, and before
> 9081a421a6. I.e. our behavior now is again the same as before that
> commit.

> +test_expect_success REFFILES 'checkout a branch without refs/heads/* prefix' '
> +	git clone --no-tags . repo-odd-prefix &&
> +	(
> +		cd repo-odd-prefix &&
> +
> +		cp .git/refs/remotes/origin/HEAD .git/refs/heads/a-branch &&

I am not sure if this is a sensible test case to begin with.

It sets up recursive symbolic ref in this way:

	HEAD points at refs/heads/a-branch
	refs/heads/a-branch points at refs/remotes/origin/HEAD
	refs/remotes/origin/HEAD points at refs/remotes/origin/branch1

The checked out branch (i.e. what HEAD points at) is nominally a
local branch, but it totally violates the spirit of the safety valve
that says "HEAD" MUST point at a local branch or otherwise it is
detached.  Creating a commit while "a-branch" is checked out would
not affect *ANY* local branch state and instead makes an update to
the remote tracking branch that does not reflect *any* past states
at the remote repository.  Even worse, a "git fetch" that updates
the remote tracking branches will make the HEAD, the index and the
working tree into an inconsistent state.

Simply put, I think the BUG() is catching a case where we should
have been diagnosing as a broken repository.

So from my point of view, BUG() is indeed inappropriate because what
the conditional statement noticed was a broken repository, and not a
programming bug.

What we should never do is to promise this "only kosher in letter
but not in spirit" violation of "HEAD must point at a local branch"
rule will be supported.

So, unless I hear more convincing arguments (and Todd's example or
anything similar that makes "git commit" from that state update a
ref outside local branches is *not*), I am hesitant to call the new
behaviour and 9081a421a6d a regression.

What did the code before that BUG() do when faced with this nonsense
configuration?  If forbidding outright broke a sensible workflow
that happened to have been "working", I am OK to demote it to
warning() and restore the previous behaviour temporarily, whatever
it was (I think it was just old_branch_info.name was left unset
because we were not on local branch, but I don't know if the missing
.name was making any irrecoverable damage).  But the longer term
direction should be that we treat the "update HEAD ends up updating
some ref outside refs/heads/" a longstanding bug that needs to be
fixed.

Thanks.

Re: [PATCH] checkout: fix BUG() case in 9081a421a6

[Todd Zullinger]

Hi,

Ævar Arnfjörð Bjarmason wrote:
> Thanks to both you and Petr for the report and easy to reproduce case,
> and sorry about causing it.
> 
> In retrospec it's a rather obvious thinko. Here's a minimal fix for
> it, along with a derived test case that I made more exhaustive to
> check the state of the repo before, after, and in-between the two "git
> checkout" commands. As noted it'll also pass with 9081a421a6d
> reverted, showing that our behavior is the same as before that commit.

I can confirm the new test succeeds and the case which Petr
reported works as it did in previous releases.

Many thanks for the quick fix and more extensive tests Ævar!

-- 
Todd

Re: [PATCH] checkout: fix BUG() case in 9081a421a6

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> So from my point of view, BUG() is indeed inappropriate because what
> the conditional statement noticed was a broken repository, and not a
> programming bug.

Forgot to say: "; it should have been at least a warning(), if not a die()".

Re: [PATCH] checkout: fix BUG() case in 9081a421a6

[Ævar Arnfjörð Bjarmason]

On Thu, Jan 20 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Fix a regression in my 9081a421a6d (checkout: fix "branch info" memory
>> leaks, 2021-11-16) where I'd assumed that the old_branch_info.path
>> would have to start with refs/heads/*, but as has been reported[1]
>> that's not the case.
>>
>> As a test case[2] to reproduce this shows the second "git checkout"
>> here runs into the BUG() in the pre-image. The test being added is
>> amended from[2] and will pass both with this change, and before
>> 9081a421a6. I.e. our behavior now is again the same as before that
>> commit.
>
>> +test_expect_success REFFILES 'checkout a branch without refs/heads/* prefix' '
>> +	git clone --no-tags . repo-odd-prefix &&
>> +	(
>> +		cd repo-odd-prefix &&
>> +
>> +		cp .git/refs/remotes/origin/HEAD .git/refs/heads/a-branch &&
>
> I am not sure if this is a sensible test case to begin with.
>
> It sets up recursive symbolic ref in this way:
>
> 	HEAD points at refs/heads/a-branch
> 	refs/heads/a-branch points at refs/remotes/origin/HEAD
> 	refs/remotes/origin/HEAD points at refs/remotes/origin/branch1
>
> The checked out branch (i.e. what HEAD points at) is nominally a
> local branch, but it totally violates the spirit of the safety valve
> that says "HEAD" MUST point at a local branch or otherwise it is
> detached.  Creating a commit while "a-branch" is checked out would
> not affect *ANY* local branch state and instead makes an update to
> the remote tracking branch that does not reflect *any* past states
> at the remote repository.  Even worse, a "git fetch" that updates
> the remote tracking branches will make the HEAD, the index and the
> working tree into an inconsistent state.
>
> Simply put, I think the BUG() is catching a case where we should
> have been diagnosing as a broken repository.

Yes, I agree that we should be spotting this . Todd/Petr: If you're able
to describe it I'm curious about the original case you encountered that
the test case is derived from.

> So from my point of view, BUG() is indeed inappropriate because what
> the conditional statement noticed was a broken repository, and not a
> programming bug.
>
> What we should never do is to promise this "only kosher in letter
> but not in spirit" violation of "HEAD must point at a local branch"
> rule will be supported.
>
> So, unless I hear more convincing arguments (and Todd's example or
> anything similar that makes "git commit" from that state update a
> ref outside local branches is *not*), I am hesitant to call the new
> behaviour and 9081a421a6d a regression.

Well, the user is doing odd things with git, but we should reserve BUG()
for things that aren't rechable. Any time a user is able to arrange our
tooling in such a way as to call BUG() is a ... bug.

> What did the code before that BUG() do when faced with this nonsense
> configuration?  If forbidding outright broke a sensible workflow
> that happened to have been "working", I am OK to demote it to
> warning() and restore the previous behaviour temporarily, whatever
> it was (I think it was just old_branch_info.name was left unset
> because we were not on local branch, but I don't know if the missing
> .name was making any irrecoverable damage).  But the longer term
> direction should be that we treat the "update HEAD ends up updating
> some ref outside refs/heads/" a longstanding bug that needs to be
> fixed.

The behavior with my patch here is exactly the same as before. I.e. it
was rather straightforward, the xstrdup() is new, but before we'd just
take the un-skipped string that didn't start with refs/heads/ as-is.

I agree that it's better to look at this more deeply, but given the rc2
being out, and this surely being something we want in the final I'd
think we'd want to keep this patch as-is.

I.e. this is all a bit odd, but it was odd in exactly this way in v2.34.0 too.

We can add a warning() or die(), but a change that does that & convinces
you it's the right thing thing will require more careful consideration,
testing etc.

So I really think we should narrow our focus and keep this as-is.

Re: [PATCH] checkout: fix BUG() case in 9081a421a6

[Petr Šplíchal]

On Fri, 21 Jan 2022 at 12:23, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> On Thu, Jan 20 2022, Junio C Hamano wrote:
>
> > Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> >
> >> Fix a regression in my 9081a421a6d (checkout: fix "branch info" memory
> >> leaks, 2021-11-16) where I'd assumed that the old_branch_info.path
> >> would have to start with refs/heads/*, but as has been reported[1]
> >> that's not the case.
> >>
> >> As a test case[2] to reproduce this shows the second "git checkout"
> >> here runs into the BUG() in the pre-image. The test being added is
> >> amended from[2] and will pass both with this change, and before
> >> 9081a421a6. I.e. our behavior now is again the same as before that
> >> commit.
> >
> >> +test_expect_success REFFILES 'checkout a branch without refs/heads/* prefix' '
> >> +    git clone --no-tags . repo-odd-prefix &&
> >> +    (
> >> +            cd repo-odd-prefix &&
> >> +
> >> +            cp .git/refs/remotes/origin/HEAD .git/refs/heads/a-branch &&
> >
> > I am not sure if this is a sensible test case to begin with.
> >
> > It sets up recursive symbolic ref in this way:
> >
> >       HEAD points at refs/heads/a-branch
> >       refs/heads/a-branch points at refs/remotes/origin/HEAD
> >       refs/remotes/origin/HEAD points at refs/remotes/origin/branch1
> >
> > The checked out branch (i.e. what HEAD points at) is nominally a
> > local branch, but it totally violates the spirit of the safety valve
> > that says "HEAD" MUST point at a local branch or otherwise it is
> > detached.  Creating a commit while "a-branch" is checked out would
> > not affect *ANY* local branch state and instead makes an update to
> > the remote tracking branch that does not reflect *any* past states
> > at the remote repository.  Even worse, a "git fetch" that updates
> > the remote tracking branches will make the HEAD, the index and the
> > working tree into an inconsistent state.
> >
> > Simply put, I think the BUG() is catching a case where we should
> > have been diagnosing as a broken repository.
>
> Yes, I agree that we should be spotting this . Todd/Petr: If you're able
> to describe it I'm curious about the original case you encountered that
> the test case is derived from.

Thanks for explaining in detail what's wrong about the approach.
We didn't know about the "HEAD must point at a local branch" rule
and copying the ref seemed to be the easiest way to create a local
branch pointing always to the latest content of the remote default
branch. I described the use case here:

    https://bugzilla.redhat.com/show_bug.cgi?id=2042920#c2

Basically we just need to checkout and reset --hard to the default
remote branch after entering a git repository while HEAD can be
pointing anywhere. Could you suggest some more straightforward way
to achieve this from a script? Thanks.

psss...

> > So from my point of view, BUG() is indeed inappropriate because what
> > the conditional statement noticed was a broken repository, and not a
> > programming bug.
> >
> > What we should never do is to promise this "only kosher in letter
> > but not in spirit" violation of "HEAD must point at a local branch"
> > rule will be supported.
> >
> > So, unless I hear more convincing arguments (and Todd's example or
> > anything similar that makes "git commit" from that state update a
> > ref outside local branches is *not*), I am hesitant to call the new
> > behaviour and 9081a421a6d a regression.
>
> Well, the user is doing odd things with git, but we should reserve BUG()
> for things that aren't rechable. Any time a user is able to arrange our
> tooling in such a way as to call BUG() is a ... bug.
>
> > What did the code before that BUG() do when faced with this nonsense
> > configuration?  If forbidding outright broke a sensible workflow
> > that happened to have been "working", I am OK to demote it to
> > warning() and restore the previous behaviour temporarily, whatever
> > it was (I think it was just old_branch_info.name was left unset
> > because we were not on local branch, but I don't know if the missing
> > .name was making any irrecoverable damage).  But the longer term
> > direction should be that we treat the "update HEAD ends up updating
> > some ref outside refs/heads/" a longstanding bug that needs to be
> > fixed.
>
> The behavior with my patch here is exactly the same as before. I.e. it
> was rather straightforward, the xstrdup() is new, but before we'd just
> take the un-skipped string that didn't start with refs/heads/ as-is.
>
> I agree that it's better to look at this more deeply, but given the rc2
> being out, and this surely being something we want in the final I'd
> think we'd want to keep this patch as-is.
>
> I.e. this is all a bit odd, but it was odd in exactly this way in v2.34.0 too.
>
> We can add a warning() or die(), but a change that does that & convinces
> you it's the right thing thing will require more careful consideration,
> testing etc.
>
> So I really think we should narrow our focus and keep this as-is.

Re: [PATCH] checkout: fix BUG() case in 9081a421a6

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> So, unless I hear more convincing arguments (and Todd's example or
>> anything similar that makes "git commit" from that state update a
>> ref outside local branches is *not*), I am hesitant to call the new
>> behaviour and 9081a421a6d a regression.
>
> Well, the user is doing odd things with git, but we should reserve BUG()
> for things that aren't rechable. Any time a user is able to arrange our
> tooling in such a way as to call BUG() is a ... bug.

Yes, I concur.

>> What did the code before that BUG() do when faced with this nonsense
>> configuration?  If forbidding outright broke a sensible workflow
>> that happened to have been "working", I am OK to demote it to
>> warning() and restore the previous behaviour temporarily, whatever
>> it was (I think it was just old_branch_info.name was left unset
>> because we were not on local branch, but I don't know if the missing
>> .name was making any irrecoverable damage).  But the longer term
>> direction should be that we treat the "update HEAD ends up updating
>> some ref outside refs/heads/" a longstanding bug that needs to be
>> fixed.
>
> The behavior with my patch here is exactly the same as before. I.e. it
> was rather straightforward, the xstrdup() is new, but before we'd just
> take the un-skipped string that didn't start with refs/heads/ as-is.

OK, that might have done a wrong thing (instead of dying) for a
strange settings like that, but the change was never about
tightening and detecting such a strangeness but only about plugging
leaks, so reverting that narrow part of the patch is the right thing
to do.

> I agree that it's better to look at this more deeply, but given the rc2
> being out, and this surely being something we want in the final I'd
> think we'd want to keep this patch as-is.

Yes, except for the update in the test.  I do not think we want to
promise what should happen to the _values_ of these refs after the
operation at all.  If it only says "checkout should not exit with
non-zero status", I would be OK.  Promising anything more than that,
I do not think it is a good idea.

For now, I plan to do the "revert the check-and-BUG and nothing
else" change.

Thanks.

Re: [PATCH] checkout: fix BUG() case in 9081a421a6

[Todd Zullinger]

Petr Šplíchal wrote:
> Thanks for explaining in detail what's wrong about the approach.

Yes, thank you Ævar and Junio for providing context and the
thoughtful planning on how to fix this in both the short and
long term.

> We didn't know about the "HEAD must point at a local branch" rule
> and copying the ref seemed to be the easiest way to create a local
> branch pointing always to the latest content of the remote default
> branch. I described the use case here:
> 
>     https://bugzilla.redhat.com/show_bug.cgi?id=2042920#c2
> 
> Basically we just need to checkout and reset --hard to the default
> remote branch after entering a git repository while HEAD can be
> pointing anywhere. Could you suggest some more straightforward way
> to achieve this from a script? Thanks.

I'm nearly positive that I don't know the best way, but
here's _a_ way to do it.  It assumes the default remote name
is origin, which seems less than ideal.

(Hopefully this isn't egregiously wrong.  But if it is, I'm
happy to serve as an example of how _not_ to do it for
others.)

    git clone https://github.com/psss/fmf /tmp/fmf
    cd /tmp/fmf
    git switch -c custom-branch
    : work work work
    defbranch=$(git symbolic-ref --short refs/remotes/origin/HEAD)
    git switch -f ${defbranch#*/}
    git reset --hard @{u}

Odds are there's a better way to do this or to arrange
things in a way that lets you solve an easier problem.

-- 
Todd

Re: [PATCH] checkout: fix BUG() case in 9081a421a6

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 6a5dd2a2a22..52a47ef40e1 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1090,13 +1090,10 @@ static int switch_branches(const struct checkout_opts *opts,
>  		FREE_AND_NULL(old_branch_info.path);
>  
>  	if (old_branch_info.path) {
> -		const char *const prefix = "refs/heads/";
> -		const char *p;
> -		if (skip_prefix(old_branch_info.path, prefix, &p))
> -			old_branch_info.name = xstrdup(p);
> -		else
> -			BUG("should be able to skip past '%s' in '%s'!",
> -			    prefix, old_branch_info.path);
> +		const char *p = old_branch_info.path;
> +
> +		skip_prefix(old_branch_info.path, "refs/heads/", &p);
> +		old_branch_info.name = xstrdup(p);
>  	}

What we want is a faithful reversion wrt the behaviour, while
keeping the leakfix.

The old code did

	skip_prefix(old_branch_info.path, "refs/heads", &old_branch_info.name);

meaning that old_branch_info.name is left unchanged if .path did not
begin with "refs/heads".

The function that uses old_branch_info.name prepared at this point
in the flow is merge_working_tree(), where it uses .name for the
textual label for the "ancestor" tree.  It is preferrable to have a
branch name, but the code is prepared to see NULL in there, where it
comes up with an abbreviated commit object name.

                o.ancestor = old_branch_info->name;
                if (old_branch_info->name == NULL) {
                        strbuf_add_unique_abbrev(&old_commit_shortname,
                                                 &old_branch_info->commit->object.oid,
                                                 DEFAULT_ABBREV);
                        o.ancestor = old_commit_shortname.buf;
                }

As 9081a421a6 properly 0-initializes old_branch_info, I think it is
sufficient to just drop the "else BUG" clause; we shouldn't have to
copy the .path that does not begin with "refs/heads/" there; .name
member would be NULL in such a case.

I do not care too much about reverting the constant that is only
used once into a literal.  So, how about doing it this way instead?

 builtin/checkout.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git i/builtin/checkout.c w/builtin/checkout.c
index 43d0275187..1fb34d537d 100644
--- i/builtin/checkout.c
+++ w/builtin/checkout.c
@@ -1094,9 +1094,6 @@ static int switch_branches(const struct checkout_opts *opts,
 		const char *p;
 		if (skip_prefix(old_branch_info.path, prefix, &p))
 			old_branch_info.name = xstrdup(p);
-		else
-			BUG("should be able to skip past '%s' in '%s'!",
-			    prefix, old_branch_info.path);
 	}
 
 	if (opts->new_orphan_branch && opts->orphan_from_empty_tree) {

Re: [PATCH] checkout: fix BUG() case in 9081a421a6

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> +test_expect_success REFFILES 'checkout a branch without refs/heads/* prefix' '
> +	git clone --no-tags . repo-odd-prefix &&
> +	(
> +		cd repo-odd-prefix &&
> +
> +		cp .git/refs/remotes/origin/HEAD .git/refs/heads/a-branch &&

I already said that I do not think we want the exact behaviour (like
"remote-tracking branches are moved when we change the local branch")
to be cast in stone like this, but we should be able to lose the
REFFILES prerequisite by using proper plumbing, e.g. these two lines

		origin=$(git symbolic-ref refs/remotes/origin/HEAD) &&
		git symbolic-ref refs/heads/a-branch "$origin" &&

instead of the direct manipulation of filesystem with "cp".

I am tempted to say that checking the command does not trigger
BUG(), like the attached, may be sufficient.  I do not care too
deeply either way, though.

diff --git i/t/t2018-checkout-branch.sh w/t/t2018-checkout-branch.sh
index 93be1c0eae..5dda5ad4cb 100755
--- i/t/t2018-checkout-branch.sh
+++ w/t/t2018-checkout-branch.sh
@@ -85,6 +85,19 @@ test_expect_success 'setup' '
 	git branch -m branch1
 '
 
+test_expect_success 'checkout a branch without refs/heads/* prefix' '
+	git clone --no-tags . repo-odd-prefix &&
+	(
+		cd repo-odd-prefix &&
+
+		origin=$(git symbolic-ref refs/remotes/origin/HEAD) &&
+		git symbolic-ref refs/heads/a-branch "$origin" &&
+
+		git checkout -f a-branch &&
+		git checkout -f a-branch
+	)
+'
+
 test_expect_success 'checkout -b to a new branch, set to HEAD' '
 	test_when_finished "
 		git checkout branch1 &&

[PATCH] checkout: avoid BUG() when hitting a broken repository

[Junio C Hamano]

So, taking the two earlier comments from me together...

I _think_ I was the one who spotted the funny skip_prefix() whose
result was not used, and suggested this unrelated check, during the
review.  Sorry about that.

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----

Subject: [PATCH] checkout: avoid BUG() when hitting a broken repository

When 9081a421 (checkout: fix "branch info" memory leaks, 2021-11-16)
cleaned up existing memory leaks, we added an unrelated sanity check
to ensure that a local branch is truly local and not a symref to
elsewhere that dies with BUG() otherwise.  This was misguided in two
ways.  First of all, such a tightening did not belong to a leak-fix
patch.  And the condition it detected was *not* a bug in our program
but a problem in user data, where warning() or die() would have been
more appropriate.

As the condition is not fatal (the result of computing the local
branch name in the code that is involved in the faulty check is only
used as a textual label for the commit), let's revert the code to
the original state, i.e. strip "refs/heads/" to compute the local
branch name if possible, and otherwise leave it NULL.  The consumer
of the information in merge_working_tree() is prepared to see NULL
in there and act accordingly.

cf. https://bugzilla.redhat.com/show_bug.cgi?id=2042920

Reported-by: Petr Šplíchal <psplicha@redhat.com>
Reported-by: Todd Zullinger <tmz@pobox.com>
Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/checkout.c         |  3 ---
 t/t2018-checkout-branch.sh | 13 +++++++++++++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 43d0275187..1fb34d537d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1094,9 +1094,6 @@ static int switch_branches(const struct checkout_opts *opts,
 		const char *p;
 		if (skip_prefix(old_branch_info.path, prefix, &p))
 			old_branch_info.name = xstrdup(p);
-		else
-			BUG("should be able to skip past '%s' in '%s'!",
-			    prefix, old_branch_info.path);
 	}
 
 	if (opts->new_orphan_branch && opts->orphan_from_empty_tree) {
diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 93be1c0eae..5dda5ad4cb 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -85,6 +85,19 @@ test_expect_success 'setup' '
 	git branch -m branch1
 '
 
+test_expect_success 'checkout a branch without refs/heads/* prefix' '
+	git clone --no-tags . repo-odd-prefix &&
+	(
+		cd repo-odd-prefix &&
+
+		origin=$(git symbolic-ref refs/remotes/origin/HEAD) &&
+		git symbolic-ref refs/heads/a-branch "$origin" &&
+
+		git checkout -f a-branch &&
+		git checkout -f a-branch
+	)
+'
+
 test_expect_success 'checkout -b to a new branch, set to HEAD' '
 	test_when_finished "
 		git checkout branch1 &&
-- 
2.35.0-rc2-150-gc312dde8e9

Re: [PATCH] checkout: avoid BUG() when hitting a broken repository

[Johannes Sixt]

Am 22.01.22 um 01:58 schrieb Junio C Hamano:
> So, taking the two earlier comments from me together...
> 
> I _think_ I was the one who spotted the funny skip_prefix() whose
> result was not used, and suggested this unrelated check, during the
> review.  Sorry about that.
> 
> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
> 
> Subject: [PATCH] checkout: avoid BUG() when hitting a broken repository
> 
> When 9081a421 (checkout: fix "branch info" memory leaks, 2021-11-16)
> cleaned up existing memory leaks, we added an unrelated sanity check
> to ensure that a local branch is truly local and not a symref to
> elsewhere that dies with BUG() otherwise.  This was misguided in two
> ways.  First of all, such a tightening did not belong to a leak-fix
> patch.  And the condition it detected was *not* a bug in our program
> but a problem in user data, where warning() or die() would have been
> more appropriate.
> 
> As the condition is not fatal (the result of computing the local
> branch name in the code that is involved in the faulty check is only
> used as a textual label for the commit), let's revert the code to
> the original state, i.e. strip "refs/heads/" to compute the local
> branch name if possible, and otherwise leave it NULL.  The consumer
> of the information in merge_working_tree() is prepared to see NULL
> in there and act accordingly.
> 
> cf. https://bugzilla.redhat.com/show_bug.cgi?id=2042920
> 
> Reported-by: Petr Šplíchal <psplicha@redhat.com>
> Reported-by: Todd Zullinger <tmz@pobox.com>
> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/checkout.c         |  3 ---
>  t/t2018-checkout-branch.sh | 13 +++++++++++++
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 43d0275187..1fb34d537d 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1094,9 +1094,6 @@ static int switch_branches(const struct checkout_opts *opts,
>  		const char *p;
>  		if (skip_prefix(old_branch_info.path, prefix, &p))
>  			old_branch_info.name = xstrdup(p);
> -		else
> -			BUG("should be able to skip past '%s' in '%s'!",
> -			    prefix, old_branch_info.path);
>  	}
>  
>  	if (opts->new_orphan_branch && opts->orphan_from_empty_tree) {
> diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
> index 93be1c0eae..5dda5ad4cb 100755
> --- a/t/t2018-checkout-branch.sh
> +++ b/t/t2018-checkout-branch.sh
> @@ -85,6 +85,19 @@ test_expect_success 'setup' '
>  	git branch -m branch1
>  '
>  
> +test_expect_success 'checkout a branch without refs/heads/* prefix' '
> +	git clone --no-tags . repo-odd-prefix &&
> +	(
> +		cd repo-odd-prefix &&
> +
> +		origin=$(git symbolic-ref refs/remotes/origin/HEAD) &&
> +		git symbolic-ref refs/heads/a-branch "$origin" &&
> +
> +		git checkout -f a-branch &&
> +		git checkout -f a-branch

I haven't grasped the hairy details of the circumstances regarding this
issue, and are observing this thread only from the sideline. I wonder
whether there is a significance that there are two identical checkout
commands in a row. In particular, could the second checkout not just
switch back to main? A comment in the test case would help me and future
readers.

> +	)
> +'
> +
>  test_expect_success 'checkout -b to a new branch, set to HEAD' '
>  	test_when_finished "
>  		git checkout branch1 &&

-- Hannes

Re: [PATCH] checkout: avoid BUG() when hitting a broken repository

[Ævar Arnfjörð Bjarmason]

On Fri, Jan 21 2022, Junio C Hamano wrote:

> So, taking the two earlier comments from me together...
>
> I _think_ I was the one who spotted the funny skip_prefix() whose
> result was not used, and suggested this unrelated check, during the
> review.  Sorry about that.

Where was that? I don't see a comment like that in the original
thread[1], or do you mean the recent one in [2]?

Doesn't matter much now, but whatever it was I can't find it nor recall
it, or I've misread this.

> [...]
> +test_expect_success 'checkout a branch without refs/heads/* prefix' '
> +	git clone --no-tags . repo-odd-prefix &&
> +	(
> +		cd repo-odd-prefix &&
> +
> +		origin=$(git symbolic-ref refs/remotes/origin/HEAD) &&
> +		git symbolic-ref refs/heads/a-branch "$origin" &&
> +
> +		git checkout -f a-branch &&
> +		git checkout -f a-branch
> +	)
> +'
> +
>  test_expect_success 'checkout -b to a new branch, set to HEAD' '
>  	test_when_finished "
>  		git checkout branch1 &&

It's shortly before the release, so whatever fixes the bug is good with
me, and this patch works.

I don't think dropping the parts of the tests that actually check the
resulting repository state is a good change in this re-imagining of my
initial fix[3].

I see that per your [4] you disagree with the current behavior being
"cast in stone". I also think we should change it, I just think testing
exactly what state we're in before and after will make that easier.

Here we're just testing that we don't die, and not even that it's not a
noop. If and when we change the behavior it'll be extra work to check
that we didn't change something we didn't expect (and basically
requiring digging up [3] again).

In this case we didn't have any test coverage (hence missing the
regression), and with this test we still don't have meaningful coverage.

If you're looking to clearly mark things that are desired v.s. expected
behavior wouldn't that be better done in general via something like a
new "test_expect_oddity"?

Again, for the upcoming release I think this is fine. I'd just like to
clarify the above, since this isn't the first time we've had a back and
forth where you wanted a less specific test that (in the "make coverage"
etc. sense) would lose coverage v.s. a more specific check.

1. https://lore.kernel.org/git/patch-1.1-9b17170b794-20211014T000949Z-avarab@gmail.com/
2. https://lore.kernel.org/git/xmqqr190d2xg.fsf@gitster.g/
3. https://lore.kernel.org/git/patch-1.1-21ddf7c628d-20220120T212233Z-avarab@gmail.com/
4. https://lore.kernel.org/git/xmqqlez8d2e6.fsf@gitster.g/

Re: [PATCH] checkout: avoid BUG() when hitting a broken repository

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 3282 bytes --]

Hi Junio,

On Fri, 21 Jan 2022, Junio C Hamano wrote:

> So, taking the two earlier comments from me together...
>
> I _think_ I was the one who spotted the funny skip_prefix() whose
> result was not used, and suggested this unrelated check, during the
> review.  Sorry about that.
>
> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
>
> Subject: [PATCH] checkout: avoid BUG() when hitting a broken repository
>
> When 9081a421 (checkout: fix "branch info" memory leaks, 2021-11-16)
> cleaned up existing memory leaks, we added an unrelated sanity check
> to ensure that a local branch is truly local and not a symref to
> elsewhere that dies with BUG() otherwise.  This was misguided in two
> ways.  First of all, such a tightening did not belong to a leak-fix
> patch.  And the condition it detected was *not* a bug in our program
> but a problem in user data, where warning() or die() would have been
> more appropriate.
>
> As the condition is not fatal (the result of computing the local
> branch name in the code that is involved in the faulty check is only
> used as a textual label for the commit), let's revert the code to
> the original state, i.e. strip "refs/heads/" to compute the local
> branch name if possible, and otherwise leave it NULL.  The consumer
> of the information in merge_working_tree() is prepared to see NULL
> in there and act accordingly.
>
> cf. https://bugzilla.redhat.com/show_bug.cgi?id=2042920

The commit message, as well as the patch, look good to me.

Thank you,
Dscho

> Reported-by: Petr Šplíchal <psplicha@redhat.com>
> Reported-by: Todd Zullinger <tmz@pobox.com>
> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/checkout.c         |  3 ---
>  t/t2018-checkout-branch.sh | 13 +++++++++++++
>  2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 43d0275187..1fb34d537d 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1094,9 +1094,6 @@ static int switch_branches(const struct checkout_opts *opts,
>  		const char *p;
>  		if (skip_prefix(old_branch_info.path, prefix, &p))
>  			old_branch_info.name = xstrdup(p);
> -		else
> -			BUG("should be able to skip past '%s' in '%s'!",
> -			    prefix, old_branch_info.path);
>  	}
>
>  	if (opts->new_orphan_branch && opts->orphan_from_empty_tree) {
> diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
> index 93be1c0eae..5dda5ad4cb 100755
> --- a/t/t2018-checkout-branch.sh
> +++ b/t/t2018-checkout-branch.sh
> @@ -85,6 +85,19 @@ test_expect_success 'setup' '
>  	git branch -m branch1
>  '
>
> +test_expect_success 'checkout a branch without refs/heads/* prefix' '
> +	git clone --no-tags . repo-odd-prefix &&
> +	(
> +		cd repo-odd-prefix &&
> +
> +		origin=$(git symbolic-ref refs/remotes/origin/HEAD) &&
> +		git symbolic-ref refs/heads/a-branch "$origin" &&
> +
> +		git checkout -f a-branch &&
> +		git checkout -f a-branch
> +	)
> +'
> +
>  test_expect_success 'checkout -b to a new branch, set to HEAD' '
>  	test_when_finished "
>  		git checkout branch1 &&
> --
> 2.35.0-rc2-150-gc312dde8e9
>
>