Re: [PATCH v2 8/9] fetch-pack: support more than one pack lockfile
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 11, 2020, 01:41 UTC
- Message-ID
- <xmqqk10ecrb4.fsf@gitster.c.googlers.com>
- In-Reply-To
- <566419ae74348ad3f7b1e5d484cf986fea29af73.1591821067.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
Show 29 quoted lines
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index 4771100072..f66891b010 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -48,8 +48,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
> struct ref **sought = NULL;
> int nr_sought = 0, alloc_sought = 0;
> int fd[2];
> - char *pack_lockfile = NULL;
> - char **pack_lockfile_ptr = NULL;
> + struct string_list pack_lockfiles = STRING_LIST_INIT_DUP;
> + struct string_list *pack_lockfiles_ptr = NULL;
> struct child_process *conn;
> struct fetch_pack_args args;
> struct oid_array shallow = OID_ARRAY_INIT;
> @@ -134,7 +134,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
> }
> if (!strcmp("--lock-pack", arg)) {
> args.lock_pack = 1;
> + pack_lockfiles_ptr = &pack_lockfiles;
> continue;
> }
> if (!strcmp("--check-self-contained-and-connected", arg)) {
> @@ -235,10 +235,15 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
> }
>
> ref = fetch_pack(&args, fd, ref, sought, nr_sought,
> + &shallow, pack_lockfiles_ptr, version);
> + if (pack_lockfiles.nr) {In other parts of this patch, "is the pack_lockfiles pointer NULL?" is used and "does the pack_lockfiles actually have any element?" is not checked, which was a bit hard to reason about, but the idea here is that the presence of the list tells the callee to use lockfiles and accumulate them in the string list and then this caller checks if we ended up locking any, which makes sense.
Show 21 quoted lines
> + int i;
> +
> + printf("lock %s\n", pack_lockfiles.items[0].string);
> fflush(stdout);
> + for (i = 1; i < pack_lockfiles.nr; i++)
> + warning(_("Lockfile created but not reported: %s"),
> + pack_lockfiles.items[i].string);
> }
> if (args.check_self_contained_and_connected &&
> args.self_contained_and_connected) {
> diff --git a/connected.c b/connected.c
> index 3135b71e19..937b4bae38 100644
> --- a/connected.c
> +++ b/connected.c
> @@ -43,10 +43,12 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
>
> if (transport && transport->smart_options &&
> transport->smart_options->self_contained_and_connected &&
> - transport->pack_lockfile &&
> - strip_suffix(transport->pack_lockfile, ".keep", &base_len)) {
> + transport->pack_lockfiles.nr == 1 &&Hmph, I can understand why "== 1" case must behave differently from "== 0" case, but shouldn't the behavior in "> 1" cases be more similar to the "== 1" case than "== 0" case?
Show 9 quoted lines
> + strip_suffix(transport->pack_lockfiles.items[0].string,
> + ".keep", &base_len)) {
> struct strbuf idx_file = STRBUF_INIT;
> - strbuf_add(&idx_file, transport->pack_lockfile, base_len);
> + strbuf_add(&idx_file, transport->pack_lockfiles.items[0].string,
> + base_len);
> strbuf_addstr(&idx_file, ".idx");
> new_pack = add_packed_git(idx_file.buf, idx_file.len, 1);
> strbuf_release(&idx_file);