On Thu, May 24, 2012 at 7:50 AM, Jens Lehmann <Jens.Lehmann@xxxxxx> wrote:
> 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?
>
>> + 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).
>> 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.
>
>> }
>>
>> #
>> @@ -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.
>
>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html