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

From: René Scharfe <l.s.r@web.de>
Date: Aug 26, 2021, 21:01 UTC
Message-ID: <9c999d1f-9263-965d-a0a1-527e1260a6d9@web.de>
In-Reply-To: <xmqqk0k8do27.fsf@gitster.g>

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é