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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

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

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