Re: [PATCH 2/5] Document details of transport function APIs

From: Daniel Barkalow <barkalow@iabervon.org>
Date: Mar 25, 2009, 16:19 UTC
Message-ID: <alpine.LNX.1.00.0903251142470.19665@iabervon.org>
In-Reply-To: <7v63hy9k0l.fsf@gitster.siamese.dyndns.org>

On Tue, 24 Mar 2009, Junio C Hamano wrote:

Show 27 quoted lines

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > In particular, explain which of the fields of struct ref is used for
> > what purpose in the input to and output from each function.
> >
> > Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
> > ---
> >  transport.h |   38 ++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 38 insertions(+), 0 deletions(-)
> >
> > diff --git a/transport.h b/transport.h
> > index 489e96a..2e1650a 100644
> > --- a/transport.h
> > +++ b/transport.h
> > @@ -18,11 +18,49 @@ struct transport {
> >  	int (*set_option)(struct transport *connection, const char *name,
> >  			  const char *value);
> >  
> > +	/**
> > +	 * Returns a list of the remote side's refs. In order to allow
> > +	 * the transport to try to share connections, for_push is a
> > +	 * hint as to whether the ultimate operation is a push or a fetch.
> > +	 *
> 
> It is unclear what this "hint" is intended to be used for, what the
> transport is and isn't allowed to use it for, and what existing transports
> typically use it for.

I don't think there's anything the transport isn't allowed to use the hint for (of course, it shouldn't go and do things that haven't been requested at all yet, but that's true with or without a hint). It's intended to be used for things like the git native protocol, where the client side has to declare up front whether it wants to talk to receive-pack or upload-pack, and it would have to disconnect and reconnect to get the other one.

It's probably also legitimate for the http transport to guess that, if "for_push" is true, that it's worth trying to list directories with webdav, although perhaps it should try both in any case.

Show 19 quoted lines

> > +	/**
> > +	 * Push the objects and refs. Send the necessary objects, and
> > +	 * then tell the remote side to update each ref in the list
> > +	 * from old_sha1 to new_sha1.
> > +	 *
> > +	 * Where possible, set the status for each ref appropriately.
> > +	 *
> > +	 * If, in the process, the transport determines that the
> > +	 * remote side actually responded to the push by updating the
> > +	 * ref to a different value, the transport should modify the
> > +	 * new_sha1 in the ref. (Note that this is a matter of the
> > +	 * remote accepting but rewriting the change, not rejecting it
> > +	 * and reporting that a different update had already taken
> > +	 * place)
> > +	 **/
> 
> It this even a sane thing to allow?  How would it interact with the
> "pretend we immediately turned around and fetched them into the remote
> tracking branches" local updates we usually do?

We already allow a git server to rewrite refs with a hook when it gets them, and we record the pre-rewriting value. This allows the transport to propagate the post-rewriting value back (if it can get it), and we'd update the tracking branches with what the server actually did instead of what we asked it to (i.e., we do what we would do if we really did turn around and fetch them immediately).

Of course, I primarily want this for the case where it's a foreign system and we don't get to control everything in the foreign history, even when it is accepted (e.g., it applies its own timestamp and attributes it to a different username), and we want to avoid simply losing the information as to what became of the commit.

Show 23 quoted lines

> >  	int (*push_refs)(struct transport *transport, struct ref *refs, int flags);
> >  	int (*push)(struct transport *connection, int refspec_nr, const char **refspec, int flags);
> >  
> > +	/** get_refs_list(), fetch(), and push_refs() can keep
> > +	 * resources (such as a connection) reserved for futher
> > +	 * use. disconnect() releases these resources.
> > +	 **/
> >  	int (*disconnect)(struct transport *connection);
> >  	char *pack_lockfile;
> >  	signed verbose : 2;
> 
> It is just a style thing, but all of our multi-line comments are
> 
>    /*
>     * of
>     * this
>     * form
>     */
> 
> and these new comments are formatted slightly differently with double
> asterisks on only the first and the last lines.  In addition, th last
> comment block uses a yet another different style, which is a bit of
> eyesore.

Ah, yes, I'll correct that.

	-Daniel
*This .sig left intentionally blank*