threads / patch / 56378

patchbranch: allow deleting dangling branches with --force

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

## tl;dr

17 messages between Aug 25, 2021 and Aug 27, 2021. Diffs are folded; open one to read it.

replies: 16people: 5as markdown or json

René Scharfe· Aug 25, 2021, 20:43 UTC · lore

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(-)
Show changes to 3 files +10 −3

Documentation/git-branch.txt, builtin/branch.c, t/t3200-branch.sh

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
Junio C Hamano· Aug 25, 2021, 21:37 UTC · re: René Scharfe · lore

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

René Scharfe <l.s.r@web.de> writes:
Show 10 quoted lines
> 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.
Show 6 quoted lines
> +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.

Show 5 quoted lines
>  test_expect_success 'use --edit-description' '
>  	write_script editor <<-\EOF &&
>  		echo "New contents" >"$1"
> --
> 2.32.0
Ævar Arnfjörð Bjarmason· Aug 25, 2021, 23:28 UTC · re: Junio C Hamano · lore

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

On Wed, Aug 25 2021, Junio C Hamano wrote:
Show 31 quoted lines
> 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.

René Scharfe· Aug 26, 2021, 18:19 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Am 26.08.21 um 01:28 schrieb Ævar Arnfjörð Bjarmason:
Show 38 quoted lines
>
> 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é
Han-Wen Nienhuys· Aug 26, 2021, 07:26 UTC · re: Junio C Hamano · lore

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

On Wed, Aug 25, 2021 at 11:37 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
> > +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
Junio C Hamano· Aug 26, 2021, 16:54 UTC · re: Han-Wen Nienhuys · lore

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

Han-Wen Nienhuys <hanwen@google.com> writes:
Show 16 quoted lines
> 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· Aug 26, 2021, 17:38 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 25 quoted lines
> 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.
Ulrich Windl· Aug 27, 2021, 07:24 UTC · re: Junio C Hamano · lore

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

>>> Junio C Hamano <gitster@pobox.com> schrieb am 26.08.2021 um 19:38 in Nachricht
<xmqq1r6gf6ne.fsf@gitster.g>:
...
Show 5 quoted lines
>  * 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 ;-)
Ævar Arnfjörð Bjarmason· Aug 27, 2021, 07:53 UTC · re: Ulrich Windl · lore

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

On Fri, Aug 27 2021, Ulrich Windl wrote:
Show 12 quoted lines
>>>> 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.

René Scharfe· Aug 26, 2021, 18:18 UTC · re: Han-Wen Nienhuys · lore

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

Am 26.08.21 um 09:26 schrieb Han-Wen Nienhuys:
Show 11 quoted lines
> 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é
Ævar Arnfjörð Bjarmason· Aug 25, 2021, 23:30 UTC · re: René Scharfe · lore

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

On Wed, Aug 25 2021, René Scharfe wrote:
Show 57 quoted lines
> 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/* ?
René Scharfe· Aug 26, 2021, 18:19 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Am 26.08.21 um 01:30 schrieb Ævar Arnfjörð Bjarmason:
Show 67 quoted lines
>
> 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é
René Scharfe· Aug 26, 2021, 18:19 UTC · re: René Scharfe · lore

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

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(-)
Show changes to 3 files +15 −3

Documentation/git-branch.txt, builtin/branch.c, t/t3200-branch.sh

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
Junio C Hamano· Aug 26, 2021, 19:05 UTC · re: René Scharfe · lore

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

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.
René Scharfe· Aug 26, 2021, 21:01 UTC · re: Junio C Hamano · lore

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

Am 26.08.21 um 21:05 schrieb Junio C Hamano:
Show 8 quoted lines
> 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.

Show 10 quoted lines
>> +	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é
Ævar Arnfjörð Bjarmason· Aug 26, 2021, 19:12 UTC · re: René Scharfe · lore

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

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

René Scharfe· Aug 27, 2021, 18:35 UTC · re: René Scharfe · lore

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

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(-)
Show changes to 3 files +16 −3

Documentation/git-branch.txt, builtin/branch.c, t/t3200-branch.sh

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

← back to recent threads