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

Ralf Thielow [off-list ref] 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.

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.

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

So, let's not do this.

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.

So, let's not do this.

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.

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

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

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