On Fri, May 25, 2012 at 4:58 AM, Phil Hord <phil.hord@xxxxxxxxx> wrote:
> On Wed, May 23, 2012 at 11:37 PM, Jon Seymour <jon.seymour@xxxxxxxxx> 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.
Sure.
>> + #
>> + # ensure all relative paths begin with ./ to enable
>
For clarity, this should be:
ensure all relative superproject origin URLs 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.
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?
>
>> + # 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?
>
>> + 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.
>
>> + # remove trailing /.
>> + remoteurl="${remoteurl%/.}"
>> + ;;
>> */*)
>> remoteurl="${remoteurl%/*}"
>> ;;
>
> I haven't tested it, but the rest of this is making more sense to me now.
>
> Phil
Thanks for your review and suggestions.
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