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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 25, 2016, 07:36 UTC
Message-ID: <CAPig+cRM_6Yj0=ZOwf8xq9fipCVOFVVLeh5yOpqTxrq4r314iw@mail.gmail.com>
In-Reply-To: <1460552110-5554-10-git-send-email-pclouds@gmail.com>

On Wed, Apr 13, 2016 at 8:54 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

Show 34 quoted lines

> 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?

Show 7 quoted lines

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

Show 7 quoted lines

>         /*
> @@ -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()?

Show 18 quoted lines

>         /* 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.

Show 8 quoted lines

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