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

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

Christian Couder <christian.couder@gmail.com> writes:

> +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;
> +		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) {
> +			item = string_list_append(names, line.buf);
> +			item->util = populate_pack_exts(line.buf);
> +		}
> +	}
> +	fclose(out);
> +
> +	strbuf_release(&line);
> +
> +	return finish_command(cmd);
> +}

Computing "is it local?" based on the value of "destination" feels
it belongs to the caller (one of the callers that do need the
computation), not to this function, especially given that the full
value of "destination" is not even used in any other way in this
function.  And the "is_local?" bit can instead be passesd into this
helper function as a parameter.

I wondered what "beautify" was about---the original looks OK to me
already, and while I do not mind to see a full sentence spelled in a
more gramatically correct way like in the postimage, I do not think
the change was worth wasting reviewer's time wondering if there are
other improvements by pointing it out in the proposed log message.

Thanks.