| Message ID | xmqqbl04d1s9.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8bbb082509e826de6735dfa02922cfc3d7ffb8bc |
| Headers | show |
| Series | checkout: avoid BUG() when hitting a broken repository | expand |
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
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/
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 > >
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 &&
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(-)