From: Stefan Beller <sbeller@google.com>
Date: Mon, 24 Apr 2017 17:20:27 GMT
Subject: Re: [PATCH 11/53] fast-import: convert to struct object_id
Message-ID: <CAGZ79kbdVEhcxP4dhSR6GaOKjgTD+bPn_+5edV4TjV3A84HQ-A@mail.gmail.com>
In-Reply-To: <20170423213453.253425-12-sandals@crustytoothpaste.net>

On Sun, Apr 23, 2017 at 2:34 PM, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> Convert the remaining parts of fast-import.c to use struct object_id.
> Convert several instances of get_sha1_hex to parse_oid_hex to avoid
> needing to specify constants.  Convert other hardcoded values to named
> constants.  Finally, use the is_empty_tree_oid function instead of a
> direct comparison against a fixed string.
>
> Note that the odd computation with GIT_MAX_HEXSZ is due to the insertion
> of a slash between every two hex digits in the path, plus one for the
> terminating NUL.

Up to and including this patch are
Reviewed-by: Stefan Beller <sbeller@google.com>

I may continue reviewing this series once my eyes are refreshed.

>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---

> @@ -2823,12 +2821,10 @@ static void parse_new_commit(const char *arg)
>         strbuf_addf(&new_data, "tree %s\n",
>                 oid_to_hex(&b->branch_tree.versions[1].oid));
>         if (!is_null_oid(&b->oid))
> -               strbuf_addf(&new_data, "parent %s\n",
> -                           oid_to_hex(&b->oid));
> +               strbuf_addf(&new_data, "parent %s\n", oid_to_hex(&b->oid));
>         while (merge_list) {
>                 struct hash_list *next = merge_list->next;
> -               strbuf_addf(&new_data, "parent %s\n",
> -                           oid_to_hex(&merge_list->oid));
> +               strbuf_addf(&new_data, "parent %s\n", oid_to_hex(&merge_list->oid));
>                 free(merge_list);
>                 merge_list = next;
>         }

This is a funny one. The only change is line rewrapping, as it fits
into 80 cols easily.
I was reviewing this series using colored --word-diff output, and this hunk does
not produce any red or green. I wonder if this is the intended
behavior of the word diffing
or if we rather want to insert a <RED> - \n - </RED>.

Thanks,
Stefan