On Thu, Feb 4, 2016 at 4:04 AM, Nguyễn Thái Ngọc Duy <pclo...@gmail.com> wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclo...@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;
> +                       }
> +               }
> +       }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to