Re: [PATCH 1/2] maintenance: add pack-refs task

From: Derrick Stolee <stolee@gmail.com>
Date: Feb 9, 2021, 12:42 UTC
Message-ID: <f8df68ee-32a9-4bd1-99d9-4641b582992d@gmail.com>
In-Reply-To: <YCHA991dv0V0hNb+@nand.local>

On 2/8/2021 5:53 PM, Taylor Blau wrote:

Show 10 quoted lines

> On Mon, Feb 08, 2021 at 02:52:22PM +0000, Derrick Stolee via GitGitGadget wrote:
>> +pack-refs::
>> +	The `pack-refs` task collects the loose reference files and
>> +	collects them into a single file. This speeds up operations that
>> +	need to iterate across many refereences. See linkgit:git-pack-refs[1]
>> +	for more information.
>> +
> 
> Do you think it's worth documenting here or in git-gc(1) that running
> this has the effect of disabling the same step during gc?

It doesn't disable the step during gc.

Perhaps I should use a better term than "extract". The 'gc' task shouldn't change as we make some of its steps also available as independent tasks.

Instead, users can select a subset of its steps by enabling them directly. Other such tasks could include:

 * prune-reflog
 * full-repack (as opposed to the existing incremental-repack task)

Show 5 quoted lines

>> +struct maintenance_run_opts;
>> +static int maintenance_task_pack_refs(struct maintenance_run_opts *opts)
> 
> It may be worth calling this "unused", since you explicitly pass NULL
> below.

Good idea.

Show 6 quoted lines

>> +	if (pack_refs && maintenance_task_pack_refs(NULL))
>> +		die(FAILED_RUN, "pack-refs");
> 
> Hmm. Am I missing where opting into the maintenance step disables this
> in gc? You suggest that it does in the commit message, but I can't seem
> to see it here.

My commit message suggests wrong.

Show 10 quoted lines

>> +test_expect_success 'pack-refs task' '
>> +	for n in $(test_seq 1 5)
>> +	do
>> +		git branch -f to-pack/$n HEAD || return 1
>> +	done &&
>> +	git maintenance run --task=pack-refs &&
>> +	ls .git/refs/heads/ >after &&
>> +	test_must_be_empty after
> 
> Worth testing that $GIT_DIR/packed-refs exists, too?

That would start breaking if we change the backing store, right? I want to be sure this doesn't create test breakages with reftable.

I _could_ add a 'test_subcommand' check here, though.

Thanks, -Stolee