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

[Junio C Hamano <gitster@pobox.com>]

Ralf Thielow <ralf.thielow@gmail.com> writes:

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

Perhaps you thought so, but no, "mailto:ralf.thielow@gmail.com" is a
perfectly valid URL.

Because you are explaining why test://html was chosen, and the real
reason is any path that is !strstr(path, "://") is subject to an
additional "This must be a local path" check and you wanted to avoid
it, "As we pass a URL" is unnecessarily vague (and incorrect--we
cannot use a mailto: URL to sidestep the check).

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

The reason becomes clear if you put your feet into shell's shues.
Before being ablt to call test_expect_success, you would need to
figure out what strings you give as its parameters.  $1 is clear in
this case, a simple string "two commits do not have the same ID"
(without double quotes).

But what goes in $2?  Especially the part around "one=..."?

Because the whole thing is inside a double-quote pair, $() and $name
are all interpolated even before test_expect_success is called.
So the above becomes equivalent to

>> test_expect_success "two commits do not have the same ID" '
>>         git commit --allow-empty -m first &&
>>         one=5cb0d5ad05e027cbddcb0a3c7518ddeea0f7c286 &&
>>         test_tick &&
>>         git commit --allow-empty -m second &&
>>         two=5cb0d5ad05e027cbddcb0a3c7518ddeea0f7c286 &&
>>         test !=
>> '

(using whatever commit HEAD was pointing at before this test starts
to run), which obviously is not what we expected to see.