Re: [PATCH v3 3/5] repack: add --sparse and pass to pack-objects

From: Derrick Stolee <stolee@gmail.com>
Date: Jun 24, 2019, 13:03 UTC
Message-ID: <25cd9bcb-9b8f-662c-75d2-a5b32d7febbc@gmail.com>
In-Reply-To: <20190624120711.27744-4-nwf20@cl.cam.ac.uk>

On 6/24/2019 8:07 AM, Nathaniel Filardo wrote:

> The sparse connectivity algorithm saves a whole lot of time when there
> are UNINTERESTING trees around.

Neat! Any chance for some example performance stats?

> ---

Looks like you forgot your Signed-off-by here.

Show 5 quoted lines

> +# repack --sparse invokes pack-objects --sparse
> +test_expect_success 'repack --sparse and fsck' '
> +	git repack -a --sparse &&
> +	git fsck
> +'

This test may not be enough to properly test the sparse algorithm, as it is only really enabled when the --revs argument is given to the pack-objects process AND there is a "NOT" ref (i.e. "!{oid}" over stdin). Using "-a" here will not have any "NOT" references. OR maybe this already is enough when you have the .keep packs. Is there a way you could set up the packs in this test to explicitly be in the situation you describe where the sparse option speeds things up?

It's hard to check that the '--sparse' option is doing anything in a test, but it is important that we run the logic. One way to see if this test is doing anything is to insert a die() somewhere. For example, this die() statement will check if we ever needed to mark things uninteresting in a workdir path with both UNINTERESTING and INTERESTING trees:

diff --git a/revision.c b/revision.c
index eb8e51bc63..8835f8e7b1 100644
--- a/revision.c
+++ b/revision.c
@@ -227,6 +227,7 @@ void mark_trees_uninteresting_sparse(struct repository *r,
        if (!has_uninteresting || !has_interesting)
                return;

+       die("we exercised the logic!");
        paths_and_oids_init(&map);

        oidset_iter_init(trees, &iter);

The implementation of the argument looks straight-forward.
It appears we are passing the argument to the sub-process,
so the bitflag isn't used yet.

-Stolee