Re: What's cooking in git.git (May 2013, #09; Wed, 29)

[Jens Lehmann <Jens.Lehmann@web.de>]

Am 04.06.2013 14:48, schrieb John Keeping:
> On Tue, Jun 04, 2013 at 09:17:17PM +1000, Heiko Voigt wrote:
>> On Tue, Jun 04, 2013 at 09:10:45AM +0100, John Keeping wrote:
>>> On Tue, Jun 04, 2013 at 03:29:51PM +1000, Heiko Voigt wrote:
>>>> On Mon, Jun 03, 2013 at 11:23:41PM +0100, John Keeping wrote:
>>>>>> Sorry, I should have been more specific here. I saw that you did some
>>>>>> changes to make "submodule add" do the right thing with relative paths,
>>>>>> but the following change to t7406 does not work like I believe it
>>>>>> should but instead makes the test fail:
>>>>>> -------------------8<---------------------
>>>>>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
>>>>>> index a4ffea0..9766b9e 100755
>>>>>> --- a/t/t7406-submodule-update.sh
>>>>>> +++ b/t/t7406-submodule-update.sh
>>>>>> @@ -559,7 +559,9 @@ test_expect_success 'add different submodules to the same pa
>>>>>>  test_expect_success 'submodule add places git-dir in superprojects git-dir' '
>>>>>>         (cd super &&
>>>>>>          mkdir deeper &&
>>>>>> -        git submodule add ../submodule deeper/submodule &&
>>>>>> +        (cd deeper &&
>>>>>> +         git submodule add ../../submodule submodule
>>>>>> +        ) &&
>>>>>>          (cd deeper/submodule &&
>>>>>>           git log > ../../expected
>>>>>>          ) &&
>>>>>> -------------------8<---------------------
>>>>>
>>>>> Ah, ok.  I think this case is problematic because the repository
>>>>> argument is either relative to "remote.origin.url" or to the top of the
>>>>> working tree if there is no "origin" remote.  I wonder if we should just
>>>>> die when a relative path is given for the repository and we're not at
>>>>> the top of the working tree.
>>>>
>>>> Why not behave as if we are at the top of the working tree for relative
>>>> paths? If there is an origin remote thats fine. If there is no origin
>>>> remote you could warn that the path used is taken relative from the root
>>>> of the superproject during add. What do you think?
>>>
>>> That's what the patch currently queued on "pu" does, which Jens wants to
>>> change, isn't it?
>>
>> True I did not realize this when reading it the first time. But I think
>> we should still not die when in a subdirectory. After all this series is
>> trying to archive that the submodule command works in subdirectories
>> seamlessly right? So you probably want to translate a relative path
>> without "origin" remote given from a subdirectory to the superproject
>> level and use that. Then you do not have to die.
> 
> The problem is that sometimes you do want to adjust the path and
> sometimes you don't.  Reading git-submodule(1), it says:
> 
>      This may be either an absolute URL, or (if it begins with ./ or
>      ../), the location relative to the superproject’s origin
>      repository.
>      [snip]
>      If the superproject doesn’t have an origin configured the
>      superproject is its own authoritative upstream and the current
>      working directory is used instead.
> 
> So I think it's quite reasonable to have a server layout that looks like
> this:
> 
>     project
>     |- libs
>     |  |- libA
>     |  `- libB
>     |- core.git
> 
> and with only core.git on your local system do:
> 
>     cd core/libs
>     git submodule add ../libs/libB
> 
> expecting that to point to libB.  But if we adjust the path then the
> user has to do:
> 
>     git submodule add ../../libs/libB
> 
> However, it is also perfectly reasonable to have no remote configured
> and the library next to the repository itself.  In which case we do want
> to specify the additional "../" so that shell completion works in the
> natural way.

Exactly.

> The only way I can see to resolve the ambiguity is to die when we hit
> this particular case.

Hmm, I'm not so sure about that. Don't the first three lines in
resolve_relative_url() show how to distinguish between these two
cases?

resolve_relative_url ()
{
	remote=$(get_default_remote)
	remoteurl=$(git config "remote.$remote.url") ||
		remoteurl=$(pwd) # the repository is its own authoritative upstream
...

And this looks like a central place to handle this issue too (but I
admit I only glanced over the call sites of resolve_relative_url(),
so I might be missing something here).