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

Reply via email to