Patrick Steinhardt <ps@xxxxxx> 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:
signature.asc
Description: PGP signature
