From: Derrick Stolee <stolee@gmail.com> To: Jonathan Tan <jonathantanmy@google.com>, git@vger.kernel.org Subject: Re: [PATCH 6/6] send-pack: support push negotiation Date: Mon, 3 May 2021 11:35:02 -0400 [thread overview] Message-ID: <c2780570-81ec-bede-2f4e-75748b788bbb@gmail.com> (raw) In-Reply-To: <a2daa1022c41820b2109d9572069d12684470cb8.1617929278.git.jonathantanmy@google.com> On 4/8/21 9:10 PM, Jonathan Tan wrote: > Teach Git the push.negotiate config variable. ... > +push.negotiate:: > + If set to "true", attempt to reduce the size of the packfile > + sent by rounds of negotiation in which the client and the > + server attempt to find commits in common. If "false", Git will > + rely solely on the server's ref advertisement to find commits > + in common. Works for me. > diff --git a/send-pack.c b/send-pack.c > index 5f215b13c7..9cb9f71650 100644 > --- a/send-pack.c > +++ b/send-pack.c > @@ -56,7 +56,9 @@ static void feed_object(const struct object_id *oid, FILE *fh, int negative) > /* > * Make a pack stream and spit it out into file descriptor fd > */ > -static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struct send_pack_args *args) > +static int pack_objects(int fd, struct ref *refs, struct oid_array *advertised, > + struct oid_array *negotiated, > + struct send_pack_args *args) At the moment, I don't see why we need two oid_arrays here. Instead, this 'extra' could instead be renamed to something like 'server_objects' or 'base_objects' to make it clear that we don't want those objects, and can even use them and their reachable objects as delta bases, when appropriate. Or, just don't touch it. > +static void get_commons_through_negotiation(const char *url, > + const struct ref *remote_refs, > + struct oid_array *commons) > +{ > + struct child_process child = CHILD_PROCESS_INIT; > + const struct ref *ref; > + int len = the_hash_algo->hexsz + 1; /* hash + NL */ > + > + child.git_cmd = 1; > + child.no_stdin = 1; > + child.out = -1; > + strvec_pushl(&child.args, "fetch", "--negotiate-only", NULL); > + for (ref = remote_refs; ref; ref = ref->next) > + strvec_pushf(&child.args, "--negotiation-tip=%s", oid_to_hex(&ref->new_oid)); > + strvec_push(&child.args, url); Oh! We are using a 'git fetch --negotiate-only' subprocess here. You can ignore my previous message about updating the docs for this to be used only by tests. > + > + if (start_command(&child)) > + die(_("send-pack: unable to fork off fetch subprocess")); > + > + do { > + char hex_hash[GIT_MAX_HEXSZ + 1]; > + int read_len = read_in_full(child.out, hex_hash, len); > + struct object_id oid; > + const char *end; > + > + if (!read_len) > + break; > + if (read_len != len) > + die("invalid length read %d", read_len); > + if (parse_oid_hex(hex_hash, &oid, &end) || *end != '\n') > + die("invalid hash"); > + oid_array_append(commons, &oid); This appends, so there is no reason why it needs to be empty before the method. Is there a way we could feed the extra_have set when calling this method? Or is it happening at a strange time? > + } while (1); > + > + if (finish_command(&child)) { > + /* > + * The information that push negotiation provides is useful but > + * not mandatory. > + */ > + warning(_("push negotiation failed; proceeding anyway with push")); > + } > +} > + > int send_pack(struct send_pack_args *args, > int fd[], struct child_process *conn, > struct ref *remote_refs, > struct oid_array *extra_have) > { > + struct oid_array commons = OID_ARRAY_INIT; > int in = fd[0]; > int out = fd[1]; > struct strbuf req_buf = STRBUF_INIT; > @@ -426,6 +474,7 @@ int send_pack(struct send_pack_args *args, > int quiet_supported = 0; > int agent_supported = 0; > int advertise_sid = 0; > + int push_negotiate = 0; > int use_atomic = 0; > int atomic_supported = 0; > int use_push_options = 0; > @@ -437,6 +486,10 @@ int send_pack(struct send_pack_args *args, > const char *push_cert_nonce = NULL; > struct packet_reader reader; > > + git_config_get_bool("push.negotiate", &push_negotiate); > + if (push_negotiate) > + get_commons_through_negotiation(args->url, remote_refs, &commons); > + > git_config_get_bool("transfer.advertisesid", &advertise_sid); > > /* Does the other end support the reporting? */ > @@ -625,7 +678,7 @@ int send_pack(struct send_pack_args *args, > PACKET_READ_DIE_ON_ERR_PACKET); > > if (need_pack_data && cmds_sent) { > - if (pack_objects(out, remote_refs, extra_have, args) < 0) { > + if (pack_objects(out, remote_refs, extra_have, &commons, args) < 0) { Here, it would be nice if extra_have and commons were merged before calling pack_objects(). I mentioned a way to perhaps make that easier above, but the context might not make that be super-simple. Running a loop here to scan 'commons' and append them to 'extra_have' might be a sufficient approach. Generally, this approach seems like it would work. I have not done any local testing, yet. Thanks, -Stolee
next prev parent reply other threads:[~2021-05-03 15:35 UTC|newest] Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-09 1:09 [PATCH 0/6] Push negotiation Jonathan Tan 2021-04-09 1:09 ` [PATCH 1/6] fetch-pack: buffer object-format with other args Jonathan Tan 2021-04-09 4:49 ` Junio C Hamano 2021-04-09 16:24 ` Jonathan Tan 2021-04-09 1:09 ` [PATCH 2/6] fetch-pack: refactor process_acks() Jonathan Tan 2021-04-09 5:08 ` Junio C Hamano 2021-05-03 16:30 ` Derrick Stolee 2021-04-09 1:10 ` [PATCH 3/6] fetch-pack: refactor add_haves() Jonathan Tan 2021-04-09 5:20 ` Junio C Hamano 2021-04-09 1:10 ` [PATCH 4/6] fetch-pack: refactor command and capability write Jonathan Tan 2021-04-09 5:27 ` Junio C Hamano 2021-04-09 1:10 ` [PATCH 5/6] fetch: teach independent negotiation (no packfile) Jonathan Tan 2021-04-09 5:41 ` Junio C Hamano 2021-04-09 16:38 ` Jonathan Tan 2021-05-03 15:25 ` Derrick Stolee 2021-05-03 15:40 ` Derrick Stolee 2021-05-03 21:52 ` Jonathan Tan 2021-04-09 1:10 ` [PATCH 6/6] send-pack: support push negotiation Jonathan Tan 2021-05-03 15:35 ` Derrick Stolee [this message] 2021-05-03 22:02 ` Jonathan Tan 2021-05-04 17:26 ` Derrick Stolee 2021-04-30 5:42 ` [PATCH 0/6] Push negotiation Junio C Hamano 2021-04-30 17:33 ` Derrick Stolee 2021-05-04 21:15 ` [PATCH v2 0/5] " Jonathan Tan 2021-05-04 21:15 ` [PATCH v2 1/5] fetch-pack: refactor process_acks() Jonathan Tan 2021-05-04 21:15 ` [PATCH v2 2/5] fetch-pack: refactor add_haves() Jonathan Tan 2021-05-04 21:16 ` [PATCH v2 3/5] fetch-pack: refactor command and capability write Jonathan Tan 2021-05-04 21:16 ` [PATCH v2 4/5] fetch: teach independent negotiation (no packfile) Jonathan Tan 2021-05-05 1:53 ` Junio C Hamano 2021-05-05 16:42 ` Derrick Stolee 2021-05-06 2:12 ` Junio C Hamano 2021-05-05 16:44 ` Derrick Stolee 2021-05-04 21:16 ` [PATCH v2 5/5] send-pack: support push negotiation Jonathan Tan
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=c2780570-81ec-bede-2f4e-75748b788bbb@gmail.com \ --to=stolee@gmail.com \ --cc=git@vger.kernel.org \ --cc=jonathantanmy@google.com \ --subject='Re: [PATCH 6/6] send-pack: support push negotiation' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).