Re: [PATCH 27/31] builtin/difftool: use parse_oid_hex

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 12, 2019, 08:27 UTC
Message-ID: <CAPig+cRTKTWPA66YTnX=RMJyCZmWK6efkxOyfYH_zn9OOSb8_w@mail.gmail.com>
In-Reply-To: <20190212012256.1005924-28-sandals@crustytoothpaste.net>

On Mon, Feb 11, 2019 at 8:24 PM brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 12 quoted lines

> Instead of using get_oid_hex and adding constants to the result, use
> parse_oid_hex to make this code independent of the hash size.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> diff --git a/builtin/difftool.c b/builtin/difftool.c
> @@ -65,14 +65,12 @@ static int parse_index_info(char *p, int *mode1, int *mode2,
>         if (*p != ' ')
>                 return error("expected ' ', got '%c'", *p);
> -       if (get_oid_hex(++p, oid1))
> +       if (parse_oid_hex(++p, oid1, (const char **)&p))
>                 return error("expected object ID, got '%s'", p + 1);

Not a problem introduced by this patch, but is the 'p + 1' in the error message correct? 'p' has already been incremented via '++p' in the call to parse_oid_hex() to point at what _should_ be the start of OID, so one would think that the error message would want to print out whatever was found there rather than what was found one character after the start of OID.

> -       if (get_oid_hex(++p, oid2))
> +       if (parse_oid_hex(++p, oid2, (const char **)&p))
>                 return error("expected object ID, got '%s'", p + 1);

Ditto.