Re: [PATCH v2 2/3] help: introduce option --exclude-guides

2016-08-26 21:06 GMT+02:00 Junio C Hamano [off-list ref]:

Ralf Thielow [off-list ref] writes:

quoted

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

quoted

In the test script we do two things I'd like to point out:

quoted

+       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.

quoted

quoted

+       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"?

quoted

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.

quoted

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.

quoted

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.

quoted

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.

quoted

+
+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.

quoted

+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.

quoted

+     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.