From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 14 Sep 2010 16:18:20 GMT
Subject: Re: [PATCH 8/8] tree_entry_interesting(): support negative pathspec
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?

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