Re: [PATCHv8 5/9] receive-pack.c: move transaction handling in a central place

> On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller <sbeller@google.com> wrote:
>> Subject: receive-pack.c: move transaction handling in a central place
>
> This is very generic, and doesn't really explain what this patch is
> about. (See below.)
>
>> No functional changes intended.
>
> Secondary information can be demoted to the end of the commit message.

>
>> This moves all code related to transactions into the execute_commands_loop
>> function which was factored out of execute_commands. This includes
>> beginning and committing the transaction as well as dealing with the
>> errors which may occur during the begin and commit phase of a transaction.
>
> This explains what you're doing, but not why. The purpose of this
> change is that a subsequent patch will be adding another mode of
> operation ("atomic") to execute_commands() which differs from the
> existing mode ("non-atomic") implemented by its main loop. In its
> high-level role, execute_commands() does not need to know or care
> about the low-level details of each mode of operation. Therefore, as
> preparation for introducing a new mode, you're factoring out the
> existing mode into its own stand-alone function.
>
>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>
>> Notes:
>>     This covers the suggestion of patch 2 and 3 by Eric
>>     > patch 2: Factor out the main 'for' loop of execute_commands() into a
>>     > new function. This new function will eventually become
>>     > execute_commands_non_atomic(). At this point, execute_commands() is
>>     > pretty much in its final form with the exception of the upcoming 'if
>>     > (use_atomic)' conditional.
>>     > patch 3: Morph the function extracted in patch 2 into
>>     > execute_commands_non_atomic() by adding transaction handling inside
>>     > the 'for' loop (and applying the changes from the early part of the
>>     > patch which go along with that).
>
> This patch is still rather heavyweight. My suggestion[1] for making
> these particular changes across two patches was quite deliberate. The
> problem with combining them into a single patch is that you're
> performing both code movement and functional changes at the same time.
>
> On its own, pure code movement is easy to review.
>
> On its own, code changes are as easy or difficult to review as the
> changes themselves.

>
> When combined, however, the review effort is greater than the sum of
> the efforts of reviewing them separately. Partly this is because the
> combined changes have a noisier diff. If you move the code in one
> patch, and then change it in a second one, the changes pop out --
> they're quite obvious. On the other hand, when they are combined, the
> reviewer has to deliberately and painstakingly search out the changes,
> which is difficult and error-prone. Combining movement and code
> changes into a single patch also places greater cognitive load on the
> reviewer due to the necessity of keeping a more complex mental
> scoreboard relating to the different types of changes.
>
> More below.
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/261706
>
>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
>> index 06eb287..5f44466 100644
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -1073,6 +1076,38 @@ static void check_shallow_bugs(struct command *commands,
>>                       "the reported refs above");
>>  }
>>
>> +static void execute_commands_loop(struct command *commands,
>> +                                 struct shallow_info *si)
>
> Style: Indent the wrapped line to align with the text following the
> '(' in the first line.

>
> It's safe to say that the code which you extracted from
> execute_commands() handled the non-atomic case, and it's safe to say
> that this new function implements the non-atomic case. Therefore, it
> would be truthful to call this function execute_commands_nonatomic().
> No need to invent the name execute_commands_loop().

>
>> +{
>> +       struct command *cmd;
>> +       struct strbuf err = STRBUF_INIT;
>> +
>> +       for (cmd = commands; cmd; cmd = cmd->next) {
>> +               if (!should_process_cmd(cmd))
>> +                       continue;
>> +
>> +               transaction = ref_transaction_begin(&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);
>> +
>> +               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);
>> +       }
>> +
>> +       strbuf_release(&err);
>> +}
>> +
>>  static void execute_commands(struct command *commands,
>>                              const char *unpacker_error,
>>                              struct shallow_info *si)
>> @@ -1107,12 +1142,8 @@ static void execute_commands(struct command *commands,
>>         free(head_name_to_free);
>>         head_name = head_name_to_free = resolve_refdup("HEAD", 0, sha1, NULL);
>>
>> -       for (cmd = commands; cmd; cmd = cmd->next) {
>> -               if (!should_process_cmd(cmd))
>> -                       continue;
>> +       execute_commands_loop(commands, si);
>>
>> -               cmd->error_string = update(cmd, si);
>> -       }
>>         check_shallow_bugs(commands, si);
>>  }
>>
>> --
>> 2.2.1.62.g3f15098