From: Junio C Hamano <junkio@cox.net>
Date: Fri, 03 Jun 2005 17:24:14 GMT
Subject: Re: [PATCH] git-daemon server
Message-ID: <7vk6lbmk01.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20050603152212.GA4598@jmcmullan.timesys>

Looks very nice.  Some comments.

    diff -u b/daemon.c b/daemon.c
    --- b/daemon.c
    +++ b/daemon.c
------------
    +
    +/* Protocol is symmetric, both client and server
    + * use the same commands.
    + *
    + * version\n -> error -- 0 <Version string>\n
    + *
    + * head <head-id>\n -> sha1 <head-id> <sha1>\n
    + *
    + * head <head-id> <old-sha1> <new-sha1>\n -> sha1 <head-id> <new-sha1>\n
    + *
    + * request <sha1>\n -> send <sha1> <hex-bytes>\n<bytes...>
    + *
    + * send <sha1> <hex-bytes>\n<bytes...> -> sha1 -- <sha1>\n
    + *
    + * exists <sha1>\n -> sha1 -- <sha1>\n
    + *
    + * sha1 <any> <sha1>\n -> no-op
    + *
    + * error <key> <hex-code> <error string>\n -> no-op
    + *
    + */

This is good for the first cut, but I have a latency concern
about "single request - single send" style of communication.
This being a dedicated GIT specific sync mechanism, you may want
to give more smarts to the server, so that the client can say "I
have these commits as HEADs in my forest, here are their SHA1s,
now sync me up to the head you said you have whose SHA1 is
this", implying he has all their HEADs dependents.  Of course
this can come later.

------------

    +static int verify_file(int fd, unsigned long mapsize, const unsigned char *sha1, char *type)
    +{
    +	void *map, *buffer;
   ~~~ 
    +		if (buffer && !strcmp(type, "delta")) {
    +			void *ref = NULL, *delta = buffer;
    +			unsigned long ref_size, delta_size = size;
    +			buffer = NULL;
   ~~~
    +			buffer = patch_delta(ref, ref_size,
    +					  delta+20, delta_size-20, 
    +					  &size);
    +			free(delta);
    +			free(ref);
    +		}

A possibility is to chuck the above special case for "delta",
and instead introduce "delta" subclass in struct object family
and make the base object of "delta" simply one object on the
obj->refs list on such a "delta" object.  I would imagine that
this would let you reuse the sha1_retrieve() loop that you
already do for "commit", "tree", and "tag".

    +		if (!strcmp(type, "blob")) {
    +			struct blob *blob = lookup_blob(sha1);
    +			parse_blob_buffer(blob, buffer, size);
   ~~~
    +			obj = &tag->object;
    +		} else {
    +			obj = NULL;
    +		}
    +
    +		free(buffer);
    +
    +		if (obj) {
    +			struct object_list *refs;
    +
    +			for (refs = obj->refs; refs ; refs = refs->next) {
    +				err = sha1_retrieve(refs->item->sha1);
    +				if (err < 0)
    +					return err;
    +			}
    +		
    +			return 0;
    +		}
    +
    +	}
    +	return -1;
    +}

------------

    +static int send_send(const unsigned char *sha1, int size, void *data)
    +{
   ~~~
    +}
   ~~~
    +static int cmd_request(int argc, char **argv)
    +{
    +	int err;
    +
    +	if (argc == 2) {
    +		char sha1[40];
    +		void *data;
    +		unsigned long size;
   ~~~
    +		err = send_send(sha1, size, data);
    +		if (err < 0)
    +			return err;

By definition, size of SHA1 blob is "unsigned long" so
send_send() should take such not "int".