Re: [PATCH 5/9] repack: refactor finishing pack-objects command

[Taylor Blau <me@ttaylorr.com>]

> diff --git a/builtin/repack.c b/builtin/repack.c
> index e591c295cf..f1adacf1d0 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -703,6 +703,42 @@ static void remove_redundant_bitmaps(struct string_list *include,
>  	strbuf_release(&path);
>  }
>
> +static int finish_pack_objects_cmd(struct child_process *cmd,
> +				   struct string_list *names,
> +				   const char *destination)
> +{
> +	int local = 1;
> +	FILE *out;
> +	struct strbuf line = STRBUF_INIT;
> +
> +	if (destination) {
> +		const char *scratch;

Maybe stick the declaration above (and consider making it static), so
that this can become

    if (destination)
      local = skip_prefix(destination, packdir, &scratch);

without the braces. Although it might be nice to either put this behind
a "has_prefix()" convenience function (which itself owns the scratch
buffer and hides that detail from the caller), or to make skip_prefix
skip trying to assign into the buffer if given NULL.

> +		local = skip_prefix(destination, packdir, &scratch);
> +	}
> +
> +	out = xfdopen(cmd->out, "r");
> +	while (strbuf_getline_lf(&line, out) != EOF) {
> +		struct string_list_item *item;
> +
> +		if (line.len != the_hash_algo->hexsz)
> +			die(_("repack: Expecting full hex object ID lines only "
> +			      "from pack-objects."));
> +		/*
> +		 * Avoid putting packs written outside of the repository in the
> +		 * list of names.
> +		 */
> +		if (local) {

Consider moving the declaration of item into this block, since it's not
used elsewhere throughout the body of the loop.

Alternatively, if you want to leave it up there, it might be easier to
adjust the if condition to be more in line with the comment, like:

    if (!local)
      continue;

> +	ret = finish_pack_objects_cmd(&cmd, &names, NULL);

OK, we don't even bother calling skip_prefix if given a NULL
destination. I wonder if it might make sense to force the caller to
compute "is this local?" ahead of time. This one would always pass "1"
trivially, and the cruft case would depend on whether or not we are
handling the `--expire-to` option.

Thanks,
Taylor