Re: [PATCH v4 2/2] submodule: fix handling of relative superproject origin URLs

From: Jon Seymour <jon.seymour@gmail.com>
Date: May 23, 2012, 22:17 UTC
Message-ID: <CAH3AnrrYm_KeP64yDc+gujkYqj-kiuLQ4URHX5Z57vj5XXLfKw@mail.gmail.com>
In-Reply-To: <4FBD5B8C.60605@web.de>

On Thu, May 24, 2012 at 7:50 AM, Jens Lehmann <Jens.Lehmann@web.de> wrote:

Show 7 quoted lines

> Am 23.05.2012 18:45, schrieb Jon Seymour:
>
> As mentioned last time I'd rather use $2 directly at the only site
> where $prefix is used. (On the other hand it might also make sense
> to give the parameters a descriptive name at the beginning of the
> function, but then I'd vote for a descriptive name like "urlprefix"
> or similar to make its meaning clearer)

Ok, I was favouring your latter heuristic. Are you ok if I use up_path here?

Show 5 quoted lines

>
>> +     remoteurl=$(echo "$remoteurl" | sed "s|^[^/:\\.][^:]*\$|./&|")
>
> A comment describing what this line actually does would be nice.
>

Sure, will do. What it does is to allow URLs of the form foo/ or foo/bar to be handled by the .*/* case in the switch below by rewriting the URL to prefix ./ in that case.

It doesn't do it for URLs that already begin with . (don't need it), / (mustn't have it) or : (assuming that any path beginning with : should probably be handled by the *:* case).

Show 24 quoted lines

>>       remoteurl=${remoteurl%/}
>>       sep=/
>>       while test -n "$url"
>> @@ -45,6 +47,11 @@ resolve_relative_url ()
>>               ../*)
>>                       url="${url#../}"
>>                       case "$remoteurl" in
>> +                     .*/*)
>> +                             remoteurl="${remoteurl%/*}"
>> +                             remoteurl="${remoteurl#./}"
>> +                             remoteurl="${prefix}${remoteurl}"
>> +                             ;;
>>                       */*)
>>                               remoteurl="${remoteurl%/*}"
>>                               ;;
>> @@ -64,7 +71,7 @@ resolve_relative_url ()
>>                       break;;
>>               esac
>>       done
>> -     echo "$remoteurl$sep${url%/}"
>> +     echo "${remoteurl%/.}$sep${url%/}"
>
> Wouldn't that better be handled in the ".*/*)" case above to avoid
> accidentally affecting the other cases?

Yes, I think you are right. Thanks.

Show 36 quoted lines

>
>>  }
>>
>>  #
>> @@ -964,8 +971,14 @@ cmd_sync()
>>               # Possibly a url relative to parent
>>               case "$url" in
>>               ./*|../*)
>> +                     up_path="$(echo "$sm_path" | sed "s/[^/]*/../g")" &&
>> +                     up_path=${up_path%/}/ &&
>> +                     remoteurl=$(resolve_relative_url "$url" "$up_path") &&
>>                       url=$(resolve_relative_url "$url") || exit
>>                       ;;
>> +             *)
>> +                     remoteurl="$url"
>> +                     ;;
>>               esac
>>
>>               if git config "submodule.$name.url" >/dev/null 2>/dev/null
>> @@ -979,7 +992,7 @@ cmd_sync()
>>                               clear_local_git_env
>>                               cd "$sm_path"
>>                               remote=$(get_default_remote)
>> -                             git config remote."$remote".url "$url"
>> +                             git config remote."$remote".url "$remoteurl"
>>                       )
>>                       fi
>>               fi
>
> I still have to wrap my head around these two hunks (I suspect it's
> too late for that in my timezone ;-), but I really wonder how you get
> away without changing cmd_add and cmd_init like you did last time.
> This looks much different than #2 and #4 of your v3 combined, which
> makes me suspicious in what direction this is evolving. Maybe you could
> tell us what you found out addressing the last problem you mentioned
> and how you handled it?

So, I subsequently noticed that v3 broke the clone done by a
subsequent update in the case of paths of the form ../foo/bar That was
because I had violated an implicit invariant - namely that the
submodule.{name}.url configuration variable in the superproject is
always a path relative to the working tree of the superproject. In v3,
I had effectively made this value relative to the working tree of the
submodule. I mostly only need to modify sync behaviour, because that
is the only path that modifies the remote.origin.url of the submodule
- the other two paths modify the submodule.{name}.url variable of the
superproject and this behaviour is mostly correct (sans normalisation
issues).

This series respects the implicit invariant that relative paths in configuration properties (submodule.{name}.url and remote.{remote}.url) are always relative to the working tree of the repository in which the configuration variable is defined.

I'll try to find a way to inject aspects of this commentary into the comments.

Show 5 quoted lines

>
>
> The only changes following here should be from test_expect_failure
> to test_expect_success as mentioned in my response to your first patch.
>

Sure.

Thanks for your review.

jon.