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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 30, 2014, 07:46 UTC
Message-ID: <CAPig+cQEO+LKZJDzE-sUmo+td=djsOdjx-ouQEMRTTkOBMwzjA@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:

Show 26 quoted lines

> No functional changes intended.
>
> This commit shortens execute_commands by moving some parts of the code
> to extra functions.
>
> 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 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]) {

Another issue: In the original code, 'si->shallow_ref[cmd->index]' is only checked if cmd->error_string is NULL, but here you check it unconditionally, despite the commit message claiming no functional changes. Did you verify that such a behavioral change is benign? (This is a genuine question.)

Show 51 quoted lines

> +                       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");
> +}
> +
>  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