Web lists-archives.com

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




On Wed, Mar 7, 2018 at 1:25 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> 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?

Probably the latter.

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

If a question is raised, it's probably not clear.

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

I can't think of one. These .keep files are originally lock files and
ignoring them sounds like a bad idea. Perhaps we could add
--no-keep-pack later to explicit not keep a pack, ignoring .keep file
if present?

>> 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?

Yeah. I guess I'll just unroll the loop.

>> +             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?

OK
-- 
Duy