Re: [PATCH v2 08/24] packfile: generalize pack directory list

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

Derrick Stolee <stolee@gmail.com> writes:

> +struct prepare_pack_data
> +{

ERROR: open brace '{' following struct go on the same line
#88: FILE: packfile.c:777:
+struct prepare_pack_data
+{

> +	struct repository *r;
> +	struct string_list *garbage;
> +	int local;
> +};
> +
> +static void prepare_pack(const char *full_name, size_t full_name_len, const char *file_name, void *_data)
> +{
> +	struct prepare_pack_data *data = (struct prepare_pack_data *)_data;
> +	struct packed_git *p;
> +	size_t base_len = full_name_len;
> +
> +	if (strip_suffix_mem(full_name, &base_len, ".idx")) {
> +		/* Don't reopen a pack we already have. */
> +		for (p = data->r->objects->packed_git; p; p = p->next) {
> +			size_t len;
> +			if (strip_suffix(p->pack_name, ".pack", &len) &&
> +			    len == base_len &&
> +			    !memcmp(p->pack_name, full_name, len))
> +				break;
> +		}
> +
> +		if (p == NULL &&
> +		    /*
> +		     * See if it really is a valid .idx file with
> +		     * corresponding .pack file that we can map.
> +		     */
> +		    (p = add_packed_git(full_name, full_name_len, data->local)) != NULL)
> +			install_packed_git(data->r, p);
> +	}

This is merely a moved code and the issue was inherited from the
original, but can we make it easier to read and at the same time
remove that assignment inside if() condition (which generally makes
the code harder to read)?  The most naïve

	if (!p) {
		p = add_packed_git(full_name, full_name_len, data->local);
		if (p)
			install_packed_git(data->r, p);
	}

isn't all that bad, but there may be even better ways.

> +	if (!report_garbage)
> +	       return;

This "return;" is indented with a run of SP not with HT?