Re: [PATCH 3/4] builtin/repack.c: write cruft packs to arbitrary locations

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

Taylor Blau <me@ttaylorr.com> writes:

> In the following commit, a new write_cruft_pack() caller will be added
> which wants to write a cruft pack to an arbitrary location. Prepare for
> this by adding a parameter which controls the destination of the cruft
> pack.
>
> For now, provide "packtmp" so that this commit does not change any
> behavior.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  builtin/repack.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 1184e8c257..a5386ac893 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -662,6 +662,7 @@ static int write_midx_included_packs(struct string_list *include,
>  }
>  
>  static int write_cruft_pack(const struct pack_objects_args *args,
> +			    const char *destination,
>  			    const char *pack_prefix,
>  			    const char *cruft_expiration,
>  			    struct string_list *names,
> @@ -673,8 +674,10 @@ static int write_cruft_pack(const struct pack_objects_args *args,
>  	struct string_list_item *item;
>  	FILE *in, *out;
>  	int ret;
> +	const char *scratch;
> +	int local = skip_prefix(destination, packdir, &scratch);

Hmph.  In an earlier step we got rid of the hardcoded assumption on
where the packtmp is, and we are passing destination in (where the
existing callers call us with packtmp) to make it even better, but
we acquire the dependency on packdir global with this step.  It's
just a couple of file-scope static global variables, so it is not a
huge deal.

> -	prepare_pack_objects(&cmd, args, packtmp);
> +	prepare_pack_objects(&cmd, args, destination);
>  
>  	strvec_push(&cmd.args, "--cruft");
>  	if (cruft_expiration)
> @@ -714,7 +717,12 @@ static int write_cruft_pack(const struct pack_objects_args *args,
>  		if (line.len != the_hash_algo->hexsz)
>  			die(_("repack: Expecting full hex object ID lines only "
>  			      "from pack-objects."));
> -		string_list_append(names, line.buf);
> +                /*
> +		 * avoid putting packs written outside of the repository in the
> +		 * list of names
> +		 */
> +		if (local)
> +			string_list_append(names, line.buf);
>  	}

Even if we do not want to contaminate the "names" list with packs
that are not in the repository, wouldn't our caller still want to be
able to tell what packs they are?

What I am wondering is if it makes more sense to have the caller
pass &names (which can be NULL to just discard the output from the
pack-objects command) so that this function can concentrate on what
it does (i.e. formulate the command to write cruft packs and then
report the packs that are created), without having to worry about
the management of the &names thing, which can be done by the caller
of this function?  We are already passing &names, so it may be the
matter of caller deciding to pass &names or NULL based on the value
of destination it passes to the function?

> @@ -986,7 +994,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  		cruft_po_args.local = po_args.local;
>  		cruft_po_args.quiet = po_args.quiet;
>  
> -		ret = write_cruft_pack(&cruft_po_args, pack_prefix,
> +		ret = write_cruft_pack(&cruft_po_args, packtmp, pack_prefix,
>  				       cruft_expiration, &names,
>  				       &existing_nonkept_packs,
>  				       &existing_kept_packs);

For example, this callsite will always want to pass &names because
it is always about local pack, right?

Thanks.