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

["Bence Ferdinandy" <bence@xxxxxxxxxxxxxx>]

On Sun Oct 13, 2024 at 16:03, Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
> 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:

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