Re: [PATCH 09/26] upload-pack: move rev-list code out of check_non_tip()

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Apr 13, 2016 at 8:54 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
> @@ -451,7 +451,7 @@ static int is_our_ref(struct object *o)
> -static void check_non_tip(void)
> +static int check_unreachable(struct object_array *src)
> @@ -461,14 +461,6 @@ static void check_non_tip(void)
> -       /*
> -        * In the normal in-process case without
> -        * uploadpack.allowReachableSHA1InWant,
> -        * non-tip requests can never happen.
> -        */
> -       if (!stateless_rpc && !(allow_unadvertised_object_request & ALLOW_REACHABLE_SHA1))
> -               goto error;
> -
> @@ -476,7 +468,7 @@ static void check_non_tip(void)
>         if (start_command(&cmd))
> -               goto error;
> +               return 0;
> @@ -495,16 +487,16 @@ static void check_non_tip(void)
>                 if (write_in_full(cmd.in, namebuf, 42) < 0)
> -                       goto error;
> +                       return 0;
> [...]
> +       for (i = 0; i < src->nr; i++) {
> +               o = src->objects[i].item;
>                 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)
> -                       goto error;
> +                       return 0;
>         }
>         close(cmd.in);

It's a little bit ugly how this close() becomes disconnected after the
refactoring. In the original code, due to the consistent application
of 'goto error', it's reasonably obvious that skipping the close() is
not harmful since the error case die()'s unconditionally (according to
the comment). However, after the refactoring, the function simply
returns without invoking close(), so there's a disconnect, and it's
not obvious without looking at the caller that the program will die().

Also, if this function later gets a new caller, is that caller going
to need intimate knowledge about this potential descriptor leak?

> @@ -516,7 +508,7 @@ static void check_non_tip(void)
>          */
>         i = read_in_full(cmd.out, namebuf, 1);
>         if (i)
> -               goto error;
> +               return 0;
>         close(cmd.out);

Same observation.

It might be clearer if you retained the 'error' label and used it to
ensure that the descriptors get closed:

    cmd.in = -1;
    cmd.out = -1;
    ...
    if (...)
        goto error;
    ...
    /* All the non-tip ... */
    return 1;

error:
    if (cmd.in >= 0)
        close(cmd.in);
    if (cmd.out >= 0)
        close(cmd.out);
    return 0;

>         /*
> @@ -525,15 +517,29 @@ static void check_non_tip(void)
>          * even when it showed no commit.
>          */
>         if (finish_command(&cmd))
> -               goto error;
> +               return 0;

Here too. Does 'cmd' need to be cleaned up if the function bails
before finish_command()?

>         /* All the non-tip ones are ancestors of what we advertised */
> -       return;
> +       return 1;
> +}
> +
> +static void check_non_tip(void)
> +{
> +       int i;
> +
> +       /*
> +        * In the normal in-process case without
> +        * uploadpack.allowReachableSHA1InWant,
> +        * non-tip requests can never happen.
> +        */
> +       if (!stateless_rpc && !(allow_unadvertised_object_request & ALLOW_REACHABLE_SHA1))
> +               ;               /* error */
> +       else if (check_unreachable(&want_obj))
> +               return;

With the loss of the 'error' label (below), this logic becomes a bit
more difficult to follow. I wonder if it would help to invert the
sense of the conditional...

    if (stateless_rpc || ...)
        if (check_unreachable(...))
            return;

Or, perhaps, retain the 'error' label:

    if (!stateless_rpc && ...)
        goto error:
    if (check_unreachable(...))
        return;

error:
    /* Pick one ... */

I think I might find the latter a bit clearer, but it's highly subjective.

> -error:
>         /* Pick one of them (we know there at least is one) */
>         for (i = 0; i < want_obj.nr; i++) {
> -               o = want_obj.objects[i].item;
> +               struct object *o = want_obj.objects[i].item;
>                 if (!is_our_ref(o))
>                         die("git upload-pack: not our ref %s",
>                             oid_to_hex(&o->oid));