Re: [PATCH v3 2/3] multi-pack-index: respect repack.packKeptObjects=false

From: Junio C Hamano <gitster@pobox.com>
Date: May 9, 2020, 17:33 UTC
Message-ID: <xmqqlfm1ui6t.fsf@gitster.c.googlers.com>
In-Reply-To: <20200509161159.GA15146@danh.dev>

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

Show 15 quoted lines

> On 2020-05-09 14:24:29+0000, Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:
>> From: Derrick Stolee <dstolee@microsoft.com>
>> 
>> +test_expect_success 'repack respects repack.packKeptObjects=false' '
>> +	test_when_finished rm -f dup/.git/objects/pack/*keep &&
>> +	(
>> +		cd dup &&
>> +		ls .git/objects/pack/*idx >idx-list &&
>
> I think ls(1) is an overkill.
> I think:
>
> 	echo .git/objects/pack/*idx
>
> is more efficient.

When there is no file whose name ends with idx, what happens?

    $ ls *idx && echo OK
    ls: cannot access '*idx': No such file or directory
    $ echo *idx && echo OK
    *idx
    OK

>> +		test_line_count = 5 idx-list &&
>> +		ls .git/objects/pack/*.pack | sed "s/\.pack/.keep/" >keep-list &&
>
> Likewise.

Likewise.

Show 6 quoted lines

>> +		for keep in $(cat keep-list)
>> +		do
>> +			touch $keep || return 1
>
> Is this intended?
> Since touch(1) accepts multiple files as argument.

Good suggestion, but doesn't .keep file record why the pack is kept in real life (i.e. not an empty file)?

Show 22 quoted lines

>> +		done &&
>> +		git multi-pack-index repack --batch-size=0 &&
>> +		ls .git/objects/pack/*idx >idx-list &&
>> +		test_line_count = 5 idx-list &&
>> +		test-tool read-midx .git/objects | grep idx >midx-list &&
>> +		test_line_count = 5 midx-list &&
>> +		THIRD_SMALLEST_SIZE=$(test-tool path-utils file-size .git/objects/pack/*pack | sort -n | head -n 3 | tail -n 1) &&
>
> This line is overly long.
> Should we write test-tool's output to temp file and process it?
>
> And I think either
>
> 	sed -n '3{p;q}'
>
> or:
>
> 	sed -n 3p
>
> is cleaner than
>
> 	head -n 3 | tail -n 1

"sed -n 3p" is the only valid way to write it ;-)