Re: [PATCH 3/3] gc: Clean garbage .bitmap files from pack dir

From: Jeff King <peff@peff.net>
Date: Dec 15, 2015, 23:23 UTC
Message-ID: <20151215232313.GB30353@sigill.intra.peff.net>
In-Reply-To: <1447461987-35450-3-git-send-email-dougk.ff7@gmail.com>

On Fri, Nov 13, 2015 at 04:46:27PM -0800, Doug Kelly wrote:

Show 21 quoted lines

> Similar to cleaning up excess .idx files, clean any garbage .bitmap
> files that are not otherwise associated with any .idx/.pack files.
> 
> Signed-off-by: Doug Kelly <dougk.ff7@gmail.com>
> ---
>  builtin/gc.c     | 12 ++++++++++--
>  t/t5304-prune.sh |  2 +-
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/gc.c b/builtin/gc.c
> index c583aad..7ddf071 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -58,8 +58,16 @@ static void clean_pack_garbage(void)
>  
>  static void report_pack_garbage(unsigned seen_bits, const char *path)
>  {
> -	if (seen_bits == PACKDIR_FILE_IDX)
> -		string_list_append(&pack_garbage, path);
> +	if (seen_bits & PACKDIR_FILE_IDX ||
> +	    seen_bits & PACKDIR_FILE_BITMAP) {

So here we're relying on report_helper to have culled the boring cases, right? (Sorry if that is totally obvious; I'm mostly just thinking out loud). That makes sense, then.

Show 8 quoted lines

> +		const char *dot = strrchr(path, '.');
> +		if (dot) {
> +			int baselen = dot - path + 1;
> +			if (!strcmp(path+baselen, "idx") ||
> +				!strcmp(path+baselen, "bitmap"))
> +				string_list_append(&pack_garbage, path);
> +		}
> +	}

I was confused at first why we couldn't just pass "path" here. But it's because we will get a garbage report for each related file, and we want to keep some of them (like .keep). Which I guess makes sense.

I wonder if this would be simpler to read as just:

  if (ends_with(path, ".idx") ||
      ends_with(path, ".bitmap"))
          string_list_append(&pack_garbage, path);

Technically it is less efficient because we will compute strlen(path) twice, but that seems like premature optimization (not to mention that ends_with is an inline, so a good compiler can probably optimize out the second call anyway).

Show 5 quoted lines

> -test_expect_failure 'clean pack garbage with gc' '
> +test_expect_success 'clean pack garbage with gc' '
>  	test_when_finished "rm -f .git/objects/pack/fake*" &&
>  	test_when_finished "rm -f .git/objects/pack/foo*" &&
>  	: >.git/objects/pack/foo.keep &&

Should we be checking at the end of this test that "*.keep" didn't get blown away? It might be nice to just test_cmp the results of "ls" on the pack directory to confirm exactly what got deleted and what didn't.

-Peff