On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller <sbeller@xxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
> ---
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html