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

[Stefan Beller <sbeller@google.com>]

On Wed, Jan 11, 2017 at 1:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

Well, s/path related output/output of paths/.

> 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?

Internally the super-prefix is ignored. Only the (input and) output
is using that super prefix for messages.

It was introduced for grepping recursively into submodules, i.e.

invoke as

    git grep --recurse-submodules \
      -e path/inside/submodule/and/further/down
    # internally it invokes:
    git -C .. --super-prefix .. grep ..
    # which operates "just normal" except for the
    # input parsing and output

The use case for this patch is working tree related things, i.e.
    git checkout --recurse-submodules
    # internally when recursing we call "git read-tree -u", but
    # reporting could be:
    Your local changes to the following files would be overwritten by checkout:
      path/inside/submodule/file.txt


>
> 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?

Yes. I tried to explain it better above, but you got it here.

>
> 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".

I need to enhance the commit message by a lot then.

>
>> --- 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?

I looked into that. What is our current stance on using single/double quotes?
Most tests use single quotes for the test title as well as the test
instructions,
such that double quotes can be used naturally in there. But single quotes
cannot be used even escaped, such that we need to do a thing like

sq="'"

test_expect_success 'test title' '
    cat <<-EOF >expect &&
        error for ${sq}path${sq}
    EOF
    test instructions go here
'

though that is not used as often as I would think as
grep \${sq} yields t1507 and t3005.

>
> 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 ...
>

good point. I'll fix that.