Re: [PATCH 24/26] upload-pack: split check_unreachable() in two, prep for get_reachable_list()

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Apr 13, 2016 at 8:55 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/upload-pack.c b/upload-pack.c
> @@ -452,24 +452,24 @@ static int is_our_ref(struct object *o)
> -static int check_unreachable(struct object_array *src)
> +static int do_reachable_revlist(struct child_process *cmd,
> +                               struct object_array *src)
> @@ -487,8 +487,8 @@ static int check_unreachable(struct object_array *src)
>                 if (!is_our_ref(o))
>                         continue;
>                 memcpy(namebuf + 1, oid_to_hex(&o->oid), GIT_SHA1_HEXSZ);
> -               if (write_in_full(cmd.in, namebuf, 42) < 0)
> -                       return 0;
> +               if (write_in_full(cmd->in, namebuf, 42) < 0)
> +                       return -1;
>         }
>         namebuf[40] = '\n';
>         for (i = 0; i < src->nr; i++) {
> @@ -496,18 +496,29 @@ static int check_unreachable(struct object_array *src)
>                 if (is_our_ref(o))
>                         continue;
>                 memcpy(namebuf, oid_to_hex(&o->oid), GIT_SHA1_HEXSZ);
> -               if (write_in_full(cmd.in, namebuf, 41) < 0)
> -                       return 0;
> +               if (write_in_full(cmd->in, namebuf, 41) < 0)
> +                       return -1;
>         }
> -       close(cmd.in);
> +       close(cmd->in);
>
>         sigchain_pop(SIGPIPE);

Not a new issue, but does it matter that there are early returns
before sigchain_pop()?

> +       return 0;
> +}
> +
> +static int check_unreachable(struct object_array *src)
> +{
> +       struct child_process cmd = CHILD_PROCESS_INIT;
> +       int i, ret = do_reachable_revlist(&cmd, src);
> +       char buf[1];
> +
> +       if (ret < 0)
> +               return 0;

Nit: It might be a bit easier to see what this conditional is checking
if it is closer to the code which sets 'ret'. Perhaps either:

    char buf[1];
    int i, ret = ...;

    if (ret < 0)

or:

    char buf[1];
    int i, ret;

    ret = ...;
    if (ret < 0)

>         /*
>          * The commits out of the rev-list are not ancestors of
>          * our ref.
>          */
> -       i = read_in_full(cmd.out, namebuf, 1);
> +       i = read_in_full(cmd.out, buf, 1);

s/1/sizeof(buf)/ perhaps or does that make the intent less clear?

>         if (i)
>                 return 0;
>         close(cmd.out);