Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> 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.
> +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.
> 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'.
> + 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;
> 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
