Re: [PATCHv8 5/9] receive-pack.c: move transaction handling in a central place
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Dec 30, 2014, 08:36 UTC
- Message-ID
- <CAPig+cSYD+gBdEZ9TzWdeTEufzH6eJTbt=ZVS5imMJGjWnUFPA@mail.gmail.com>
- In-Reply-To
- <1419907007-19387-6-git-send-email-sbeller@google.com>
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.
Show 15 quoted lines
> 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
Show 10 quoted lines
> 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().
Show 49 quoted lines
> +{
> + 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