On Tue, Feb 6, 2018 at 5:12 PM, Brandon Williams <bmwill@xxxxxxxxxx> wrote:
> When communicating with a v2 server, perform a fetch by requesting the
> 'fetch' command.
>
> Signed-off-by: Brandon Williams <bmwill@xxxxxxxxxx>
> ---
> builtin/fetch-pack.c | 2 +-
> fetch-pack.c | 252 ++++++++++++++++++++++++++++++++++++++++++++++++-
> fetch-pack.h | 4 +-
> t/t5702-protocol-v2.sh | 84 +++++++++++++++++
> transport.c | 7 +-
> 5 files changed, 342 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index f492e8abd..867dd3cc7 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -213,7 +213,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
> }
>
> ref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,
> - &shallow, pack_lockfile_ptr);
> + &shallow, pack_lockfile_ptr, protocol_v0);
> if (pack_lockfile) {
> printf("lock %s\n", pack_lockfile);
> fflush(stdout);
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 9f6b07ad9..4fb5805dd 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1008,6 +1008,247 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
> return ref;
> }
>
> +static void add_wants(const struct ref *wants, struct strbuf *req_buf)
> +{
> + for ( ; wants ; wants = wants->next) {
> + const struct object_id *remote = &wants->old_oid;
> + const char *remote_hex;
> + struct object *o;
> +
> + /*
> + * If that object is complete (i.e. it is an ancestor of a
> + * local ref), we tell them we have it but do not have to
> + * tell them about its ancestors, which they already know
> + * about.
> + *
> + * We use lookup_object here because we are only
> + * interested in the case we *know* the object is
> + * reachable and we have already scanned it.
> + */
> + if (((o = lookup_object(remote->hash)) != NULL) &&
> + (o->flags & COMPLETE)) {
> + continue;
> + }
> +
> + remote_hex = oid_to_hex(remote);
> + packet_buf_write(req_buf, "want %s\n", remote_hex);
> + }
> +}
> +
> +static void add_common(struct strbuf *req_buf, struct oidset *common)
> +{
> + struct oidset_iter iter;
> + const struct object_id *oid;
> + oidset_iter_init(common, &iter);
> +
> + while ((oid = oidset_iter_next(&iter))) {
> + packet_buf_write(req_buf, "have %s\n", oid_to_hex(oid));
> + }
> +}
> +
> +static int add_haves(struct strbuf *req_buf, int *in_vain)
> +{
> + int ret = 0;
> + int haves_added = 0;
> + const struct object_id *oid;
> +
> + while ((oid = get_rev())) {
> + packet_buf_write(req_buf, "have %s\n", oid_to_hex(oid));
> + if (++haves_added >= INITIAL_FLUSH)
> + break;
> + };
> +
> + *in_vain += haves_added;
> + if (!haves_added || *in_vain >= MAX_IN_VAIN) {
> + /* Send Done */
> + packet_buf_write(req_buf, "done\n");
> + ret = 1;
> + }
> +
> + return ret;
> +}
> +
> +static int send_fetch_request(int fd_out, const struct fetch_pack_args *args,
> + const struct ref *wants, struct oidset *common,
> + int *in_vain)
> +{
> + int ret = 0;
> + struct strbuf req_buf = STRBUF_INIT;
> +
> + if (server_supports_v2("fetch", 1))
> + packet_buf_write(&req_buf, "command=fetch");
> + if (server_supports_v2("agent", 0))
> + packet_buf_write(&req_buf, "agent=%s", git_user_agent_sanitized());
> +
> + packet_buf_delim(&req_buf);
> + if (args->use_thin_pack)
> + packet_buf_write(&req_buf, "thin-pack");
> + if (args->no_progress)
> + packet_buf_write(&req_buf, "no-progress");
> + if (args->include_tag)
> + packet_buf_write(&req_buf, "include-tag");
> + if (prefer_ofs_delta)
> + packet_buf_write(&req_buf, "ofs-delta");
> +
> + /* add wants */
> + add_wants(wants, &req_buf);
The comment might convey too much redundant information
instead of helping the reader gain more understanding. ;)
> +
> + /* Add all of the common commits we've found in previous rounds */
> + add_common(&req_buf, common);
nit:
Maybe s/add_common/add_common_haves/ or
add_previous_haves ?
> +
> + /* Add initial haves */
> + ret = add_haves(&req_buf, in_vain);
I like the shortness and conciseness of this send_fetch_request
function as it makes clear what is happening over the wire, however
I wonder if we can improve on that, still.
The functions 'add_common' and 'add_haves' seem like they do the
same (sending haves), except for different sets of oids.
So I would imagine that a structure like
{
struct set haves = compute_haves_from(in_vain, common, ...);
struct set wants = compute_wants&wants);
request_capabilities(args)
send_haves(&haves);
send_wants(&wants);
flush();
}
That way we would have an even more concise way of writing
one request, and factoring out the business logic. (Coming up
with the "right" haves is a heuristic that we plan on changing in
the future, so we'd want to have that encapsulated into one function
that computes all the haves?
> +
> +/*
> + * Processes a section header in a server's response and checks if it matches
> + * `section`. If the value of `peek` is 1, the header line will be peeked (and
> + * not consumed); if 0, the line will be consumed and the function will die if
> + * the section header doesn't match what was expected.
> + */
> +static int process_section_header(struct packet_reader *reader,
> + const char *section, int peek)
> +{
> + int ret;
> +
> + if (packet_reader_peek(reader) != PACKET_READ_NORMAL)
> + die("error reading packet");
> +
> + ret = !strcmp(reader->line, section);
> +
> + if (!peek) {
> + if (!ret)
> + die("expected '%s', received '%s'",
> + section, reader->line);
> + packet_reader_read(reader);
> + }
> +
> + return ret;
> +}
> +
> +static int process_acks(struct packet_reader *reader, struct oidset *common)
> +{
> + /* received */
> + int received_ready = 0;
> + int received_ack = 0;
> +
> + process_section_header(reader, "acknowledgments", 0);
> + while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
> + const char *arg;
> +
> + if (!strcmp(reader->line, "NAK"))
> + continue;
> +
> + if (skip_prefix(reader->line, "ACK ", &arg)) {
> + struct object_id oid;
> + if (!get_oid_hex(arg, &oid)) {
> + struct commit *commit;
> + oidset_insert(common, &oid);
> + commit = lookup_commit(&oid);
> + mark_common(commit, 0, 1);
> + }
> + continue;
> + }
> +
> + if (!strcmp(reader->line, "ready")) {
> + clear_prio_queue(&rev_list);
> + received_ready = 1;
> + continue;
> + }
> +
> + die(_("git fetch-pack: expected ACK/NAK, got '%s'"), reader->line);
This is slightly misleading, it could also expect "ready" ?
> + }
> +
> + if (reader->status != PACKET_READ_FLUSH &&
> + reader->status != PACKET_READ_DELIM)
> + die("Error during processing acks: %d", reader->status);
Why is this not translated unlike the one 5 lines prior to this?
Do we expect these conditions to come up due to different
root causes?
> +static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
> + int fd[2],
> + const struct ref *orig_ref,
> + struct ref **sought, int nr_sought,
> + char **pack_lockfile)
> +{
> + struct ref *ref = copy_ref_list(orig_ref);
> + enum fetch_state state = FETCH_CHECK_LOCAL;
> + struct oidset common = OIDSET_INIT;
> + struct packet_reader reader;
> + int in_vain = 0;
> + packet_reader_init(&reader, fd[0], NULL, 0,
> + PACKET_READ_CHOMP_NEWLINE);
> +
> + while (state != FETCH_DONE) {
> + switch (state) {
> + case FETCH_CHECK_LOCAL:
> + sort_ref_list(&ref, ref_compare_name);
> + QSORT(sought, nr_sought, cmp_ref_by_name);
> +
> + /* v2 supports these by default */
Is there a doc that says what is all on by default?
Thanks,
Stefan
