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

["René Scharfe" <l.s.r@web.de>]

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é