From: Đoàn Trần Công Danh <congdanhqx@gmail.com>
Date: Sun, 10 May 2020 06:38:44 GMT
Subject: Re: [PATCH v3 2/3] multi-pack-index: respect repack.packKeptObjects=false
Message-ID: <20200510063844.GA14311@danh.dev>
In-Reply-To: <xmqqlfm1ui6t.fsf@gitster.c.googlers.com>

On 2020-05-09 10:33:30-0700, Junio C Hamano <gitster@pobox.com> wrote:
> Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:
> 
> > 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

Yes, but I think the next line is checking for the number of lines.
This is better to fail faster.

(My suggestion was wrong anyway, it should be "printf "%s\\n" *idx)

> >> +		test_line_count = 5 idx-list &&
> >> +		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)?

Yes, in real life, we usually provide a reason in this .keep file.
But, we also allow empty file with git-index-pack --keep
I think simple touch is fine for this test.

Missing piece for my previous command:
if `keep-list` is empty, we may want to fail fast,
touch with empty list will error out (at least in my system).


-- 
Danh