Re: [PATCH v3 3/6] update-ref: add support for 'symref-delete' command

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 4052 bytes --]

On Thu, May 30, 2024 at 03:09:37PM +0300, Karthik Nayak wrote:
> diff --git a/refs.c b/refs.c
> index cdc4d25557..e29abebe1d 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -950,7 +950,7 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,
>  	transaction = ref_store_transaction_begin(refs, &err);
>  	if (!transaction ||
>  	    ref_transaction_delete(transaction, refname, old_oid,
> -				   flags, msg, &err) ||
> +				   flags, NULL, msg, &err) ||
>  	    ref_transaction_commit(transaction, &err)) {
>  		error("%s", err.buf);
>  		ref_transaction_free(transaction);
> @@ -1283,14 +1283,20 @@ int ref_transaction_create(struct ref_transaction *transaction,
>  int ref_transaction_delete(struct ref_transaction *transaction,
>  			   const char *refname,
>  			   const struct object_id *old_oid,
> -			   unsigned int flags, const char *msg,
> +			   unsigned int flags,
> +			   const char *old_target,

The old target is somewhat weirdly placed here, as I'd have expected it
to live next to `old_oid`. That's only a minor nit, nothing that's worth
a reroll in my opinion.

> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index 52801be07d..848d6fc42e 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -1731,6 +1731,60 @@ do
>  		test_cmp expect actual
>  	'
>  
> +	test_expect_success "stdin $type symref-delete fails without --no-deref" '
> +		git symbolic-ref refs/heads/symref $a &&
> +		format_command $type "symref-delete refs/heads/symref" "$a" >stdin &&
> +		test_must_fail git update-ref --stdin $type <stdin 2>err &&
> +		grep "fatal: symref-delete: cannot operate with deref mode" err
> +	'
> +
> +	test_expect_success "stdin $type symref-delete fails with no ref" '
> +		format_command $type "symref-delete " >stdin &&
> +		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
> +		grep "fatal: symref-delete: missing <ref>" err
> +	'
> +
> +	test_expect_success "stdin $type symref-delete fails with too many arguments" '
> +		format_command $type "symref-delete refs/heads/symref" "$a" "$a" >stdin &&
> +		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
> +		if test "$type" = "-z"
> +		then
> +			grep "fatal: unknown command: $a" err
> +		else
> +			grep "fatal: symref-delete refs/heads/symref: extra input:  $a" err
> +		fi
> +	'
> +
> +	test_expect_success "stdin $type symref-delete fails with wrong old value" '
> +		format_command $type "symref-delete refs/heads/symref" "$m" >stdin &&
> +		test_must_fail git update-ref --stdin $type --no-deref <stdin 2>err &&
> +		grep "fatal: verifying symref target: ${SQ}refs/heads/symref${SQ}: is at $a but expected refs/heads/main" err &&
> +		git symbolic-ref refs/heads/symref >expect &&
> +		echo $a >actual &&
> +		test_cmp expect actual
> +	'
> +
> +	test_expect_success "stdin $type symref-delete works with right old value" '
> +		format_command $type "symref-delete refs/heads/symref" "$a" >stdin &&
> +		git update-ref --stdin $type --no-deref <stdin &&
> +		test_must_fail git rev-parse --verify -q refs/heads/symref
> +	'
> +
> +	test_expect_success "stdin $type symref-delete works with empty old value" '
> +		git symbolic-ref refs/heads/symref $a >stdin &&
> +		format_command $type "symref-delete refs/heads/symref" "" >stdin &&
> +		git update-ref --stdin $type --no-deref <stdin &&
> +		test_must_fail git rev-parse --verify -q $b
> +	'
> +
> +	test_expect_success "stdin $type symref-delete succeeds for dangling reference" '
> +		test_must_fail git symbolic-ref refs/heads/nonexistent &&
> +		git symbolic-ref refs/heads/symref2 refs/heads/nonexistent &&
> +		format_command $type "symref-delete refs/heads/symref2" "refs/heads/nonexistent" >stdin &&
> +		git update-ref --stdin $type --no-deref <stdin &&
> +		test_must_fail git symbolic-ref -d refs/heads/symref2
> +	'
> +

Not sure whether I overlooked it, but one test I missed was when trying
to delete a non-symbolic-ref.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]