Re: [PATCH] checkout: avoid BUG() when hitting a broken repository
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Jan 23, 2022, 16:38 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2201231738050.2121@tvgsbejvaqbjf.bet>
- In-Reply-To
- <xmqqbl04d1s9.fsf_-_@gitster.g>
Hi Junio,
On Fri, 21 Jan 2022, Junio C Hamano wrote:
Show 28 quoted lines
> 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
Show 51 quoted lines
> 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
>
>