[PATCH RESEND] branch: allow deleting dangling branches with --force

[PATCH RESEND] branch: allow deleting dangling branches with --force

[René Scharfe]

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
+'
+
 test_expect_success 'use --edit-description' '
 	write_script editor <<-\EOF &&
 		echo "New contents" >"$1"
--
2.32.0

Re: [PATCH RESEND] branch: allow deleting dangling branches with --force

[Junio C Hamano]

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

Re: [PATCH RESEND] branch: allow deleting dangling branches with --force

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

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.

Re: [PATCH RESEND] branch: allow deleting dangling branches with --force

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

Also: How does "git tag -d" handle this scenario if the same sort of
data were added to .git/refs/tags/* ?

Re: [PATCH RESEND] branch: allow deleting dangling branches with --force

[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).
>
> 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 - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Re: [PATCH RESEND] branch: allow deleting dangling branches with --force

[Junio C Hamano]

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.

Re: [PATCH RESEND] branch: allow deleting dangling branches with --force

[Junio C Hamano]

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.

Re: [PATCH RESEND] branch: allow deleting dangling branches with --force

[René Scharfe]

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é

Re: [PATCH RESEND] branch: allow deleting dangling branches with --force

[René Scharfe]

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é

Re: [PATCH RESEND] branch: allow deleting dangling branches with --force

[René Scharfe]

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é

[PATCH v2] branch: allow deleting dangling branches with --force

[René Scharfe]

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.

Reported-by: Ulrich Windl <Ulrich.Windl@rz.uni-regensburg.de>
Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
Changes since v1:
- Added Reported-by and Helped-by.
- Made test independent of ref store.

 Documentation/git-branch.txt |  3 ++-
 builtin/branch.c             |  2 +-
 t/t3200-branch.sh            | 12 ++++++++++++
 3 files changed, 15 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..d0d28c8ea7 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1272,6 +1272,18 @@ 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' '
+	git checkout main &&
+	test_commit unstable &&
+	hash=$(git rev-parse HEAD) &&
+	objpath=$(echo $hash | sed -e "s|^..|.git/objects/&/|") &&
+	git branch --no-track dangling &&
+	test_when_finished "test -f $objpath.x && mv $objpath.x $objpath" &&
+	mv $objpath $objpath.x &&
+	git branch --delete --force dangling &&
+	test -z "$(git for-each-ref refs/heads/dangling)"
+'
+
 test_expect_success 'use --edit-description' '
 	write_script editor <<-\EOF &&
 		echo "New contents" >"$1"
--
2.33.0

Re: [PATCH v2] branch: allow deleting dangling branches with --force

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> +	hash=$(git rev-parse HEAD) &&
> +	objpath=$(echo $hash | sed -e "s|^..|.git/objects/&/|") &&
> +	git branch --no-track dangling &&
> +	test_when_finished "test -f $objpath.x && mv $objpath.x $objpath" &&

Do we need test -f here?

> +	mv $objpath $objpath.x &&
> +	git branch --delete --force dangling &&

> +	test -z "$(git for-each-ref refs/heads/dangling)"

It is not wrong per-se, but maybe

	git show-ref --quiet refs/heads/dangling

is more straight-forward.

Thanks.

Re: [PATCH v2] branch: allow deleting dangling branches with --force

[Ævar Arnfjörð Bjarmason]

On Thu, Aug 26 2021, René Scharfe wrote:

> - Added Reported-by and Helped-by.

Thanks, this whole thing looks good to me.

> - Made test independent of ref store.

Also thanks. Just my 0.02: I think even with v1 this patch is fine to go
in (but thanks for the re-roll!). I.e. under a full run of the testsuite
with reftable a bunch of things are broken currently.

It's not really that much more effort to just fix up code like in the v1
of this patch when we get to fixing those with the reftable integration,
and putting the onus on patch authors on testing that topic in "seen"
with their tests is probably not a good time investment overall
v.s. just fixing them in bulk later.

Particularly since in this case we can make it refstore independent,
since it's about a disappearing loose object, but in some other cases
it's either the whole test that needs to be skipped, or we'd be better
off with some helpers to produce the corruption in one way under
REFFILES, and in another way under !REFFILES....

Re: [PATCH v2] branch: allow deleting dangling branches with --force

[René Scharfe]

Am 26.08.21 um 21:05 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> +	hash=$(git rev-parse HEAD) &&
>> +	objpath=$(echo $hash | sed -e "s|^..|.git/objects/&/|") &&
>> +	git branch --no-track dangling &&
>> +	test_when_finished "test -f $objpath.x && mv $objpath.x $objpath" &&
>
> Do we need test -f here?

If the mv in the next line fails, then test in the cleanup prevents it
from adding another confusing error.  So it's not really needed, but
kinda nice to have.

>> +	mv $objpath $objpath.x &&
>> +	git branch --delete --force dangling &&
>
>> +	test -z "$(git for-each-ref refs/heads/dangling)"
>
> It is not wrong per-se, but maybe
>
> 	git show-ref --quiet refs/heads/dangling
>
> is more straight-forward.

Actually it *is* wrong, because that check passes even if the dangling
ref still exists due to for-each-ref checking if the ref target exists
and just erroring out if it doesn't.  I somehow assumed it wouldn't do
this needless verification.  So we'd need to check its return value:

	git for-each-ref refs/heads/dangling >actual &&
	test_must_be_empty actual

git show-ref fails both if the ref is missing and if it's dangling, so
we'd need to check its stderr to distinguish between those cases:

	test_must_fail git show-ref --quiet refs/heads/dangling 2>err &&
	test_must_be_empty err

To avoid these complications we could ask git branch itself:

	test -z $(git branch --list dangling)

René

Antw: [EXT] Re: [PATCH RESEND] branch: allow deleting dangling branches with ‑‑force

[Ulrich Windl]

>>> 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 ;-)

Re: Antw: [EXT] Re: [PATCH RESEND] branch: allow deleting dangling branches with ‑‑force

[Ævar Arnfjörð Bjarmason]

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.

[PATCH v3] branch: allow deleting dangling branches with --force

[René Scharfe]

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.

Reported-by: Ulrich Windl <Ulrich.Windl@rz.uni-regensburg.de>
Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
Changes since v2:
- move test_when_finished down to avoid need for test -f
- check return code of git for-each-ref  in test to distinguish
  between a deleted and a still existing, but dangling branch

 Documentation/git-branch.txt |  3 ++-
 builtin/branch.c             |  2 +-
 t/t3200-branch.sh            | 13 +++++++++++++
 3 files changed, 16 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..e575ffb4ff 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1272,6 +1272,19 @@ 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' '
+	git checkout main &&
+	test_commit unstable &&
+	hash=$(git rev-parse HEAD) &&
+	objpath=$(echo $hash | sed -e "s|^..|.git/objects/&/|") &&
+	git branch --no-track dangling &&
+	mv $objpath $objpath.x &&
+	test_when_finished "mv $objpath.x $objpath" &&
+	git branch --delete --force dangling &&
+	git for-each-ref refs/heads/dangling >actual &&
+	test_must_be_empty actual
+'
+
 test_expect_success 'use --edit-description' '
 	write_script editor <<-\EOF &&
 		echo "New contents" >"$1"
--
2.33.0