On Mon, Oct 28, 2013 at 09:16:19PM +0100, Torsten Bögershausen wrote:
> Commit 8d3d28f5 added test cases for URLs which should be ssh.
>
> Add more tests testing all the combinations:
> -IPv4 or IPv6
> -path starting with "/" or with "/~"
> -with and without the ssh:// scheme
>
> Add tests for ssh:// with port number.
>
> When a git repository "foo:bar" exist, git clone will call
> absolute_path() and git_connect() will be called with
> something like "/home/user/projects/foo:bar"
>
> Tighten the test and use "foo:bar" instead of "./foo:bar",
> refactor clear_ssh() and expect_ssh() into test_clone_url().
>
> "git clone foo/bar:baz" should not be ssh:
> Make the rule
> "if there is a slash before the first colon, it is not ssh"
> more strict by using the same is_local() in both connect.c
> and transport.c. Add a test case.
>
> Bug fixes for corner cases:
> - Handle clone of [::1]:/~repo correctly (2e7766655):
> Git should call "ssh ::1 ~repo", not ssh ::1 /~repo
> This was caused by looking at (host != url), which never
> worked for host names with literal IPv6 adresses, embedded by []
> Support for the [] URLs was introduced in 356bece0a.
>
> - Do not tamper local URLs starting with '[' which have a ']'
IMHO, this would have been a little easier to follow as separate
patches, as there are a few things going on. I think the changes to the
code are fine, though.
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index 1d1c875..a126f08 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -294,39 +294,95 @@ test_expect_success 'setup ssh wrapper' '
> export TRASH_DIRECTORY
> '
>
> -clear_ssh () {
> - >"$TRASH_DIRECTORY/ssh-output"
> -}
> -
> -expect_ssh () {
I'd prefer if you left the expect_ssh interface intact, as it is
potentially useful outside of t5601 (but your patch ties it very closely
to a particular set of clone tests). Can you call out to expect_ssh
instead from test_clone_url instead?
> +i5601=0
The name of this variable confused me for a bit. Maybe "counter" or
something would be a little more descriptive?
> +# $1 url
> +# $2 none|host
> +# $3 path
> +test_clone_url () {
> + i5601=$(($i5601 + 1))
> + >"$TRASH_DIRECTORY/ssh-output" &&
> + test_might_fail git clone "$1" tmp$i5601 &&
Do we always want test_might_fail in these tests? I really would prefer
that the test actually accomplish the clone, as then we know that
nothing funny is going on (e.g., multiple invocations of ssh, one of
which is good and one of which is not). I think I gave more specific
examples in the last round of review.
> - (cd "$TRASH_DIRECTORY" && test_cmp ssh-expect ssh-output)
> + (
> + cd "$TRASH_DIRECTORY" &&
> + test_cmp ssh-expect ssh-output &&
> + rm -rf ssh-expect ssh-output
> + )
Here we clean up at the end of the test, which is a good improvement on
my existing functions. But I notice that you also clear ssh-output at
the beginning of the test, still (which you must do to not leave a
polluted state after a failing test). I think this might be better as:
test_when_finished '
(cd "$TRASH_DIRECTORY" && rm -f ssh-expect ssh-output)
'
and then the "clear_ssh" can just go away (and your reset of ssh-output,
which is the analogous line), as tests can expect a clean state.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html