Re: [PATCH v3 0/6] Keep all info in command-list.txt in git binary

[Ramsay Jones]

On 22/04/18 17:12, Duy Nguyen wrote:
> On Sun, Apr 22, 2018 at 5:58 PM, Ramsay Jones
>  wrote:
 I think you need to try a little harder than this! ;-)
>>>
>>> Yeah. I did think about grepping the output but decided not to because
>>> of gettext poison stuff and column output in "git help". If we do want
>>> to test this, how about I extend --list-cmds= option to take a few
>>> more parameters? --list-cmds=common would output all common commands,
>>> --list-cmds= does the same for other command category. This
>>> way we can verify without worrying about text formatting, paging or
>>> translation.
>>
>> Hmm, my immediate reaction would be to prefer my simple tests.
>> Yes, they are not exactly rigorous and they would be affected
>> by changing the help formatting, but they are effective. ;-)
>>
>> [I don't think the formatting would change that often, or at
>> all - whoever submits that patch would get to update the tests!]
> 
> Hmm.. for non-column output that's true. "git help" with column output
> should probably fine as well because even though we add more and more
> commands every month, these are not marked common (and unlikely so).
> So yeah I agree.
> 
>> What did you think about adding the BUG() checks?
> 
> I was thinking if there was a way to fail the build after running
> ./generate-cmds.sh and generating empty output but it does not sound
> easy to do. So yeah, BUG() checks sound good.

Yeah, failing the build would be preferable, but I didn't get
that far. ;-)

[BTW, I just applied your patch series to the 'next' branch
(I just couldn't be bothered to revert your last series from
the 'pu' branch, etc.) and, as expected, everything built OK,
passed the test-suite and 'git help' is working just fine.]

Thanks!

ATB,
Ramsay Jones

Re: [PATCH v3 0/6] Keep all info in command-list.txt in git binary

[Duy Nguyen]

On Sun, Apr 22, 2018 at 5:58 PM, Ramsay Jones
 wrote:
>>> I think you need to try a little harder than this! ;-)
>>
>> Yeah. I did think about grepping the output but decided not to because
>> of gettext poison stuff and column output in "git help". If we do want
>> to test this, how about I extend --list-cmds= option to take a few
>> more parameters? --list-cmds=common would output all common commands,
>> --list-cmds= does the same for other command category. This
>> way we can verify without worrying about text formatting, paging or
>> translation.
>
> Hmm, my immediate reaction would be to prefer my simple tests.
> Yes, they are not exactly rigorous and they would be affected
> by changing the help formatting, but they are effective. ;-)
>
> [I don't think the formatting would change that often, or at
> all - whoever submits that patch would get to update the tests!]

Hmm.. for non-column output that's true. "git help" with column output
should probably fine as well because even though we add more and more
commands every month, these are not marked common (and unlikely so).
So yeah I agree.

> What did you think about adding the BUG() checks?

I was thinking if there was a way to fail the build after running
./generate-cmds.sh and generating empty output but it does not sound
easy to do. So yeah, BUG() checks sound good.

-- 
Duy

Re: [PATCH v3 0/6] Keep all info in command-list.txt in git binary

[Ramsay Jones]

On 22/04/18 16:22, Duy Nguyen wrote:
> On Sun, Apr 22, 2018 at 4:45 PM, Ramsay Jones
>  wrote:
>>
>>
>> On 21/04/18 17:56, Duy Nguyen wrote:
>>> On Sat, Apr 21, 2018 at 06:54:08PM +0200, Nguyễn Thái Ngọc Duy wrote:
 Changes:

 - remove the deprecated column in command-list.txt. My change break it
   anyway if anyone uses it.
 - fix up failed tests that I marked in the RFC and kinda forgot about it.
 - fix bashisms in generate-cmdlist.sh
 - fix segfaul in "git help"
>>>
>>> Sorry I forgot the interdiff
>>>
>> [snip]
>>
>>> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
>>> index fd2a7f27dc..53208ab20e 100755
>>> --- a/t/t0012-help.sh
>>> +++ b/t/t0012-help.sh
>>> @@ -25,6 +25,15 @@ test_expect_success "setup" '
>>>   EOF
>>>  '
>>>
>>> +# make sure to exercise these code paths, the output is a bit tricky
>>> +# to verify
>>> +test_expect_success 'basic help commands' '
>>> + git help >/dev/null &&
>>> + git help -a >/dev/null &&
>>> + git help -g >/dev/null &&
>>> + git help -av >/dev/null
>>> +'
>>> +
>> I think you need to try a little harder than this! ;-)
> 
> Yeah. I did think about grepping the output but decided not to because
> of gettext poison stuff and column output in "git help". If we do want
> to test this, how about I extend --list-cmds= option to take a few
> more parameters? --list-cmds=common would output all common commands,
> --list-cmds= does the same for other command category. This
> way we can verify without worrying about text formatting, paging or
> translation.

Hmm, my immediate reaction would be to prefer my simple tests.
Yes, they are not exactly rigorous and they would be affected 
by changing the help formatting, but they are effective. ;-)

[I don't think the formatting would change that often, or at
all - whoever submits that patch would get to update the tests!]

What did you think about adding the BUG() checks?

ATB,
Ramsay Jones

Re: [PATCH v3 0/6] Keep all info in command-list.txt in git binary

[Duy Nguyen]

On Sun, Apr 22, 2018 at 4:45 PM, Ramsay Jones
 wrote:
>
>
> On 21/04/18 17:56, Duy Nguyen wrote:
>> On Sat, Apr 21, 2018 at 06:54:08PM +0200, Nguyễn Thái Ngọc Duy wrote:
>>> Changes:
>>>
>>> - remove the deprecated column in command-list.txt. My change break it
>>>   anyway if anyone uses it.
>>> - fix up failed tests that I marked in the RFC and kinda forgot about it.
>>> - fix bashisms in generate-cmdlist.sh
>>> - fix segfaul in "git help"
>>
>> Sorry I forgot the interdiff
>>
> [snip]
>
>> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
>> index fd2a7f27dc..53208ab20e 100755
>> --- a/t/t0012-help.sh
>> +++ b/t/t0012-help.sh
>> @@ -25,6 +25,15 @@ test_expect_success "setup" '
>>   EOF
>>  '
>>
>> +# make sure to exercise these code paths, the output is a bit tricky
>> +# to verify
>> +test_expect_success 'basic help commands' '
>> + git help >/dev/null &&
>> + git help -a >/dev/null &&
>> + git help -g >/dev/null &&
>> + git help -av >/dev/null
>> +'
>> +
> I think you need to try a little harder than this! ;-)

Yeah. I did think about grepping the output but decided not to because
of gettext poison stuff and column output in "git help". If we do want
to test this, how about I extend --list-cmds= option to take a few
more parameters? --list-cmds=common would output all common commands,
--list-cmds= does the same for other command category. This
way we can verify without worrying about text formatting, paging or
translation.
-- 
Duy

Re: [PATCH v3 0/6] Keep all info in command-list.txt in git binary

[Ramsay Jones]

On 21/04/18 17:56, Duy Nguyen wrote:
> On Sat, Apr 21, 2018 at 06:54:08PM +0200, Nguyễn Thái Ngọc Duy wrote:
>> Changes:
>>
>> - remove the deprecated column in command-list.txt. My change break it
>>   anyway if anyone uses it.
>> - fix up failed tests that I marked in the RFC and kinda forgot about it.
>> - fix bashisms in generate-cmdlist.sh
>> - fix segfaul in "git help"
> 
> Sorry I forgot the interdiff
> 
[snip]

> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
> index fd2a7f27dc..53208ab20e 100755
> --- a/t/t0012-help.sh
> +++ b/t/t0012-help.sh
> @@ -25,6 +25,15 @@ test_expect_success "setup" '
>   EOF
>  '
>  
> +# make sure to exercise these code paths, the output is a bit tricky
> +# to verify
> +test_expect_success 'basic help commands' '
> + git help >/dev/null &&
> + git help -a >/dev/null &&
> + git help -g >/dev/null &&
> + git help -av >/dev/null
> +'
> +
I think you need to try a little harder than this! ;-)

This test would not have noticed the recent failure (what annoyed me was
that git build without error and then the test-suite sailed through with
nary a squeak of complaint). Viz:

  $ ./git help
  usage: git [--version] [--help] [-C ] [-c =]
 [--exec-path[=]] [--html-path] [--man-path] [--info-path]
 [-p | --paginate | --no-pager] [--no-replace-objects] [--bare]
 [--git-dir=] [--work-tree=] [--namespace=]
  []
  
  These are common Git commands used in various situations:
  
  'git help -a' and 'git help -g' list available subcommands and some
  concept guides. See 'git help ' or 'git help '
  to read about a specific subcommand or concept.
  $ echo $?
  0
  $ 

  $ ./git help -g
  The common Git guides are:
  
  
  'git help -a' and 'git help -g' list available subcommands and some
  concept guides. See 'git help ' or 'git help '
  to read about a specific subcommand or concept.
  $ echo $?
  0
  $ 

  $ ./git help -av
  Main Porcelain Commands
  
  
  Ancillary Commands / Manipulators
  
  
  Ancillary Commands / Interrogators
  
  
  Interacting with Others
  
  
  Low-level Commands / Manipulators
  
  
  Low-level Commands / Interrogators
  
  
  Low-level Commands / Synching Repositories
  
  
  Low-level Commands / Internal Helpers
  
  $ echo $?
  0
  $ 
 
I started to add some tests, like so:

  diff --git a/t/t0012-help.sh b/t/t0012-help.sh
  index fd2a7f27d..7e10c2862 100755
  --- a/t/t0012-help.sh
  +++ b/t/t0012-help.sh
  @@ -49,6 +49,22 @@ test_expect_success "--help does not work for guides" "
  test_i18ncmp expect actual
   "
   
  +test_expect_success 'git help' '
  +   git help >help.output &&
  +   test_i18ngrep "^   clone  " help.output &&
  +   test_i18ngrep "^   add" help.output &&
  +   test_i18ngrep "^   log" help.output &&
  +   test_i18ngrep "^   commit " help.output &&
  +   test_i18ngrep "^   fetch  " help.output
  +'
  +
  +test_expect_success 'git help -g' '
  +   git help -g >help.output &&
  +   test_i18ngrep "^   attributes " help.output &&
  +   test_i18ngrep "^   everyday   " help.output &&
  +   test_i18ngrep "^   tutorial   " help.output
  +'
  +
   test_expect_success 'generate builtin list' '
  git --list-cmds=builtins >builtins
   '

These tests, while not rigorous, did at least correctly catch the bad
build for me. I was about to add the obvious one for 'git help -av',
when I decided to catch the problem 'at source', viz:

  diff --git a/help.c b/help.c
  index a47f7e2c4..13790bb54 100644
  --- a/help.c
  +++ b/help.c
  @@ -196,6 +196,9 @@ static void extract_common_cmds(struct cmdname_help 
**p_common_cmds,
int i, nr = 0;
struct cmdname_help *common_cmds;
   
  + if (ARRAY_SIZE(command_list) == 0)
  + BUG("empty command_list");
  +
ALLOC_ARRAY(common_cmds, ARRAY_SIZE(command_list));
   
for (i = 0; i < ARRAY_SIZE(command_list); i++) {
  @@ -279,6 +282,9 @@ void list_porcelain_cmds(void)
int i, nr = ARRAY_SIZE(command_list);
struct cmdname_help *cmds = command_list;
   
  + if (nr == 0)
  + BUG("empty command_list");
  +
for (i = 0; i < nr; i++) {
if (cmds[i].category != CAT_mainporcelain)
continue;
  @@ -321,6 +327,9 @@ void list_common_guides_help(void)
int nr = ARRAY_SIZE(command_list);
struct cmdname_help *cmds = command_list;
   
  + if (nr == 0)
  + BUG("empty command_list");
  +
QSORT(cmds, nr, cmd_category_cmp);
   
for (i = 0; i < nr; i++) {
  @@ -343,6 +352,9 @@ void list_all_cmds_help(void)
int nr = ARRAY_SIZE(command_list);
struct cmdname_help *cmds = command_list;
   
  + if (nr == 0)
  + BUG("empty command_list");
  +
for (i = 0; i < nr; i++) {
struct cmdname_help *cmd = cmds + i;
   
  
This had a very dramatic effect on the test-suite, since every single
test file 

Re: [PATCH v3 0/6] Keep all info in command-list.txt in git binary

[Duy Nguyen]

On Sat, Apr 21, 2018 at 06:54:08PM +0200, Nguyễn Thái Ngọc Duy wrote:
> Changes:
> 
> - remove the deprecated column in command-list.txt. My change break it
>   anyway if anyone uses it.
> - fix up failed tests that I marked in the RFC and kinda forgot about it.
> - fix bashisms in generate-cmdlist.sh
> - fix segfaul in "git help"

Sorry I forgot the interdiff

diff --git a/command-list.txt b/command-list.txt
index 0809a19184..1835f1a928 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -9,7 +9,7 @@ history  grow, mark and tweak your common history
 remote   collaborate (see also: git help workflows)
 
 ### command list (do not change this line)
-# command name  category [deprecated] [common]
+# command name  category[common]
 git-add mainporcelain   worktree
 git-am  mainporcelain
 git-annotateancillaryinterrogators
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 9f17703aa7..7d17ca23f6 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -835,19 +835,23 @@ __git_complete_strategy ()
 }
 
 __git_commands () {
-   if test -n "${GIT_TESTING_COMMAND_COMPLETION:-}"
+   if test -n "$GIT_TESTING_COMPLETION"
then
-   printf "%s" "${GIT_TESTING_COMMAND_COMPLETION}"
+   case "$1" in
+   porcelain)
+   printf "%s" "$GIT_TESTING_PORCELAIN_COMMAND_LIST";;
+   all)
+   printf "%s" "$GIT_TESTING_ALL_COMMAND_LIST";;
+   esac
else
git --list-cmds=$1
fi
 }
 
-__git_list_all_commands ()
+__git_list_commands ()
 {
local i IFS=" "$'\n'
-   local category=${1-all}
-   for i in $(__git_commands $category)
+   for i in $(__git_commands $1)
do
case $i in
*--*) : helper pattern;;
@@ -860,14 +864,14 @@ __git_all_commands=
 __git_compute_all_commands ()
 {
test -n "$__git_all_commands" ||
-   __git_all_commands=$(__git_list_all_commands)
+   __git_all_commands=$(__git_list_commands all)
 }
 
 __git_porcelain_commands=
 __git_compute_porcelain_commands ()
 {
test -n "$__git_porcelain_commands" ||
-   __git_porcelain_commands=$(__git_list_all_commands porcelain)
+   __git_porcelain_commands=$(__git_list_commands porcelain)
 }
 
 # Lists all set config variables starting with the given section prefix,
diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index e35f3e357b..86d59419b3 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh
@@ -36,7 +36,7 @@ sed -n '
' "$1"
 printf '};\n\n'
 
-echo "#define GROUP_NONE 0xff /* no common group */"
+echo "#define GROUP_NONE 0xff"
 n=0
 while read grp
 do
@@ -45,15 +45,6 @@ do
 done <"$grps"
 echo
 
-echo '/*'
-printf 'static const char *cmd_categories[] = {\n'
-category_list "$1" |
-while read category; do
-   printf '\t\"'$category'\",\n'
-done
-printf '\tNULL\n};\n\n'
-echo '*/'
-
 n=0
 category_list "$1" |
 while read category; do
@@ -68,10 +59,11 @@ sort |
 while read cmd category tags
 do
if [ "$category" = guide ]; then
-   name=${cmd/git}
+   prefix=git
else
-   name=${cmd/git-}
+   prefix=git-
fi
+   name=$(echo $cmd | sed "s/^${prefix}//")
sed -n '
/^NAME/,/'"$cmd"'/H
${
diff --git a/help.c b/help.c
index a44f4a113e..88127fdd6f 100644
--- a/help.c
+++ b/help.c
@@ -201,7 +201,8 @@ static void extract_common_cmds(struct cmdname_help 
**p_common_cmds,
for (i = 0; i < ARRAY_SIZE(command_list); i++) {
const struct cmdname_help *cmd = command_list + i;
 
-   if (cmd->category != CAT_mainporcelain)
+   if (cmd->category != CAT_mainporcelain ||
+   cmd->group == GROUP_NONE)
continue;
 
common_cmds[nr++] = *cmd;
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index fd2a7f27dc..53208ab20e 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -25,6 +25,15 @@ test_expect_success "setup" '
EOF
 '
 
+# make sure to exercise these code paths, the output is a bit tricky
+# to verify
+test_expect_success 'basic help commands' '
+   git help >/dev/null &&
+   git help -a >/dev/null &&
+   git help -g >/dev/null &&
+   git help -av >/dev/null
+'
+
 test_expect_success "works for commands and guides by default" '
configure_help &&
git help status &&
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 4bfd26ddf9..5a23a46fcf 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -13,7 +13,7 @@ complete ()
return 0
 }
 
-# Be careful when updating this list:
+# Be careful when 

[PATCH v3 0/6] Keep all info in command-list.txt in git binary

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

Changes:

- remove the deprecated column in command-list.txt. My change break it
  anyway if anyone uses it.
- fix up failed tests that I marked in the RFC and kinda forgot about it.
- fix bashisms in generate-cmdlist.sh
- fix segfaul in "git help"

Nguyễn Thái Ngọc Duy (6):
  git.c: convert --list-*builtins to --list-cmds=*
  git.c: implement --list-cmds=all and use it in git-completion.bash
  generate-cmdlist.sh: keep all information in common-cmds.h
  git.c: implement --list-cmds=porcelain
  help: add "-a --verbose" to list all commands with synopsis
  help: use command-list.txt for the source of guides

 Documentation/git-help.txt |   4 +-
 Documentation/gitattributes.txt|   2 +-
 Documentation/gitmodules.txt   |   2 +-
 Documentation/gitrevisions.txt |   2 +-
 builtin/help.c |  39 ++
 command-list.txt   |  10 +-
 contrib/completion/git-completion.bash | 108 ++--
 generate-cmdlist.sh|  57 ++---
 git.c  |  16 ++-
 help.c | 164 -
 help.h |   4 +
 t/t0012-help.sh|  11 +-
 t/t9902-completion.sh  |   6 +-
 13 files changed, 263 insertions(+), 162 deletions(-)

-- 
2.17.0.367.g5dd2e386c3