Re: [PATCH v2 2/5] repack: add --keep-pack option

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 6, 2018, 18:25 UTC
Message-ID: <xmqq1sgx5adl.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180306104158.6541-3-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> +--keep-pack=<pack name>::
> +	Ignore the given pack. This is the equivalent of having
> +	`.keep` file on the pack. Implies `--honor-pack-keep`.
> +

A few questions I am not sure how I would answer:

 - Do we want to have this listed in the SYNOPSIS section, too?

 - We would want to make the SP in "<pack name>" consistent with
   the dash in "<missing-action>" in the same document; which way do
   we make it uniform?

 - Is this description clear enough to convey that we allow more
   than one instance of this option specified, and the pack names
   accumulate?

 - Are there use cases where we want to _ignore_ on-disk ".keep" and
   only honor the ones given via the "--keep-pack" options?

 - Is this description clear enough to convey that <pack name> is
   just the filename part (i.e. "pack-[0-9a-f]{40}.pack") in our
   local $GIT_OBJECT_DIRECTORY/pack/ and not a full path to the
   packfile?  I think that design is sensible, simplifies the
   implementation and reduces mistakes.

Show 25 quoted lines

> +static void add_extra_kept_packs(const struct string_list *names)
> +{
> +	struct packed_git *p;
> +
> +	if (!names->nr)
> +		return;
> +
> +	prepare_packed_git();
> +	for (p = packed_git; p; p = p->next) {
> +		const char *name = basename(p->pack_name);
> +		int i;
> +
> +		if (!p->pack_local)
> +			continue;
> +
> +		for (i = 0; i < names->nr; i++) {
> +			if (fspathcmp(name, names->items[i].string))
> +				continue;
> +
> +			p->pack_keep = 1;
> +			ignore_packed_keep = 1;
> +			break;
> +		}
> +	}
> +}

OK.

Show 21 quoted lines

> diff --git a/builtin/repack.c b/builtin/repack.c
> index 7bdb40142f..6a1dade0e1 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -86,7 +86,8 @@ static void remove_pack_on_signal(int signo)
>   * have a corresponding .keep or .promisor file. These packs are not to
>   * be kept if we are going to pack everything into one file.
>   */
> -static void get_non_kept_pack_filenames(struct string_list *fname_list)
> +static void get_non_kept_pack_filenames(struct string_list *fname_list,
> +					const struct string_list *extra_keep)
>  {
>  	DIR *dir;
>  	struct dirent *e;
> @@ -97,6 +98,14 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list)
>  
>  	while ((e = readdir(dir)) != NULL) {
>  		size_t len;
> +		int i;
> +
> +		for (i = 0;i < extra_keep->nr; i++)

Style: SP after ';' before 'i'.

Show 7 quoted lines

> +			if (!fspathcmp(e->d_name, extra_keep->items[i].string))
> +				break;
> +		if (extra_keep->nr > 0 && i < extra_keep->nr)
> +			continue;
> +
>  		if (!strip_suffix(e->d_name, ".pack", &len))
>  			continue;

Show 16 quoted lines

> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
> index 38247afbec..553d907d34 100755
> --- a/t/t7700-repack.sh
> +++ b/t/t7700-repack.sh
> @@ -196,5 +196,24 @@ test_expect_success 'objects made unreachable by grafts only are kept' '
>  	git cat-file -t $H1
>  '
>  
> +test_expect_success 'repack --keep-pack' '
> +	test_create_repo keep-pack &&
> +	(
> +		cd keep-pack &&
> +		for cmit in one two three four; do
> +			test_commit $cmit &&
> +			git repack -d
> +		done &&

Style: replace "; " before do with LF followed by a few HT.

This 'for' loop would not exit and report error if an early test_commit or "git repack -d" fails, would it?

> +		( cd .git/objects/pack && ls *.pack ) >pack-list &&
> +		test_line_count = 4 pack-list &&
> +		KEEP1=`head -n1 pack-list` &&
> +		KEEP4=`tail -n1 pack-list` &&

Style: $()

> +		git repack -a -d --keep-pack $KEEP1 --keep-pack $KEEP4 &&
> +		ls .git/objects/pack/*.pack >new-counts &&
> +		test_line_count = 3 new-counts &&
> +		git fsck

One important invariant for this new feature is that $KEEP1 and $KEEP4 will both appear in new-counts file, no? Rename new-counts to new-pack-list and inspect the contents, not just line count, perhaps?

> +	)
> +'
> +
>  test_done