threads / patch / 25057

patch, 2 partsDo not let lstree output recursively when a directory whose name is a prefix of the others is given in the path list.

Subject: [PATCH 1/2] Do not let lstree output recursively when a directory whose name is a prefix of the others is given in the path list.

## tl;dr

5 messages between Sep 9, 2010 and Sep 9, 2010. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

davi.reis@gmail.com· Sep 9, 2010, 18:21 UTC · lore

From: Davi Reis <davi@davi-macbookpro.local>

---
 builtin/ls-tree.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Show changes to builtin/ls-tree.c +2 −0

diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index dc86b0d..fa427e2 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -52,6 +52,8 @@ static int show_recursive(const char *base, int baselen, const char *pathname)
 		speclen = strlen(spec);
 		if (speclen <= len)
 			continue;
+		if (spec[len] != 0 && spec[len] != '/')
+			continue;
 		if (memcmp(pathname, spec, len))
 			continue;
 		return 1;

-- 
1.7.2.2

davi.reis@gmail.com· Sep 9, 2010, 18:21 UTC · re: davi.reis@gmail.com · lore

[PATCH 2/2] Added test from David Glasser.

From: Davi Reis <davi@dhcp-172-19-8-195.mtv.corp.google.com>

---
 builtin/ls-tree.c           |    2 +-
 t/t3100-ls-tree-restrict.sh |    9 +++++++++
 2 files changed, 10 insertions(+), 1 deletions(-)

Show changes to 2 files +10 −1

builtin/ls-tree.c, t/t3100-ls-tree-restrict.sh

diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index fa427e2..37920d0 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -52,7 +52,7 @@ static int show_recursive(const char *base, int baselen, const char *pathname)
 		speclen = strlen(spec);
 		if (speclen <= len)
 			continue;
-		if (spec[len] != 0 && spec[len] != '/')
+		if (spec[len] != '/')
 			continue;
 		if (memcmp(pathname, spec, len))
 			continue;
diff --git a/t/t3100-ls-tree-restrict.sh b/t/t3100-ls-tree-restrict.sh
index eee0d34..9dd74b5 100755
--- a/t/t3100-ls-tree-restrict.sh
+++ b/t/t3100-ls-tree-restrict.sh
@@ -165,4 +165,13 @@ test_expect_success \
 EOF
      test_output'
 
+test_expect_success \
+    'ls-tree with one path a prefix of the other' \
+    'git ls-tree $tree path2/baz path2/bazbo >current &&
+     make_expected <<\EOF &&
+040000 tree X  path2/baz
+120000 blob X  path2/bazbo
+EOF
+     test_output'
+
 test_done

-- 
1.7.2.2

Matthieu Moy· Sep 9, 2010, 21:33 UTC · re: davi.reis@gmail.com · lore

Re: [PATCH 2/2] Added test from David Glasser.

davi.reis@gmail.com writes:

Show 8 quoted lines

> --- a/builtin/ls-tree.c
> +++ b/builtin/ls-tree.c
> @@ -52,7 +52,7 @@ static int show_recursive(const char *base, int baselen, const char *pathname)
>  		speclen = strlen(spec);
>  		if (speclen <= len)
>  			continue;
> -		if (spec[len] != 0 && spec[len] != '/')
> +		if (spec[len] != '/')

This change is not the one advertized for in the title. If you didn't mean it, then

  git send-email --annotate

can be your friend, it gives you a last opportunity to check your patch before it is sent.

If you did mean it, then it should be justified in the commit message.

Show 16 quoted lines

> --- a/t/t3100-ls-tree-restrict.sh
> +++ b/t/t3100-ls-tree-restrict.sh
> @@ -165,4 +165,13 @@ test_expect_success \
>  EOF
>       test_output'
>  
> +test_expect_success \
> +    'ls-tree with one path a prefix of the other' \
> +    'git ls-tree $tree path2/baz path2/bazbo >current &&
> +     make_expected <<\EOF &&
> +040000 tree X  path2/baz
> +120000 blob X  path2/bazbo
> +EOF
> +     test_output'
> +
>  test_done

Adding the test can help people to understand what the first patch is fixing, hence, I'd suggest either squashing both patches, or putting the test patch first (with a test_expect_failure), and having the second turn the test_expect_failure into a test_expect_success (hence, it's obvious reading the patch that it fixes the test).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Davi Reis· Sep 9, 2010, 21:44 UTC · re: Matthieu Moy · lore

Re: [PATCH 2/2] Added test from David Glasser.

I will squash the patches and try again from scratch. Thanks Matthieu.

On Thu, Sep 9, 2010 at 2:33 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 48 quoted lines

> davi.reis@gmail.com writes:
>
>> --- a/builtin/ls-tree.c
>> +++ b/builtin/ls-tree.c
>> @@ -52,7 +52,7 @@ static int show_recursive(const char *base, int baselen, const char *pathname)
>>               speclen = strlen(spec);
>>               if (speclen <= len)
>>                       continue;
>> -             if (spec[len] != 0 && spec[len] != '/')
>> +             if (spec[len] != '/')
>
> This change is not the one advertized for in the title. If you didn't
> mean it, then
>
>  git send-email --annotate
>
> can be your friend, it gives you a last opportunity to check your
> patch before it is sent.
>
> If you did mean it, then it should be justified in the commit message.
>
>> --- a/t/t3100-ls-tree-restrict.sh
>> +++ b/t/t3100-ls-tree-restrict.sh
>> @@ -165,4 +165,13 @@ test_expect_success \
>>  EOF
>>       test_output'
>>
>> +test_expect_success \
>> +    'ls-tree with one path a prefix of the other' \
>> +    'git ls-tree $tree path2/baz path2/bazbo >current &&
>> +     make_expected <<\EOF &&
>> +040000 tree X  path2/baz
>> +120000 blob X  path2/bazbo
>> +EOF
>> +     test_output'
>> +
>>  test_done
>
> Adding the test can help people to understand what the first patch is
> fixing, hence, I'd suggest either squashing both patches, or putting
> the test patch first (with a test_expect_failure), and having the
> second turn the test_expect_failure into a test_expect_success (hence,
> it's obvious reading the patch that it fixes the test).
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
>

-- 
[]s
Davi de Castro Reis

Matthieu Moy· Sep 9, 2010, 21:30 UTC · re: davi.reis@gmail.com · lore

Re: [PATCH 1/2] Do not let lstree output recursively when a directory whose name is a prefix of the others is given in the path list.

davi.reis@gmail.com writes:

> Subject: Re: [PATCH 1/2] Do not let lstree output recursively when a directory whose name is a prefix of the others is give

Don't try to put everything in the subject line. The subject line should be < 80 characters (fit in a standard terminal), and it's best if commands like "git log --oneline" & friends can show it without truncating it, hence, ~50-60 characters is good.

What about

ls-tree: do not recurse when a directory is a prefix of another

<then, a more detailed description, after a blank line>

(I don't know the code of ls-tree, and I don't really understand either the bug or the fix by reading this. A good commit message help people to get convinced that your code is good)

> From: Davi Reis <davi@davi-macbookpro.local>

If you use the same identifier for the commit and when you send the email, you'll get rid of this « From » in the body. I don't think you want this davi-macbookpro.local to appear publicly indeed.

You don't have a Signed-Off-By:, please read Documentation/SubmittingPatches to see how to add it and what it means.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/