Re: [PATCH 3/3] receive-pack: use batched reference updates

[Karthik Nayak <karthik.188@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6080 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

[snip]

>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
>> index be314879e8..b4fceb3837 100644
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -1843,35 +1843,91 @@ static void BUG_if_skipped_connectivity_check(struct command *commands,
>>  	BUG_if_bug("connectivity check skipped???");
>>  }
>>
>> +static void ref_transaction_rejection_handler(const char *refname,
>> +					      const struct object_id *old_oid UNUSED,
>> +					      const struct object_id *new_oid UNUSED,
>> +					      const char *old_target UNUSED,
>> +					      const char *new_target UNUSED,
>> +					      enum ref_transaction_error err,
>> +					      void *cb_data)
>> +{
>> +	struct strmap *failed_refs = (struct strmap *)cb_data;
>
> This cast is unnecessary.
>

Yes, will remove.

>> +	const char *reason = "";
>> +
>> +	switch (err) {
>> +	case REF_TRANSACTION_ERROR_NAME_CONFLICT:
>> +		reason = "refname conflict";
>> +		break;
>> +	case REF_TRANSACTION_ERROR_CREATE_EXISTS:
>> +		reason = "reference already exists";
>> +		break;
>> +	case REF_TRANSACTION_ERROR_NONEXISTENT_REF:
>> +		reason = "reference does not exist";
>> +		break;
>> +	case REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:
>> +		reason = "incorrect old value provided";
>> +		break;
>> +	case REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:
>> +		reason = "invalid new value provided";
>> +		break;
>> +	case REF_TRANSACTION_ERROR_EXPECTED_SYMREF:
>> +		reason = "expected symref but found regular ref";
>> +		break;
>> +	default:
>> +		reason = "unkown failure";
>> +	}
>> +
>> +	strmap_put(failed_refs, refname, xstrdup(reason));
>> +}
>
> I'd have expected something like this for git-fetch(1), as well, so that
> we don't silently swallow failed ref updates. Would it make sense to
> maybe provide an array of reasons by enum so that we can reuse those
> messages?
>

I think you've convinced me of this, As Junio also mentions, I've now
added a function to provide a mapping from 'enum -> char *'.

>>  static void execute_commands_non_atomic(struct command *commands,
>>  					struct shallow_info *si)
>>  {
>>  	struct command *cmd;
>>  	struct strbuf err = STRBUF_INIT;
>> +	const char *reported_error = "";
>> +	struct strmap failed_refs = STRMAP_INIT;
>> +
>> +	transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
>> +						  REF_TRANSACTION_ALLOW_FAILURE, &err);
>> +	if (!transaction) {
>> +		rp_error("%s", err.buf);
>> +		strbuf_reset(&err);
>> +		reported_error = "transaction failed to start";
>> +		goto failure;
>> +	}
>
> Okay. We now create a single transaction with failures being allowed.
>
>>  	for (cmd = commands; cmd; cmd = cmd->next) {
>>  		if (!should_process_cmd(cmd) || cmd->run_proc_receive)
>>  			continue;
>>
>> -		transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
>> -							  0, &err);
>> -		if (!transaction) {
>> -			rp_error("%s", err.buf);
>> -			strbuf_reset(&err);
>> -			cmd->error_string = "transaction failed to start";
>> -			continue;
>> -		}
>> -
>>  		cmd->error_string = update(cmd, si);
>> +	}
>
> So here we only need to queue each update.
>
>> -		if (!cmd->error_string
>> -		    && ref_transaction_commit(transaction, &err)) {
>> -			rp_error("%s", err.buf);
>> -			strbuf_reset(&err);
>> -			cmd->error_string = "failed to update ref";
>> -		}
>> -		ref_transaction_free(transaction);
>> +	if (ref_transaction_commit(transaction, &err)) {
>> +		rp_error("%s", err.buf);
>> +		reported_error = "failed to update refs";
>> +		goto failure;
>>  	}
>> +
>> +	ref_transaction_for_each_rejected_update(transaction,
>> +						 ref_transaction_rejection_handler,
>> +						 &failed_refs);
>> +
>> +	if (strmap_empty(&failed_refs))
>> +		goto cleanup;
>> +
>> +failure:
>> +	for (cmd = commands; cmd; cmd = cmd->next) {
>> +		if (strmap_empty(&failed_refs))
>> +			cmd->error_string = reported_error;
>
> The reported error may have one of there values now:
>
>   - The empty string. Is it correct to set the error string to that
>     value? Shouldn't it rather be a `NULL` pointer?
>
>   - "transaction failed to start". It makes sense to update every
>     command accordingly, as we wouldn't have updated any of them.
>
>   - "failed to update refs", in case the commit failed. Does the commit
>     fail only in cases where we couldn't update _any_ reference, or does
>     it also retrun an error when only one of the updates failed? If the
>     latter, we shouldn't update all the others to "failed", should we?
>
> In any case, it feels weird that we use `strmap_empty()` to check for
> this condition. I'd have expected that we rather check for
> `reported_error` to be a non-empty string directly to figure out whether
> the transaction itself has failed as a whole. Like that, we'd know that
> we only ever do this if we hit a `goto failure` branch.
>

This is silly mistake on my side. This is exactly what I wanted to do,
but instead of:

  const char *reported_error = NULL;

I did:

  const char *reported_error;

Which obviously raised:

  In function ‘execute_commands_non_atomic’,
      inlined from ‘execute_commands’ at ../builtin/receive-pack.c:2059:3,
      inlined from ‘cmd_receive_pack’ at ../builtin/receive-pack.c:2636:3:
  ../builtin/receive-pack.c:1899:43: warning: ‘reported_error’ may be
used uninitialized [-Wmaybe-uninitialized]
   1899 |                         cmd->error_string = reported_error;
        |                         ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~
  ../builtin/receive-pack.c: In function ‘cmd_receive_pack’:
  ../builtin/receive-pack.c:1864:21: note: ‘reported_error’ was declared here
   1864 |         const char *reported_error;
        |                     ^~~~~~~~~~~~~~
  [31/31] Linking target git-upload-archive

prompting me to do this. Let me fix this. Thanks for pointing it out :)

> Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]