[PATCH] attr: fix off-by-one directory component length calculation

[PATCH] attr: fix off-by-one directory component length calculation

[Nguyễn Thái Ngọc Duy]

94bc671 (Add directory pattern matching to attributes - 2012-12-08)
uses find_basename() to calculate the length of directory part in
prepare_attr_stack. This function expects the directory without the
trailing slash (as "origin" field in match_attr struct is without the
trailing slash). find_basename() includes the trailing slash and
confuses push/pop algorithm.

Consider path = "abc/def" and the push down code:

	while (1) {
		len = strlen(attr_stack->origin);
		if (dirlen <= len)
			break;
		cp = memchr(path + len + 1, '/', dirlen - len - 1);
		if (!cp)
			cp = path + dirlen;

dirlen is 4, not 3, without this patch. So when attr_stack->origin is
"abc", it'll miss the exit condition because 4 <= 3 is wrong. It'll
then try to push "abc/" down the attr stack (because "cp" would be
NULL). So we have both "abc" and "abc/" in the stack.

Next time when "abc/ghi" is checked, "abc/" is popped out because of
the off-by-one dirlen, only to be pushed back in again by the above
code. This repeats for all files in the same directory. Which means
at least one failed open syscall per file, or more if .gitattributes
exists.

This is the perf result with 10 runs on git.git:

Test                                     94bc671^          94bc671                   HEAD
----------------------------------------------------------------------------------------------------------
7810.1: grep worktree, cheap regex       0.02(0.01+0.04)   0.05(0.03+0.05) +150.0%   0.02(0.01+0.04) +0.0%
7810.2: grep worktree, expensive regex   0.25(0.94+0.01)   0.26(0.94+0.02) +4.0%     0.25(0.93+0.02) +0.0%
7810.3: grep --cached, cheap regex       0.11(0.10+0.00)   0.12(0.10+0.02) +9.1%     0.10(0.10+0.00) -9.1%
7810.4: grep --cached, expensive regex   0.61(0.60+0.01)   0.62(0.61+0.01) +1.6%     0.61(0.60+0.00) +0.0%

Reported-by: Ross Lagerwall <rosslagerwall@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 This may be an indication that our perf framework is never actively used :-(

 attr.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/attr.c b/attr.c
index 466c93f..bb9a470 100644
--- a/attr.c
+++ b/attr.c
@@ -584,6 +584,13 @@ static void prepare_attr_stack(const char *path)
 	dirlen = find_basename(path) - path;
 
 	/*
+	 * find_basename() includes the trailing slash, but we do
+	 * _not_ want it.
+	 */
+	if (dirlen)
+		dirlen--;
+
+	/*
 	 * At the bottom of the attribute stack is the built-in
 	 * set of attribute definitions, followed by the contents
 	 * of $(prefix)/etc/gitattributes and a file specified by
-- 
1.8.0.rc2.23.g1fb49df

Re: [PATCH] attr: fix off-by-one directory component length calculation

[Junio C Hamano]

Good spotting and a nicely explained patch.  Thanks.

Re: [PATCH] attr: fix off-by-one directory component length calculation

[Jean-Noël AVILA]

Thank you for the explanation.

I did not monitor the system calls when writing that patch. 
Where is the perf framework?

As the mistake is located in the "find_basename" function, I would propose a 
fix directly into it so that the output fits what the other functions expect.

Something in the line of:

diff --git a/attr.c b/attr.c
index d6d7190..b6e72f3 100644
--- a/attr.c
+++ b/attr.c
@@ -572,7 +572,7 @@ static const char *find_basename(const char *path)
                if (*cp == '/' && cp[1])
                        last_slash = cp;
        }
-       return last_slash ? last_slash + 1 : path;
+       return last_slash ? last_slash : path;
 }
 
 static void prepare_attr_stack(const char *path)
@@ -770,6 +770,10 @@ static void collect_all_attrs(const char *path)
                check_all_attr[i].value = ATTR__UNKNOWN;
 
        basename = find_basename(path);
+       /* the slash is included in the basename
+          so that it can be matched by a directory pattern */
+       if (basename != path)
+               basename++;
        pathlen = strlen(path);
        rem = attr_nr;
        for (stk = attr_stack; 0 < rem && stk; stk = stk->prev)

Re: [PATCH] attr: fix off-by-one directory component length calculation

[Junio C Hamano]

"Jean-Noël AVILA" <avila.jn@gmail.com> writes:

> Thank you for the explanation.
>
> I did not monitor the system calls when writing that patch. 
> Where is the perf framework?
>
> As the mistake is located in the "find_basename" function, I would propose a 
> fix directly into it so that the output fits what the other functions expect.

Isn't that a crazy semantics for the function, though?  I would
expect find_basename("/a/path/to/file") to return "file", not
"/file".

>
> Something in the line of:
>
> diff --git a/attr.c b/attr.c
> index d6d7190..b6e72f3 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -572,7 +572,7 @@ static const char *find_basename(const char *path)
>                 if (*cp == '/' && cp[1])
>                         last_slash = cp;
>         }
> -       return last_slash ? last_slash + 1 : path;
> +       return last_slash ? last_slash : path;
>  }
>  
>  static void prepare_attr_stack(const char *path)
> @@ -770,6 +770,10 @@ static void collect_all_attrs(const char *path)
>                 check_all_attr[i].value = ATTR__UNKNOWN;
>  
>         basename = find_basename(path);
> +       /* the slash is included in the basename
> +          so that it can be matched by a directory pattern */
> +       if (basename != path)
> +               basename++;
>         pathlen = strlen(path);
>         rem = attr_nr;
>         for (stk = attr_stack; 0 < rem && stk; stk = stk->prev)

Re: [PATCH] attr: fix off-by-one directory component length calculation

[Jean-Noël AVILA]

Le mardi 15 janvier 2013 20:29:16, Junio C Hamano a écrit :
> "Jean-Noël AVILA" <avila.jn@gmail.com> writes:
> > Thank you for the explanation.
> > 
> > I did not monitor the system calls when writing that patch.
> > Where is the perf framework?
> > 
> > As the mistake is located in the "find_basename" function, I would
> > propose a fix directly into it so that the output fits what the other
> > functions expect.
> 
> Isn't that a crazy semantics for the function, though?  I would
> expect find_basename("/a/path/to/file") to return "file", not
> "/file".
> 

Yes, it is. I was wrong on the meaning of basename. 

Btw, the test 10 to t9902 is failing on my Debian testing. Is it a known 
issue?

Re: [PATCH] attr: fix off-by-one directory component length calculation

[Junio C Hamano]

"Jean-Noël AVILA" <avila.jn@gmail.com> writes:

> Btw, the test 10 to t9902 is failing on my Debian testing. Is it a known 
> issue?

Which branch?

If you mean "'master' with the patch in this discussion applied", I
didn't even have a chance to start today's integration cycle, so I
don't know (it is not known to me).

Re: [PATCH] attr: fix off-by-one directory component length calculation

[Jeff King]

On Tue, Jan 15, 2013 at 12:49:05PM -0800, Junio C Hamano wrote:

> "Jean-Noël AVILA" <avila.jn@gmail.com> writes:
> 
> > Btw, the test 10 to t9902 is failing on my Debian testing. Is it a known 
> > issue?
> 
> Which branch?

t9902.10 is overly sensitive to extra git commands in your PATH, as well
as cruft in your build dir (especially if you have been building 'pu',
which has git-check-ignore). Try "make clean && make test".

-Peff

Re: [PATCH] attr: fix off-by-one directory component length calculation

[Duy Nguyen]

On Wed, Jan 16, 2013 at 2:14 AM, Jean-Noël AVILA <avila.jn@gmail.com> wrote:
> I did not monitor the system calls when writing that patch.
> Where is the perf framework?

It's in t/perf. I think you can do:

./run HEAD .

to run and compare performance of HEAD and working directory (assume
you haven't commit yet). Check out the README file.
-- 
Duy

Re: [PATCH] attr: fix off-by-one directory component length calculation

[Duy Nguyen]

On Wed, Jan 16, 2013 at 2:29 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Jean-Noël AVILA" <avila.jn@gmail.com> writes:
>
>> Thank you for the explanation.
>>
>> I did not monitor the system calls when writing that patch.
>> Where is the perf framework?
>>
>> As the mistake is located in the "find_basename" function, I would propose a
>> fix directly into it so that the output fits what the other functions expect.
>
> Isn't that a crazy semantics for the function, though?  I would
> expect find_basename("/a/path/to/file") to return "file", not

Actually I'd like to remove that function. The function is called twice:

 - collect_all_attrs
   + prepare_attr_stack
     * find_basename
   + find_basename

which could be reordered to

 - collect_all_attrs
   + find_basename
   + prepare_attr_stack (modified to take dirlen from caller)

and because that'll be the only place find_basename is used, we could
just inline the code there.
-- 
Duy

Re: [PATCH] attr: fix off-by-one directory component length calculation

[Duy Nguyen]

On Wed, Jan 16, 2013 at 08:08:03AM +0700, Duy Nguyen wrote:
> Actually I'd like to remove that function.

This is what I had in mind:

-- 8< --
Subject: [PATCH] attr: avoid calling find_basename() twice per path

find_basename() is only used inside collect_all_attrs(), called once
in prepare_attr_stack, then again after prepare_attr_stack()
returns. Both calls return exact same value. Reorder the code to do it
once.

While at it, make use of "pathlen" to stop searching early if
possible.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 attr.c | 46 +++++++++++++++++++---------------------------
 1 file changed, 19 insertions(+), 27 deletions(-)

diff --git a/attr.c b/attr.c
index cfc6748..04cb9a0 100644
--- a/attr.c
+++ b/attr.c
@@ -564,32 +564,12 @@ static void bootstrap_attr_stack(void)
 	attr_stack = elem;
 }
 
-static const char *find_basename(const char *path)
-{
-	const char *cp, *last_slash = NULL;
-
-	for (cp = path; *cp; cp++) {
-		if (*cp == '/' && cp[1])
-			last_slash = cp;
-	}
-	return last_slash ? last_slash + 1 : path;
-}
-
-static void prepare_attr_stack(const char *path)
+static void prepare_attr_stack(const char *path, int dirlen)
 {
 	struct attr_stack *elem, *info;
-	int dirlen, len;
+	int len;
 	const char *cp;
 
-	dirlen = find_basename(path) - path;
-
-	/*
-	 * find_basename() includes the trailing slash, but we do
-	 * _not_ want it.
-	 */
-	if (dirlen)
-		dirlen--;
-
 	/*
 	 * At the bottom of the attribute stack is the built-in
 	 * set of attribute definitions, followed by the contents
@@ -769,15 +749,27 @@ static int macroexpand_one(int attr_nr, int rem)
 static void collect_all_attrs(const char *path)
 {
 	struct attr_stack *stk;
-	int i, pathlen, rem;
-	const char *basename;
+	int i, pathlen, rem, dirlen = 0;
+	const char *basename = path, *cp;
 
-	prepare_attr_stack(path);
+	pathlen = strlen(path);
+
+	/*
+	 * This loop is similar to strrchr(path, '/') except that the
+	 * trailing slash is skipped.
+	 */
+	for (cp = path + pathlen - 2; cp >= path; cp--) {
+		if (*cp == '/') {
+			basename = cp + 1;
+			dirlen = cp - path;
+			break;
+		}
+	}
+
+	prepare_attr_stack(path, dirlen);
 	for (i = 0; i < attr_nr; i++)
 		check_all_attr[i].value = ATTR__UNKNOWN;
 
-	basename = find_basename(path);
-	pathlen = strlen(path);
 	rem = attr_nr;
 	for (stk = attr_stack; 0 < rem && stk; stk = stk->prev)
 		rem = fill(path, pathlen, basename, stk, rem);
-- 
1.8.0.rc3.18.g0d9b108

-- 8< --

Re: [PATCH] attr: fix off-by-one directory component length calculation

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Wed, Jan 16, 2013 at 08:08:03AM +0700, Duy Nguyen wrote:
>> Actually I'd like to remove that function.
>
> This is what I had in mind:

I think the replacement logic to find the basename is moderately
inferiour to the original.  For one thing (this may be somewhat
subjective), it is less readable now.  Also the original only
scanned the string from the beginning once (instead of letting
strlen() to scan once and go back).

The new code structure to inline the basename finding part and to
pass the dirlen down the callchain may make sense, though.

>> -- 8< --
> Subject: [PATCH] attr: avoid calling find_basename() twice per path
>
> find_basename() is only used inside collect_all_attrs(), called once
> in prepare_attr_stack, then again after prepare_attr_stack()
> returns. Both calls return exact same value. Reorder the code to do it
> once.
>
> While at it, make use of "pathlen" to stop searching early if
> possible.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  attr.c | 46 +++++++++++++++++++---------------------------
>  1 file changed, 19 insertions(+), 27 deletions(-)
>
> diff --git a/attr.c b/attr.c
> index cfc6748..04cb9a0 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -564,32 +564,12 @@ static void bootstrap_attr_stack(void)
>  	attr_stack = elem;
>  }
>  
> -static const char *find_basename(const char *path)
> -{
> -	const char *cp, *last_slash = NULL;
> -
> -	for (cp = path; *cp; cp++) {
> -		if (*cp == '/' && cp[1])
> -			last_slash = cp;
> -	}
> -	return last_slash ? last_slash + 1 : path;
> -}
> -
> -static void prepare_attr_stack(const char *path)
> +static void prepare_attr_stack(const char *path, int dirlen)
>  {
>  	struct attr_stack *elem, *info;
> -	int dirlen, len;
> +	int len;
>  	const char *cp;
>  
> -	dirlen = find_basename(path) - path;
> -
> -	/*
> -	 * find_basename() includes the trailing slash, but we do
> -	 * _not_ want it.
> -	 */
> -	if (dirlen)
> -		dirlen--;
> -
>  	/*
>  	 * At the bottom of the attribute stack is the built-in
>  	 * set of attribute definitions, followed by the contents
> @@ -769,15 +749,27 @@ static int macroexpand_one(int attr_nr, int rem)
>  static void collect_all_attrs(const char *path)
>  {
>  	struct attr_stack *stk;
> -	int i, pathlen, rem;
> -	const char *basename;
> +	int i, pathlen, rem, dirlen = 0;
> +	const char *basename = path, *cp;
>  
> -	prepare_attr_stack(path);
> +	pathlen = strlen(path);
> +
> +	/*
> +	 * This loop is similar to strrchr(path, '/') except that the
> +	 * trailing slash is skipped.
> +	 */
> +	for (cp = path + pathlen - 2; cp >= path; cp--) {
> +		if (*cp == '/') {
> +			basename = cp + 1;
> +			dirlen = cp - path;
> +			break;
> +		}
> +	}
> +
> +	prepare_attr_stack(path, dirlen);
>  	for (i = 0; i < attr_nr; i++)
>  		check_all_attr[i].value = ATTR__UNKNOWN;
>  
> -	basename = find_basename(path);
> -	pathlen = strlen(path);
>  	rem = attr_nr;
>  	for (stk = attr_stack; 0 < rem && stk; stk = stk->prev)
>  		rem = fill(path, pathlen, basename, stk, rem);

Re: [PATCH] attr: fix off-by-one directory component length calculation

[Duy Nguyen]

On Wed, Jan 16, 2013 at 9:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Wed, Jan 16, 2013 at 08:08:03AM +0700, Duy Nguyen wrote:
>>> Actually I'd like to remove that function.
>>
>> This is what I had in mind:
>
> I think the replacement logic to find the basename is moderately
> inferiour to the original.  For one thing (this may be somewhat
> subjective), it is less readable now.

Yeah, maybe it's micro optimization.

> Also the original only
> scanned the string from the beginning once (instead of letting
> strlen() to scan once and go back).

But we do need to strlen() anyway in collect_all_attrs(). So we scan
the string 3 times (strlen + 2 * find_basename) in the original. Now
we do it twice
-- 
Duy

Re: [PATCH] attr: fix off-by-one directory component length calculation

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Wed, Jan 16, 2013 at 9:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Duy Nguyen <pclouds@gmail.com> writes:
>>
>>> On Wed, Jan 16, 2013 at 08:08:03AM +0700, Duy Nguyen wrote:
>>>> Actually I'd like to remove that function.
>>>
>>> This is what I had in mind:
>>
>> I think the replacement logic to find the basename is moderately
>> inferiour to the original.  For one thing (this may be somewhat
>> subjective), it is less readable now.
>
> Yeah, maybe it's micro optimization.

Your change is micro unoptimization (and making the result less
readable).  I wouldn't worry too much about micro-optimizing an
existing piece of code, but making an efficient code into a worse
one without a good reason is a different story.

>> Also the original only
>> scanned the string from the beginning once (instead of letting
>> strlen() to scan once and go back).
>
> But we do need to strlen() anyway in collect_all_attrs().

That is exactly my point, isn't it?

The loop to find the basename has to run to the end of the string at
least once, as it cannot not stop at the last slash---it goes from
front to back and it won't know which one is the last slash until it
sees the end of the string.  After the loop exits, you know the
length of the string without running a separate strlen() to assign
to "pathlen".

> So we scan
> the string 3 times (strlen + 2 * find_basename) in the original. Now
> we do it twice.

I already said that overall restructure of the code may be a good
idea to reduce the calls to the function.  I was only comparing the
implementations of the loop that finds the basename, so I do not
understand what you mean by that "2 *" in that comparison.  It does
not make sense to me.

Re: [PATCH] attr: fix off-by-one directory component length calculation

[Duy Nguyen]

On Tue, Jan 15, 2013 at 09:35:21PM -0800, Junio C Hamano wrote:
> >> Also the original only
> >> scanned the string from the beginning once (instead of letting
> >> strlen() to scan once and go back).
> >
> > But we do need to strlen() anyway in collect_all_attrs().
> 
> That is exactly my point, isn't it?

OK I get your point now. Something like this?

-- 8< --
Subject: [PATCH] attr: avoid calling find_basename() twice per path

find_basename() is only used inside collect_all_attrs(), called once
in prepare_attr_stack, then again after prepare_attr_stack()
returns. Both calls return exact same value. Reorder the code to do
the same task once. Also avoid strlen() because we knows the length
after finding basename.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 attr.c | 45 ++++++++++++++++++---------------------------
 1 file changed, 18 insertions(+), 27 deletions(-)

diff --git a/attr.c b/attr.c
index cfc6748..880f862 100644
--- a/attr.c
+++ b/attr.c
@@ -564,32 +564,12 @@ static void bootstrap_attr_stack(void)
 	attr_stack = elem;
 }
 
-static const char *find_basename(const char *path)
-{
-	const char *cp, *last_slash = NULL;
-
-	for (cp = path; *cp; cp++) {
-		if (*cp == '/' && cp[1])
-			last_slash = cp;
-	}
-	return last_slash ? last_slash + 1 : path;
-}
-
-static void prepare_attr_stack(const char *path)
+static void prepare_attr_stack(const char *path, int dirlen)
 {
 	struct attr_stack *elem, *info;
-	int dirlen, len;
+	int len;
 	const char *cp;
 
-	dirlen = find_basename(path) - path;
-
-	/*
-	 * find_basename() includes the trailing slash, but we do
-	 * _not_ want it.
-	 */
-	if (dirlen)
-		dirlen--;
-
 	/*
 	 * At the bottom of the attribute stack is the built-in
 	 * set of attribute definitions, followed by the contents
@@ -769,15 +749,26 @@ static int macroexpand_one(int attr_nr, int rem)
 static void collect_all_attrs(const char *path)
 {
 	struct attr_stack *stk;
-	int i, pathlen, rem;
-	const char *basename;
+	int i, pathlen, rem, dirlen;
+	const char *basename, *cp, *last_slash = NULL;
+
+	for (cp = path; *cp; cp++) {
+		if (*cp == '/' && cp[1])
+			last_slash = cp;
+	}
+	pathlen = cp - path;
+	if (last_slash) {
+		basename = last_slash + 1;
+		dirlen = last_slash - path;
+	} else {
+		basename = path;
+		dirlen = 0;
+	}
 
-	prepare_attr_stack(path);
+	prepare_attr_stack(path, dirlen);
 	for (i = 0; i < attr_nr; i++)
 		check_all_attr[i].value = ATTR__UNKNOWN;
 
-	basename = find_basename(path);
-	pathlen = strlen(path);
 	rem = attr_nr;
 	for (stk = attr_stack; 0 < rem && stk; stk = stk->prev)
 		rem = fill(path, pathlen, basename, stk, rem);
-- 
1.8.0.rc3.18.g0d9b108

-- 8< --

Re: t9902 fails (Was: [PATCH] attr: fix off-by-one directory component length calculation)

[Torsten Bögershausen]

On 01/16/2013 12:24 AM, Jeff King wrote:
> On Tue, Jan 15, 2013 at 12:49:05PM -0800, Junio C Hamano wrote:
>
>> "Jean-Noël AVILA"<avila.jn@gmail.com>  writes:
>>
>>> Btw, the test 10 to t9902 is failing on my Debian testing. Is it a known
>>> issue?
>>
>> Which branch?
>
> t9902.10 is overly sensitive to extra git commands in your PATH, as well
> as cruft in your build dir (especially if you have been building 'pu',
> which has git-check-ignore). Try "make clean&&  make test".
>
> -Peff
This may help, or it may not.

If there are other binaries like
"git-check-email" or "git-check-ignore" in the PATH
.....

When you switch to a branch generating a file like
git-check-ignore then "make clean" will know about it
and will remove it.
If you switch to master, then "make clean" will not remove it.

What does "git status" say?

We had a discussion about this some weeks ago, but never concluded.

How about this:
http://comments.gmane.org/gmane.comp.version-control.git/211907

Re: [PATCH] attr: fix off-by-one directory component length calculation

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> OK I get your point now. Something like this?
>
> -- 8< --
> Subject: [PATCH] attr: avoid calling find_basename() twice per path
>
> find_basename() is only used inside collect_all_attrs(), called once
> in prepare_attr_stack, then again after prepare_attr_stack()
> returns. Both calls return exact same value. Reorder the code to do
> the same task once. Also avoid strlen() because we knows the length
> after finding basename.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Yeah, I think this is a nice code reduction, readability improvement
and micro optimization rolled into one.

>  attr.c | 45 ++++++++++++++++++---------------------------
>  1 file changed, 18 insertions(+), 27 deletions(-)
>
> diff --git a/attr.c b/attr.c
> index cfc6748..880f862 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -564,32 +564,12 @@ static void bootstrap_attr_stack(void)
>  	attr_stack = elem;
>  }
>  
> -static const char *find_basename(const char *path)
> -{
> -	const char *cp, *last_slash = NULL;
> -
> -	for (cp = path; *cp; cp++) {
> -		if (*cp == '/' && cp[1])
> -			last_slash = cp;
> -	}
> -	return last_slash ? last_slash + 1 : path;
> -}
> -
> -static void prepare_attr_stack(const char *path)
> +static void prepare_attr_stack(const char *path, int dirlen)
>  {
>  	struct attr_stack *elem, *info;
> -	int dirlen, len;
> +	int len;
>  	const char *cp;
>  
> -	dirlen = find_basename(path) - path;
> -
> -	/*
> -	 * find_basename() includes the trailing slash, but we do
> -	 * _not_ want it.
> -	 */
> -	if (dirlen)
> -		dirlen--;
> -
>  	/*
>  	 * At the bottom of the attribute stack is the built-in
>  	 * set of attribute definitions, followed by the contents
> @@ -769,15 +749,26 @@ static int macroexpand_one(int attr_nr, int rem)
>  static void collect_all_attrs(const char *path)
>  {
>  	struct attr_stack *stk;
> -	int i, pathlen, rem;
> -	const char *basename;
> +	int i, pathlen, rem, dirlen;
> +	const char *basename, *cp, *last_slash = NULL;
> +
> +	for (cp = path; *cp; cp++) {
> +		if (*cp == '/' && cp[1])
> +			last_slash = cp;
> +	}
> +	pathlen = cp - path;
> +	if (last_slash) {
> +		basename = last_slash + 1;
> +		dirlen = last_slash - path;
> +	} else {
> +		basename = path;
> +		dirlen = 0;
> +	}
>  
> -	prepare_attr_stack(path);
> +	prepare_attr_stack(path, dirlen);
>  	for (i = 0; i < attr_nr; i++)
>  		check_all_attr[i].value = ATTR__UNKNOWN;
>  
> -	basename = find_basename(path);
> -	pathlen = strlen(path);
>  	rem = attr_nr;
>  	for (stk = attr_stack; 0 < rem && stk; stk = stk->prev)
>  		rem = fill(path, pathlen, basename, stk, rem);

Re: t9902 fails (Was: [PATCH] attr: fix off-by-one directory component length calculation)

[Jean-Noël AVILA]

Le mercredi 16 janvier 2013 07:15:51, Torsten Bögershausen a écrit :
> On 01/16/2013 12:24 AM, Jeff King wrote:
> > On Tue, Jan 15, 2013 at 12:49:05PM -0800, Junio C Hamano wrote:
> >> "Jean-Noël AVILA"<avila.jn@gmail.com>  writes:
> >>> Btw, the test 10 to t9902 is failing on my Debian testing. Is it a
> >>> known issue?
> >> 
> >> Which branch?
> > 
> > t9902.10 is overly sensitive to extra git commands in your PATH, as well
> > as cruft in your build dir (especially if you have been building 'pu',
> > which has git-check-ignore). Try "make clean&&  make test".
> > 
> > -Peff
> 
> This may help, or it may not.
> 
> If there are other binaries like
> "git-check-email" or "git-check-ignore" in the PATH
> .....
> 
> When you switch to a branch generating a file like
> git-check-ignore then "make clean" will know about it
> and will remove it.
> If you switch to master, then "make clean" will not remove it.
> 
> What does "git status" say?
> 
> We had a discussion about this some weeks ago, but never concluded.
> 
> How about this:
> http://comments.gmane.org/gmane.comp.version-control.git/211907

OK. I have installed practically everything related to git from the package 
manager and there is a git-checkout-branches utility available.

That result defeats the purpose of the test. This needs a tighter environment 
to work whatever the configuration of the user may be.

Re: t9902 fails (Was: [PATCH] attr: fix off-by-one directory component length calculation)

[Jonathan Nieder]

Jean-Noël AVILA wrote:

> OK. I have installed practically everything related to git from the package 
> manager and there is a git-checkout-branches utility available.
>
> That result defeats the purpose of the test. This needs a tighter environment 
> to work whatever the configuration of the user may be.

Presumably 'git checkout-branches' is from git-stuff.

Here's a patch to make the tested command a little less likely to
conflict with commands from the user's $PATH.  I'm not thrilled with
it because the contents of $PATH are still not tightly controlled, and
this does nothing to avoid problems due to existence of, for example,
a "git cherry-pick-branches" command.

Thoughts?  Maybe it would be enough to check that the intended get
commands are present in the completion list and other git commands are
not, ignoring binaries that might live elsewhere on the $PATH?

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t9902-completion.sh | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3cd53f8..06dcfb2 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -192,19 +192,19 @@ test_expect_success 'general options' '
 '
 
 test_expect_success 'general options plus command' '
-	test_completion "git --version check" "checkout " &&
-	test_completion "git --paginate check" "checkout " &&
-	test_completion "git --git-dir=foo check" "checkout " &&
-	test_completion "git --bare check" "checkout " &&
-	test_completion "git --help des" "describe " &&
-	test_completion "git --exec-path=foo check" "checkout " &&
-	test_completion "git --html-path check" "checkout " &&
-	test_completion "git --no-pager check" "checkout " &&
-	test_completion "git --work-tree=foo check" "checkout " &&
-	test_completion "git --namespace=foo check" "checkout " &&
-	test_completion "git --paginate check" "checkout " &&
-	test_completion "git --info-path check" "checkout " &&
-	test_completion "git --no-replace-objects check" "checkout "
+	test_completion "git --version cherry-p" "cherry-pick " &&
+	test_completion "git --paginate cherry-p" "cherry-pick " &&
+	test_completion "git --git-dir=foo cherry-p" "cherry-pick " &&
+	test_completion "git --bare cherry-p" "cherry-pick " &&
+	test_completion "git --help cherry-p" "cherry-pick " &&
+	test_completion "git --exec-path=foo cherry-p" "cherry-pick " &&
+	test_completion "git --html-path cherry-p" "cherry-pick " &&
+	test_completion "git --no-pager cherry-p" "cherry-pick " &&
+	test_completion "git --work-tree=foo cherry-p" "cherry-pick " &&
+	test_completion "git --namespace=foo cherry-p" "cherry-pick " &&
+	test_completion "git --paginate cherry-p" "cherry-pick " &&
+	test_completion "git --info-path cherry-p" "cherry-pick " &&
+	test_completion "git --no-replace-objects cherry-p" "cherry-pick "
 '
 
 test_expect_success 'setup for ref completion' '
-- 
1.8.1

Re: t9902 fails

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Thoughts?  Maybe it would be enough to check that the intended get
> commands are present in the completion list and other git commands are
> not, ignoring binaries that might live elsewhere on the $PATH?

Yeah, it would be a robust alternative approach to verify that (1)
output is a superset of what we expect, and (2) they all share the
string we fed to the machinery as the common prefix, I would think.

Re: t9902 fails (Was: [PATCH] attr: fix off-by-one directory component length calculation)

[Torsten Bögershausen]

On 18.01.13 01:04, Jonathan Nieder wrote:
> Jean-Noël AVILA wrote:
>
>> OK. I have installed practically everything related to git from the package 
>> manager and there is a git-checkout-branches utility available.
>>
>> That result defeats the purpose of the test. This needs a tighter environment 
>> to work whatever the configuration of the user may be.
> Presumably 'git checkout-branches' is from git-stuff.
>
> Here's a patch to make the tested command a little less likely to
> conflict with commands from the user's $PATH.  I'm not thrilled with
> it because the contents of $PATH are still not tightly controlled, and
> this does nothing to avoid problems due to existence of, for example,
> a "git cherry-pick-branches" command.
>
> Thoughts?  Maybe it would be enough to check that the intended get
> commands are present in the completion list and other git commands are
> not, ignoring binaries that might live elsewhere on the $PATH?
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  t/t9902-completion.sh | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 3cd53f8..06dcfb2 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -192,19 +192,19 @@ test_expect_success 'general options' '
>  '
>  
>  test_expect_success 'general options plus command' '
> -	test_completion "git --version check" "checkout " &&
> -	test_completion "git --paginate check" "checkout " &&
> -	test_completion "git --git-dir=foo check" "checkout " &&
> -	test_completion "git --bare check" "checkout " &&
> -	test_completion "git --help des" "describe " &&
> -	test_completion "git --exec-path=foo check" "checkout " &&
> -	test_completion "git --html-path check" "checkout " &&
> -	test_completion "git --no-pager check" "checkout " &&
> -	test_completion "git --work-tree=foo check" "checkout " &&
> -	test_completion "git --namespace=foo check" "checkout " &&
> -	test_completion "git --paginate check" "checkout " &&
> -	test_completion "git --info-path check" "checkout " &&
> -	test_completion "git --no-replace-objects check" "checkout "
> +	test_completion "git --version cherry-p" "cherry-pick " &&
> +	test_completion "git --paginate cherry-p" "cherry-pick " &&
> +	test_completion "git --git-dir=foo cherry-p" "cherry-pick " &&
> +	test_completion "git --bare cherry-p" "cherry-pick " &&
> +	test_completion "git --help cherry-p" "cherry-pick " &&
> +	test_completion "git --exec-path=foo cherry-p" "cherry-pick " &&
> +	test_completion "git --html-path cherry-p" "cherry-pick " &&
> +	test_completion "git --no-pager cherry-p" "cherry-pick " &&
> +	test_completion "git --work-tree=foo cherry-p" "cherry-pick " &&
> +	test_completion "git --namespace=foo cherry-p" "cherry-pick " &&
> +	test_completion "git --paginate cherry-p" "cherry-pick " &&
> +	test_completion "git --info-path cherry-p" "cherry-pick " &&
> +	test_completion "git --no-replace-objects cherry-p" "cherry-pick "
>  '
>  
>  test_expect_success 'setup for ref completion' '
That looks good to me, thanks.

Re: t9902 fails

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Here's a patch to make the tested command a little less likely to
> conflict with commands from the user's $PATH.  I'm not thrilled with
> it because the contents of $PATH are still not tightly controlled, and
> this does nothing to avoid problems due to existence of, for example,
> a "git cherry-pick-branches" command.
>
> Thoughts?

How about doing something like this and set that variable in the
test instead?  If STD_ONLY is not set, you will get everything, but
when STD_ONLY is set, we will stop reading from "help -a" when it
starts listing additional stuff.

 contrib/completion/git-completion.bash | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index a4c48e1..415a078 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -534,7 +534,8 @@ __git_complete_strategy ()
 __git_list_all_commands ()
 {
 	local i IFS=" "$'\n'
-	for i in $(git help -a|egrep '^  [a-zA-Z0-9]')
+	for i in $(LANG=C LC_ALL=C git help -a |
+		   sed -n ${GIT_HELP_STD_ONLY+-e /^git.*elsewhere/q} -e '/^  [a-zA-Z0-9]/p')
 	do
 		case $i in
 		*--*)             : helper pattern;;

Re: t9902 fails

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> How about doing something like this and set that variable in the
> test instead?  If STD_ONLY is not set, you will get everything, but
> when STD_ONLY is set, we will stop reading from "help -a" when it
> starts listing additional stuff.
>
>  contrib/completion/git-completion.bash | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index a4c48e1..415a078 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -534,7 +534,8 @@ __git_complete_strategy ()
>  __git_list_all_commands ()
>  {
>  	local i IFS=" "$'\n'
> -	for i in $(git help -a|egrep '^  [a-zA-Z0-9]')
> +	for i in $(LANG=C LC_ALL=C git help -a |
> +		   sed -n ${GIT_HELP_STD_ONLY+-e /^git.*elsewhere/q} -e '/^  [a-zA-Z0-9]/p')
>  	do
>  		case $i in
>  		*--*)             : helper pattern;;

Alternatively, we could do this and replace everything inside $()
with "git help --standard", but that requires the completion script
update to go in sync with the core update, which is a downside.

 builtin/help.c | 21 +++++++++++++++++----
 help.c         |  3 +++
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index bd86253..e6b9b5f 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -36,11 +36,16 @@ enum help_format {
 
 static const char *html_path;
 
-static int show_all = 0;
+#define HELP_ALL 1
+#define HELP_STANDARD 2
+static int show_what;
 static unsigned int colopts;
 static enum help_format help_format = HELP_FORMAT_NONE;
 static struct option builtin_help_options[] = {
-	OPT_BOOLEAN('a', "all", &show_all, N_("print all available commands")),
+	OPT_SET_INT('a', "all", &show_what, N_("print all available commands"),
+		    HELP_ALL),
+	OPT_SET_INT(0, "standard", &show_what, N_("list subcommands that comes with git"),
+		    HELP_STANDARD),
 	OPT_SET_INT('m', "man", &help_format, N_("show man page"), HELP_FORMAT_MAN),
 	OPT_SET_INT('w', "web", &help_format, N_("show manual in web browser"),
 			HELP_FORMAT_WEB),
@@ -436,19 +441,27 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 	int nongit;
 	const char *alias;
 	enum help_format parsed_help_format;
-	load_command_list("git-", &main_cmds, &other_cmds);
 
 	argc = parse_options(argc, argv, prefix, builtin_help_options,
 			builtin_help_usage, 0);
 	parsed_help_format = help_format;
 
-	if (show_all) {
+	load_command_list("git-", &main_cmds,
+			  show_what == HELP_STANDARD ? NULL : &other_cmds);
+
+	if (show_what == HELP_ALL) {
 		git_config(git_help_config, NULL);
 		printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
 		list_commands(colopts, &main_cmds, &other_cmds);
 		printf("%s\n", _(git_more_info_string));
 		return 0;
 	}
+	if (show_what == HELP_STANDARD) {
+		int i;
+		for (i = 0; i < main_cmds.cnt; i++)
+			printf("%s\n", main_cmds.names[i]->name);
+		return 0;
+	}
 
 	if (!argv[0]) {
 		printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
diff --git a/help.c b/help.c
index 2a42ec6..3e6b04c 100644
--- a/help.c
+++ b/help.c
@@ -182,6 +182,9 @@ void load_command_list(const char *prefix,
 		uniq(main_cmds);
 	}
 
+	if (!other_cmds)
+		return;
+
 	if (env_path) {
 		char *paths, *path, *colon;
 		path = paths = xstrdup(env_path);

Re: t9902 fails

[Jean-Noël AVILA]

Le vendredi 18 janvier 2013 21:15:23, Junio C Hamano a écrit :
> Junio C Hamano <gitster@pobox.com> writes:
> > How about doing something like this and set that variable in the
> > test instead?  If STD_ONLY is not set, you will get everything, but
> > when STD_ONLY is set, we will stop reading from "help -a" when it
> > starts listing additional stuff.

I tried both of your propositions but none made test 10 of t9902 pass.

Am I supposed to run "make install" before running the test?

Re: t9902 fails

[Junio C Hamano]

"Jean-Noël AVILA" <avila.jn@gmail.com> writes:

> Le vendredi 18 janvier 2013 21:15:23, Junio C Hamano a écrit :
>> Junio C Hamano <gitster@pobox.com> writes:
>> > How about doing something like this and set that variable in the
>> > test instead?  If STD_ONLY is not set, you will get everything, but
>> > when STD_ONLY is set, we will stop reading from "help -a" when it
>> > starts listing additional stuff.
>
> I tried both of your propositions but none made test 10 of t9902 pass.

Do you have a leftover git-check-ignore or something from a previous
build that is *not* known to "git" you just built?

Neither will help in such a case.  The test pretty much runs with
GIT_EXEC_PATH set to the build area, and we do want to be able to
test what we have in the build area before we decide to install
them, so that is nothing new.

Re: t9902 fails

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "Jean-Noël AVILA" <avila.jn@gmail.com> writes:
>
>> Le vendredi 18 janvier 2013 21:15:23, Junio C Hamano a écrit :
>>> Junio C Hamano <gitster@pobox.com> writes:
>>> > How about doing something like this and set that variable in the
>>> > test instead?  If STD_ONLY is not set, you will get everything, but
>>> > when STD_ONLY is set, we will stop reading from "help -a" when it
>>> > starts listing additional stuff.
>>
>> I tried both of your propositions but none made test 10 of t9902 pass.
>
> Do you have a leftover git-check-ignore or something from a previous
> build that is *not* known to "git" you just built?

"... in the build directory" was missing from the sentence.  Sorry
about noise.

> Neither will help in such a case.  The test pretty much runs with
> GIT_EXEC_PATH set to the build area, and we do want to be able to
> test what we have in the build area before we decide to install
> them, so that is nothing new.

Re: t9902 fails

[Torsten Bögershausen]

On 18.01.13 23:23, Jean-Noël AVILA wrote:
> Le vendredi 18 janvier 2013 21:15:23, Junio C Hamano a écrit :
>> Junio C Hamano <gitster@pobox.com> writes:
>>> How about doing something like this and set that variable in the
>>> test instead?  If STD_ONLY is not set, you will get everything, but
>>> when STD_ONLY is set, we will stop reading from "help -a" when it
>>> starts listing additional stuff.
> 
> I tried both of your propositions but none made test 10 of t9902 pass.
> 
> Am I supposed to run "make install" before running the test?

No. The test suite could (and should) be run before you make install.
So a typical sequence could be:
Run test suite, and if that passes, install the binaries on my system.
This could look like this on the command line:
make test && sudo make install

Jean-Noël,
would it be possible to run
"git status"
and share the result with us?

And did you try Jonathans patch?

/Torsten

Re* t9902 fails

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> would it be possible to run
> "git status"
> and share the result with us?
>
> And did you try Jonathans patch?

It may hide the immediate issue, but I am afraid Jonathan's patch
does not fix anything fundamental.  If you do this:

	git checkout next
        make
        git checkout master ;# without 'make clean'
        make && cd t && sh ./t9902-*.sh

then the completion machinery will see the leftover git-check-ignore
at the top of the working tree (which is the $GIT_EXEC_PATH) and the
test that expects "check" to expand only to "checkout" will fail,
because 'master' does not have exclusion definition for check-ignore,
even though it knows check-attr, check-ref-format and checkout-index
are to be excluded as "plumbing".

So if you come up with a brilliant idea to add "git cherry-pack"
command and did this:

	git checkout -b tb/cherry-pack
        edit Makefile builtin/cherry-pack.c builtin.h git.c ...
        git add builtin/cherry-pack.c
        make test
        git commit -a -m "cherry-pack: new command"
        git checkout master ;# without 'make clean'
        make && cd t && sh ./t9902-*.sh

the test will break exactly the same way.

If we really wanted to exclude random build artifacts that the
current checkout did not build, you have to do one of these things:

 (1) at the beginning of t9902, "rm -fr" a temporary location,
     install the built product with a custom DESTDIR set to that
     temporary location that we now know is empty, and point
     GIT_EXEC_PATH to the libexec/git-core directory in that
     temporary location, so that "git help -a" run in the completion
     script will find _only_ the executable we would install; or

 (2) instead of being inclusive, collecting all executable in
     GIT_EXEC_PATH that happens to be named "git-", add a mode to
     "git help" that lists those that we know to be standard
     commands that the users may want to complete from the command
     line.

An outline to do (2) would look like this patch, but I didn't check
other consumers of command-list.txt, so this may be breaking them in
unplanned ways.

 builtin/help.c                         | 35 ++++++++++---------------
 command-list.txt                       |  4 +--
 contrib/completion/git-completion.bash | 14 +---------
 generate-cmdlist.sh                    | 13 +++++++++-
 help.c                                 | 47 ++++++++++++++++++++++++++++++++--
 help.h                                 |  1 +
 6 files changed, 75 insertions(+), 39 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index bd86253..32e7d64 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -6,7 +6,6 @@
 #include "cache.h"
 #include "builtin.h"
 #include "exec_cmd.h"
-#include "common-cmds.h"
 #include "parse-options.h"
 #include "run-command.h"
 #include "column.h"
@@ -36,11 +35,16 @@ enum help_format {
 
 static const char *html_path;
 
-static int show_all = 0;
+#define HELP_SHOW_ALL 1
+#define HELP_SHOW_STANDARD 2
+static int show_what;
 static unsigned int colopts;
 static enum help_format help_format = HELP_FORMAT_NONE;
 static struct option builtin_help_options[] = {
-	OPT_BOOLEAN('a', "all", &show_all, N_("print all available commands")),
+	OPT_SET_INT('a', "all", &show_what, N_("print all available commands"),
+		HELP_SHOW_ALL),
+	OPT_SET_INT(0, "standard", &show_what, N_("print all available commands"),
+		HELP_SHOW_STANDARD),
 	OPT_SET_INT('m', "man", &help_format, N_("show man page"), HELP_FORMAT_MAN),
 	OPT_SET_INT('w', "web", &help_format, N_("show manual in web browser"),
 			HELP_FORMAT_WEB),
@@ -287,23 +291,6 @@ static int git_help_config(const char *var, const char *value, void *cb)
 
 static struct cmdnames main_cmds, other_cmds;
 
-void list_common_cmds_help(void)
-{
-	int i, longest = 0;
-
-	for (i = 0; i < ARRAY_SIZE(common_cmds); i++) {
-		if (longest < strlen(common_cmds[i].name))
-			longest = strlen(common_cmds[i].name);
-	}
-
-	puts(_("The most commonly used git commands are:"));
-	for (i = 0; i < ARRAY_SIZE(common_cmds); i++) {
-		printf("   %s   ", common_cmds[i].name);
-		mput_char(' ', longest - strlen(common_cmds[i].name));
-		puts(_(common_cmds[i].help));
-	}
-}
-
 static int is_git_command(const char *s)
 {
 	return is_in_cmdlist(&main_cmds, s) ||
@@ -442,12 +429,18 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 			builtin_help_usage, 0);
 	parsed_help_format = help_format;
 
-	if (show_all) {
+	if (show_what == HELP_SHOW_ALL) {
 		git_config(git_help_config, NULL);
 		printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
 		list_commands(colopts, &main_cmds, &other_cmds);
 		printf("%s\n", _(git_more_info_string));
 		return 0;
+	} else if (show_what == HELP_SHOW_STANDARD) {
+		int i;
+		limit_to_standard(&main_cmds);
+		for (i = 0; i < main_cmds.cnt; i++)
+			printf("%s\n", main_cmds.names[i]->name);
+		return 0;
 	}
 
 	if (!argv[0]) {
diff --git a/command-list.txt b/command-list.txt
index 7e8cfec..94ce8ec 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -116,8 +116,8 @@ git-show                                mainporcelain common
 git-show-branch                         ancillaryinterrogators
 git-show-index                          plumbinginterrogators
 git-show-ref                            plumbinginterrogators
-git-sh-i18n                             purehelpers
-git-sh-setup                            purehelpers
+git-sh-i18n                             purehelpers nocomplete
+git-sh-setup                            purehelpers nocomplete
 git-stash                               mainporcelain
 git-status                              mainporcelain common
 git-stripspace                          purehelpers
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index a4c48e1..46f22af 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -531,23 +531,11 @@ __git_complete_strategy ()
 	return 1
 }
 
-__git_list_all_commands ()
-{
-	local i IFS=" "$'\n'
-	for i in $(git help -a|egrep '^  [a-zA-Z0-9]')
-	do
-		case $i in
-		*--*)             : helper pattern;;
-		*) echo $i;;
-		esac
-	done
-}
-
 __git_all_commands=
 __git_compute_all_commands ()
 {
 	test -n "$__git_all_commands" ||
-	__git_all_commands=$(__git_list_all_commands)
+	__git_all_commands=$(git help --standard)
 }
 
 __git_list_porcelain_commands ()
diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index 9a4c9b9..7800af3 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh
@@ -9,7 +9,7 @@ struct cmdname_help {
 static struct cmdname_help common_cmds[] = {"
 
 sed -n -e 's/^git-\([^ 	]*\)[ 	].* common.*/\1/p' command-list.txt |
-sort |
+LC_ALL=C LANG=C sort |
 while read cmd
 do
      sed -n '
@@ -20,4 +20,15 @@ do
 	    p
      }' "Documentation/git-$cmd.txt"
 done
+echo "};
+
+static const char *standard_cmd[] = {"
+
+LC_ALL=C LANG=C sort command-list.txt |
+sed -n -e '
+	/^git-[^ 	]*[ 	].* deprecated.*/d
+	/^git-[^ 	]*[ 	].* nocomplete.*/d
+	s/^git-\([^ 	]*\)[ 	].*/  "\1",/p
+'
+
 echo "};"
diff --git a/help.c b/help.c
index 2a42ec6..2ad10db 100644
--- a/help.c
+++ b/help.c
@@ -182,7 +182,7 @@ void load_command_list(const char *prefix,
 		uniq(main_cmds);
 	}
 
-	if (env_path) {
+	if (env_path && other_cmds) {
 		char *paths, *path, *colon;
 		path = paths = xstrdup(env_path);
 		while (1) {
@@ -201,7 +201,33 @@ void load_command_list(const char *prefix,
 		      sizeof(*other_cmds->names), cmdname_compare);
 		uniq(other_cmds);
 	}
-	exclude_cmds(other_cmds, main_cmds);
+
+	if (other_cmds)
+		exclude_cmds(other_cmds, main_cmds);
+}
+
+void limit_to_standard(struct cmdnames *cmds)
+{
+	int src = 0, dst = 0, ref = 0;
+
+	while (src < cmds->cnt && ref < ARRAY_SIZE(standard_cmd)) {
+		int cmp = strcmp(cmds->names[src]->name, standard_cmd[ref]);
+		if (cmp < 0) {
+			src++; /* not a standard command */
+		} else if (!cmp) {
+			if (dst != src) {
+				free(cmds->names[dst]);
+				cmds->names[dst] = cmds->names[src];
+			}
+			ref++;
+			dst++;
+		} else {
+			ref++; /* uninstalled standard command */
+		}
+	}
+	for (src = dst; src < cmds->cnt; src++)
+		free(cmds->names[src]);
+	cmds->cnt = dst;
 }
 
 void list_commands(unsigned int colopts,
@@ -223,6 +249,23 @@ void list_commands(unsigned int colopts,
 	}
 }
 
+void list_common_cmds_help(void)
+{
+	int i, longest = 0;
+
+	for (i = 0; i < ARRAY_SIZE(common_cmds); i++) {
+		if (longest < strlen(common_cmds[i].name))
+			longest = strlen(common_cmds[i].name);
+	}
+
+	puts(_("The most commonly used git commands are:"));
+	for (i = 0; i < ARRAY_SIZE(common_cmds); i++) {
+		printf("   %s   ", common_cmds[i].name);
+		mput_char(' ', longest - strlen(common_cmds[i].name));
+		puts(_(common_cmds[i].help));
+	}
+}
+
 int is_in_cmdlist(struct cmdnames *c, const char *s)
 {
 	int i;
diff --git a/help.h b/help.h
index 0ae5a12..ce0d2a5 100644
--- a/help.h
+++ b/help.h
@@ -21,6 +21,7 @@ extern const char *help_unknown_cmd(const char *cmd);
 extern void load_command_list(const char *prefix,
 			      struct cmdnames *main_cmds,
 			      struct cmdnames *other_cmds);
+extern void limit_to_standard(struct cmdnames *);
 extern void add_cmdname(struct cmdnames *cmds, const char *name, int len);
 /* Here we require that excludes is a sorted list. */
 extern void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes);

[PATCH 1/2] help: include <common-cmds.h> only in one file

[Junio C Hamano]

This header not only declares but also defines the contents of the
array that holds the list of command names and help text.  Do not
include it in multiple places to waste text space.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This is a real "cleanup" patch.

 builtin/help.c | 18 ------------------
 help.c         | 17 +++++++++++++++++
 2 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index bd86253..6067a61 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -6,7 +6,6 @@
 #include "cache.h"
 #include "builtin.h"
 #include "exec_cmd.h"
-#include "common-cmds.h"
 #include "parse-options.h"
 #include "run-command.h"
 #include "column.h"
@@ -287,23 +286,6 @@ static int git_help_config(const char *var, const char *value, void *cb)
 
 static struct cmdnames main_cmds, other_cmds;
 
-void list_common_cmds_help(void)
-{
-	int i, longest = 0;
-
-	for (i = 0; i < ARRAY_SIZE(common_cmds); i++) {
-		if (longest < strlen(common_cmds[i].name))
-			longest = strlen(common_cmds[i].name);
-	}
-
-	puts(_("The most commonly used git commands are:"));
-	for (i = 0; i < ARRAY_SIZE(common_cmds); i++) {
-		printf("   %s   ", common_cmds[i].name);
-		mput_char(' ', longest - strlen(common_cmds[i].name));
-		puts(_(common_cmds[i].help));
-	}
-}
-
 static int is_git_command(const char *s)
 {
 	return is_in_cmdlist(&main_cmds, s) ||
diff --git a/help.c b/help.c
index 2a42ec6..1dfa0b0 100644
--- a/help.c
+++ b/help.c
@@ -223,6 +223,23 @@ void list_commands(unsigned int colopts,
 	}
 }
 
+void list_common_cmds_help(void)
+{
+	int i, longest = 0;
+
+	for (i = 0; i < ARRAY_SIZE(common_cmds); i++) {
+		if (longest < strlen(common_cmds[i].name))
+			longest = strlen(common_cmds[i].name);
+	}
+
+	puts(_("The most commonly used git commands are:"));
+	for (i = 0; i < ARRAY_SIZE(common_cmds); i++) {
+		printf("   %s   ", common_cmds[i].name);
+		mput_char(' ', longest - strlen(common_cmds[i].name));
+		puts(_(common_cmds[i].help));
+	}
+}
+
 int is_in_cmdlist(struct cmdnames *c, const char *s)
 {
 	int i;
-- 
1.8.1.1.454.g48d39c0

[PATCH 2/2] help --standard: list standard commands

[Junio C Hamano]

If you run "make" on a branch that adds "git check-ignore", checkout
an older branch that did not know about the command without "make clean",
and the run t9902 test, the completion script fails to exclude the
"check-ignore" command from candidates to complete "check".

This is because the completion script asks "git help -a" to show
every executable that begins with "git-" in the GIT_EXEC_PATH, and
because we run tests with GIT_EXEC_PATH set to the top of the
working tree, that has the executable we just built, in order to
test these before installing.  Leftover "git check-ignore" that we
did not build for the current version gets in the way.

One way to solve this is to restrict the completion to only the
commands we know about.

Note that this will make the completion useless in real life for
some people, as they do want to get their custom commands on their
$PATH to be included in the completion.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This is not a real patch, for the reasons stated.

 builtin/help.c                         | 17 ++++++++++++++---
 command-list.txt                       |  4 ++--
 contrib/completion/git-completion.bash | 14 +-------------
 generate-cmdlist.sh                    | 13 ++++++++++++-
 help.c                                 | 30 ++++++++++++++++++++++++++++--
 help.h                                 |  1 +
 6 files changed, 58 insertions(+), 21 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index 6067a61..32e7d64 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -35,11 +35,16 @@ enum help_format {
 
 static const char *html_path;
 
-static int show_all = 0;
+#define HELP_SHOW_ALL 1
+#define HELP_SHOW_STANDARD 2
+static int show_what;
 static unsigned int colopts;
 static enum help_format help_format = HELP_FORMAT_NONE;
 static struct option builtin_help_options[] = {
-	OPT_BOOLEAN('a', "all", &show_all, N_("print all available commands")),
+	OPT_SET_INT('a', "all", &show_what, N_("print all available commands"),
+		HELP_SHOW_ALL),
+	OPT_SET_INT(0, "standard", &show_what, N_("print all available commands"),
+		HELP_SHOW_STANDARD),
 	OPT_SET_INT('m', "man", &help_format, N_("show man page"), HELP_FORMAT_MAN),
 	OPT_SET_INT('w', "web", &help_format, N_("show manual in web browser"),
 			HELP_FORMAT_WEB),
@@ -424,12 +429,18 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 			builtin_help_usage, 0);
 	parsed_help_format = help_format;
 
-	if (show_all) {
+	if (show_what == HELP_SHOW_ALL) {
 		git_config(git_help_config, NULL);
 		printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
 		list_commands(colopts, &main_cmds, &other_cmds);
 		printf("%s\n", _(git_more_info_string));
 		return 0;
+	} else if (show_what == HELP_SHOW_STANDARD) {
+		int i;
+		limit_to_standard(&main_cmds);
+		for (i = 0; i < main_cmds.cnt; i++)
+			printf("%s\n", main_cmds.names[i]->name);
+		return 0;
 	}
 
 	if (!argv[0]) {
diff --git a/command-list.txt b/command-list.txt
index 7e8cfec..94ce8ec 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -116,8 +116,8 @@ git-show                                mainporcelain common
 git-show-branch                         ancillaryinterrogators
 git-show-index                          plumbinginterrogators
 git-show-ref                            plumbinginterrogators
-git-sh-i18n                             purehelpers
-git-sh-setup                            purehelpers
+git-sh-i18n                             purehelpers nocomplete
+git-sh-setup                            purehelpers nocomplete
 git-stash                               mainporcelain
 git-status                              mainporcelain common
 git-stripspace                          purehelpers
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index a4c48e1..46f22af 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -531,23 +531,11 @@ __git_complete_strategy ()
 	return 1
 }
 
-__git_list_all_commands ()
-{
-	local i IFS=" "$'\n'
-	for i in $(git help -a|egrep '^  [a-zA-Z0-9]')
-	do
-		case $i in
-		*--*)             : helper pattern;;
-		*) echo $i;;
-		esac
-	done
-}
-
 __git_all_commands=
 __git_compute_all_commands ()
 {
 	test -n "$__git_all_commands" ||
-	__git_all_commands=$(__git_list_all_commands)
+	__git_all_commands=$(git help --standard)
 }
 
 __git_list_porcelain_commands ()
diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index 9a4c9b9..7800af3 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh
@@ -9,7 +9,7 @@ struct cmdname_help {
 static struct cmdname_help common_cmds[] = {"
 
 sed -n -e 's/^git-\([^ 	]*\)[ 	].* common.*/\1/p' command-list.txt |
-sort |
+LC_ALL=C LANG=C sort |
 while read cmd
 do
      sed -n '
@@ -20,4 +20,15 @@ do
 	    p
      }' "Documentation/git-$cmd.txt"
 done
+echo "};
+
+static const char *standard_cmd[] = {"
+
+LC_ALL=C LANG=C sort command-list.txt |
+sed -n -e '
+	/^git-[^ 	]*[ 	].* deprecated.*/d
+	/^git-[^ 	]*[ 	].* nocomplete.*/d
+	s/^git-\([^ 	]*\)[ 	].*/  "\1",/p
+'
+
 echo "};"
diff --git a/help.c b/help.c
index 1dfa0b0..2ad10db 100644
--- a/help.c
+++ b/help.c
@@ -182,7 +182,7 @@ void load_command_list(const char *prefix,
 		uniq(main_cmds);
 	}
 
-	if (env_path) {
+	if (env_path && other_cmds) {
 		char *paths, *path, *colon;
 		path = paths = xstrdup(env_path);
 		while (1) {
@@ -201,7 +201,33 @@ void load_command_list(const char *prefix,
 		      sizeof(*other_cmds->names), cmdname_compare);
 		uniq(other_cmds);
 	}
-	exclude_cmds(other_cmds, main_cmds);
+
+	if (other_cmds)
+		exclude_cmds(other_cmds, main_cmds);
+}
+
+void limit_to_standard(struct cmdnames *cmds)
+{
+	int src = 0, dst = 0, ref = 0;
+
+	while (src < cmds->cnt && ref < ARRAY_SIZE(standard_cmd)) {
+		int cmp = strcmp(cmds->names[src]->name, standard_cmd[ref]);
+		if (cmp < 0) {
+			src++; /* not a standard command */
+		} else if (!cmp) {
+			if (dst != src) {
+				free(cmds->names[dst]);
+				cmds->names[dst] = cmds->names[src];
+			}
+			ref++;
+			dst++;
+		} else {
+			ref++; /* uninstalled standard command */
+		}
+	}
+	for (src = dst; src < cmds->cnt; src++)
+		free(cmds->names[src]);
+	cmds->cnt = dst;
 }
 
 void list_commands(unsigned int colopts,
diff --git a/help.h b/help.h
index 0ae5a12..ce0d2a5 100644
--- a/help.h
+++ b/help.h
@@ -21,6 +21,7 @@ extern const char *help_unknown_cmd(const char *cmd);
 extern void load_command_list(const char *prefix,
 			      struct cmdnames *main_cmds,
 			      struct cmdnames *other_cmds);
+extern void limit_to_standard(struct cmdnames *);
 extern void add_cmdname(struct cmdnames *cmds, const char *name, int len);
 /* Here we require that excludes is a sorted list. */
 extern void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes);
-- 
1.8.1.1.454.g48d39c0

Re: t9902 fails

[Jean-Noël AVILA]

Le samedi 19 janvier 2013 06:38:54, Torsten Bögershausen a écrit :
> On 18.01.13 23:23, Jean-Noël AVILA wrote:
> > Le vendredi 18 janvier 2013 21:15:23, Junio C Hamano a écrit :
> >> Junio C Hamano <gitster@pobox.com> writes:
> >>> How about doing something like this and set that variable in the
> >>> test instead?  If STD_ONLY is not set, you will get everything, but
> >>> when STD_ONLY is set, we will stop reading from "help -a" when it
> >>> starts listing additional stuff.
> > 
> > I tried both of your propositions but none made test 10 of t9902 pass.
> > 
> > Am I supposed to run "make install" before running the test?
> 
> No. The test suite could (and should) be run before you make install.
> So a typical sequence could be:
> Run test suite, and if that passes, install the binaries on my system.
> This could look like this on the command line:
> make test && sudo make install
> 
> Jean-Noël,
> would it be possible to run
> "git status"
> and share the result with us?
> 
> And did you try Jonathans patch?
> 
> /Torsten


Hi all,

My workdir is clean. 


jnavila@cayenne git (master)]$ make
    GEN perl/PM.stamp
    SUBDIR gitweb
    SUBDIR ../
make[2]: « GIT-VERSION-FILE » est à jour.
    GEN git-instaweb
    SUBDIR git-gui
    SUBDIR gitk-git
make[1]: Rien à faire pour « all ».
    SUBDIR perl
    SUBDIR git_remote_helpers
    SUBDIR templates
[jnavila@cayenne git (master)]$ git branch -vv
  attr_pattern   3cb6a4c Add directory pattern matching to attributes
  fix_test_t9902 02f55e6 Merge git://bogomips.org/git-svn
  maint          611fa18 Add directory pattern matching to attributes
* master         02f55e6 [origin/master] Merge git://bogomips.org/git-svn
  next           82c5000 [origin/next: ahead 157, behind 550] Merge branch 
'jc/doc-diff-blobs' into next
  pu             25f269c [origin/pu: ahead 68, behind 137] Merge branch 
'mp/diff-algo-config' into pu
  todo           70e0e3e [origin/todo: behind 1] What's cooking (2013/01 #06)
[jnavila@cayenne git (master)]$ git status
# On branch master
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#       gitk-git/gitk-wish
nothing added to commit but untracked files present (use "git add" to track)
[jnavila@cayenne git (master)]$ ls -l | grep git-check
-rwxr-xr-x 108 jnavila jnavila 7677476 janv. 19 10:30 git-check-attr
-rwxr-xr-x 108 jnavila jnavila 7677476 janv. 19 10:30 git-checkout
-rwxr-xr-x 108 jnavila jnavila 7677476 janv. 19 10:30 git-checkout-index
-rwxr-xr-x 108 jnavila jnavila 7677476 janv. 19 10:30 git-check-ref-format


If I move git-checkout-branches out of /usr/bin, the test passes. So somehow 
GIT_EXEC_PATH is not what is expected.

Re: [PATCH 2/2] help --standard: list standard commands

[Jean-Noël AVILA]

Le samedi 19 janvier 2013 09:02:13, Junio C Hamano a écrit :
> If you run "make" on a branch that adds "git check-ignore", checkout
> an older branch that did not know about the command without "make clean",
> and the run t9902 test, the completion script fails to exclude the
> "check-ignore" command from candidates to complete "check".
> 
> This is because the completion script asks "git help -a" to show
> every executable that begins with "git-" in the GIT_EXEC_PATH, and
> because we run tests with GIT_EXEC_PATH set to the top of the
> working tree, that has the executable we just built, in order to
> test these before installing.  Leftover "git check-ignore" that we
> did not build for the current version gets in the way.
> 
> One way to solve this is to restrict the completion to only the
> commands we know about.
> 
> Note that this will make the completion useless in real life for
> some people, as they do want to get their custom commands on their
> $PATH to be included in the completion.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 
>  * This is not a real patch, for the reasons stated.
> 
>  builtin/help.c                         | 17 ++++++++++++++---
>  command-list.txt                       |  4 ++--
>  contrib/completion/git-completion.bash | 14 +-------------
>  generate-cmdlist.sh                    | 13 ++++++++++++-
>  help.c                                 | 30 ++++++++++++++++++++++++++++--
>  help.h                                 |  1 +
>  6 files changed, 58 insertions(+), 21 deletions(-)
> 
> diff --git a/builtin/help.c b/builtin/help.c
> index 6067a61..32e7d64 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -35,11 +35,16 @@ enum help_format {
> 
>  static const char *html_path;
> 
> -static int show_all = 0;
> +#define HELP_SHOW_ALL 1
> +#define HELP_SHOW_STANDARD 2
> +static int show_what;
>  static unsigned int colopts;
>  static enum help_format help_format = HELP_FORMAT_NONE;
>  static struct option builtin_help_options[] = {
> -	OPT_BOOLEAN('a', "all", &show_all, N_("print all available commands")),
> +	OPT_SET_INT('a', "all", &show_what, N_("print all available commands"),
> +		HELP_SHOW_ALL),
> +	OPT_SET_INT(0, "standard", &show_what, N_("print all available
> commands"), +		HELP_SHOW_STANDARD),
>  	OPT_SET_INT('m', "man", &help_format, N_("show man page"),
> HELP_FORMAT_MAN), OPT_SET_INT('w', "web", &help_format, N_("show manual in
> web browser"), HELP_FORMAT_WEB),
> @@ -424,12 +429,18 @@ int cmd_help(int argc, const char **argv, const char
> *prefix) builtin_help_usage, 0);
>  	parsed_help_format = help_format;
> 
> -	if (show_all) {
> +	if (show_what == HELP_SHOW_ALL) {
>  		git_config(git_help_config, NULL);
>  		printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
>  		list_commands(colopts, &main_cmds, &other_cmds);
>  		printf("%s\n", _(git_more_info_string));
>  		return 0;
> +	} else if (show_what == HELP_SHOW_STANDARD) {
> +		int i;
> +		limit_to_standard(&main_cmds);
> +		for (i = 0; i < main_cmds.cnt; i++)
> +			printf("%s\n", main_cmds.names[i]->name);
> +		return 0;
>  	}
> 
>  	if (!argv[0]) {
> diff --git a/command-list.txt b/command-list.txt
> index 7e8cfec..94ce8ec 100644
> --- a/command-list.txt
> +++ b/command-list.txt
> @@ -116,8 +116,8 @@ git-show                                mainporcelain
> common git-show-branch                         ancillaryinterrogators
>  git-show-index                          plumbinginterrogators
>  git-show-ref                            plumbinginterrogators
> -git-sh-i18n                             purehelpers
> -git-sh-setup                            purehelpers
> +git-sh-i18n                             purehelpers nocomplete
> +git-sh-setup                            purehelpers nocomplete
>  git-stash                               mainporcelain
>  git-status                              mainporcelain common
>  git-stripspace                          purehelpers
> diff --git a/contrib/completion/git-completion.bash
> b/contrib/completion/git-completion.bash index a4c48e1..46f22af 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -531,23 +531,11 @@ __git_complete_strategy ()
>  	return 1
>  }
> 
> -__git_list_all_commands ()
> -{
> -	local i IFS=" "$'\n'
> -	for i in $(git help -a|egrep '^  [a-zA-Z0-9]')
> -	do
> -		case $i in
> -		*--*)             : helper pattern;;
> -		*) echo $i;;
> -		esac
> -	done
> -}
> -
>  __git_all_commands=
>  __git_compute_all_commands ()
>  {
>  	test -n "$__git_all_commands" ||
> -	__git_all_commands=$(__git_list_all_commands)
> +	__git_all_commands=$(git help --standard)
>  }
> 
>  __git_list_porcelain_commands ()
> diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
> index 9a4c9b9..7800af3 100755
> --- a/generate-cmdlist.sh
> +++ b/generate-cmdlist.sh
> @@ -9,7 +9,7 @@ struct cmdname_help {
>  static struct cmdname_help common_cmds[] = {"
> 
>  sed -n -e 's/^git-\([^ 	]*\)[ 	].* common.*/\1/p' command-list.txt |
> -sort |
> +LC_ALL=C LANG=C sort |
>  while read cmd
>  do
>       sed -n '
> @@ -20,4 +20,15 @@ do
>  	    p
>       }' "Documentation/git-$cmd.txt"
>  done
> +echo "};
> +
> +static const char *standard_cmd[] = {"
> +
> +LC_ALL=C LANG=C sort command-list.txt |
> +sed -n -e '
> +	/^git-[^ 	]*[ 	].* deprecated.*/d
> +	/^git-[^ 	]*[ 	].* nocomplete.*/d
> +	s/^git-\([^ 	]*\)[ 	].*/  "\1",/p
> +'
> +
>  echo "};"
> diff --git a/help.c b/help.c
> index 1dfa0b0..2ad10db 100644
> --- a/help.c
> +++ b/help.c
> @@ -182,7 +182,7 @@ void load_command_list(const char *prefix,
>  		uniq(main_cmds);
>  	}
> 
> -	if (env_path) {
> +	if (env_path && other_cmds) {
>  		char *paths, *path, *colon;
>  		path = paths = xstrdup(env_path);
>  		while (1) {
> @@ -201,7 +201,33 @@ void load_command_list(const char *prefix,
>  		      sizeof(*other_cmds->names), cmdname_compare);
>  		uniq(other_cmds);
>  	}
> -	exclude_cmds(other_cmds, main_cmds);
> +
> +	if (other_cmds)
> +		exclude_cmds(other_cmds, main_cmds);
> +}
> +
> +void limit_to_standard(struct cmdnames *cmds)
> +{
> +	int src = 0, dst = 0, ref = 0;
> +
> +	while (src < cmds->cnt && ref < ARRAY_SIZE(standard_cmd)) {
> +		int cmp = strcmp(cmds->names[src]->name, standard_cmd[ref]);
> +		if (cmp < 0) {
> +			src++; /* not a standard command */
> +		} else if (!cmp) {
> +			if (dst != src) {
> +				free(cmds->names[dst]);
> +				cmds->names[dst] = cmds->names[src];
> +			}
> +			ref++;
> +			dst++;
> +		} else {
> +			ref++; /* uninstalled standard command */
> +		}
> +	}
> +	for (src = dst; src < cmds->cnt; src++)
> +		free(cmds->names[src]);
> +	cmds->cnt = dst;
>  }
> 
>  void list_commands(unsigned int colopts,
> diff --git a/help.h b/help.h
> index 0ae5a12..ce0d2a5 100644
> --- a/help.h
> +++ b/help.h
> @@ -21,6 +21,7 @@ extern const char *help_unknown_cmd(const char *cmd);
>  extern void load_command_list(const char *prefix,
>  			      struct cmdnames *main_cmds,
>  			      struct cmdnames *other_cmds);
> +extern void limit_to_standard(struct cmdnames *);
>  extern void add_cmdname(struct cmdnames *cmds, const char *name, int len);
>  /* Here we require that excludes is a sorted list. */
>  extern void exclude_cmds(struct cmdnames *cmds, struct cmdnames
> *excludes);

With these two patches, the test passes.

Re: Re* t9902 fails

[Jean-Noël AVILA]

Le samedi 19 janvier 2013 08:52:25, Junio C Hamano a écrit :
>  (2) instead of being inclusive, collecting all executable in
>      GIT_EXEC_PATH that happens to be named "git-", add a mode to
>      "git help" that lists those that we know to be standard
>      commands that the users may want to complete from the command
>      line.

Am I wrong when I say that "git help -a" already provides the difference 
between core git commands and other commands available through path?

If we use this, then we can instruct git-completion that we are in test mode 
and that it should not provide additional completions.

---
 contrib/completion/git-completion.bash | 13 +++++++++++--
 t/t9902-completion.sh                  |  2 +-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-
completion.bash
index 14dd5e7..dc0ea5b 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -23,6 +23,8 @@
 #    3) Consider changing your PS1 to also show the current branch,
 #       see git-prompt.sh for details.
 
+__testing_git=$1
+
 case "$COMP_WORDBREAKS" in
 *:*) : great ;;
 *)   COMP_WORDBREAKS="$COMP_WORDBREAKS:"
@@ -533,8 +535,15 @@ __git_complete_strategy ()
 
 __git_list_all_commands ()
 {
-	local i IFS=" "$'\n'
-	for i in $(git help -a|egrep '^  [a-zA-Z0-9]')
+	local i cmdlist IFS=" "$'\n'
+
+	if [ "x$__testing_git" != "xTEST" ]; then
+		cmdlist=$(git help -a|egrep '^  [a-zA-Z0-9]')
+	else
+		cmdlist=$(git help -a| egrep -m 1 -B1000 PATH | egrep '^  [a-zA-Z0-9]')
+	fi
+
+	for i in $cmdlist
 	do
 		case $i in
 		*--*)             : helper pattern;;
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3cd53f8..51463b2 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -13,7 +13,7 @@ complete ()
 	return 0
 }
 
-. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
+. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" TEST
 
 # We don't need this function to actually join words or do anything special.
 # Also, it's cleaner to avoid touching bash's internal completion variables.
-- 
1.8.1.1.271.g02f55e6