Re: [PATCH 25/26] upload-pack: add get_reachable_list()

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Apr 13, 2016 at 8:55 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
> @@ -505,10 +513,48 @@ static int do_reachable_revlist(struct child_process *cmd,
> +static int get_reachable_list(struct object_array *src,
> +                             struct object_array *reachable)
> +{
> +       struct child_process cmd = CHILD_PROCESS_INIT;
> +       int i, ret = do_reachable_revlist(&cmd, src, reachable);
> +       struct object *o;
> +       char namebuf[42]; /* ^ + SHA-1 + LF */
> +
> +       if (ret < 0)
> +               return -1;

Same comment as previous patch: It's a bit difficult to see what this
conditional is checking since it is so far removed (textually) from
the assignment. Perhaps:

    ret = do_reachable_revlist(...);
    if (ret < 0)

would make it clearer.

> +       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; i--) {
> +               o = get_indexed_object(i - 1);
> +               if (o && o->type == OBJ_COMMIT &&
> +                   (o->flags & TMP_MARK)) {
> +                       add_object_array(o, NULL, reachable);
> +                               o->flags &= ~TMP_MARK;
> +               }
> +       }
> +       close(cmd.out);
> +
> +       if (finish_command(&cmd))
> +               return -1;
> +
> +       return 0;
> +}