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

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Feb 4, 2016 at 4:04 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,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...

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

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