Re: [PATCH v7 4/6] refs: add TRANSACTION_CREATE_EXISTS error

From: Bence Ferdinandy <bence@ferdinandy.com>
Date: Oct 13, 2024, 20:52 UTC
Message-ID: <D4UZ5DJWM3WM.MYD9YL6SLE@ferdinandy.com>
In-Reply-To: <cf7c4831-a766-430c-aac6-5cd5c2ceabed@gmail.com>

On Sun Oct 13, 2024 at 16:03, Phillip Wood <phillip.wood123@gmail.com> wrote:

Show 14 quoted lines

> Hi Bence
>
> On 13/10/2024 00:03, Bence Ferdinandy wrote:
>> Currently there is only one special error for transaction, for when
>> there is a naming conflict, all other errors are dumped under a generic
>> error. Add a new special error case for when the caller requests the
>> reference to be updated only when it does not yet exist and the
>> reference actually does exist.
>
> This looks like useful improvement. Are the changes to 
> reftable-backend.c correct - it looks like where it previously returned 
> TRANSACTION_GENERIC_ERR it now returns TRANSACTION_NAME_CONFLICT which I 
> think is used to indicate a file/directory conflict (e.g. trying to 
> create refs/heads/topic/one when refs/heads/topic exists)

This passes: GIT_TEST_DEFAULT_REF_FORMAT=reftable prove --timer --jobs 8 ./t[0-9]*.sh so I'm hoping it's correct :)

[snip]

I guess you are referring to this part below:

Show 27 quoted lines

>> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
>> index 3c96fbf66f..ebf8e57fbc 100644
>> --- a/refs/reftable-backend.c
>> +++ b/refs/reftable-backend.c
>> @@ -1206,10 +1206,13 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
>>   				goto done;
>>   			}
>>   		} else if ((u->flags & REF_HAVE_OLD) && !oideq(&current_oid, &u->old_oid)) {
>> -			if (is_null_oid(&u->old_oid))
>> +			ret = TRANSACTION_NAME_CONFLICT;
>> +			if (is_null_oid(&u->old_oid)) {
>>   				strbuf_addf(err, _("cannot lock ref '%s': "
>>   						   "reference already exists"),
>>   					    ref_update_original_update_refname(u));
>> +				ret = TRANSACTION_CREATE_EXISTS;
>> +			}
>>   			else if (is_null_oid(&current_oid))
>>   				strbuf_addf(err, _("cannot lock ref '%s': "
>>   						   "reference is missing but expected %s"),
>> @@ -1221,7 +1224,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
>>   					    ref_update_original_update_refname(u),
>>   					    oid_to_hex(&current_oid),
>>   					    oid_to_hex(&u->old_oid));
>> -			ret = -1;
>>   			goto done;
>>   		}
>>

This originally returned -1, and it still returns that if it doesn't return -2, I just used the named variable instead of the integer itself. It might still be that this should be -3 GENERIC_ERROR, but if that is the case I think fixing that should be a different patch? I didn't check if changing that -1 to something else breaks anything or not.

Best, Bence

-- 
bence.ferdinandy.com