[PATCH RESEND] 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.

Signed-off-by: René Scharfe <redacted>
---
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

On Wed, Aug 25 2021, René Scharfe wrote:

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/* ?

Am 26.08.21 um 01:30 schrieb Ævar Arnfjörð Bjarmason:

On Wed, Aug 25 2021, René Scharfe wrote:

quoted

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 <redacted>
---
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é

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 <redacted>
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: René Scharfe <redacted>
---
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

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

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 <redacted>
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: René Scharfe <redacted>
---
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