On 09/27, Junio C Hamano wrote:
> Brandon Williams <bmwill@xxxxxxxxxx> writes:
>
> > Teach the connection logic to tell a serve that it understands protocol
> > v1. This is done in 2 different ways for the built in protocols.
> >
> > 1. git://
> > A normal request is structured as "command path/to/repo\0host=..\0"
> > and due to a bug in an old version of git-daemon 73bb33a94 (daemon:
> > Strictly parse the "extra arg" part of the command, 2009-06-04) we
> > aren't able to place any extra args (separated by NULs) besides the
> > host. In order to get around this limitation put protocol version
> > information after a second NUL byte so the request is structured
> > like: "command path/to/repo\0host=..\0\0version=1\0". git-daemon can
> > then parse out the version number and set GIT_PROTOCOL.
>
> Same question as a previous step, wrt the cited commit. It reads as
> if we are saying that the commit introduced a bug and left it there,
> that we cannot use \0host=..\0version=..\0other=..\0 until that bug
> is fixed, and that in the meantime we use \0host=..\0\0version=.. as
> a workaround, but that reading leaves readers wondering if we want
> to eventually drop this double-NUL workaround. I am guessing that
> we want to declare that the current protocol has a glitch that
> prevents us to use \0host=..\0version=..\0 but we accept that and
> plan to keep it that way, and we'll use the double-NUL for anything
> other than host from now on, as it is compatible with the current
> version of Git before this patch (the extras are safely ignored),
> but then it still leaves readers wonder if the mention of the
> old commit from 2009 means that this double-NUL would not even work
> if the other end is running a version of Git before that commit, or
> we are safe to talk with versions of Git even older than that.
>
> I do not think it is a showstopper if we did not work with v1.6.4,
> but it still needs to be clarified.
I wrote an updated commit msg for the daemon change, I can make a
similar change here. And this mechanism shouldn't cause any issues with
both the pre and post 73bb33a94 git-daemon servers.
>
> > 2. ssh://, file://
> > Set GIT_PROTOCOL envvar with the desired protocol version. The
> > envvar can be sent across ssh by using '-o SendEnv=GIT_PROTOCOL' and
> > having the server whitelist this envvar.
>
> OpenSSH lets us do this, but I do not know how well this works with
> other implementations of SSH clients. The log message perhaps needs
> to ask for volunteers to check if it is OK with the implementations
> they use, and offer conditional code (just like we have for putty
> and plink customizations) otherwise.
I'll make a comment indicating that
>
> Other than that, the code changes looked good.
>
> > diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh
> > new file mode 100755
> > index 000000000..1988bbce6
> > --- /dev/null
> > +++ b/t/t5700-protocol-v1.sh
> > @@ -0,0 +1,223 @@
> > +#!/bin/sh
> > +
> > +test_description='test git wire-protocol transition'
> > +
> > +TEST_NO_CREATE_REPO=1
> > +
> > +. ./test-lib.sh
> > +
> > +# Test protocol v1 with 'git://' transport
> > +#
> > +. "$TEST_DIRECTORY"/lib-git-daemon.sh
> > +start_git_daemon --export-all --enable=receive-pack
> > +daemon_parent=$GIT_DAEMON_DOCUMENT_ROOT_PATH/parent
> > +
> > +test_expect_success 'create repo to be served by git-daemon' '
> > + git init "$daemon_parent" &&
> > + test_commit -C "$daemon_parent" one
> > +'
> > +
> > +test_expect_success 'clone with git:// using protocol v1' '
> > + GIT_TRACE_PACKET=1 git -c protocol.version=1 \
> > + clone "$GIT_DAEMON_URL/parent" daemon_child 2>log &&
> > +
> > + git -C daemon_child log -1 --format=%s >actual &&
> > + git -C "$daemon_parent" log -1 --format=%s >expect &&
> > + test_cmp expect actual &&
> > +
> > + # Client requested to use protocol v1
> > + grep "version=1" log &&
> > + # Server responded using protocol v1
> > + grep "clone< version 1" log
>
> This looked a bit strange to check "clone< version 1" for one
> direction, but did not check "$something> version 1" for the other
> direction. Doesn't "version=1" end up producing 2 hits?
I think you discovered this in your next email but the "version=1" check
is to check for the request sent to git-daemon, the "command
path/to/repo\0host=blah\0\0version=1\0" one. While the "clone< version
1" check is to make sure that the server responded with the correct
version.
>
> Not a complaint, but wondering if we can write it in such a way that
> does not have to make readers wonder.
>
> > +'
> > +
> > +test_expect_success 'fetch with git:// using protocol v1' '
> > + test_commit -C "$daemon_parent" two &&
> > +
> > + GIT_TRACE_PACKET=1 git -C daemon_child -c protocol.version=1 \
> > + fetch 2>log &&
> > +
> > + git -C daemon_child log -1 --format=%s FETCH_HEAD >actual &&
> > + git -C "$daemon_parent" log -1 --format=%s >expect &&
> > + test_cmp expect actual &&
>
> OK. So the origin repository gained one commit on the 'master'
> branch (and a tag 'two'). By fetching, but not pulling, our
> 'master' would not advance, and that is where check on FETCH_HEAD
> comes from. I suspect that the tag 'two' is also auto-followed with
> this operation and would be in FETCH_HEAD; is that something we want
> to check? Alternatively, the "actual" log may want to see what the
> remote tracking branch for their 'master' has---then we do not have
> to worry about "FETCH_HEAD has two refs---which one are we checking?"
Yeah I can do that instead if you would prefer.
>
> > +
> > + # Client requested to use protocol v1
> > + grep "version=1" log &&
> > + # Server responded using protocol v1
> > + grep "fetch< version 1" log
> > +'
>
> Same "version=1" vs "fetch< version 1" strangeness appears here.
>
> > +test_expect_success 'pull with git:// using protocol v1' '
> > + GIT_TRACE_PACKET=1 git -C daemon_child -c protocol.version=1 \
> > + pull 2>log &&
> > +
> > + git -C daemon_child log -1 --format=%s >actual &&
> > + git -C "$daemon_parent" log -1 --format=%s >expect &&
> > + test_cmp expect actual &&
>
> Here we can check our 'master', as we pulled their 'master' into it.
> What is this testing, though? The fact that protocol.version=1
> given via "git -c var=val" mechanism is propagated to the underlying
> fetch?
Yeah, i guess we could realistically drop either the fetch or pull test
as they essentially do the same thing. I was just being overly
cautious.
>
> > + # Client requested to use protocol v1
> > + grep "version=1" log &&
> > + # Server responded using protocol v1
> > + grep "fetch< version 1" log
> > +'
> > +
> > +test_expect_success 'push with git:// using protocol v1' '
> > + test_commit -C daemon_child three &&
> > +
> > + # Since the repository being served isnt bare we need to push to
> > + # another branch explicitly to avoid mangling the master branch
>
> The other end avoids mangling the master just fine without us doing
> anything special ;-). You are pushing to another branch because you
> cannot push into a branch that is currently checked out.
>
> # Push to another branch, as the target repository has the
> # master branch checked out and we cannot push into it.
Sounds good I'll change that.
>
> perhaps?
>
> The tests for file:// looked identical, so the same set of comments
> apply.
>
> > +# Test protocol v1 with 'ssh://' transport
> > +#
> > +test_expect_success 'setup ssh wrapper' '
> > + GIT_SSH="$GIT_BUILD_DIR/t/helper/test-fake-ssh$X" &&
> > + export GIT_SSH &&
> > + export TRASH_DIRECTORY &&
> > + >"$TRASH_DIRECTORY"/ssh-output
> > +'
> > +
> > +expect_ssh () {
> > + test_when_finished '(cd "$TRASH_DIRECTORY" && rm -f ssh-expect && >ssh-output)' &&
> > + echo "ssh: -o SendEnv=GIT_PROTOCOL myhost $1 '$PWD/ssh_parent'" >"$TRASH_DIRECTORY/ssh-expect" &&
> > + (cd "$TRASH_DIRECTORY" && test_cmp ssh-expect ssh-output)
> > +}
> > +
> > +test_expect_success 'create repo to be served by ssh:// transport' '
> > + git init ssh_parent &&
> > + test_commit -C ssh_parent one
> > +'
> > +
> > +test_expect_success 'clone with ssh:// using protocol v1' '
> > + GIT_TRACE_PACKET=1 git -c protocol.version=1 \
> > + clone "ssh://myhost:$(pwd)/ssh_parent" ssh_child 2>log &&
>
> Hmm, this is a fun one, as we deliberately make $(pwd) to have
> whitespace in the test setup. I am impressed/kinda surprised that
> this works ;-)
>
> Other than that, these also look more or less identical to file://
> and git:// tests, so the same set of comments apply.
>
> Overall very nicely done.
Thanks! :D
>
> Thanks.
--
Brandon Williams
