Re: [PATCH 1/2] pack-objects: Teach --use-bitmap-index codepath to respect --local, --honor-pack-keep and --incremental

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 9, 2016, 16:52 UTC
Message-ID: <xmqqtwetc2bh.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160809112123.GA14273@teco.navytux.spb.ru>

Kirill Smelkov <kirr@nexedi.com> writes:

> Would you please explain why we should not use touch if we do not care
> about timestamps? Simply style?

To help readers.

"touch A" forcess the readers wonder "does the timestamp of A matter, and if so in what way?" and "does any later test care what is _in_ A, and if so in what way?" Both of them is wasting their time when there is no reason why "touch" should have been used.

Show 13 quoted lines

> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index cce95d8..44914ac 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -8,16 +8,15 @@ objpath () {
>  }
>  
>  # show objects present in pack ($1 should be associated *.idx)
> -packobjects () {
> -	git show-index <$1 | cut -d' ' -f2
> +pack_list_objects () {
> +	git show-index <"$1" | cut -d' ' -f2
>  }

pack-list-objects still sounds as if you are packing "list objects", though. If you are listing packed objects (or objects in a pack), list-packed-objects (or list-objects-in-pack) reads clearer and more to the point, at least to me.

Show 9 quoted lines

> -# hasany pattern-file content-file
> +# has_any pattern-file content-file
>  # tests whether content-file has any entry from pattern-file with entries being
>  # whole lines.
> -hasany () {
> -	# NOTE `grep -f` is not portable
> -	git grep --no-index -qFf $1 $2
> +has_any () {
> +	grep -qFf "$1" "$2"

Omitting "-q" would help those who have to debug breakage in this test or the code that this test checks. What test_expect_success outputs is not shown by default, and running the test script with "-v" would show them as a debugging aid.

Thanks.