Re: [PATCH] update-server-info: avoid needless overwrites

From: Eric Sunshine <sunshine@sunshineco.com>
To: Eric Wong <e@80x24.org>
Cc: Git List <git@vger.kernel.org>
Subject: Re: [PATCH] update-server-info: avoid needless overwrites
Date: Sat, 11 May 2019 03:35:23 -0400	[thread overview]
Message-ID: <CAPig+cRZy4yM+ZuHhZK3tPV_QRvSy63aHBZZj6AcFTu+5FNG0A@mail.gmail.com> (raw)
In-Reply-To: <20190511013455.5886-1-e@80x24.org>

On Fri, May 10, 2019 at 9:35 PM Eric Wong <e@80x24.org> wrote:
> Do not change the existing info/refs and objects/info/packs
> files if they match the existing content on the filesystem.
> This is intended to preserve mtime and make it easier for dumb
> HTTP pollers to rely on the If-Modified-Since header.
> [...]
> Signed-off-by: Eric Wong <e@80x24.org>
> ---
> diff --git a/server-info.c b/server-info.c
> @@ -6,37 +6,78 @@
> +static int files_differ(FILE *fp, const char *path)
> +{
> +       [...]
> +       struct strbuf tmp = STRBUF_INIT;
> +       [...]
> +       if (strbuf_fread(&tmp, (size_t)new_len, fp) != (size_t)new_len)
> +               return 1;

Although strbuf_fread() will release 'tmp' automatically if the read
actually fails, it won't do so otherwise. So, if it reads more or
fewer bytes than expected, for some reason, then this early return
will leak the strbuf, won't it?

> +       the_hash_algo->init_fn(&c);
> +       the_hash_algo->update_fn(&c, tmp.buf, tmp.len);
> +       the_hash_algo->final_fn(oid_new.hash, &c);
> +       strbuf_release(&tmp);
> +
> +       if (strbuf_read_file(&tmp, path, (size_t)st.st_size) < 0)
> +               return 1;

Same issue.

> diff --git a/t/t5200-update-server-info.sh b/t/t5200-update-server-info.sh
> @@ -0,0 +1,41 @@
> +test_description='Test git stash show configuration.'

Copy/paste error for test description?