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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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