Re: [PATCH 15/18] fill_sha1_file: write into a strbuf

[Jacob Keller <jacob.keller@gmail.com>]

On Mon, Oct 3, 2016 at 1:36 PM, Jeff King <peff@peff.net> wrote:
> It's currently the responsibility of the caller to give
> fill_sha1_file() enough bytes to write into, leading them to
> manually compute the required lengths. Instead, let's just
> write into a strbuf so that it's impossible to get this
> wrong.

Yea this makes sense.

>
> The alt_odb caller already has a strbuf, so this makes
> things strictly simpler. The other caller, sha1_file_name(),
> uses a static PATH_MAX buffer and dies when it would
> overflow. We can convert this to a static strbuf, which
> means our allocation cost is amortized (and as a bonus, we
> no longer have to worry about PATH_MAX being too short for
> normal use).
>
> This does introduce some small overhead in fill_sha1_file(),
> as each strbuf_addchar() will check whether it needs to
> grow. However, between the optimization in fec501d
> (strbuf_addch: avoid calling strbuf_grow, 2015-04-16) and
> the fact that this is not generally called in a tight loop
> (after all, the next step is typically to access the file!)
> this probably doesn't matter. And even if it did, the right
> place to micro-optimize is inside fill_sha1_file(), by
> calling a single strbuf_grow() there.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  sha1_file.c | 34 ++++++++++------------------------
>  1 file changed, 10 insertions(+), 24 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index efc8cee..80a3333 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -172,36 +172,28 @@ enum scld_error safe_create_leading_directories_const(const char *path)
>         return result;
>  }
>
> -static void fill_sha1_path(char *pathbuf, const unsigned char *sha1)
> +static void fill_sha1_path(struct strbuf *buf, const unsigned char *sha1)
>  {
>         int i;
>         for (i = 0; i < 20; i++) {
>                 static char hex[] = "0123456789abcdef";
>                 unsigned int val = sha1[i];
> -               *pathbuf++ = hex[val >> 4];
> -               *pathbuf++ = hex[val & 0xf];
> +               strbuf_addch(buf, hex[val >> 4]);
> +               strbuf_addch(buf, hex[val & 0xf]);
>                 if (!i)
> -                       *pathbuf++ = '/';
> +                       strbuf_addch(buf, '/');
>         }
> -       *pathbuf = '\0';
>  }
>
>  const char *sha1_file_name(const unsigned char *sha1)
>  {
> -       static char buf[PATH_MAX];
> -       const char *objdir;
> -       int len;
> +       static struct strbuf buf = STRBUF_INIT;
>
> -       objdir = get_object_directory();
> -       len = strlen(objdir);
> +       strbuf_reset(&buf);
> +       strbuf_addf(&buf, "%s/", get_object_directory());
>
> -       /* '/' + sha1(2) + '/' + sha1(38) + '\0' */
> -       if (len + 43 > PATH_MAX)
> -               die("insanely long object directory %s", objdir);
> -       memcpy(buf, objdir, len);
> -       buf[len] = '/';
> -       fill_sha1_path(buf + len + 1, sha1);
> -       return buf;

I'm definitely a fan of seeing the magic number here go away.

> +       fill_sha1_path(&buf, sha1);
> +       return buf.buf;
>  }
>
>  struct strbuf *alt_scratch_buf(struct alternate_object_database *alt)
> @@ -213,14 +205,8 @@ struct strbuf *alt_scratch_buf(struct alternate_object_database *alt)
>  static const char *alt_sha1_path(struct alternate_object_database *alt,
>                                  const unsigned char *sha1)
>  {
> -       /* hex sha1 plus internal "/" */
> -       size_t len = GIT_SHA1_HEXSZ + 1;
>         struct strbuf *buf = alt_scratch_buf(alt);

Funny story.. While reviewing this code on my screen, my monitor has a
nice little bit of gunk just between the lines that made this one look
like it was being deleted. So I was really confused as to what strbuf
you were using and why you removed a call to alt_scratch_buf()..
Obviously this line just isn't being removed.

> -
> -       strbuf_grow(buf, len);
> -       fill_sha1_path(buf->buf + buf->len, sha1);
> -       strbuf_setlen(buf, buf->len + len);
> -
> +       fill_sha1_path(buf, sha1);
>         return buf->buf;
>  }
>
> --
> 2.10.0.618.g82cc264
>