Re: [PATCH 12/31] fast-import: make hash-size independent

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 12, 2019, 03:44 UTC
Message-ID: <CAPig+cQivuPjDztK30QO0xKx1Tx+nYDPzTvgXvi9WVyRX5-aRQ@mail.gmail.com>
In-Reply-To: <20190212012256.1005924-13-sandals@crustytoothpaste.net>

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

Show 10 quoted lines

> Replace several uses of GIT_SHA1_HEXSZ and 40-based constants with
> references to the_hash_algo.  Update the note handling code here to
> compute path sizes based on GIT_MAX_RAWSZ as well.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> diff --git a/fast-import.c b/fast-import.c
> @@ -2047,7 +2047,8 @@ static uintmax_t do_change_note_fanout(
> -       char realpath[60];
> +       char realpath[GIT_MAX_RAWSZ * 3];

I wonder if the fixed multiplier deserves a comment explaining that this is reserving enough space for a hex representation with '/' between each digit pair plus NUL. Which leads to the next question: Is there is GIT_MAX_HEXSZ constant? If so, this might be more clearly represented (or not) by taking advantage of that value.

Also, there are a number of hardcoded 60's in the code earlier in this file, such as:

    if ((max_packsize && (pack_size + 60 + len) > max_packsize)
        || (pack_size + 60 + len) < pack_size)
        cycle_packfile();

Is that just a coincidence or is it related to the 60 characters allocated for 'realpath'?

Show 6 quoted lines

> @@ -2405,7 +2406,7 @@ static void note_change_n(const char *p, struct branch *b, unsigned char *old_fa
>                 char *buf = read_object_with_reference(&commit_oid,
>                                                        commit_type, &size,
>                                                        &commit_oid);
> -               if (!buf || size < 46)
> +               if (!buf || size < the_hash_algo->hexsz)

What exactly did the 46 represent and how does it relate to 'hexsz'? Stated differently, why didn't this become:

    the_hash_algo->hexsz + 6'

Show 5 quoted lines

> @@ -2456,7 +2457,7 @@ static void file_change_deleteall(struct branch *b)
>  static void parse_from_commit(struct branch *b, char *buf, unsigned long size)
>  {
> -       if (!buf || size < GIT_SHA1_HEXSZ + 6)
> +       if (!buf || size < the_hash_algo->hexsz + 6)

...as it seems to have here...

> @@ -2555,7 +2556,7 @@ static struct hash_list *parse_merge(unsigned int *count)
> -                       if (!buf || size < 46)
> +                       if (!buf || size < the_hash_algo->hexsz)

...but not here.