threads / patch / 32639

patchattr: fix off-by-one directory component length calculation

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

## tl;dr

32 messages between Jan 15, 2013 and Jan 19, 2013. Diffs are folded; open one to read it.

replies: 31people: 6as markdown or json

Nguyễn Thái Ngọc Duy· Jan 15, 2013, 13:35 UTC · lore

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(+)
Show changes to attr.c +7 −0
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
Jean-Noël AVILA· Jan 15, 2013, 19:14 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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:
Show changes to attr.c +5 −1
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)
Junio C Hamano· Jan 15, 2013, 19:29 UTC · re: Jean-Noël AVILA · lore

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

"Jean-Noël AVILA" <avila.jn@gmail.com> writes:
Show 7 quoted lines
> 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".

Show 27 quoted lines
>
> 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)
Jean-Noël AVILA· Jan 15, 2013, 19:53 UTC · re: Junio C Hamano · lore

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

Le mardi 15 janvier 2013 20:29:16, Junio C Hamano a écrit :
Show 14 quoted lines
> "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?

Junio C Hamano· Jan 15, 2013, 20:49 UTC · re: Jean-Noël AVILA · lore

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

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

Jeff King· Jan 15, 2013, 23:24 UTC · re: Junio C Hamano · lore

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

On Tue, Jan 15, 2013 at 12:49:05PM -0800, Junio C Hamano wrote:
Show 6 quoted lines
> "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
Torsten Bögershausen· Jan 16, 2013, 06:15 UTC · re: Jeff King · lore

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

On 01/16/2013 12:24 AM, Jeff King wrote:
Show 14 quoted lines
> 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

Jean-Noël AVILA· Jan 17, 2013, 22:47 UTC · re: Torsten Bögershausen · lore

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

Le mercredi 16 janvier 2013 07:15:51, Torsten Bögershausen a écrit :
Show 31 quoted lines
> 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.

Jonathan Nieder· Jan 18, 2013, 00:04 UTC · re: Jean-Noël AVILA · lore

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

Jean-Noël AVILA wrote:
Show 5 quoted lines
> 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(-)
Show changes to t/t9902-completion.sh +13 −13
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
Junio C Hamano· Jan 18, 2013, 00:12 UTC · re: Jonathan Nieder · lore

Re: t9902 fails

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.

Torsten Bögershausen· Jan 18, 2013, 05:20 UTC · re: Jonathan Nieder · lore

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

On 18.01.13 01:04, Jonathan Nieder wrote:
Show 61 quoted lines
> 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.
Junio C Hamano· Jan 18, 2013, 16:49 UTC · re: Jonathan Nieder · lore

Re: t9902 fails

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 7 quoted lines
> 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(-)
Show changes to contrib/completion/git-completion.bash +2 −1
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;;
Junio C Hamano· Jan 18, 2013, 20:15 UTC · re: Junio C Hamano · lore

Re: t9902 fails

Junio C Hamano <gitster@pobox.com> writes:
Show 22 quoted lines
> 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(-)
Show changes to 2 files +20 −4

builtin/help.c, help.c

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);
Jean-Noël AVILA· Jan 18, 2013, 22:23 UTC · re: Junio C Hamano · lore

Re: t9902 fails

Le vendredi 18 janvier 2013 21:15:23, Junio C Hamano a écrit :
Show 5 quoted lines
> 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?
Junio C Hamano· Jan 18, 2013, 22:37 UTC · re: Jean-Noël AVILA · lore

Re: t9902 fails

"Jean-Noël AVILA" <avila.jn@gmail.com> writes:
Show 8 quoted lines
> 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.

Junio C Hamano· Jan 18, 2013, 22:57 UTC · re: Junio C Hamano · lore

Re: t9902 fails

Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
> "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.
Torsten Bögershausen· Jan 19, 2013, 05:38 UTC · re: Jean-Noël AVILA · lore

Re: t9902 fails

On 18.01.13 23:23, Jean-Noël AVILA wrote:
Show 10 quoted lines
> 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
Junio C Hamano· Jan 19, 2013, 07:52 UTC · re: Torsten Bögershausen · lore

Re* t9902 fails

Torsten Bögershausen <tboegi@web.de> writes:
Show 5 quoted lines
> 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(-)
Show changes to 6 files +75 −39

builtin/help.c, command-list.txt, contrib/completion/git-completion.bash, generate-cmdlist.sh, help.c, help.h

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);
Junio C Hamano· Jan 19, 2013, 08:00 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 2 files +17 −18

builtin/help.c, help.c

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
Junio C Hamano· Jan 19, 2013, 08:02 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 6 files +58 −21

builtin/help.c, command-list.txt, contrib/completion/git-completion.bash, generate-cmdlist.sh, help.c, help.h

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
Jean-Noël AVILA· Jan 19, 2013, 10:32 UTC · re: Junio C Hamano · lore

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

Le samedi 19 janvier 2013 09:02:13, Junio C Hamano a écrit :
Show 209 quoted lines
> 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.
Jean-Noël AVILA· Jan 19, 2013, 13:43 UTC · re: Junio C Hamano · lore

Re: Re* t9902 fails

Le samedi 19 janvier 2013 08:52:25, Junio C Hamano a écrit :
Show 5 quoted lines
>  (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(-)
Show changes to 2 files +12 −3

contrib/completion/git-completion.bash, t/t9902-completion.sh

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
Jean-Noël AVILA· Jan 19, 2013, 10:23 UTC · re: Torsten Bögershausen · lore

Re: t9902 fails

Le samedi 19 janvier 2013 06:38:54, Torsten Bögershausen a écrit :
Show 26 quoted lines
> 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.

Duy Nguyen· Jan 16, 2013, 01:08 UTC · re: Junio C Hamano · lore

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

On Wed, Jan 16, 2013 at 2:29 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
> "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
Duy Nguyen· Jan 16, 2013, 02:09 UTC · re: Duy Nguyen · lore

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

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(-)
Show changes to attr.c +19 −27
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< --
Junio C Hamano· Jan 16, 2013, 02:33 UTC · re: Duy Nguyen · lore

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

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.

Show 88 quoted lines
>> -- 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);
Duy Nguyen· Jan 16, 2013, 03:12 UTC · re: Junio C Hamano · lore

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

On Wed, Jan 16, 2013 at 9:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
> 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
Junio C Hamano· Jan 16, 2013, 05:35 UTC · re: Duy Nguyen · lore

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

Duy Nguyen <pclouds@gmail.com> writes:
Show 13 quoted lines
> 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.

Show 5 quoted lines
>> 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.

Duy Nguyen· Jan 16, 2013, 06:02 UTC · re: Junio C Hamano · lore

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

On Tue, Jan 15, 2013 at 09:35:21PM -0800, Junio C Hamano wrote:
Show 7 quoted lines
> >> 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(-)
Show changes to attr.c +18 −27
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< --
Junio C Hamano· Jan 16, 2013, 19:07 UTC · re: Duy Nguyen · lore

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

Duy Nguyen <pclouds@gmail.com> writes:
Show 12 quoted lines
> 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.

Show 74 quoted lines
>  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);
Duy Nguyen· Jan 16, 2013, 01:03 UTC · re: Jean-Noël AVILA · lore

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

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

← back to recent threads