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

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

[Daniel Barkalow]

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.
+	 *
+	 * If the transport is able to determine the remote hash for
+	 * the ref without a huge amount of effort, it should store it
+	 * in the ref's old_sha1 field; otherwise it should be all 0.
+	 **/
 	struct ref *(*get_refs_list)(struct transport *transport, int for_push);
+
+	/**
+	 * Fetch the objects for the given refs. Note that this gets
+	 * an array, and should ignore the list structure.
+	 *
+	 * If the transport did not get hashes for refs in
+	 * get_refs_list(), it should set the old_sha1 fields in the
+	 * provided refs now.
+	 **/
 	int (*fetch)(struct transport *transport, int refs_nr, struct ref **refs);
+
+	/**
+	 * 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)
+	 **/
 	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;
-- 
1.6.2.1.476.g9bf04b

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

[Junio C Hamano]

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.

> +	/**
> +	 * 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?

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

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

[Daniel Barkalow]

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

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

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

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

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

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

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

But how are you guaranteeing that objects necessary to complete the
history the remote end re-written are already available on the local end?
Do you have a reverse object transfer phase now in send-pack protocol?

Otherwise I am afraid that you are corrupting the local repository.

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

[Daniel Barkalow]

On Wed, 25 Mar 2009, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> >> > +	 * 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).
> 
> But how are you guaranteeing that objects necessary to complete the
> history the remote end re-written are already available on the local end?
> Do you have a reverse object transfer phase now in send-pack protocol?

The current send-pack protocol wouldn't be able to use this feature (I 
don't think it can even report the replacement hashes, so there's little 
danger of that).

The foreign VCS transport would actually reimport the objects (in the 
process of discovering the hash), so that's fine.

> Otherwise I am afraid that you are corrupting the local repository.

It wouldn't be any different from fetch() reporting success when it hadn't 
actually succeeded, but I think it would be worth being explicit here that 
you can't simply report the other end's hash if you may not have it.

	-Daniel
*This .sig left intentionally blank*