Re: [PATCH 3/3] fetch.c: use a single ref transaction for all ref updates

> On Tue, Apr 22, 2014 at 2:45 PM, Ronnie Sahlberg <sahlberg@google.com> wrote:
>> Change store_updated_refs to use a single ref transaction for all refs that
>> are updated during the fetch. This makes the fetch more atomic when update
>> failures occur.
>>
>> Since ref update failures will now no longer occur in the code path for
>> updating a single ref in s_update_ref, we no longer have as detailed error
>> message logging the exact reference and the ref log action as in the old code.
>> Instead since we fail the entire transaction we log a much more generic
>> message. But since we commit the transaction using MSG_ON_ERR we will log
>> an error containing the ref name if either locking of writing the ref would
>> so the regression in the log message is minor.
>>
>> This will also change the order in which errors are checked for and logged
>> which may alter which error will be logged if there are multiple errors
>> occuring during a fetch.
>>
>> For example, assuming we have a fetch for two refs that both would fail.
>
> s/assuming/assume/ perhaps?
>
>> Where the first ref would fail with ENOTDIR due to a directory in the ref
>> path not existing, and the second ref in the fetch would fail due to
>> the check in update_logical_ref():
>>         if (current_branch &&
>>             !strcmp(ref->name, current_branch->name) &&
>>             !(update_head_ok || is_bare_repository()) &&
>>             !is_null_sha1(ref->old_sha1)) {
>>                 /*
>>                  * If this is the head, and it's not okay to update
>>                  * the head, and the old value of the head isn't empty...
>>                  */
>>
>> In the old code sicne we would update the refs one ref at a time we would
>
> s/sicne/since/
>
>> first fail the ENOTDIR and then fail the second update of HEAD as well.
>> But since the first ref failed with ENOTDIR we would eventually fail the whole
>> fetch with STORE_REF_ERROR_DF_CONFLICT
>>
>> In the new code, since we defer committing the transaction until all refs
>> has been processed, we would now detect that the second ref was bad and
>
> s/has/have/
>
>> rollback the transaction before we would even try start writing the update to
>> disk and thus we would not return STORE_REF_ERROR_DF_CONFLICT for this case.
>>
>> I think this new behaviour is more correct, since if there was a problem
>> we would not even try to commit the transaction but need to highlight this
>> change in how/what errors are reported.
>> This change in what error is returned only occurs if there are multiple
>> refs that fail to update and only some, but not all, of them fail due to
>> ENOTDIR.
>>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>