| Message ID | 7894f736-4681-7656-e2d4-5945d2c71d31@web.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RESEND] branch: allow deleting dangling branches with --force | expand |
René Scharfe <l.s.r@web.de> writes: > git branch only allows deleting branches that point to valid commits. > Skip that check if --force is given, as the caller is indicating with > it that they know what they are doing and accept the consequences. > This allows deleting dangling branches, which previously had to be > reset to a valid start-point using --force first. > > Signed-off-by: René Scharfe <l.s.r@web.de> > --- > Original submission: > http://public-inbox.org/git/52847a99-db7c-9634-b3b1-fd9b1342bc32@web.de/ Thanks. > +test_expect_success 'branch --delete --force removes dangling branch' ' > + test_when_finished "rm -f .git/refs/heads/dangling" && > + echo $ZERO_OID >.git/refs/heads/dangling && > + git branch --delete --force dangling && > + test_path_is_missing .git/refs/heads/dangling > +' This goes against the spirit of the series merged at c9780bb2 (Merge branch 'hn/prep-tests-for-reftable', 2021-07-13). Can we creat the dangling ref and test the lack of "dangling" ref in the end in a less transparent way? An escape hatch is to make this test depend on the REFFILES prerequisite, just like dc474899 (t4202: mark bogus head hash test with REFFILES, 2021-05-31) did, which may be more appropriate. > test_expect_success 'use --edit-description' ' > write_script editor <<-\EOF && > echo "New contents" >"$1" > -- > 2.32.0
On Wed, Aug 25 2021, Junio C Hamano wrote: > René Scharfe <l.s.r@web.de> writes: > >> git branch only allows deleting branches that point to valid commits. >> Skip that check if --force is given, as the caller is indicating with >> it that they know what they are doing and accept the consequences. >> This allows deleting dangling branches, which previously had to be >> reset to a valid start-point using --force first. >> >> Signed-off-by: René Scharfe <l.s.r@web.de> >> --- >> Original submission: >> http://public-inbox.org/git/52847a99-db7c-9634-b3b1-fd9b1342bc32@web.de/ > > Thanks. > >> +test_expect_success 'branch --delete --force removes dangling branch' ' >> + test_when_finished "rm -f .git/refs/heads/dangling" && >> + echo $ZERO_OID >.git/refs/heads/dangling && >> + git branch --delete --force dangling && >> + test_path_is_missing .git/refs/heads/dangling >> +' > > This goes against the spirit of the series merged at c9780bb2 (Merge > branch 'hn/prep-tests-for-reftable', 2021-07-13). > > Can we creat the dangling ref and test the lack of "dangling" ref in > the end in a less transparent way? > > An escape hatch is to make this test depend on the REFFILES > prerequisite, just like dc474899 (t4202: mark bogus head hash test > with REFFILES, 2021-05-31) did, which may be more appropriate. I'm not sure, but this may also be a good example of the sort of thing that we should probably go beyond REFFILES with, i.e. is it even possible under reftable to run into this sort of situation? Not really a topic for this series, but something to make a mental note of for the reftable topic, i.e. we may eventually want to edit the docs etc. appropriately if and when the new backend is more mature.
On Wed, Aug 25 2021, René Scharfe wrote: > git branch only allows deleting branches that point to valid commits. > Skip that check if --force is given, as the caller is indicating with > it that they know what they are doing and accept the consequences. > This allows deleting dangling branches, which previously had to be > reset to a valid start-point using --force first. > > Signed-off-by: René Scharfe <l.s.r@web.de> > --- > Original submission: > http://public-inbox.org/git/52847a99-db7c-9634-b3b1-fd9b1342bc32@web.de/ > > Documentation/git-branch.txt | 3 ++- > builtin/branch.c | 2 +- > t/t3200-branch.sh | 7 +++++++ > 3 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt > index 94dc9a54f2..5449767121 100644 > --- a/Documentation/git-branch.txt > +++ b/Documentation/git-branch.txt > @@ -118,7 +118,8 @@ OPTIONS > Reset <branchname> to <startpoint>, even if <branchname> exists > already. Without `-f`, 'git branch' refuses to change an existing branch. > In combination with `-d` (or `--delete`), allow deleting the > - branch irrespective of its merged status. In combination with > + branch irrespective of its merged status, or whether it even > + points to a valid commit. In combination with > `-m` (or `--move`), allow renaming the branch even if the new > branch name already exists, the same applies for `-c` (or `--copy`). > > diff --git a/builtin/branch.c b/builtin/branch.c > index b23b1d1752..03c7b7253a 100644 > --- a/builtin/branch.c > +++ b/builtin/branch.c > @@ -168,7 +168,7 @@ static int check_branch_commit(const char *branchname, const char *refname, > int kinds, int force) > { > struct commit *rev = lookup_commit_reference(the_repository, oid); > - if (!rev) { > + if (!force && !rev) { > error(_("Couldn't look up commit object for '%s'"), refname); > return -1; > } > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh > index cc4b10236e..ec61a10c29 100755 > --- a/t/t3200-branch.sh > +++ b/t/t3200-branch.sh > @@ -1272,6 +1272,13 @@ test_expect_success 'attempt to delete a branch merged to its base' ' > test_must_fail git branch -d my10 > ' > > +test_expect_success 'branch --delete --force removes dangling branch' ' > + test_when_finished "rm -f .git/refs/heads/dangling" && > + echo $ZERO_OID >.git/refs/heads/dangling && > + git branch --delete --force dangling && > + test_path_is_missing .git/refs/heads/dangling > +' Isn't a more meaningful test here to use a "real" SHA, instead of the $ZERO_OID? You can use $(test_oid deadbeef) to get one of those. That way we know that this this test & logic is really testing that we can delete a branch that's been racily GC'd away or whatever, and not one in the already-broken state of referring to the $ZERO_OID. Also: How does "git tag -d" handle this scenario if the same sort of data were added to .git/refs/tags/* ?
On Wed, Aug 25, 2021 at 11:37 PM Junio C Hamano <gitster@pobox.com> wrote: > > +test_expect_success 'branch --delete --force removes dangling branch' ' > > + test_when_finished "rm -f .git/refs/heads/dangling" && > > + echo $ZERO_OID >.git/refs/heads/dangling && > > + git branch --delete --force dangling && > > + test_path_is_missing .git/refs/heads/dangling > > +' > > This goes against the spirit of the series merged at c9780bb2 (Merge > branch 'hn/prep-tests-for-reftable', 2021-07-13). > > Can we creat the dangling ref and test the lack of "dangling" ref in > the end in a less transparent way? agreed. Try the ref-store test-helper's update-ref command?
Han-Wen Nienhuys <hanwen@google.com> writes: > On Wed, Aug 25, 2021 at 11:37 PM Junio C Hamano <gitster@pobox.com> wrote: > >> > +test_expect_success 'branch --delete --force removes dangling branch' ' >> > + test_when_finished "rm -f .git/refs/heads/dangling" && >> > + echo $ZERO_OID >.git/refs/heads/dangling && >> > + git branch --delete --force dangling && >> > + test_path_is_missing .git/refs/heads/dangling >> > +' >> >> This goes against the spirit of the series merged at c9780bb2 (Merge >> branch 'hn/prep-tests-for-reftable', 2021-07-13). >> >> Can we creat the dangling ref and test the lack of "dangling" ref in >> the end in a less transparent way? > > agreed. Try the ref-store test-helper's update-ref command? I thought the approach taken by dc474899 (t4202: mark bogus head hash test with REFFILES, 2021-05-31) to hide it behind a prerequisite was good enough, but if we can ensure the same behaviour under the reftable backend, that is even better. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Han-Wen Nienhuys <hanwen@google.com> writes: > >> On Wed, Aug 25, 2021 at 11:37 PM Junio C Hamano <gitster@pobox.com> wrote: >> >>> > +test_expect_success 'branch --delete --force removes dangling branch' ' >>> > + test_when_finished "rm -f .git/refs/heads/dangling" && >>> > + echo $ZERO_OID >.git/refs/heads/dangling && >>> > + git branch --delete --force dangling && >>> > + test_path_is_missing .git/refs/heads/dangling >>> > +' >>> >>> This goes against the spirit of the series merged at c9780bb2 (Merge >>> branch 'hn/prep-tests-for-reftable', 2021-07-13). >>> >>> Can we creat the dangling ref and test the lack of "dangling" ref in >>> the end in a less transparent way? >> >> agreed. Try the ref-store test-helper's update-ref command? > > I thought the approach taken by dc474899 (t4202: mark bogus head > hash test with REFFILES, 2021-05-31) to hide it behind a > prerequisite was good enough, but if we can ensure the same > behaviour under the reftable backend, that is even better. > > Thanks. Having said that, there are a few observations to make about this test script. * It is hopefully becoming harder and harder to check for behaviour in broken repositories in a "portable" way, simply because we are making it harder to corrupt repository. We hopefully won't point a ref to point at a missing object, we hopefully won't prune an object away that is still pointed at by a ref, etc. * This script to test "branch" is full of tests that rely on direct manipulation of .git/refs/ filesystem hierarchy. For these two reasons, it probably is OK to accept this patch as-is and leave the "clean-up" to a later follow-on series, that would cover both "what's our recommended approach to 'corrupt' the test repository so that we can use different ref (and other) backends?" and "make sure the tests in the script are happy with both ref backends." issues. Thanks.
Am 26.08.21 um 09:26 schrieb Han-Wen Nienhuys: > On Wed, Aug 25, 2021 at 11:37 PM Junio C Hamano <gitster@pobox.com> wrote: > >>> +test_expect_success 'branch --delete --force removes dangling branch' ' >>> + test_when_finished "rm -f .git/refs/heads/dangling" && >>> + echo $ZERO_OID >.git/refs/heads/dangling && >>> + git branch --delete --force dangling && >>> + test_path_is_missing .git/refs/heads/dangling >>> +' >> >> This goes against the spirit of the series merged at c9780bb2 (Merge >> branch 'hn/prep-tests-for-reftable', 2021-07-13). I assume the file backend won't go away anytime soon. So I guess the idea is that the test suite is supposed to be run with the new backend as default and exercise it? >> Can we creat the dangling ref and test the lack of "dangling" ref in >> the end in a less transparent way? > > agreed. Try the ref-store test-helper's update-ref command? It requires the new hash to refer to an existing object, so we can't use it in this test. René
Am 26.08.21 um 01:28 schrieb Ævar Arnfjörð Bjarmason: > > On Wed, Aug 25 2021, Junio C Hamano wrote: > >> René Scharfe <l.s.r@web.de> writes: >> >>> git branch only allows deleting branches that point to valid commits. >>> Skip that check if --force is given, as the caller is indicating with >>> it that they know what they are doing and accept the consequences. >>> This allows deleting dangling branches, which previously had to be >>> reset to a valid start-point using --force first. >>> >>> Signed-off-by: René Scharfe <l.s.r@web.de> >>> --- >>> Original submission: >>> http://public-inbox.org/git/52847a99-db7c-9634-b3b1-fd9b1342bc32@web.de/ >> >> Thanks. >> >>> +test_expect_success 'branch --delete --force removes dangling branch' ' >>> + test_when_finished "rm -f .git/refs/heads/dangling" && >>> + echo $ZERO_OID >.git/refs/heads/dangling && >>> + git branch --delete --force dangling && >>> + test_path_is_missing .git/refs/heads/dangling >>> +' >> >> This goes against the spirit of the series merged at c9780bb2 (Merge >> branch 'hn/prep-tests-for-reftable', 2021-07-13). >> >> Can we creat the dangling ref and test the lack of "dangling" ref in >> the end in a less transparent way? >> >> An escape hatch is to make this test depend on the REFFILES >> prerequisite, just like dc474899 (t4202: mark bogus head hash test >> with REFFILES, 2021-05-31) did, which may be more appropriate. > > I'm not sure, but this may also be a good example of the sort of thing > that we should probably go beyond REFFILES with, i.e. is it even > possible under reftable to run into this sort of situation? Probably yes: A commit can disappear when its object file or pack or alternate object database gets lost somehow, and a ref store could only compensate for that loss if it kept a copy of the ref target, which seems impractical. René
Am 26.08.21 um 01:30 schrieb Ævar Arnfjörð Bjarmason: > > On Wed, Aug 25 2021, René Scharfe wrote: > >> git branch only allows deleting branches that point to valid commits. >> Skip that check if --force is given, as the caller is indicating with >> it that they know what they are doing and accept the consequences. >> This allows deleting dangling branches, which previously had to be >> reset to a valid start-point using --force first. >> >> Signed-off-by: René Scharfe <l.s.r@web.de> >> --- >> Original submission: >> http://public-inbox.org/git/52847a99-db7c-9634-b3b1-fd9b1342bc32@web.de/ >> >> Documentation/git-branch.txt | 3 ++- >> builtin/branch.c | 2 +- >> t/t3200-branch.sh | 7 +++++++ >> 3 files changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt >> index 94dc9a54f2..5449767121 100644 >> --- a/Documentation/git-branch.txt >> +++ b/Documentation/git-branch.txt >> @@ -118,7 +118,8 @@ OPTIONS >> Reset <branchname> to <startpoint>, even if <branchname> exists >> already. Without `-f`, 'git branch' refuses to change an existing branch. >> In combination with `-d` (or `--delete`), allow deleting the >> - branch irrespective of its merged status. In combination with >> + branch irrespective of its merged status, or whether it even >> + points to a valid commit. In combination with >> `-m` (or `--move`), allow renaming the branch even if the new >> branch name already exists, the same applies for `-c` (or `--copy`). >> >> diff --git a/builtin/branch.c b/builtin/branch.c >> index b23b1d1752..03c7b7253a 100644 >> --- a/builtin/branch.c >> +++ b/builtin/branch.c >> @@ -168,7 +168,7 @@ static int check_branch_commit(const char *branchname, const char *refname, >> int kinds, int force) >> { >> struct commit *rev = lookup_commit_reference(the_repository, oid); >> - if (!rev) { >> + if (!force && !rev) { >> error(_("Couldn't look up commit object for '%s'"), refname); >> return -1; >> } >> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh >> index cc4b10236e..ec61a10c29 100755 >> --- a/t/t3200-branch.sh >> +++ b/t/t3200-branch.sh >> @@ -1272,6 +1272,13 @@ test_expect_success 'attempt to delete a branch merged to its base' ' >> test_must_fail git branch -d my10 >> ' >> >> +test_expect_success 'branch --delete --force removes dangling branch' ' >> + test_when_finished "rm -f .git/refs/heads/dangling" && >> + echo $ZERO_OID >.git/refs/heads/dangling && >> + git branch --delete --force dangling && >> + test_path_is_missing .git/refs/heads/dangling >> +' > > Isn't a more meaningful test here to use a "real" SHA, instead of the > $ZERO_OID? You can use $(test_oid deadbeef) to get one of those. > > That way we know that this this test & logic is really testing that we > can delete a branch that's been racily GC'd away or whatever, and not > one in the already-broken state of referring to the $ZERO_OID. Right, git branch --delete could cheat by treating all-zero object IDs specially, and the test would then not exercise the original scenario. > Also: How does "git tag -d" handle this scenario if the same sort of > data were added to .git/refs/tags/* ? It deletes that tag, no --force needed. René
>>> Junio C Hamano <gitster@pobox.com> schrieb am 26.08.2021 um 19:38 in Nachricht <xmqq1r6gf6ne.fsf@gitster.g>: ... > * It is hopefully becoming harder and harder to check for behaviour > in broken repositories in a "portable" way, simply because we are > making it harder to corrupt repository. We hopefully won't point > a ref to point at a missing object, we hopefully won't prune an > object away that is still pointed at by a ref, etc. ... Maybe git needs a "--disarm-safety-belt" option that disables all those nice checks for testing purposes ;-)
On Fri, Aug 27 2021, Ulrich Windl wrote: >>>> Junio C Hamano <gitster@pobox.com> schrieb am 26.08.2021 um 19:38 in Nachricht > <xmqq1r6gf6ne.fsf@gitster.g>: > > ... >> * It is hopefully becoming harder and harder to check for behaviour >> in broken repositories in a "portable" way, simply because we are >> making it harder to corrupt repository. We hopefully won't point >> a ref to point at a missing object, we hopefully won't prune an >> object away that is still pointed at by a ref, etc. > ... > > Maybe git needs a "--disarm-safety-belt" option that disables all those nice checks for testing purposes ;-) I haven't tested, but I think in both of those cases a way to accomplish this corruption in a way that bypasses the safety of our tooling is also to setup an alternate object directory with the relevant object(s), and then simply drop that alternate to simulate the case of an object disappearing or other such corruption.
diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt index 94dc9a54f2..5449767121 100644 --- a/Documentation/git-branch.txt +++ b/Documentation/git-branch.txt @@ -118,7 +118,8 @@ OPTIONS Reset <branchname> to <startpoint>, even if <branchname> exists already. Without `-f`, 'git branch' refuses to change an existing branch. In combination with `-d` (or `--delete`), allow deleting the - branch irrespective of its merged status. In combination with + branch irrespective of its merged status, or whether it even + points to a valid commit. In combination with `-m` (or `--move`), allow renaming the branch even if the new branch name already exists, the same applies for `-c` (or `--copy`). diff --git a/builtin/branch.c b/builtin/branch.c index b23b1d1752..03c7b7253a 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -168,7 +168,7 @@ static int check_branch_commit(const char *branchname, const char *refname, int kinds, int force) { struct commit *rev = lookup_commit_reference(the_repository, oid); - if (!rev) { + if (!force && !rev) { error(_("Couldn't look up commit object for '%s'"), refname); return -1; } diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index cc4b10236e..ec61a10c29 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -1272,6 +1272,13 @@ test_expect_success 'attempt to delete a branch merged to its base' ' test_must_fail git branch -d my10 ' +test_expect_success 'branch --delete --force removes dangling branch' ' + test_when_finished "rm -f .git/refs/heads/dangling" && + echo $ZERO_OID >.git/refs/heads/dangling && + git branch --delete --force dangling && + test_path_is_missing .git/refs/heads/dangling +' + test_expect_success 'use --edit-description' ' write_script editor <<-\EOF && echo "New contents" >"$1"
git branch only allows deleting branches that point to valid commits. Skip that check if --force is given, as the caller is indicating with it that they know what they are doing and accept the consequences. This allows deleting dangling branches, which previously had to be reset to a valid start-point using --force first. Signed-off-by: René Scharfe <l.s.r@web.de> --- Original submission: http://public-inbox.org/git/52847a99-db7c-9634-b3b1-fd9b1342bc32@web.de/ Documentation/git-branch.txt | 3 ++- builtin/branch.c | 2 +- t/t3200-branch.sh | 7 +++++++ 3 files changed, 10 insertions(+), 2 deletions(-) -- 2.32.0