On Thu, Feb 26, 2015 at 12:13 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Duy Nguyen <pclouds@xxxxxxxxx> writes:
>
>> Step 1 then should be identifying these wrongdoings and assumptions.
>>
>> We can really go wild with these capabilities. The only thing that
>> can't be changed is perhaps sending the first ref. I don't know
>> whether we can accept a dummy first ref... After that point, you can
>> turn the protocol upside down because both client and server know what
>> it would be.
>
> Yes, exactly. To up/down/side-grade from v1 is technically
> possible, but being technically possible is different from being
> sensible. The capability-based sidegrade does not solve the problem
> when the problem to be solved is that the server side needs to spend
> a lot of cycles and the network needs to carry megabytes of data
> before capability exchange happens. Yes, the newer server and the
> newer client can notice that the counterparty is new and start
> talking in new protocol (which may or may not benefit from already
> knowing the result of ref advertisement), but by the time that
> happens, the resource has already been spent and wasted.
>
> I do not think v1 can be fixed by "send one ref with capability,
> newer client may respond immediately so we can stop enumerating
> remaining refs and older one will get stuck so we can have a timeout
> to see if the connection is from the newer one, and send the rest
> for the older client", because anything that involves such a timeout
> would not reliably work over WAN.
>
>> You realize you're advertising v2 as a new capability, right? Instead
>> of defining v2 feature set then advertise v2, people could simply add
>> new features directly. I don't see v2 (at least with these patches)
>> adds any value.
>
> I agree with the value assessment of these patches 98%, but these
> bits can be taken as the "we have v2 server availble for you on the
> side, by the way" hint you mentioned in the older thread, I think.
>
>> And we already does that, except that we don't state what version (as
>> a number) exactly, but what feature that version supports. The focus
>> should be the new protocol at daemon.c and maybe remote-curl.c where
>> we do know the current protocol is not flexible enough.
>
> The "first" thing the client tells the server is what service it
> requests. A request over git:// protocol is read by "git daemon" to
> choose which service to run, and it is read directly by the login
> shell if it comes over ssh:// protocol.
>
> There is nothing that prevents us from defining that service to be a
> generic "git service", not "upload-pack", "archive", "receive-pack".
> And the early protocol exchange, once "git service" is spawned, with
> the client can be "what real services does the server end support?"
> capability list responded with "wow, you are new enough to support
> the 'trickle-pack' service---please connect me to it" request.
>
So I am not quite sure how to understand this input.
I wonder if a high level test could look like the following,
which just tests the workflow with git fetch, but not the
internals.
(Note: patch formatting may be broken as it's send via gmail web ui)
---8<---
From: Stefan Beller <sbeller@xxxxxxxxxx>
Date: Thu, 26 Feb 2015 17:19:30 -0800
Subject: [PATCH] Propose new tests for transitioning to the new option
transport.capabilitiesfirst
Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
---
t/t5544-capability-handshake.sh | 81 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 81 insertions(+)
create mode 100755 t/t5544-capability-handshake.sh
diff --git a/t/t5544-capability-handshake.sh b/t/t5544-capability-handshake.sh
new file mode 100755
index 0000000..aa2b52d
--- /dev/null
+++ b/t/t5544-capability-handshake.sh
@@ -0,0 +1,81 @@
+#!/bin/sh
+
+test_description='fetching from a repository using the "capabilities
first" push option'
+
+. ./test-lib.sh
+
+mk_repo_pair () {
+ rm -rf workbench upstream &&
+ test_create_repo upstream &&
+ test_create_repo workbench &&
+ (
+ cd upstream &&
+ git config receive.denyCurrentBranch warn
+ ) &&
+ (
+ cd workbench &&
+ git remote add origin ../upstream
+ )
+}
+
+generate_commits_upstream () {
+ (
+ cd upstream &&
+ echo "more content" >>file &&
+ git add file &&
+ git commit -a -m "create a commit"
+ )
+}
+
+# Compare the ref ($1) in upstream with a ref value from workbench ($2)
+# i.e. test_refs second HEAD@{2}
+test_refs () {
+ test $# = 2 &&
+ git -C upstream rev-parse --verify "$1" >expect &&
+ git -C workbench rev-parse --verify "$2" >actual &&
+ test_cmp expect actual
+}
+
+test_expect_success 'transport.capabilitiesfirst is not overridden
when set already' '
+ mk_repo_pair &&
+ (
+ cd workbench &&
+ git config transport.capabilitiesfirst 0
+ git config --get transport.capabilitiesfirst 0 >expected
+ )
+ generate_commits_upstream &&
+ (
+ cd workbench &&
+ git fetch --all
+ git config --get transport.capabilitiesfirst >actual
+ test_cmp expected actual
+ )
+'
+
+test_expect_success 'enable transport by fetching from new server' '
+ mk_repo_pair &&
+ (
+ cd workbench &&
+ git fetch origin
+ ) &&
+ generate_commits_upstream &&
+ (
+ cd workbench &&
+ git config --get transport.capabilitiesfirst >actual &&
+ echo "1" >expected &&
+ test_cmp expected actual
+ )
+'
+
+test_expect_success 'test new transport still works' '
+ # continue from last test
+ generate_commits_upstream &&
+ (
+ cd workbench &&
+ git fetch origin
+ )
+ test_refs HEAD refs/remotes/origin/HEAD
+ test_refs master refs/remotes/origin/master
+'
+
+test_done
--
2.3.0.81.gc37f363
--
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