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

Commit Message

karthik nayak April 12, 2024, 9:59 a.m. UTC

From: Karthik Nayak <[e-mail]>

The `ref_transaction[_add]_update` functions obtain ref information and
flags to create a `ref_update` and add it to the transaction at hand.

To extend symref support in transactions, we need to also accept the
old and new ref values and process it. In this commit, let's add the
required paramaters to the function and modify all call sites.

The two paramaters added are `new_ref` and `old_ref`. The `new_ref` is
used to denote what the reference should point to when the transaction
is applied. Some functions allow this parameter to be NULL, meaning that
the reference is not changed, or `""`, meaning that the reference should
be deleted.

The `old_ref` denotes the value of that the reference must have before
the update. Some functions allow this parameter to be NULL, meaning that
the old value of the reference is not checked, or `""`, meaning that the
reference must not exist before the update. A copy of this value is made
in the transaction.

The handling logic of these parameters will be added in consequent
commits as we implement symref-{create, update, delete, verify}.

Signed-off-by: Karthik Nayak <[e-mail]>
---
 branch.c                |  2 +-
 builtin/fast-import.c   |  5 +++--
 builtin/fetch.c         |  2 +-
 builtin/receive-pack.c  |  1 +
 builtin/replace.c       |  2 +-
 builtin/tag.c           |  1 +
 builtin/update-ref.c    |  1 +
 refs.c                  | 15 ++++++++++-----
 refs.h                  |  9 ++++++++-
 refs/files-backend.c    | 12 ++++++------
 refs/refs-internal.h    | 14 ++++++++++++++
 refs/reftable-backend.c |  4 ++--
 sequencer.c             |  9 +++++----
 walker.c                |  2 +-
 14 files changed, 55 insertions(+), 24 deletions(-)

Comments

Christian Couder April 18, 2024, 2:25 p.m. UTC | #1

On Fri, Apr 12, 2024 at 11:59 AM Karthik Nayak <[e-mail]> wrote:
>
> From: Karthik Nayak <[e-mail]>
>
> The `ref_transaction[_add]_update` functions obtain ref information and
> flags to create a `ref_update` and add it to the transaction at hand.
>
> To extend symref support in transactions, we need to also accept the
> old and new ref values and process it. In this commit, let's add the
> required paramaters to the function and modify all call sites.

s/paramaters/parameters/

> The two paramaters added are `new_ref` and `old_ref`. The `new_ref` is

s/paramaters/parameters/

> used to denote what the reference should point to when the transaction
> is applied. Some functions allow this parameter to be NULL, meaning that
> the reference is not changed, or `""`, meaning that the reference should
> be deleted.

> The `old_ref` denotes the value of that the reference must have before

s/the value of that the reference/the value the reference/

> the update. Some functions allow this parameter to be NULL, meaning that
> the old value of the reference is not checked, or `""`, meaning that the
> reference must not exist before the update. A copy of this value is made
> in the transaction.
>
> The handling logic of these parameters will be added in consequent
> commits as we implement symref-{create, update, delete, verify}.
>
> Signed-off-by: Karthik Nayak <[e-mail]>

> diff --git a/refs.h b/refs.h
> index d278775e08..645fe9fdb8 100644
> --- a/refs.h
> +++ b/refs.h

There is the following big comment in this file:

/*
 * Reference transaction updates
 *
 * The following four functions add a reference check or update to a
 * ref_transaction.  They have some common similar parameters:
 *
 *     transaction -- a pointer to an open ref_transaction, obtained
 *         from ref_transaction_begin().
 *
 *     refname -- the name of the reference to be affected.
 *
 *     new_oid -- the object ID that should be set to be the new value
 *         of the reference. Some functions allow this parameter to be
 *         NULL, meaning that the reference is not changed, or
 *         null_oid, meaning that the reference should be deleted. A
 *         copy of this value is made in the transaction.
 *
 *     old_oid -- the object ID that the reference must have before
 *         the update. Some functions allow this parameter to be NULL,
 *         meaning that the old value of the reference is not checked,
 *         or null_oid, meaning that the reference must not exist
 *         before the update. A copy of this value is made in the
 *         transaction.
 *
 *     flags -- flags affecting the update, passed to
 *         update_ref_lock(). Possible flags: REF_NO_DEREF,
 *         REF_FORCE_CREATE_REFLOG. See those constants for more
 *         information.
 *
 *     msg -- a message describing the change (for the reflog).
 *
 *     err -- a strbuf for receiving a description of any error that
 *         might have occurred.
 *
 * The functions make internal copies of refname and msg, so the
 * caller retains ownership of these parameters.
 *
 * The functions return 0 on success and non-zero on failure. A
 * failure means that the transaction as a whole has failed and needs
 * to be rolled back.
 */

I would expect the patch to update this comment.

> @@ -722,6 +728,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
>                            const char *refname,
>                            const struct object_id *new_oid,
>                            const struct object_id *old_oid,
> +                          const char *new_ref, const char *old_ref,
>                            unsigned int flags, const char *msg,
>                            struct strbuf *err);

The patch might also want to update the comment just above the
ref_transaction_update() declaration as it is changing what the
function can (or will be able to) do.

> 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).

What if this value is NULL?

> +        */
> +       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).

What if this value is NULL?


> +        */
> +       const char *old_ref;
> +
>         /*
>          * One or more of REF_NO_DEREF, REF_FORCE_CREATE_REFLOG,
>          * REF_HAVE_NEW, REF_HAVE_OLD, or backend-specific flags.

Phillip Wood April 18, 2024, 3:08 p.m. UTC | #2

Hi Karthik

I agree with Christian's comments on this patch, I've got a couple of 
additional comments below

On 12/04/2024 10:59, Karthik Nayak wrote:
> diff --git a/refs.c b/refs.c
> index 55d2e0b2cb..967c81167e 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1228,6 +1228,7 @@ struct ref_update *ref_transaction_add_update(
>   		const char *refname, unsigned int flags,
>   		const struct object_id *new_oid,
>   		const struct object_id *old_oid,
> +		const char *new_ref, const char *old_ref,
>   		const char *msg)
>   {
>   	struct ref_update *update;
> @@ -1253,6 +1254,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
>   			   const char *refname,
>   			   const struct object_id *new_oid,
>   			   const struct object_id *old_oid,
> +			   const char *new_ref, const char *old_ref,
>   			   unsigned int flags, const char *msg,
>   			   struct strbuf *err)
>   {

Adding these two new parameters is quite disruptive as all the existing 
callers have to be updated. It makes it easy for callers to misuse this 
function for example by providing old_oid and old_ref (I'm assuming that 
is an error but it is hard to know for sure without any documentation). 
It also makes the calling code harder to read because there are so many 
parameters it is hard to keep track of exactly what is being passed. An 
alternative strategy would be to add a new function that takes a struct 
instead of lots of individual parameters. That would make the calling 
code more readable as it would be clear which struct members are being 
set (see reset.h for an example of this). The approach of adding a 
struct is still prone to setting the wrong combination of options so 
either way it would be helpful to add some assertions to detect mistakes

	if (old_oid && old_ref)
		BUG("Only one of old_oid and old_ref should be non NULL");
	if (new_oid && new_ref)
		BUG("Only one of new_oid and new_ref should be non NULL");


> diff --git a/refs.h b/refs.h
> index d278775e08..645fe9fdb8 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -696,13 +696,19 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);
>    */
>   #define REF_SKIP_REFNAME_VERIFICATION (1 << 11)
>   
> +/*
> + * The reference update is considered to be done on a symbolic reference. This
> + * ensures that we verify, delete, create and update the ref correspondingly.
> + */
> +#define REF_SYMREF_UPDATE (1 << 12)

I'm confused as to why we need this as I assumed that we could use the 
presence of old_ref/new_ref to determine that the caller wants to update 
symbolic ref. Having this flag means that there are more possibilities 
to misuse the new API setting this flag but providing NULL for old_ref 
and new_ref.

Best Wishes

Phillip

Patrick Steinhardt April 19, 2024, 9:40 a.m. UTC | #3

On Fri, Apr 12, 2024 at 11:59:02AM +0200, Karthik Nayak wrote:
> From: Karthik Nayak <[e-mail]>
> 
> The `ref_transaction[_add]_update` functions obtain ref information and
> flags to create a `ref_update` and add it to the transaction at hand.
> 
> To extend symref support in transactions, we need to also accept the
> old and new ref values and process it. In this commit, let's add the
> required paramaters to the function and modify all call sites.
> 
> The two paramaters added are `new_ref` and `old_ref`. The `new_ref` is

Would `new_target` and `old_target` be easier to understand? `new_ref`
and `old_ref` to me sound as if they might also apply to the ref itself,
for example when doing a rename.

[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`?

Patrick

Patrick Steinhardt April 19, 2024, 9:40 a.m. UTC | #4

On Thu, Apr 18, 2024 at 04:08:00PM +0100, Phillip Wood wrote:
[snip]
> > diff --git a/refs.h b/refs.h
> > index d278775e08..645fe9fdb8 100644
> > --- a/refs.h
> > +++ b/refs.h
> > @@ -696,13 +696,19 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);
> >    */
> >   #define REF_SKIP_REFNAME_VERIFICATION (1 << 11)
> > +/*
> > + * The reference update is considered to be done on a symbolic reference. This
> > + * ensures that we verify, delete, create and update the ref correspondingly.
> > + */
> > +#define REF_SYMREF_UPDATE (1 << 12)
> 
> I'm confused as to why we need this as I assumed that we could use the
> presence of old_ref/new_ref to determine that the caller wants to update
> symbolic ref. Having this flag means that there are more possibilities to
> misuse the new API setting this flag but providing NULL for old_ref and
> new_ref.

In my opinion the same comment applies to `REF_HAVE_NEW` and
`REF_HAVE_OLD`, which I found to be redundant, as well. Those may make
sense in the internals when the object IDs are stored as non-pointers,
but queueing ref updates only accepts pointers anyway.

Patrick

karthik nayak April 19, 2024, 10:28 a.m. UTC | #5

Christian Couder <[e-mail]> writes:

> On Fri, Apr 12, 2024 at 11:59 AM Karthik Nayak <[e-mail]> wrote:
>>
>> From: Karthik Nayak <[e-mail]>
>>
>> The `ref_transaction[_add]_update` functions obtain ref information and
>> flags to create a `ref_update` and add it to the transaction at hand.
>>
>> To extend symref support in transactions, we need to also accept the
>> old and new ref values and process it. In this commit, let's add the
>> required paramaters to the function and modify all call sites.
>
> s/paramaters/parameters/
>
>> The two paramaters added are `new_ref` and `old_ref`. The `new_ref` is
>
> s/paramaters/parameters/
>

Thanks, will fix both.

>> used to denote what the reference should point to when the transaction
>> is applied. Some functions allow this parameter to be NULL, meaning that
>> the reference is not changed, or `""`, meaning that the reference should
>> be deleted.
>
>> The `old_ref` denotes the value of that the reference must have before
>
> s/the value of that the reference/the value the reference/
>

Will change.

>> the update. Some functions allow this parameter to be NULL, meaning that
>> the old value of the reference is not checked, or `""`, meaning that the
>> reference must not exist before the update. A copy of this value is made
>> in the transaction.
>>
>> The handling logic of these parameters will be added in consequent
>> commits as we implement symref-{create, update, delete, verify}.
>>
>> Signed-off-by: Karthik Nayak <[e-mail]>
>
>> diff --git a/refs.h b/refs.h
>> index d278775e08..645fe9fdb8 100644
>> --- a/refs.h
>> +++ b/refs.h
>
> There is the following big comment in this file:
>
> /*
>  * Reference transaction updates
>  *
>  * The following four functions add a reference check or update to a
>  * ref_transaction.  They have some common similar parameters:
>  *
>  *     transaction -- a pointer to an open ref_transaction, obtained
>  *         from ref_transaction_begin().
>  *
>  *     refname -- the name of the reference to be affected.
>  *
>  *     new_oid -- the object ID that should be set to be the new value
>  *         of the reference. Some functions allow this parameter to be
>  *         NULL, meaning that the reference is not changed, or
>  *         null_oid, meaning that the reference should be deleted. A
>  *         copy of this value is made in the transaction.
>  *
>  *     old_oid -- the object ID that the reference must have before
>  *         the update. Some functions allow this parameter to be NULL,
>  *         meaning that the old value of the reference is not checked,
>  *         or null_oid, meaning that the reference must not exist
>  *         before the update. A copy of this value is made in the
>  *         transaction.
>  *
>  *     flags -- flags affecting the update, passed to
>  *         update_ref_lock(). Possible flags: REF_NO_DEREF,
>  *         REF_FORCE_CREATE_REFLOG. See those constants for more
>  *         information.
>  *
>  *     msg -- a message describing the change (for the reflog).
>  *
>  *     err -- a strbuf for receiving a description of any error that
>  *         might have occurred.
>  *
>  * The functions make internal copies of refname and msg, so the
>  * caller retains ownership of these parameters.
>  *
>  * The functions return 0 on success and non-zero on failure. A
>  * failure means that the transaction as a whole has failed and needs
>  * to be rolled back.
>  */
>
> I would expect the patch to update this comment.
>

Since this patch doesn't use this value, I think its best to modify
this in each patch as we start using it, I'll do that.

>> @@ -722,6 +728,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
>>                            const char *refname,
>>                            const struct object_id *new_oid,
>>                            const struct object_id *old_oid,
>> +                          const char *new_ref, const char *old_ref,
>>                            unsigned int flags, const char *msg,
>>                            struct strbuf *err);
>
> The patch might also want to update the comment just above the
> ref_transaction_update() declaration as it is changing what the
> function can (or will be able to) do.
>

Agreed, same as above, will modify in each patch. Also will add a
comment in the commit.

>> 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).
>
> What if this value is NULL?
>
>> +        */
>> +       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).
>
> What if this value is NULL?
>

Well we ignore NULL values, but I see that the documentation is lacking,
will update.

karthik nayak April 19, 2024, 3:47 p.m. UTC | #6

Hey Phillip,

Phillip Wood <[e-mail]> writes:

> Hi Karthik
>
> I agree with Christian's comments on this patch, I've got a couple of
> additional comments below
>
> On 12/04/2024 10:59, Karthik Nayak wrote:
>> diff --git a/refs.c b/refs.c
>> index 55d2e0b2cb..967c81167e 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1228,6 +1228,7 @@ struct ref_update *ref_transaction_add_update(
>>   		const char *refname, unsigned int flags,
>>   		const struct object_id *new_oid,
>>   		const struct object_id *old_oid,
>> +		const char *new_ref, const char *old_ref,
>>   		const char *msg)
>>   {
>>   	struct ref_update *update;
>> @@ -1253,6 +1254,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
>>   			   const char *refname,
>>   			   const struct object_id *new_oid,
>>   			   const struct object_id *old_oid,
>> +			   const char *new_ref, const char *old_ref,
>>   			   unsigned int flags, const char *msg,
>>   			   struct strbuf *err)
>>   {
>
> Adding these two new parameters is quite disruptive as all the existing
> callers have to be updated. It makes it easy for callers to misuse this
> function for example by providing old_oid and old_ref (I'm assuming that
> is an error but it is hard to know for sure without any documentation).



> It also makes the calling code harder to read because there are so many
> parameters it is hard to keep track of exactly what is being passed. An
> alternative strategy would be to add a new function that takes a struct
> instead of lots of individual parameters. That would make the calling
> code more readable as it would be clear which struct members are being
> set (see reset.h for an example of this). The approach of adding a
> struct is still prone to setting the wrong combination of options so

We do already have refs-internal.h:ref_update and this struct would be
the best candidate for what you're saying. I even thought of exposing
this struct and using it. I think I realized that it's a lot more work
to do this, because there are checks and cleanups which are built around
this struct. So exposing and using it would mean we refactor a bunch of
that code. Which while necessary I believe should be out of this series.
I'd actually be happy to do it right after we can agree that this is a
good direction.

> either way it would be helpful to add some assertions to detect mistakes
>
> 	if (old_oid && old_ref)
> 		BUG("Only one of old_oid and old_ref should be non NULL");
> 	if (new_oid && new_ref)
> 		BUG("Only one of new_oid and new_ref should be non NULL");
>

I have slightly modified it to:

 	if (old_oid && !is_null_oid(old_oid) && old_ref)
 		BUG("Only one of old_oid and old_ref should be non NULL");
 	if (new_oid && !is_null_oid(new_oid) && new_ref)
 		BUG("Only one of new_oid and new_ref should be non NULL");

But I agree, this is needed and have added it.

>> diff --git a/refs.h b/refs.h
>> index d278775e08..645fe9fdb8 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -696,13 +696,19 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);
>>    */
>>   #define REF_SKIP_REFNAME_VERIFICATION (1 << 11)
>>
>> +/*
>> + * The reference update is considered to be done on a symbolic reference. This
>> + * ensures that we verify, delete, create and update the ref correspondingly.
>> + */
>> +#define REF_SYMREF_UPDATE (1 << 12)
>
> I'm confused as to why we need this as I assumed that we could use the
> presence of old_ref/new_ref to determine that the caller wants to update
> symbolic ref. Having this flag means that there are more possibilities
> to misuse the new API setting this flag but providing NULL for old_ref
> and new_ref.
>

I think I started with this flag but as the direction of the series
changed and I moved using zero_oid values for deletion or for using the
verify command, this is not really needed anymore. I just tried removing
all the code around the flags and fixing up things and all the tests
still pass. Thanks for brining this up.

Patrick Steinhardt <[e-mail]> writes:
>> I'm confused as to why we need this as I assumed that we could use the
>> presence of old_ref/new_ref to determine that the caller wants to update
>> symbolic ref. Having this flag means that there are more possibilities to
>> misuse the new API setting this flag but providing NULL for old_ref and
>> new_ref.
>
> In my opinion the same comment applies to `REF_HAVE_NEW` and
> `REF_HAVE_OLD`, which I found to be redundant, as well. Those may make
> sense in the internals when the object IDs are stored as non-pointers,
> but queueing ref updates only accepts pointers anyway.
>

Yeah like you mentioned, since we're dealing with pointers, checking the
if its set is enough indication, which doesn't work with the static OID
values.

karthik nayak April 19, 2024, 6:09 p.m. UTC | #7

Patrick Steinhardt <[e-mail]> writes:

> On Fri, Apr 12, 2024 at 11:59:02AM +0200, Karthik Nayak wrote:
>> From: Karthik Nayak <[e-mail]>
>>
>> The `ref_transaction[_add]_update` functions obtain ref information and
>> flags to create a `ref_update` and add it to the transaction at hand.
>>
>> To extend symref support in transactions, we need to also accept the
>> old and new ref values and process it. In this commit, let's add the
>> required paramaters to the function and modify all call sites.
>>
>> The two paramaters added are `new_ref` and `old_ref`. The `new_ref` is
>
> Would `new_target` and `old_target` be easier to understand? `new_ref`
> and `old_ref` to me sound as if they might also apply to the ref itself,
> for example when doing a rename.
>

I guess that makes sense, I'll add it.

> [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.

Patrick Steinhardt April 23, 2024, 6:31 a.m. UTC | #8

On Fri, Apr 19, 2024 at 01:09:39PM -0500, Karthik Nayak wrote:
> Patrick Steinhardt <[e-mail]> writes:
> 
> > On Fri, Apr 12, 2024 at 11:59:02AM +0200, Karthik Nayak wrote:
> >> From: Karthik Nayak <[e-mail]>
> > [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

karthik nayak April 23, 2024, 10:48 a.m. UTC | #9

Patrick Steinhardt <[e-mail]> writes:

> On Fri, Apr 19, 2024 at 01:09:39PM -0500, Karthik Nayak wrote:
>> Patrick Steinhardt <[e-mail]> writes:
>>
>> > On Fri, Apr 12, 2024 at 11:59:02AM +0200, Karthik Nayak wrote:
>> >> From: Karthik Nayak <[e-mail]>
>> > [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

I think after that email, I agreed to Phillip's suggestion [1] and now
I've added an explicit check for this.

[1]: https://lore.kernel.org/git/CAOLa=ZSwtOQXwbgregzKMtVA30wUCH8R=[e-mail]/

Phillip Wood May 4, 2024, 3:15 p.m. UTC | #10

Hi Karthik

Sorry for the slow response on this

On 19/04/2024 16:47, Karthik Nayak wrote:
>> either way it would be helpful to add some assertions to detect mistakes
>>
>> 	if (old_oid && old_ref)
>> 		BUG("Only one of old_oid and old_ref should be non NULL");
>> 	if (new_oid && new_ref)
>> 		BUG("Only one of new_oid and new_ref should be non NULL");
>>
> 
> I have slightly modified it to:
> 
>   	if (old_oid && !is_null_oid(old_oid) && old_ref)
>   		BUG("Only one of old_oid and old_ref should be non NULL");
>   	if (new_oid && !is_null_oid(new_oid) && new_ref)
>   		BUG("Only one of new_oid and new_ref should be non NULL");
> 
> But I agree, this is needed and have added it.

I'm confused as to why we want to allow old_ref in conjunction with 
old_oid being the null oid. Reading the documentation a null oid means 
"this ref must not exist" and NULL means "I don't care about the current 
state of the ref" so NULL and the null oid are not equivalent.

Best Wishes

Phillip

> 
>>> diff --git a/refs.h b/refs.h
>>> index d278775e08..645fe9fdb8 100644
>>> --- a/refs.h
>>> +++ b/refs.h
>>> @@ -696,13 +696,19 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);
>>>     */
>>>    #define REF_SKIP_REFNAME_VERIFICATION (1 << 11)
>>>
>>> +/*
>>> + * The reference update is considered to be done on a symbolic reference. This
>>> + * ensures that we verify, delete, create and update the ref correspondingly.
>>> + */
>>> +#define REF_SYMREF_UPDATE (1 << 12)
>>
>> I'm confused as to why we need this as I assumed that we could use the
>> presence of old_ref/new_ref to determine that the caller wants to update
>> symbolic ref. Having this flag means that there are more possibilities
>> to misuse the new API setting this flag but providing NULL for old_ref
>> and new_ref.
>>
> 
> I think I started with this flag but as the direction of the series
> changed and I moved using zero_oid values for deletion or for using the
> verify command, this is not really needed anymore. I just tried removing
> all the code around the flags and fixing up things and all the tests
> still pass. Thanks for brining this up.
> 
> Patrick Steinhardt <[e-mail]> writes:
>>> I'm confused as to why we need this as I assumed that we could use the
>>> presence of old_ref/new_ref to determine that the caller wants to update
>>> symbolic ref. Having this flag means that there are more possibilities to
>>> misuse the new API setting this flag but providing NULL for old_ref and
>>> new_ref.
>>
>> In my opinion the same comment applies to `REF_HAVE_NEW` and
>> `REF_HAVE_OLD`, which I found to be redundant, as well. Those may make
>> sense in the internals when the object IDs are stored as non-pointers,
>> but queueing ref updates only accepts pointers anyway.
>>
> 
> Yeah like you mentioned, since we're dealing with pointers, checking the
> if its set is enough indication, which doesn't work with the static OID
> values.

Patch

diff --git a/branch.c b/branch.c
index 621019fcf4..3ebcfdca65 100644
--- a/branch.c
+++ b/branch.c
@@ -627,7 +627,7 @@  void create_branch(struct repository *r,
 	if (!transaction ||
 		ref_transaction_update(transaction, ref.buf,
 					&oid, forcing ? NULL : null_oid(),
-					0, msg, &err) ||
+					NULL, NULL, 0, msg, &err) ||
 		ref_transaction_commit(transaction, &err))
 		die("%s", err.buf);
 	ref_transaction_free(transaction);
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 782bda007c..6a0b39de32 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -1634,7 +1634,7 @@  static int update_branch(struct branch *b)
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_update(transaction, b->name, &b->oid, &old_oid,
-				   0, msg, &err) ||
+				   NULL, NULL, 0, msg, &err) ||
 	    ref_transaction_commit(transaction, &err)) {
 		ref_transaction_free(transaction);
 		error("%s", err.buf);
@@ -1675,7 +1675,8 @@  static void dump_tags(void)
 		strbuf_addf(&ref_name, "refs/tags/%s", t->name);
 
 		if (ref_transaction_update(transaction, ref_name.buf,
-					   &t->oid, NULL, 0, msg, &err)) {
+					   &t->oid, NULL, NULL, NULL,
+					   0, msg, &err)) {
 			failure |= error("%s", err.buf);
 			goto cleanup;
 		}
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 5857d860db..66840b7c5b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -668,7 +668,7 @@  static int s_update_ref(const char *action,
 
 	ret = ref_transaction_update(transaction, ref->name, &ref->new_oid,
 				     check_old ? &ref->old_oid : NULL,
-				     0, msg, &err);
+				     NULL, NULL, 0, msg, &err);
 	if (ret) {
 		ret = STORE_REF_ERROR_OTHER;
 		goto out;
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 56d8a77ed7..ebea1747cb 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1595,6 +1595,7 @@  static const char *update(struct command *cmd, struct shallow_info *si)
 		if (ref_transaction_update(transaction,
 					   namespaced_name,
 					   new_oid, old_oid,
+					   NULL, NULL,
 					   0, "push",
 					   &err)) {
 			rp_error("%s", err.buf);
diff --git a/builtin/replace.c b/builtin/replace.c
index da59600ad2..7690687b0e 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -201,7 +201,7 @@  static int replace_object_oid(const char *object_ref,
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_update(transaction, ref.buf, repl, &prev,
-				   0, NULL, &err) ||
+				   NULL, NULL, 0, NULL, &err) ||
 	    ref_transaction_commit(transaction, &err))
 		res = error("%s", err.buf);
 
diff --git a/builtin/tag.c b/builtin/tag.c
index 9a33cb50b4..40a65fdebc 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -660,6 +660,7 @@  int cmd_tag(int argc, const char **argv, const char *prefix)
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_update(transaction, ref.buf, &object, &prev,
+				   NULL, NULL,
 				   create_reflog ? REF_FORCE_CREATE_REFLOG : 0,
 				   reflog_msg.buf, &err) ||
 	    ref_transaction_commit(transaction, &err)) {
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index e46afbc46d..21fdbf6ac8 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -204,6 +204,7 @@  static void parse_cmd_update(struct ref_transaction *transaction,
 
 	if (ref_transaction_update(transaction, refname,
 				   &new_oid, have_old ? &old_oid : NULL,
+				   NULL, NULL,
 				   update_flags | create_reflog_flag,
 				   msg, &err))
 		die("%s", err.buf);
diff --git a/refs.c b/refs.c
index 55d2e0b2cb..967c81167e 100644
--- a/refs.c
+++ b/refs.c
@@ -1228,6 +1228,7 @@  struct ref_update *ref_transaction_add_update(
 		const char *refname, unsigned int flags,
 		const struct object_id *new_oid,
 		const struct object_id *old_oid,
+		const char *new_ref, const char *old_ref,
 		const char *msg)
 {
 	struct ref_update *update;
@@ -1253,6 +1254,7 @@  int ref_transaction_update(struct ref_transaction *transaction,
 			   const char *refname,
 			   const struct object_id *new_oid,
 			   const struct object_id *old_oid,
+			   const char *new_ref, const char *old_ref,
 			   unsigned int flags, const char *msg,
 			   struct strbuf *err)
 {
@@ -1280,7 +1282,7 @@  int ref_transaction_update(struct ref_transaction *transaction,
 	flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);
 
 	ref_transaction_add_update(transaction, refname, flags,
-				   new_oid, old_oid, msg);
+				   new_oid, old_oid, new_ref, old_ref, msg);
 	return 0;
 }
 
@@ -1295,7 +1297,8 @@  int ref_transaction_create(struct ref_transaction *transaction,
 		return 1;
 	}
 	return ref_transaction_update(transaction, refname, new_oid,
-				      null_oid(), flags, msg, err);
+				      null_oid(), NULL, NULL, flags,
+				      msg, err);
 }
 
 int ref_transaction_delete(struct ref_transaction *transaction,
@@ -1308,7 +1311,8 @@  int ref_transaction_delete(struct ref_transaction *transaction,
 		BUG("delete called with old_oid set to zeros");
 	return ref_transaction_update(transaction, refname,
 				      null_oid(), old_oid,
-				      flags, msg, err);
+				      NULL, NULL, flags,
+				      msg, err);
 }
 
 int ref_transaction_verify(struct ref_transaction *transaction,
@@ -1321,6 +1325,7 @@  int ref_transaction_verify(struct ref_transaction *transaction,
 		BUG("verify called with old_oid set to NULL");
 	return ref_transaction_update(transaction, refname,
 				      NULL, old_oid,
+				      NULL, NULL,
 				      flags, NULL, err);
 }
 
@@ -1335,8 +1340,8 @@  int refs_update_ref(struct ref_store *refs, const char *msg,
 
 	t = ref_store_transaction_begin(refs, &err);
 	if (!t ||
-	    ref_transaction_update(t, refname, new_oid, old_oid, flags, msg,
-				   &err) ||
+	    ref_transaction_update(t, refname, new_oid, old_oid, NULL, NULL,
+				   flags, msg, &err) ||
 	    ref_transaction_commit(t, &err)) {
 		ret = 1;
 		ref_transaction_free(t);
diff --git a/refs.h b/refs.h
index d278775e08..645fe9fdb8 100644
--- a/refs.h
+++ b/refs.h
@@ -696,13 +696,19 @@  struct ref_transaction *ref_transaction_begin(struct strbuf *err);
  */
 #define REF_SKIP_REFNAME_VERIFICATION (1 << 11)
 
+/*
+ * The reference update is considered to be done on a symbolic reference. This
+ * ensures that we verify, delete, create and update the ref correspondingly.
+ */
+#define REF_SYMREF_UPDATE (1 << 12)
+
 /*
  * Bitmask of all of the flags that are allowed to be passed in to
  * ref_transaction_update() and friends:
  */
 #define REF_TRANSACTION_UPDATE_ALLOWED_FLAGS                                  \
 	(REF_NO_DEREF | REF_FORCE_CREATE_REFLOG | REF_SKIP_OID_VERIFICATION | \
-	 REF_SKIP_REFNAME_VERIFICATION)
+	 REF_SKIP_REFNAME_VERIFICATION | REF_SYMREF_UPDATE )
 
 /*
  * Add a reference update to transaction. `new_oid` is the value that
@@ -722,6 +728,7 @@  int ref_transaction_update(struct ref_transaction *transaction,
 			   const char *refname,
 			   const struct object_id *new_oid,
 			   const struct object_id *old_oid,
+			   const char *new_ref, const char *old_ref,
 			   unsigned int flags, const char *msg,
 			   struct strbuf *err);
 
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a098d14ea0..e4d0aa3d41 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1198,7 +1198,7 @@  static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)
 	ref_transaction_add_update(
 			transaction, r->name,
 			REF_NO_DEREF | REF_HAVE_NEW | REF_HAVE_OLD | REF_IS_PRUNING,
-			null_oid(), &r->oid, NULL);
+			null_oid(), &r->oid, NULL, NULL, NULL);
 	if (ref_transaction_commit(transaction, &err))
 		goto cleanup;
 
@@ -1292,7 +1292,7 @@  static int files_pack_refs(struct ref_store *ref_store,
 		 * packed-refs transaction:
 		 */
 		if (ref_transaction_update(transaction, iter->refname,
-					   iter->oid, NULL,
+					   iter->oid, NULL, NULL, NULL,
 					   REF_NO_DEREF, NULL, &err))
 			die("failure preparing to create packed reference %s: %s",
 			    iter->refname, err.buf);
@@ -2309,7 +2309,7 @@  static int split_head_update(struct ref_update *update,
 			transaction, "HEAD",
 			update->flags | REF_LOG_ONLY | REF_NO_DEREF,
 			&update->new_oid, &update->old_oid,
-			update->msg);
+			NULL, NULL, update->msg);
 
 	/*
 	 * Add "HEAD". This insertion is O(N) in the transaction
@@ -2372,7 +2372,7 @@  static int split_symref_update(struct ref_update *update,
 	new_update = ref_transaction_add_update(
 			transaction, referent, new_flags,
 			&update->new_oid, &update->old_oid,
-			update->msg);
+			NULL, NULL, update->msg);
 
 	new_update->parent_update = update;
 
@@ -2763,7 +2763,7 @@  static int files_transaction_prepare(struct ref_store *ref_store,
 					packed_transaction, update->refname,
 					REF_HAVE_NEW | REF_NO_DEREF,
 					&update->new_oid, NULL,
-					NULL);
+					NULL, NULL, NULL);
 		}
 	}
 
@@ -3048,7 +3048,7 @@  static int files_initial_transaction_commit(struct ref_store *ref_store,
 		ref_transaction_add_update(packed_transaction, update->refname,
 					   update->flags & ~REF_HAVE_OLD,
 					   &update->new_oid, &update->old_oid,
-					   NULL);
+					   NULL, NULL, NULL);
 	}
 
 	if (packed_refs_lock(refs->packed_ref_store, 0, err)) {
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;
+
 	/*
 	 * One or more of REF_NO_DEREF, REF_FORCE_CREATE_REFLOG,
 	 * REF_HAVE_NEW, REF_HAVE_OLD, or backend-specific flags.
@@ -173,6 +186,7 @@  struct ref_update *ref_transaction_add_update(
 		const char *refname, unsigned int flags,
 		const struct object_id *new_oid,
 		const struct object_id *old_oid,
+		const char *new_ref, const char *old_ref,
 		const char *msg);
 
 /*
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 1cda48c504..6104471199 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -829,7 +829,7 @@  static int reftable_be_transaction_prepare(struct ref_store *ref_store,
 			new_update = ref_transaction_add_update(
 					transaction, "HEAD",
 					u->flags | REF_LOG_ONLY | REF_NO_DEREF,
-					&u->new_oid, &u->old_oid, u->msg);
+					&u->new_oid, &u->old_oid, NULL, NULL, u->msg);
 			string_list_insert(&affected_refnames, new_update->refname);
 		}
 
@@ -908,7 +908,7 @@  static int reftable_be_transaction_prepare(struct ref_store *ref_store,
 				 */
 				new_update = ref_transaction_add_update(
 						transaction, referent.buf, new_flags,
-						&u->new_oid, &u->old_oid, u->msg);
+						&u->new_oid, &u->old_oid, NULL, NULL, u->msg);
 				new_update->parent_update = u;
 
 				/*
diff --git a/sequencer.c b/sequencer.c
index 2c19846385..af1b25692b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -616,7 +616,7 @@  static int fast_forward_to(struct repository *r,
 	if (!transaction ||
 	    ref_transaction_update(transaction, "HEAD",
 				   to, unborn && !is_rebase_i(opts) ?
-				   null_oid() : from,
+				   null_oid() : from, NULL, NULL,
 				   0, sb.buf, &err) ||
 	    ref_transaction_commit(transaction, &err)) {
 		ref_transaction_free(transaction);
@@ -1248,7 +1248,7 @@  int update_head_with_reflog(const struct commit *old_head,
 	if (!transaction ||
 	    ref_transaction_update(transaction, "HEAD", new_head,
 				   old_head ? &old_head->object.oid : null_oid(),
-				   0, sb.buf, err) ||
+				   NULL, NULL, 0, sb.buf, err) ||
 	    ref_transaction_commit(transaction, err)) {
 		ret = -1;
 	}
@@ -3764,8 +3764,9 @@  static int do_label(struct repository *r, const char *name, int len)
 	} else if (repo_get_oid(r, "HEAD", &head_oid)) {
 		error(_("could not read HEAD"));
 		ret = -1;
-	} else if (ref_transaction_update(transaction, ref_name.buf, &head_oid,
-					  NULL, 0, msg.buf, &err) < 0 ||
+	} else if (ref_transaction_update(transaction, ref_name.buf,
+					  &head_oid, NULL, NULL, NULL,
+					  0, msg.buf, &err) < 0 ||
 		   ref_transaction_commit(transaction, &err)) {
 		error("%s", err.buf);
 		ret = -1;
diff --git a/walker.c b/walker.c
index c0fd632d92..1b3df43906 100644
--- a/walker.c
+++ b/walker.c
@@ -324,7 +324,7 @@  int walker_fetch(struct walker *walker, int targets, char **target,
 		strbuf_reset(&refname);
 		strbuf_addf(&refname, "refs/%s", write_ref[i]);
 		if (ref_transaction_update(transaction, refname.buf,
-					   oids + i, NULL, 0,
+					   oids + i, NULL, NULL, NULL, 0,
 					   msg ? msg : "fetch (unknown)",
 					   &err)) {
 			error("%s", err.buf);