Re: [PATCH v5 2/2] submodule: fix handling of relative superproject origin URLs
On Fri, May 25, 2012 at 4:58 AM, Phil Hord <phil.hord@gmail.com> wrote:
Show 8 quoted lines
> On Wed, May 23, 2012 at 11:37 PM, Jon Seymour <jon.seymour@gmail.com> wrote:
>> +#
>> +# 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.
>> + #
>> + # ensure all relative paths begin with ./ to enable
>
For clarity, this should be:
ensure all relative superproject origin URLs begin with ./ to enable ...
Show 7 quoted lines
> 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.
How about I rewrite the function blurb as something like:
"The function takes at most 2 arguments. The first argument is the relative URL that navigates from the superproject origin repo to the submodule origin repo. The second up_path argument, if specified, is the relative path that navigates from the submodule working tree to the superproject working tree.
The output of the function is the origin URL of the submodule.
The output will either be an absolute URL or filesystem path (if the superproject origin URL is an absolute URL or filesystem path, respectively) or a relative file system path (if the superproject origin URL is a relative file system path).
When the output is a relative file system path, the path is either relative to the submodule working tree, if up_path is specified, or to the superproject working tree otherwise."
Phil/Jens: Perhaps we should rename the function to be: submodule_origin_url to reflect its intended use?
Show 16 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.
Perhaps I should indicate 'why' these paths are not touched? Something like:
"Rewrites foo/bar foo/bar as ./foo/bar but leaves ./foo, ../foo, /foo, foo:bar, :bar untouched because the first 2 forms do not require additional qualification and the last forms are assumed to be absolute URLs or paths."
Perhaps I shouldn't be treating :foo as a possible absolute file system path ? My knowledge of filesystems is not good enough to know if there are filesystems that git supports where :foo might be regarded a valid absolute path?
Show 21 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.
>This branch of the case statement is specifically when the origin superproject URL is a relative file system path.
Show 11 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.
>
> PhilThanks for your review and suggestions.
jon.
Jon Seymour