Re: [PATCHv2 4/4] unpack-trees: support super-prefix option

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 12, 2017, 21:40 UTC
Message-ID: <xmqqtw94hs8f.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170112001253.27679-1-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

> This is only patchv4 that is rerolled, patches 1-3 remain as is.

Good timing, as I was about to send a reminder to suggest rerolling 4/4 alone ;-)

> +static const char *super_prefixed(const char *path)
> +{

There used to be a comment that explains why we keep two static buffers around here. Even though it is in the log message, the in-code comment would save people trouble of having to go through "git blame" output.

I'd say something like

	/*
	 * It is necessary and sufficient to have two static buffers
	 * as the return value of this function is fed to error()
	 * using the unpack_*_errors[] templates we can see above.
	 */

perhaps.

> +	static struct strbuf buf[2] = {STRBUF_INIT, STRBUF_INIT};
> +	static int super_prefix_len = -1;
> +	static unsigned idx = 0;
> +

If we initialize this to 1 (or even better, "ARRAY_SIZE(buf) - 1"), then we would use buf[0] first and then buf[1], which feels more natural to me.

Other than that, this looks OK.  Will queue.

Thanks.

Show 56 quoted lines

> +	if (super_prefix_len < 0) {
> +		if (!get_super_prefix())
> +			super_prefix_len = 0;
> +		else {
> +			int i;
> +
> +			super_prefix_len = strlen(get_super_prefix());
> +			for (i = 0; i < ARRAY_SIZE(buf); i++)
> +				strbuf_addstr(&buf[i], get_super_prefix());
> +		}
> +	}
> +
> +	if (!super_prefix_len)
> +		return path;
> +
> +	if (++idx >= ARRAY_SIZE(buf))
> +		idx = 0;
> +
> +	strbuf_setlen(&buf[idx], super_prefix_len);
> +	strbuf_addstr(&buf[idx], path);
> +
> +	return buf[idx].buf;
> +}
> +
>  void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
>  				  const char *cmd)
>  {
> @@ -172,7 +202,7 @@ static int add_rejected_path(struct unpack_trees_options *o,
>  			     const char *path)
>  {
>  	if (!o->show_all_errors)
> -		return error(ERRORMSG(o, e), path);
> +		return error(ERRORMSG(o, e), super_prefixed(path));
>  
>  	/*
>  	 * Otherwise, insert in a list for future display by
> @@ -196,7 +226,7 @@ static void display_error_msgs(struct unpack_trees_options *o)
>  			something_displayed = 1;
>  			for (i = 0; i < rejects->nr; i++)
>  				strbuf_addf(&path, "\t%s\n", rejects->items[i].string);
> -			error(ERRORMSG(o, e), path.buf);
> +			error(ERRORMSG(o, e), super_prefixed(path.buf));
>  			strbuf_release(&path);
>  		}
>  		string_list_clear(rejects, 0);
> @@ -1918,7 +1948,9 @@ int bind_merge(const struct cache_entry * const *src,
>  			     o->merge_size);
>  	if (a && old)
>  		return o->gently ? -1 :
> -			error(ERRORMSG(o, ERROR_BIND_OVERLAP), a->name, old->name);
> +			error(ERRORMSG(o, ERROR_BIND_OVERLAP),
> +			      super_prefixed(a->name),
> +			      super_prefixed(old->name));
>  	if (!a)
>  		return keep_entry(old, o);
>  	else