On Fri, Nov 13, 2015 at 04:46:27PM -0800, Doug Kelly wrote:
> 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@xxxxxxxxx>
> ---
> 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.
> + 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).
> -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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html