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

[Junio C Hamano <gitster@pobox.com>]

Jonathan Tan <jonathantanmy@google.com> 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...".

> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> index 819b9dd..2de52f5 100755
> --- a/t/t5512-ls-remote.sh
> +++ b/t/t5512-ls-remote.sh
> @@ -207,5 +207,44 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '
>  	test_cmp expect actual
>  '
>  
> +test_lazy_prereq GIT_DAEMON '
> +	test_tristate GIT_TEST_GIT_DAEMON &&
> +	test "$GIT_TEST_GIT_DAEMON" != false
> +'
> +
> +JGIT_DAEMON_PORT=${JGIT_DAEMON_PORT-${this_test#t}}

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.