Re: [PATCH] clone: local URLs are not for ssh

From: Jeff King <peff@peff.net>
Date: Oct 3, 2013, 01:31 UTC
Message-ID: <20131003013127.GA7917@sigill.intra.peff.net>
In-Reply-To: <CACsJy8DjPiwpMvLe2p+dGBdpU6iTg2my7tddBsQwLnHiHdG8wQ@mail.gmail.com>

On Thu, Oct 03, 2013 at 08:01:23AM +0700, Nguyen Thai Ngoc Duy wrote:

Show 6 quoted lines

> > Sorry for the noise, I noticed it when I was trying to construct test cases.
> >
> > What do we think about adding this at the end of t5505:
> 
> As usual more tests are usually better. But is t5505-remote.sh the
> best place? That file seems about "git remote"..

Yeah, agreed. How about at the end of t5601, after the ssh wrapper I set up here:

  http://article.gmane.org/gmane.comp.version-control.git/235473

I don't know of Jonathan squashed those in to your commit...neither is in his 'pu' yet.

Show 7 quoted lines

> > test_expect_success 'fetch fail [noexistinghost0:2223]:blink.git' '
> >         (
> >                 ! git fetch [noexistinghost0:2223]:blink.git 2>err &&
> >                 grep ssh err &&
> >                 rm err
> >         )
> > '

This one looks like basically the same test I added in the message above (except because of the ssh wrapper, we can check that it did indeed try to ssh to noexistinghost0:2223).

The other tests can check that we fed ssh various host/port/path combinations. I'm not clear on what we're expecting, though...

Show 7 quoted lines

> > test_expect_success 'fetch fail noexistinghost1:2223:blink.git' '
> >         (
> >                 ! git fetch "noexistinghost1:2223:blink.git" 2>err &&
> >                 grep ssh err &&
> >                 rm err
> >         )
> > '

We are expecting this to be host=noexistinghost1, and path=2223:blink.git?

Show 7 quoted lines

> > test_expect_success 'fetch fail noexistinghost2:2223' '
> >         (
> >                 ! git fetch "noexistinghost2:2223" 2>err &&
> >                 grep ssh err &&
> >                 rm err
> >         )
> > '

And this is host=noexistinghost2, path=2223?

Show 7 quoted lines

> > test_expect_success 'fetch fail ./noexistinghost4:2223"' '
> >         (
> >                 ! git fetch "./noexistinghost4:2223" 2>err &&
> >                 grep "does not appear to be a git repository" err &&
> >                 rm err
> >         )
> > '

And this one we would be checking that ssh is _not_ used. It seems redundant with the "./foo:bar" test already in t5601, but perhaps it is worth double-checking the numeric path. It would be more robust if we actually had a repo called "noexistinghost4:2223" and checked that we did clone it, as the existing test does (maybe that test can just "s/bar/2223/" ?).

-Peff