Re: [PATCH v2 1/7] refs: accept symref values in `ref_transaction[_add]_update`

> On Fri, Apr 19, 2024 at 01:09:39PM -0500, Karthik Nayak wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>>
>> > On Fri, Apr 12, 2024 at 11:59:02AM +0200, Karthik Nayak wrote:
>> >> From: Karthik Nayak <karthik.188@gmail.com>
>> > [snip]
>> >> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
>> >> index 56641aa57a..4c5fe02687 100644
>> >> --- a/refs/refs-internal.h
>> >> +++ b/refs/refs-internal.h
>> >> @@ -124,6 +124,19 @@ struct ref_update {
>> >>  	 */
>> >>  	struct object_id old_oid;
>> >>
>> >> +	/*
>> >> +	 * If (flags & REF_SYMREF_UPDATE), set the reference to this
>> >> +	 * value (or delete it, if `new_ref` is an empty string).
>> >> +	 */
>> >> +	const char *new_ref;
>> >> +
>> >> +	/*
>> >> +	 * If (type & REF_SYMREF_UPDATE), check that the reference
>> >> +	 * previously had this value (or didn't previously exist,
>> >> +	 * if `old_ref` is an empty string).
>> >> +	 */
>> >> +	const char *old_ref;
>> >
>> > I think one important bit of information here would be how to handle the
>> > update from a plain ref to a symref or vice versa. Would I set both
>> > `REF_SYMREF_UPDATE` and `REF_HAVE_NEW`/`REF_HAVE_OLD`?
>>
>> I'll now remove `REF_SYMREF_UPDATE` and add documentation around the
>> usage on `new_target` and `old_target`, where if either of them are set,
>> they take precedence over `old_oid` and `new_oid`. The `new_target` will
>> ensure that the ref is now a symbolic ref which points to the
>> `new_target` value. While the `old_target` will treat the ref as a
>> symbolic ref and check its old value.
>>
>> `REF_HAVE_NEW`/`REF_HAVE_OLD` should however never be set by users of
>> ref.c, this is set internally. See REF_TRANSACTION_UPDATE_ALLOWED_FLAGS.
>
> Should they really take precedence, or should it be forbidden to pass
> both old target and old object ID or new target and new object ID,
> respectively? I'd rather claim the latter, and that should be detected
> such that there is no bad surprises here.
>
> Patrick