Re: [PATCH v2 24/25] upload-pack: make check_reachable_object() return unreachable list if asked

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 5, 2016, 05:41 UTC
Message-ID: <CAPig+cQhYrS0LY5uv3KWSn0JTO8XHD74iUmV2cvCHHBRQH6nKA@mail.gmail.com>
In-Reply-To: <1454576641-29615-25-git-send-email-pclouds@gmail.com>

On Thu, Feb 4, 2016 at 4:04 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

Show 13 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,8 +451,16 @@ static int is_our_ref(struct object *o)
> -static int check_unreachable(struct object_array *src)
> +/*
> + * If reachable is NULL, return 1 if there is no unreachable object,
> + * zero otherwise.
> + *
> + * If reachable is not NULL, it's filled with reachable objects.
> + * Return value is irrelevant. The caller has to compare reachable and
> + * src to find out if there's any unreachable object.
> + */

This dual-purpose function serving up two entirely orthogonal modes feel strange. Would it make sense to split this into two functions, one for each paragraph in the above documentation? (Of course, they could share common implementation behind-the-scenes as needed.)

More below...

Show 30 quoted lines

> +static int check_unreachable(struct object_array *reachable,
> +                            struct object_array *src)
>  {
>         static const char *argv[] = {
>                 "rev-list", "--stdin", NULL,
> @@ -507,9 +522,31 @@ static int check_unreachable(struct object_array *src)
>          * The commits out of the rev-list are not ancestors of
>          * our ref.
>          */
> -       i = read_in_full(cmd.out, namebuf, 1);
> -       if (i)
> -               return 0;
> +       if (!reachable) {
> +               i = read_in_full(cmd.out, namebuf, 1);
> +               if (i)
> +                       return 0;
> +       } else {
> +               while ((i = read_in_full(cmd.out, namebuf, 41)) == 41) {
> +                       struct object_id sha1;
> +
> +                       if (namebuf[40] != '\n' || get_oid_hex(namebuf, &sha1))
> +                               break;
> +
> +                       o = lookup_object(sha1.hash);
> +                       if (o && o->type == OBJ_COMMIT) {
> +                               o->flags &= ~TMP_MARK;
> +                       }
> +               }
> +               for (i = get_max_object_index(); 0 < i; ) {
> +                       o = get_indexed_object(--i);

Perhaps code this as:

    for (i = get_max_object_index(); 0 < i; i--) {
        o = get_indexed_object(i - 1);

in order to keep the loop control within the 'for' itself?

Show 7 quoted lines

> +                       if (o && o->type == OBJ_COMMIT &&
> +                           (o->flags & TMP_MARK)) {
> +                               add_object_array(o, NULL, reachable);
> +                               o->flags &= ~TMP_MARK;
> +                       }
> +               }
> +       }