Re: [PATCH 5/6] submodule update: add super-reference flag

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 5, 2016, 21:16 UTC
Message-ID: <xmqq1t22x6fj.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160804195159.7788-6-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

That's a bit sketchy description. From the title, I expected that we would see one additional 'unsigned super_reference : 1;' field in some structure, but that is not what is happening. The log message needs to describe what these string list are and why they are needed a bit better.

At least, please don't name a multiple_word field "multipleword" ;-)

Show 11 quoted lines

> @@ -715,6 +716,15 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
>  		for_each_string_list_item(item, &suc->references)
>  			argv_array_pushl(&child->args, "--reference", item->string, NULL);
>  	}
> +	if (suc->superreferences.nr) {
> +		struct string_list_item *item;
> +		for_each_string_list_item(item, &suc->superreferences) {
> +			strbuf_reset(&sb);
> +			argv_array_pushf(&child->args, "--reference=%s/%s",
> +					 relative_path(item->string, suc->prefix, &sb),
> +					 sub->path);

The phrase "super reference" made me imagine it is some kind of "reference" that is applicable to the superproject, but this code smells like it is a "prefix to create reference suited for use in submodule". Whatever it is, it should be explained better (than "no desciption" which is what we have here ;-), and given a name that match the explanation.

Thanks.