Re: [PATCHv8 4/9] receive-pack.c: simplify execute_commands

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller <sbeller@google.com> wrote:
> No functional changes intended.

This is useful to know but is of secondary importance, thus should be
placed after the explanation and justification of the change.

> Subject: receive-pack.c: simplify execute_commands
> This commit shortens execute_commands by moving some parts of the code
> to extra functions.

The _real_ reason for moving these bits of code into their own
functions is that you intend to introduce additional callers in
subsequent patches. That is what the commit message (including
subject) should be conveying to the reader.

The claimed simplification is questionable and not of particular
importance; and it's misleading to paint it as a goal of the patch.
Consequently, you could drop mention of it altogether.

More below.

> Suggested-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 4e8eaf7..06eb287 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1043,11 +1043,40 @@ static void reject_updates_to_hidden(struct command *commands)
>         }
>  }
>
> +static int should_process_cmd(struct command *cmd)
> +{
> +       if (cmd->error_string)
> +               return 0;
> +       if (cmd->skip_update)
> +               return 0;
> +       return 1;

Alternately, depending upon the polarity of your brain, you could
collapse the entire function body to:

    return !cmd->error_string && !cmd->skip_update;

or:

    return !(cmd->error_string || cmd->skip_update);

> +}
> +
> +static void check_shallow_bugs(struct command *commands,
> +                              struct shallow_info *si)
> +{
> +       struct command *cmd;
> +       int checked_connectivity = 1;
> +       for (cmd = commands; cmd; cmd = cmd->next) {
> +               if (!should_process_cmd(cmd))
> +                       continue;
> +
> +               if (shallow_update && si->shallow_ref[cmd->index]) {

Here, inside the loop, you check 'shallow_update'...

> +                       error("BUG: connectivity check has not been run on ref %s",
> +                             cmd->ref_name);
> +                       checked_connectivity = 0;
> +               }
> +       }
> +       if (shallow_update && !checked_connectivity)

And here, after the loop, you check 'shallow_update'.

But, if you examine the overall logic, you will find that this
function does _nothing_ at all when 'shallow_update' is false (other
than uselessly looping through 'commands'). Therefore, either check
'shallow_update' just once at the beginning of the function and exit
early if false, or have the caller check 'shallow_update' and only
invoke check_shallow_bugs() if true.

Also, since nothing happens between them, the two conditionals inside
the loop can be coalesced:

    if (should_process_cmd(cmd) && si->shallow_ref[cmd->index]) {

> +               error("BUG: run 'git fsck' for safety.\n"
> +                     "If there are errors, try to remove "
> +                     "the reported refs above");

In v6, you considered this a fatal error in the atomic case, which
caused the entire transaction to be rolled back. However, in this
version, this error has no effect whatsoever on the atomic
transaction, which is a rather significant behavioral departure. Which
is correct? (This is a genuine question; not at all rhetorical.) If
failing the entire transaction is the correct thing to do, then this
is going to need some more work.

> +}
> +
>  static void execute_commands(struct command *commands,
>                              const char *unpacker_error,
>                              struct shallow_info *si)
>  {
> -       int checked_connectivity;
>         struct command *cmd;
>         unsigned char sha1[20];
>         struct iterate_data data;
> @@ -1078,27 +1107,13 @@ static void execute_commands(struct command *commands,
>         free(head_name_to_free);
>         head_name = head_name_to_free = resolve_refdup("HEAD", 0, sha1, NULL);
>
> -       checked_connectivity = 1;
>         for (cmd = commands; cmd; cmd = cmd->next) {
> -               if (cmd->error_string)
> -                       continue;
> -
> -               if (cmd->skip_update)
> +               if (!should_process_cmd(cmd))
>                         continue;
>
>                 cmd->error_string = update(cmd, si);
> -               if (shallow_update && !cmd->error_string &&
> -                   si->shallow_ref[cmd->index]) {
> -                       error("BUG: connectivity check has not been run on ref %s",
> -                             cmd->ref_name);
> -                       checked_connectivity = 0;
> -               }
>         }
> -
> -       if (shallow_update && !checked_connectivity)
> -               error("BUG: run 'git fsck' for safety.\n"
> -                     "If there are errors, try to remove "
> -                     "the reported refs above");
> +       check_shallow_bugs(commands, si);
>  }
>
>  static struct command **queue_command(struct command **tail,
> --
> 2.2.1.62.g3f15098