Re: [PATCH v3 3/5] bundle API: change "flags" to be "extra_index_pack_args"

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 28, 2021, 01:44 UTC
Message-ID: <xmqqy28m8hsu.fsf@gitster.g>
In-Reply-To: <patch-v3-3.5-637039634e7-20210826T140414Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 8 quoted lines

> Since the "flags" parameter was added in be042aff24c (Teach progress
> eye-candy to fetch_refs_from_bundle(), 2011-09-18) there's never been
> more than the one flag: BUNDLE_VERBOSE.
>
> Let's have the only caller who cares about that pass "-v" itself
> instead through new "extra_index_pack_args" parameter. The flexibility
> of being able to pass arbitrary arguments to "unbundle" will be used
> in a subsequent commit.

As bundle API has been stable over a long period of time, I do not think the loss of flexibility is too bad (i.e. we no longer can cary any extra information that cannot be expressed as a textual argument given to the index-pack process), compared to gained flexibility of the index-pack command line we can use.

Show 13 quoted lines

>  int unbundle(struct repository *r, struct bundle_header *header,
> -	     int bundle_fd, int flags)
> +	     int bundle_fd, struct strvec *extra_index_pack_args)
>  {
> -	const char *argv_index_pack[] = {"index-pack",
> -					 "--fix-thin", "--stdin", NULL, NULL};
>  	struct child_process ip = CHILD_PROCESS_INIT;
>  
> -	if (flags & BUNDLE_VERBOSE)
> -		argv_index_pack[3] = "-v";
> +	strvec_push(&ip.args, "index-pack");
> +	strvec_push(&ip.args, "--fix-thin");
> +	strvec_push(&ip.args, "--stdin");

I would have expected you'd use pushl() here.

Show 30 quoted lines

> +	if (extra_index_pack_args) {
> +		strvec_pushvec(&ip.args, extra_index_pack_args);
> +		strvec_clear(extra_index_pack_args);
> +	}
>  
>  	if (verify_bundle(r, header, 0))
>  		return -1;
> -	ip.argv = argv_index_pack;
>  	ip.in = bundle_fd;
>  	ip.no_stdout = 1;
>  	ip.git_cmd = 1;
> diff --git a/bundle.h b/bundle.h
> index 84a6df1b65d..d47a7a3c69c 100644
> --- a/bundle.h
> +++ b/bundle.h
> @@ -26,16 +26,20 @@ int create_bundle(struct repository *r, const char *path,
>  		  int argc, const char **argv, struct strvec *pack_options,
>  		  int version);
>  int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);
> -#define BUNDLE_VERBOSE 1
>  
>  /**
>   * Unbundle after reading the header with read_bundle_header().
>   *
>   * We'll invoke "git index-pack --stdin --fix-thin" for you on the
>   * provided `bundle_fd` from read_bundle_header().
> + *
> + * Provide extra_index_pack_args to pass any extra arguments
> + * (e.g. "-v" for verbose/progress), NULL otherwise. The provided
> + * extra_index_pack_args (if any) will be strvec_clear()'d for you

Good to comment on this. I found it a bit surprising that the in argument "extra_index_pack_args" is eaten by the consumer, instead of leaving it up to the caller when to clear (or reuse before clear) it.

> + * (like the run-command.h API itself does).

This is not even run-command.h, so the mention of "run-command.h API itself" sounded strange to me. Without "itself", perhaps I would have slightly less puzzled. IIRC, run_command() only clears upon success and does not clear if start_command() fails, so it is not a very good comparison. I think the comment reads better without this last parenthesized note.

Show 26 quoted lines

>   */
>  int unbundle(struct repository *r, struct bundle_header *header,
> -	     int bundle_fd, int flags);
> +	     int bundle_fd, struct strvec *extra_index_pack_args);
>  int list_bundle_refs(struct bundle_header *header,
>  		int argc, const char **argv);
>  
> diff --git a/transport.c b/transport.c
> index 17e9629710a..99b2498e5dd 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -162,12 +162,16 @@ static int fetch_refs_from_bundle(struct transport *transport,
>  			       int nr_heads, struct ref **to_fetch)
>  {
>  	struct bundle_transport_data *data = transport->data;
> +	struct strvec extra_index_pack_args = STRVEC_INIT;
>  	int ret;
>  
> +	if (transport->progress)
> +		strvec_push(&extra_index_pack_args, "-v");
> +
>  	if (!data->get_refs_from_bundle_called)
>  		get_refs_from_bundle(transport, 0, NULL);
>  	ret = unbundle(the_repository, &data->header, data->fd,
> -			   transport->progress ? BUNDLE_VERBOSE : 0);
> +		       transport->progress ? &extra_index_pack_args : NULL);

Why conditional?

If transport->progress is false, extra_index_pack_args is a strvec that has 0 elements in it, and the strvec_pushvec() code knows how to handle it correctly and efficiently anyway.

I do not see a reason why we want to make the caller harder to read with the same conditional twice (i.e. if we pass NULL in non-verbose case, we can unconditionally put "-v" in the array, and clear the array on the side of this caller, and that would allow us lose the clear from the unbundle() function).

Other than that, looks very simple.  Nicely done.

Thanks.