Re: [PATCH v2 2/7] update-ref: add support for symref-verify

>> +symref-verify::
>> +	Verify symbolic <ref> against <old-ref> but do not change it.
>> +	If <old-ref> is missing, the ref must not exist.  Can only be
>> +	used in `no-deref` mode.
>
> Should this say "is zero or missing", like the comment for "verify"
> does?
>

> [snip]
>> @@ -297,11 +321,48 @@ static void parse_cmd_verify(struct ref_transaction *transaction,
>>  		die("verify %s: extra input: %s", refname, next);
>>
>>  	if (ref_transaction_verify(transaction, refname, &old_oid,
>> -				   update_flags, &err))
>> +				   NULL, update_flags, &err))
>> +		die("%s", err.buf);
>> +
>> +	update_flags = default_flags;
>> +	free(refname);
>> +	strbuf_release(&err);
>> +}
>> +
>> +static void parse_cmd_symref_verify(struct ref_transaction *transaction,
>> +                                    const char *next, const char *end)
>> +{
>> +	struct strbuf err = STRBUF_INIT;
>> +	struct object_id old_oid;
>> +	char *refname, *old_ref;
>> +
>> +	if (!(update_flags & REF_NO_DEREF))
>> +		die("symref-verify: cannot operate with deref mode");
>
> This feels quite restrictive to me. Wouldn't it be preferable to simply
> ignore `REF_NO_DEREF` here? It basically means that this command can't
> ever be used in a normal `git update-ref --stdin` session.
>

>>
>>  	update->flags = flags;
>>
>> -	if (flags & REF_HAVE_NEW)
>> -		oidcpy(&update->new_oid, new_oid);
>> -	if (flags & REF_HAVE_OLD)
>> -		oidcpy(&update->old_oid, old_oid);
>> +	/*
>> +	 * The ref values are to be considered over the oid values when we're
>> +	 * doing symref operations.
>> +	 */
>
> I feel like this is a statement that should be backed up by a deeper
> explanation of why that is. I'm still wondering here why we cannot
> assert that the old value is an object ID when I want to update it to a
> symref, or alternatively why it would even be possible to have both
> `REF_SYMREF_UPDATE` and a set of other, incompatible fields set. It
> feels like this should be a `BUG()` instead if this is supposedly an
> unsupported configuration rather than silently ignoring it.
>
> In any case, I feel like it would be easier to reason about if this was
> introduced together with the actual user. As far as I can see this code
> shouldn't ever be hit for "verify-symref", right? Currently, the reader
> is forced to figure out what is and isn't related to the new command.
>

>> @@ -2464,8 +2495,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>>  			       struct strbuf *err)
>>  {
>>  	struct strbuf referent = STRBUF_INIT;
>> -	int mustexist = (update->flags & REF_HAVE_OLD) &&
>> -		!is_null_oid(&update->old_oid);
>> +	int mustexist = (update->flags & REF_HAVE_OLD) && !is_null_oid(&update->old_oid);
>
> This change is a no-op, right? If so, let's rather drop it.
>

>>  	int ret = 0;
>>  	struct ref_lock *lock;
>>
>> @@ -2514,6 +2544,18 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>>  					ret = TRANSACTION_GENERIC_ERROR;
>>  					goto out;
>>  				}
>> +			}
>> +
>> +			/*
>> +			 * For symref verification, we need to check the referent value
>> +			 * rather than the oid. If we're dealing with regular refs or we're
>> +			 * verifying a dereferenced symref, we then check the oid.
>> +			 */
>> +			if (update->flags & REF_SYMREF_UPDATE && update->old_ref) {
>> +				if (check_old_ref(update, referent.buf, err)) {
>> +					ret = TRANSACTION_GENERIC_ERROR;
>> +					goto out;
>> +				}
>>  			} else if (check_old_oid(update, &lock->old_oid, err)) {
>>  				ret = TRANSACTION_GENERIC_ERROR;
>>  				goto out;
>> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
>> index 4c5fe02687..21c6b940d8 100644
>> --- a/refs/refs-internal.h
>> +++ b/refs/refs-internal.h
>> @@ -749,4 +749,11 @@ void base_ref_store_init(struct ref_store *refs, struct repository *repo,
>>   */
>>  struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_store *store);
>>
>> +/*
>> + * Helper function to check if the new value is null, this
>> + * takes into consideration that the update could be a regular
>> + * ref or a symbolic ref.
>> + */
>> +int null_new_value(struct ref_update *update);
>
> When adding it to the header we should probably prefix this to avoid
> name collisions. `ref_update_is_null_new_value()` might be a mouth full,
> but feels preferable to me.
>

>>  #endif /* REFS_REFS_INTERNAL_H */
>> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
>> index 6104471199..7a03922c7b 100644
>> --- a/refs/reftable-backend.c
>> +++ b/refs/reftable-backend.c
>> @@ -938,7 +938,28 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
>>  		 * individual refs. But the error messages match what the files
>>  		 * backend returns, which keeps our tests happy.
>>  		 */
>> -		if (u->flags & REF_HAVE_OLD && !oideq(&current_oid, &u->old_oid)) {
>> +		if ((u->flags & REF_HAVE_OLD) &&
>> +		    (u->flags & REF_SYMREF_UPDATE) &&
>> +		    u->old_ref) {
>> +			if   (strcmp(referent.buf, u->old_ref)) {
>
> s/   / /
>
>> +				if (!strcmp(u->old_ref, ""))
>> +					strbuf_addf(err, "cannot lock ref '%s': "
>> +						    "reference already exists",
>> +						    original_update_refname(u));
>> +				else if (!strcmp(referent.buf, ""))
>> +					strbuf_addf(err, "cannot lock ref '%s': "
>> +						    "reference is missing but expected %s",
>> +						    original_update_refname(u),
>> +						    u->old_ref);
>> +				else
>> +					strbuf_addf(err, "cannot lock ref '%s': "
>> +						    "is at %s but expected %s",
>> +						    original_update_refname(u),
>> +						    referent.buf, u->old_ref);
>
> I'd use better-matching error messages here. I know that we talk about
> "cannot lock ref" in the next branch, as well. But the only reason we
> did this is to have the same error messages as the "files" backend.
> Semantically, those errors don't make much sense as the "reftable"
> backend never locks specific refs, but only the complete stack.
>

>> +				ret = -1;
>> +				goto done;
>> +			}
>> +		} else if (u->flags & REF_HAVE_OLD && !oideq(&current_oid, &u->old_oid)) {
>>  			if (is_null_oid(&u->old_oid))
>>  				strbuf_addf(err, _("cannot lock ref '%s': "
>>  					    "reference already exists"),
>> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
>> index ec3443cc87..d8ffda4096 100755
>> --- a/t/t1400-update-ref.sh
>> +++ b/t/t1400-update-ref.sh
>> @@ -890,17 +890,23 @@ test_expect_success 'stdin update/create/verify combination works' '
>>  '
>>
>>  test_expect_success 'stdin verify succeeds for correct value' '
>> +	test-tool ref-store main for-each-reflog-ent $m >before &&
>>  	git rev-parse $m >expect &&
>>  	echo "verify $m $m" >stdin &&
>>  	git update-ref --stdin <stdin &&
>>  	git rev-parse $m >actual &&
>> -	test_cmp expect actual
>> +	test_cmp expect actual &&
>> +	test-tool ref-store main for-each-reflog-ent $m >after &&
>> +	test_cmp before after
>>  '
>>
>>  test_expect_success 'stdin verify succeeds for missing reference' '
>> +	test-tool ref-store main for-each-reflog-ent $m >before &&
>>  	echo "verify refs/heads/missing $Z" >stdin &&
>>  	git update-ref --stdin <stdin &&
>> -	test_must_fail git rev-parse --verify -q refs/heads/missing
>> +	test_must_fail git rev-parse --verify -q refs/heads/missing &&
>> +	test-tool ref-store main for-each-reflog-ent $m >after &&
>> +	test_cmp before after
>>  '
>
> The updated tests merely assert that the refs didn't change, right?
>

>>  test_expect_success 'stdin verify treats no value as missing' '
>> @@ -1641,4 +1647,74 @@ test_expect_success PIPE 'transaction flushes status updates' '
>>  	test_cmp expected actual
>>  '
>>
>> +create_stdin_buf ()
>> +{
>
> The curly brace should go on the same line as the function name.
>

>> +	if test "$1" = "-z"
>> +	then
>> +		shift
>> +		printf "$F" "$@" >stdin
>> +	else
>> +		echo "$@" >stdin
>> +	fi
>> +}
>> +
>> +for type in "" "-z"
>> +do
>
> We should probably indent all of the tests to make it easier to see that
> they run in a loop.
>
> Patrick
>