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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 11, 2017, 21:32 UTC
Message-ID: <xmqq37gpnuyi.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170110014542.19352-5-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

> Add support for the super-prefix option for commands that unpack trees.
> For testing purposes enable it in read-tree, which has no other path
> related output.

"path related output"?  I am not sure I understand this.

When read-tree reads N trees, or unpack_trees() is asked to "merge" N trees, how does --super-prefix supposed to interact with the paths in these trees? Does the prefix added to paths in all trees?

Did you mean that read-tree (and unpack_trees() in general) is a whole-tree operation and there is no path related input (i.e. pathspec limiting), but if another command that started in the superproject is running us in its submodule, it wants us to prefix the path to the submodule when we report errors?

If that is what is going on, I think it makes sense, but it may be asking too much from the readers to guess that from "Add support for the super-prefix option".

Show 15 quoted lines

> --- a/t/t1001-read-tree-m-2way.sh
> +++ b/t/t1001-read-tree-m-2way.sh
> @@ -363,6 +363,15 @@ test_expect_success 'a/b (untracked) vs a, plus c/d case test.' '
>  	test -f a/b
>  '
>  
> +cat <<-EOF >expect &&
> +	error: Updating 'fictional/a' would lose untracked files in it
> +EOF
> +
> +test_expect_success 'read-tree supports the super-prefix' '
> +	test_must_fail git --super-prefix fictional/ read-tree -u -m "$treeH" "$treeM" 2>actual &&
> +	test_cmp expect actual
> +'
> +

Preparing the expected output "expect" outside test_expect_success block is also old-school. Move it inside the new test?

Show 38 quoted lines

> diff --git a/unpack-trees.c b/unpack-trees.c
> index 7a6df99d10..bc56195e27 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -52,6 +52,37 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
>  	  ? ((o)->msgs[(type)])      \
>  	  : (unpack_plumbing_errors[(type)]) )
>  
> +static const char *super_prefixed(const char *path)
> +{
> +	/*
> +	 * This is used for the error messages above.
> +	 * We need to have exactly two buffer spaces.
> +	 */
> +	static struct strbuf buf[2] = {STRBUF_INIT, STRBUF_INIT};
> +	static int super_prefix_len = -1;
> +	static unsigned idx = 0;
> +
> +	if (!get_super_prefix())
> +		return path;
> +
> +	if (super_prefix_len < 0) {
> +		int i;
> +
> +		for (i = 0; i < ARRAY_SIZE(buf); i++)
> +			strbuf_addstr(&buf[i], get_super_prefix());
> +
> +		super_prefix_len = strlen(get_super_prefix());
> +	}
> +
> +	if (++idx >= ARRAY_SIZE(buf))
> +		idx = 0;
> +
> +	strbuf_setlen(&buf[idx], super_prefix_len);
> +	strbuf_addstr(&buf[idx], path);
> +
> +	return buf[idx].buf;
> +}

Hmph, as a reader of the code, I do not even want to wonder how expensive get_super_prefix() is. If the executable part of the above were written like this, it would have been easier to understand:

	if (super_prefix_len < 0) {
		if (!get_super_prefix())
			super_prefix_len = 0;
		else {
			int i;
			... prepare buf[] and set super_prefix_len ...;
                }
	}

        if (!super_prefix_len)
		return path;

	... use buf[] to do the prefixing and return it ...