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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 7, 2016, 17:02 UTC
Message-ID: <xmqqshtbod6y.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <174c8ca6638f1cd3145a628925e65655b56af366.1472853827.git.jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 5 quoted lines

> 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.

Show 17 quoted lines

> 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...".

Show 14 quoted lines

> 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?

Show 6 quoted lines

> +# 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 &&

Show 23 quoted lines

> +	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.