Stefan Beller <sbel...@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".
> --- 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?
> 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 ...
