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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 30, 2014, 06:11 UTC
Message-ID: <CAPig+cT9JVkDvUUsbx9HW8HpakCb9SkoQf3LJZc4h3TQTt2ZXQ@mail.gmail.com>
In-Reply-To: <1419907007-19387-5-git-send-email-sbeller@google.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.

Show 18 quoted lines

> 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);

Show 12 quoted lines

> +}
> +
> +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'...

Show 6 quoted lines

> +                       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.

Show 42 quoted lines

> +}
> +
>  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