Re: [PATCH v2 2/2] connect: advertized capability is not a ref

Jonathan Tan [off-list ref] writes:

Git advertises the same capabilities^{} ref in its ref advertisement for push
but since it never remembered to do so for fetch, the client forgot to handle
this case. Handle it.
...
In this aspect, JGit is compliant with the specification in pack-protocol.txt.

I agree with Peff that the above explanation distorts the history.
It should be reworded.

I do not have an issue with being more lenient to what JGit servers
have been doing for a few years, though.

Because git client versions without this fix are expected to exist in the wild
for a while, we should not change the server to always send the capabilities^{}
line when there are no refs to advertise yet.  A transition will take multiple
steps:

 1. This patch, which updates the client

 2. Update pack-protocol to clarify that both server behaviors must be
    tolerated.

 3. Add a configuration variable to allow git upload-pack to advertise
    capabilities when there are no refs to advertise.  Leave it disabled
    by default since git clients can't be counted on to have this patch (1)
    yet.

 4. After a year or so, flip the default for that server configuration
    variable to true.

The above assumes it is an unconditionally good thing to send
capabilities^{}; I do not think we established that in this
discussion, and more importantly, this client-side change is a good
thing to do regardless of the outcome of the discussion.

I'd suggest dropping everything below "Because Git client versions
without...".

Shouldn't this be inside the next expect_success?

+# This test spawns a daemon, so run it only if the user would be OK with
+# testing with git-daemon.
+test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '
+	JGIT_DAEMON_PID= &&
+	git init --bare empty.git &&
+	touch empty.git/git-daemon-export-ok &&

To make it clear that the existence of the file is the thing you
care the most, not that the file having a recent timestamp:

	>empty.git/git-daemon-export-ok &&

+	mkfifo jgit_daemon_output &&
+	{
+		jgit daemon --port="$JGIT_DAEMON_PORT" . >jgit_daemon_output &
+		JGIT_DAEMON_PID=$!
+	} &&
+	test_when_finished kill "$JGIT_DAEMON_PID" &&
+	{
+		read line &&
+		case $line in
+		Exporting*)
+			;;
+		*)
+			echo "Expected: Exporting" &&
+			false;;
+		esac &&
+		read line &&
+		case $line in
+		"Listening on"*)
+			;;
+		*)
+			echo "Expected: Listening on" &&
+			false;;
+		esac

OK, so this is a nice way to wait until the daemon becomes ready to
serve.

+	} <jgit_daemon_output &&

An in-code comment that explains what the significance of "2" here

	# --exit-code asks the command to exit with 2 when no
        # matching refs are found.

would be nice.

+	test_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git
+'

Thanks.