Subject: Re: [PATCH v2 8/9] fetch-pack: support more than one pack lockfile

From: Junio C Hamano <[email-masked]>
Date: Wed, 10 Jun 2020 18:41:19 -0700

Jonathan Tan <[email-masked]> writes:

> 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.

> +		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?

> +	    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);