2016-08-26 21:06 GMT+02:00 Junio C Hamano <gits...@pobox.com>: > Ralf Thielow <ralf.thie...@gmail.com> writes: > >> Introduce option --exclude-guides to the help command. With this option >> being passed, "git help" will open man pages only for actual commands. > > Let's hide this option from command help of "git help" itself, drop > the short-and-sweet "-e", not command-line complete it, and leave it > not-mentioned here. > > It is a different story if this option would really help the end > users, but I do not think that is the case. If this were to face > the end users properly, we would need to worry about making sure > that "git help -g -e" would error out, and getting all the other > possible corner cases right. I do not think the amount of effort > required to do so (even the "trying to enumerate what other possible > corner cases there may be" part) is worth it. >
I'm fine with that as the reason for me was just a "why not?", and you just gave the reason to not do this. Thanks >> In the test script we do two things I'd like to point out: >> >>> + test_config help.htmlpath test://html && >> >> As we pass a URL, Git won't check if the given path looks like >> a documentation directory. Another solution would be to create >> a directory, add a file "git.html" to it and just use this path. > > I think this is OK; with s|As we pass a URL|As we pass a string with > :// in it|, the first sentence can be a in-code comment in the test > that does this and will help readers of the code in the future. > Hmm. The "://" is really a URL thing. That's why it's in the code, no? The code may have some room for improvements in checking for URLs. >>> + test_config help.browser firefox >> >> Git checks if the browser is known, so the "test-browser" needs to >> pretend it is one of them. > > Are you talking about the hardcoded list in valid_tool() helper > function in git-web--browse.sh? If we use the established escape > hatch implemented by valid_custom_tool() helper there by setting > browser.*.cmd, would that be sufficient to work around the "Git > checks if the browser is known"? > >> diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt >> index 40d328a..eeb1950 100644 >> --- a/Documentation/git-help.txt >> +++ b/Documentation/git-help.txt >> @@ -8,7 +8,7 @@ git-help - Display help information about Git >> SYNOPSIS >> -------- >> [verse] >> -'git help' [-a|--all] [-g|--guide] >> +'git help' [-a|--all] [-e|--exclude-guides] [-g|--guide] >> [-i|--info|-m|--man|-w|--web] [COMMAND|GUIDE] > > So, let's not do this. > >> diff --git a/builtin/help.c b/builtin/help.c >> index e8f79d7..40901a9 100644 >> --- a/builtin/help.c >> +++ b/builtin/help.c >> @@ -37,8 +37,10 @@ static int show_all = 0; >> static int show_guides = 0; >> static unsigned int colopts; >> static enum help_format help_format = HELP_FORMAT_NONE; >> +static int exclude_guides; >> static struct option builtin_help_options[] = { >> OPT_BOOL('a', "all", &show_all, N_("print all available commands")), >> + OPT_BOOL('e', "exclude-guides", &exclude_guides, N_("exclude guides")), > > So I'd suggest using PARSE_OPT_HIDDEN for this one and drop 'e' shorthand. > The only caller of this mode does not use it. > >> diff --git a/contrib/completion/git-completion.bash >> b/contrib/completion/git-completion.bash >> index c1b2135..b148164 100644 >> --- a/contrib/completion/git-completion.bash >> +++ b/contrib/completion/git-completion.bash >> @@ -1393,7 +1393,7 @@ _git_help () >> { >> case "$cur" in >> --*) >> - __gitcomp "--all --guides --info --man --web" >> + __gitcomp "--all --exclude-guides --guides --info --man --web" >> return >> ;; >> esac > > So, let's not do this. > >> diff --git a/t/t0012-help.sh b/t/t0012-help.sh >> new file mode 100755 >> index 0000000..fb1abd7 >> --- /dev/null >> +++ b/t/t0012-help.sh >> @@ -0,0 +1,33 @@ >> +#!/bin/sh >> + >> +test_description='help' >> + >> +. ./test-lib.sh >> + >> +configure_help () { >> + test_config help.format html && >> + test_config help.htmlpath test://html && >> + test_config help.browser firefox >> +} > > Would replacing the last line with: > > test_config browser.test.cmd ./test-browser && > test_config help.browser test > > and then writing to test-browser work just as well? If so, that > would be much cleaner and more preferrable. > I wasn't aware that this is a way to configure things. Thanks. >> + >> +test_expect_success "setup" " >> + write_script firefox <<-\EOF >> + exit 0 >> + EOF >> +" > > Unless there is a good reason you MUST do so, avoid quoting the test > body with double quotes, as it invites mistakes [*1*]. > The test-browser was supposed to be returning just a success which is good enough for my usage. > Also, how about using something like: > > write_script test-browser <<-\EOF > i=0 > for arg > do > i=$(( $i + 1 )) > echo "$i: $arg" > done >test-browser.log > EOF > > instead? That way, you can ensure that "git help status" attempts > to call git-status.html with the expected path, not gitstatus.html > or status.html, or somesuch, immediately after running "git help > status" in the next test by inspecting test-browser.log ... > We can use this to check whether the correct file was tried to open. Not a part of this "does it work" test, but good for new ones. >> +test_expect_success "works for commands and guides by default" " >> + configure_help && >> + git help status && > > ... right here. > > The output from the test-browser does not have to be multi-line; > just doing > > echo "$*" > > might be sufficient. > >> + git help revisions >> +" > > Thanks. > > [Footnote] > > *1* Can you immediately tell why this test is broken? > > test_expect_success "two commits do not have the same ID" " > git commit --allow-empty -m first && > one=$(git rev-parse --verify HEAD) && > test_tick && > git commit --allow-empty -m second && > two=$(git rev-parse --verify HEAD) && > test $one != $two > " > I'm afraid I can't. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html