Re: [PATCH v2 7/9] connect: tell server that the client understands v1

[Brandon Williams <bmwill@google.com>]

On 09/27, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> 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