Re: [PATCH/RFC] remote-helper: allow fetch to discover sha1 later

From: Devin J. Pohly <djpohly@gmail.com>
Date: Sep 14, 2012, 20:49 UTC
Message-ID: <20120914204912.GG1072@chiri.hsd1.pa.comcast.net>
In-Reply-To: <7vmx0t6sod.fsf@alter.siamese.dyndns.org>

On Thu, Sep 13, 2012 at 11:10:26PM -0700, Junio C Hamano wrote:

Show 6 quoted lines

> I do not think it is a good idea to allow such a helper to claim that
> it supports "fetch" capability, for at least two reasons:
> 
>  * Being able to "list" is essential for "fetch" based helpers.
>    think, far from "arbitrary".
>    ...

Oh, I don't mean to change the semantics of the list command at all. What I thought seemed arbitrary was limiting the *fetch* command to refs with pre-known sha1s. Any list-time optimization in place or possible for such refs wouldn't be affected.

(In light of this, specifying a new sha1 for a ref which already had one in the list should probably be an error rather than a warning. I'll update this in the next version and make it clear that a "ref" message must only be issued for refs reported without a sha1.)

Show 6 quoted lines

>  * Existing versions of "git" that can drive remote helpers that
>    claim to have "fetch" capability are not prepared to accept an
>    unknown "refs" protocol message from the helper, so a helper
>    written for your new semantics of "fetch" capability will not
>    work with them.
>    ...

I see. I didn't realize that new helper + old Git client is a supported combination. Still, hear me out.

1. A new helper must only send a "ref" message when git asks for a ref
   without a known sha1.
2. Asking for a ref without a known sha1 is unsupported, according to
   git-remote-helpers.txt: "Only objects which were reported in the ref
   list with a sha1 may be fetched [with fetch]."
3. Furthermore, asking for a ref without a known sha1 *already* breaks
   in existing versions of git with current handlers:

    $ git fetch testfetch::../git1 foo
    fatal: bad object 0000000000000000000000000000000000000000
    error: testfetch::../git1 did not send all necessary objects

   Compare - existing version of git, with a new handler:

    $ git fetch testref::../git1 foo
    warning: testref unexpectedly said: 'ref 0f31<snip> refs/heads/foo'
    fatal: bad object 0000000000000000000000000000000000000000
    error: testref::../git1 did not send all necessary objects

So the proposed change doesn't break anything that isn't already broken. :)

That said, if you're still not sold, a new capability is fine. Though I think it can be done without it, I'm certainly not opposed to adding one.