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

From: Phil Hord <phil.hord@gmail.com>
Date: May 24, 2012, 18:58 UTC
Message-ID: <CABURp0pE7TjSOx7aWNqak4iyK5dBS-70wf0xvBU4KXtsGu5tRw@mail.gmail.com>
In-Reply-To: <1337830657-16400-3-git-send-email-jon.seymour@gmail.com>

On Wed, May 23, 2012 at 11:37 PM, Jon Seymour <jon.seymour@gmail.com> wrote:

Show 10 quoted lines

> -# Resolve relative url by appending to parent's url
> +# Resolve relative url by appending the submodule url
> +# to the superproject's origin URL
> +#
> +# If the origin URL is itself a relative URL prepend
> +# an additional prefix, if present, that represents
> +# the relative path from the submodule's working tree
> +# to the superprojects' working tree.
> +#
> +# This behaviour is required to ensure that the origin URL

"Required behavior" always seems overstated to me when I read it in comments and so I tend to distrust it. I'd prefer to see "This behaviour is intended to ensure..." But this is only my personal preference.

Show 13 quoted lines

> +# of a submodule, when relative, is relative to the
> +# submodule's work tree and not to the superproject's work tree.
> +#
>  resolve_relative_url ()
>  {
>        remote=$(get_default_remote)
>        remoteurl=$(git config "remote.$remote.url") ||
>                remoteurl=$(pwd) # the repository is its own authoritative upstream
>        url="$1"
> +       up_path="$2"
> +
> +       #
> +       # ensure all relative paths begin with ./ to enable

Are we talking about remote urls or local filesystem paths? I think this is all confusing enough without the comments also using terminology inconsistently. Would it be more correct to say "all relatiive urls" here? Or is this function only interested in local filesystem paths?

I realize that urls are paths of a different nature. I pick this nit only in the cause of clarity.

Show 6 quoted lines

> +       # selection relative branch of subsequent case "$remoteurl"
> +       # statement.
> +       #
> +       # rewrites foo/bar to ./foo/bar but leaves /foo, :foo ./foo
> +       # and ../foo untouched.
> +       #

In many filesystems, ":foo" and "\foo" are valid filenames. I suspect it is unwise to employ them and expect them not to cause trouble, so I don't know if we should make special efforts to accept them here. But I think it is worth noting.

On the other hand, I do not know how these special characters are represented in urls.

Show 15 quoted lines

> +       remoteurl=$(echo "$remoteurl" | sed "s|^[^/:\\.][^:]*\$|./&|")
>        remoteurl=${remoteurl%/}
>        sep=/
>        while test -n "$url"
> @@ -45,6 +67,16 @@ resolve_relative_url ()
>                ../*)
>                        url="${url#../}"
>                        case "$remoteurl" in
> +                       .*/*)
> +                               # remove last part
> +                               remoteurl="${remoteurl%/*}"
> +                               # remove redundant leading ./
> +                               remoteurl="${remoteurl#./}"
> +                               # prefix path from submodule work tree to superproject work tree
> +                               remoteurl="${up_path}${remoteurl}"

Here we seem to be talking about paths since we are concerned with the worktrees. So maybe my earlier concern about "urls" vs. "paths" was miguided. My head swims.

Show 6 quoted lines

> +                               # remove trailing /.
> +                               remoteurl="${remoteurl%/.}"
> +                               ;;
>                        */*)
>                                remoteurl="${remoteurl%/*}"
>                                ;;

I haven't tested it, but the rest of this is making more sense to me now.

Phil