Re: [PATCH 8/8] tree_entry_interesting(): support negative pathspec

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 14, 2010, 16:18 UTC
Message-ID: <7vd3sg8fjn.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1283961023-4491-9-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  t/t9999-test.sh |   44 ++++++++++++++++++++++++++++++++++++++++++++

If this is about the basic tree traversal logic, that belongs somewhere in 2xxx series. You seem to be testing only diff-tree and nothing else, so I suspect that you shouldn't even consume a new test number but just adding a new test or two to 4013 should suffice.

> +test_expect_success 'diff' '
> +	cat >expected <<-\EOF &&

What's the point of the dash in "<<-\EOF" if you are not going to indent the here doc?

> +:100644 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 d00491fd7e5bb6fa28c517a0bb32b8b506539d4d M	one/file
> +:100644 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 d00491fd7e5bb6fa28c517a0bb32b8b506539d4d M	one/two/file
> +:100644 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 d00491fd7e5bb6fa28c517a0bb32b8b506539d4d M	one/zoo
> +EOF

Outside t0xxx series please avoid making the test depend on absolute object names like this. Since you are using only two blobs, you could run two hash-objects to learn the hash for "" and "1\n" and say

	cat >expected <<-EOF &&
	:100644 100644 $none $one M	one/file
        ...
	EOF

Better yet, think if you even need the raw output here? Perhaps using output formats like --name-only or --name-status is more appropriate and make the test more readable?

Show 10 quoted lines

> diff --git a/tree-walk.c b/tree-walk.c
> index a2f4a00..fd19681 100644
> --- a/tree-walk.c
> +++ b/tree-walk.c
> @@ -532,7 +532,7 @@ int tree_entry_interesting(const struct name_entry *entry, const char *base, int
>  
>  	pathlen = tree_entry_len(entry->path, entry->sha1);
>  
> -	for (i = 0; i < ps->nr; i++) {
> +	for (i = ps->nr-1; i >= 0; i--) {

No explanation in the commit log message why. Besides, spell binary "-" operator with a SP each on both ends, please.

Also the code seems to use a new term subpathspec without defining nor explaining. Not good.