Re: [PATCH v3 0/6] Keep all info in command-list.txt in git binary
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
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
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
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
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 fail
Re: [PATCH v3 0/6] Keep all info in command-list.txt in git binary
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 updating
[PATCH v3 0/6] Keep all info in command-list.txt in git binary
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