Jonathan Tan <jonathantanmy@xxxxxxxxxx> 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.