Re: [PATCH v3 20/35] upload-pack: introduce fetch server command

[Brandon Williams]

On 02/21, Jonathan Tan wrote:
> On Tue,  6 Feb 2018 17:12:57 -0800
> Brandon Williams  wrote:
> 
> > +want 
> > +   Indicates to the server an object which the client wants to
> > +   retrieve.
> 
> Mention that the client can "want" anything even if not advertised by
> the server (like uploadpack.allowanysha1inwant).

Will do.

> 
> > +output = *section
> > +section = (acknowledgments | packfile)
> > + (flush-pkt | delim-pkt)
> > +
> > +acknowledgments = PKT-LINE("acknowledgments" LF)
> > + *(ready | nak | ack)
> 
> Can this part be described more precisely in the BNF section? I see that
> you describe later that there can be multiple ACKs or one NAK (but not
> both), and "ready" can be sent regardless of whether ACKs or a NAK is
> sent.

Yep I'll fix that.

> 
> > +ready = PKT-LINE("ready" LF)
> > +nak = PKT-LINE("NAK" LF)
> > +ack = PKT-LINE("ACK" SP obj-id LF)
> > +
> > +packfile = PKT-LINE("packfile" LF)
> > +  [PACKFILE]
> > +
> > +
> > +acknowledgments section
> > +   * Always begins with the section header "acknowledgments"
> > +
> > +   * The server will respond with "NAK" if none of the object ids sent
> > + as have lines were common.
> > +
> > +   * The server will respond with "ACK obj-id" for all of the
> > + object ids sent as have lines which are common.
> > +
> > +   * A response cannot have both "ACK" lines as well as a "NAK"
> > + line.
> > +
> > +   * The server will respond with a "ready" line indicating that
> > + the server has found an acceptable common base and is ready to
> > + make and send a packfile (which will be found in the packfile
> > + section of the same response)
> > +
> > +   * If the client determines that it is finished with negotiations
> > + by sending a "done" line, the acknowledgments sections can be
> > + omitted from the server's response as an optimization.
> 
> Should this be changed to "must"? The current implementation does not
> support it (on the client side).

This is actually a great question and one which may need to be thought
about in terms of its application to future extensions to the fetch
command.  Since fetch's response is now broken up into sections we may
want the client to cope with sections being in any order and maybe even
skipping sections it doesn't know about.  Not sure if its necessary but
its an idea.

> 
> > +#define UPLOAD_PACK_DATA_INIT { OBJECT_ARRAY_INIT, OID_ARRAY_INIT, 0, 0, 
> > 0, 0, 0, 0 }
> 
> Optional: the trailing zeroes can be omitted. (That's shorter, and also
> easier to maintain when we add new fields.)
> 
> > +int upload_pack_v2(struct repository *r, struct argv_array *keys,
> > +  struct argv_array *args)
> > +{
> > +   enum fetch_state state = FETCH_PROCESS_ARGS;
> > +   struct upload_pack_data data = UPLOAD_PACK_DATA_INIT;
> > +   use_sideband = LARGE_PACKET_MAX;
> > +
> > +   while (state != FETCH_DONE) {
> > +   switch (state) {
> > +   case FETCH_PROCESS_ARGS:
> > +   process_args(args, );
> > +
> > +   if (!want_obj.nr) {
> > +   /*
> > +* Request didn't contain any 'want' lines,
> > +* guess they didn't want anything.
> > +*/
> > +   state = FETCH_DONE;
> > +   } else if (data.haves.nr) {
> > +   /*
> > +* Request had 'have' lines, so lets ACK them.
> > +*/
> > +   state = FETCH_SEND_ACKS;
> > +   } else {
> > +   /*
> > +* Request had 'want's but no 'have's so we can
> > +* immedietly go to construct and send a pack.
> > +*/
> > +   state = FETCH_SEND_PACK;
> > +   }
> > +   break;
> > +   case FETCH_READ_HAVES:
> > +   read_haves();
> > +   state = FETCH_SEND_ACKS;
> > +   break;
> 
> This branch seems to never be taken?

Must be left over from another version, I'll remove it.

> 
> > +   case FETCH_SEND_ACKS:
> > +   if (process_haves_and_send_acks())
> > +   state = FETCH_SEND_PACK;
> > +   else
> > +   state = FETCH_DONE;
> > +   break;
> > +   case FETCH_SEND_PACK:
> > +   packet_write_fmt(1, "packfile\n");
> > +   create_pack_file();
> > +   state = FETCH_DONE;
> > +   break;
> > +   case FETCH_DONE:
> > +   continue;
> > +   }
> > +   }
> > +
> > +   upload_pack_data_clear();
> > +   return 0;
> > +}

-- 
Brandon Williams

Re: [PATCH v3 20/35] upload-pack: introduce fetch server command

[Jonathan Tan]

On Tue,  6 Feb 2018 17:12:57 -0800
Brandon Williams  wrote:

> +want 
> + Indicates to the server an object which the client wants to
> + retrieve.

Mention that the client can "want" anything even if not advertised by
the server (like uploadpack.allowanysha1inwant).

> +output = *section
> +section = (acknowledgments | packfile)
> +   (flush-pkt | delim-pkt)
> +
> +acknowledgments = PKT-LINE("acknowledgments" LF)
> +   *(ready | nak | ack)

Can this part be described more precisely in the BNF section? I see that
you describe later that there can be multiple ACKs or one NAK (but not
both), and "ready" can be sent regardless of whether ACKs or a NAK is
sent.

> +ready = PKT-LINE("ready" LF)
> +nak = PKT-LINE("NAK" LF)
> +ack = PKT-LINE("ACK" SP obj-id LF)
> +
> +packfile = PKT-LINE("packfile" LF)
> +[PACKFILE]
> +
> +
> +acknowledgments section
> + * Always begins with the section header "acknowledgments"
> +
> + * The server will respond with "NAK" if none of the object ids sent
> +   as have lines were common.
> +
> + * The server will respond with "ACK obj-id" for all of the
> +   object ids sent as have lines which are common.
> +
> + * A response cannot have both "ACK" lines as well as a "NAK"
> +   line.
> +
> + * The server will respond with a "ready" line indicating that
> +   the server has found an acceptable common base and is ready to
> +   make and send a packfile (which will be found in the packfile
> +   section of the same response)
> +
> + * If the client determines that it is finished with negotiations
> +   by sending a "done" line, the acknowledgments sections can be
> +   omitted from the server's response as an optimization.

Should this be changed to "must"? The current implementation does not
support it (on the client side).

> +#define UPLOAD_PACK_DATA_INIT { OBJECT_ARRAY_INIT, OID_ARRAY_INIT, 0, 0, 0, 
> 0, 0, 0 }

Optional: the trailing zeroes can be omitted. (That's shorter, and also
easier to maintain when we add new fields.)

> +int upload_pack_v2(struct repository *r, struct argv_array *keys,
> +struct argv_array *args)
> +{
> + enum fetch_state state = FETCH_PROCESS_ARGS;
> + struct upload_pack_data data = UPLOAD_PACK_DATA_INIT;
> + use_sideband = LARGE_PACKET_MAX;
> +
> + while (state != FETCH_DONE) {
> + switch (state) {
> + case FETCH_PROCESS_ARGS:
> + process_args(args, );
> +
> + if (!want_obj.nr) {
> + /*
> +  * Request didn't contain any 'want' lines,
> +  * guess they didn't want anything.
> +  */
> + state = FETCH_DONE;
> + } else if (data.haves.nr) {
> + /*
> +  * Request had 'have' lines, so lets ACK them.
> +  */
> + state = FETCH_SEND_ACKS;
> + } else {
> + /*
> +  * Request had 'want's but no 'have's so we can
> +  * immedietly go to construct and send a pack.
> +  */
> + state = FETCH_SEND_PACK;
> + }
> + break;
> + case FETCH_READ_HAVES:
> + read_haves();
> + state = FETCH_SEND_ACKS;
> + break;

This branch seems to never be taken?

> + case FETCH_SEND_ACKS:
> + if (process_haves_and_send_acks())
> + state = FETCH_SEND_PACK;
> + else
> + state = FETCH_DONE;
> + break;
> + case FETCH_SEND_PACK:
> + packet_write_fmt(1, "packfile\n");
> + create_pack_file();
> + state = FETCH_DONE;
> + break;
> + case FETCH_DONE:
> + continue;
> + }
> + }
> +
> + upload_pack_data_clear();
> + return 0;
> +}