Re: [PATCH 6/6] send-pack: support push negotiation
- From
Derrick Stolee <stolee@gmail.com>
- Date
- May 3, 2021, 15:35 UTC
- Message-ID
- <c2780570-81ec-bede-2f4e-75748b788bbb@gmail.com>
- 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.
...
Show 6 quoted lines
> +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.
Show 12 quoted lines
> 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.
Show 15 quoted lines
> +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.
Show 17 quoted lines
> +
> + 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?
Show 45 quoted lines
> + } 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