From: Jon Seymour Date: Wed, 23 May 2012 22:17:19 GMT Subject: Re: [PATCH v4 2/2] submodule: fix handling of relative superproject origin URLs Message-ID: In-Reply-To: <4FBD5B8C.60605@web.de> On Thu, May 24, 2012 at 7:50 AM, Jens Lehmann 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.