[Svnmerge] [PATCH] 1/5 - #2817: clearly differentiate urls, directories, and repo-relative paths

[Giovanni Bajo]

On 12/07/2007 5.55, Dustin J. Mitchell wrote:

> [[[
> clearly differentiate urls, directories, and repo-relative paths
> 
> * svn/trunk/contrib/client-side/svnmerge/svnmerge.py: fix comments,
>  variable names for urls, directories, and "repostitory-relative 
>  paths" to be more explicit and call the repo-relative paths 
>  "path identifiers"
> ]]]

+1 to commit. It's a nice cleanup, thanks! I just love consistency.

I have just a couple of nits:

> +# Identifiers for branches:
> +# A branch is identified in three ways within this source:
> +# - as a working copy (variable name usually includes 'dir')
> +# - as a fully qualified URL
> +# - as a path identifier (an opaque string indicating a particular path
> +#   in a particular repository; variable name includes 'pathid')
> +# A "target" is generally user-specified, and may be a working copy or
> +# a URL.
> +
> +def merge_props_to_revision_set(merge_props, pathid):

I think a comment like this is best put at the top of the file.

> @@ -1919,25 +1928,25 @@
>          # trailing /'s.
>          source = rstrip(source, "/")
>          if not is_wc(source) and not is_url(source):
> -            # Check if it is a substring of a repo-relative URL recorded
> +            # Check if it is a substring of a pathid recorded
>              # within the branch properties.
>              found = []
> -            for repos_path in branch_props.keys():
> -                if repos_path.find(source) > 0:
> -                    found.append(repos_path)
> +            for pathid in branch_props.keys():
> +                if pathid.find(source) > 0:
> +                    found.append(pathid)
>              if len(found) == 1:
> +                # (XXX assumes pathid is a repository-relative-path)
>                  source = get_repo_root(branch_dir) + found[0]

I'm not sure why you put a XXX here. What's unclear about it?

> +               source_pathid == target_to_pathid("."):
> +            error("cannot init integration source path '%s'\nIts repository-relative path must "
> +                  "differ from the repository-relative path of the current directory." % source_pathid)
> +        opts["source-pathid"] = source_pathid

I'd say: let's dump this repository-relative path for helping the user.
-- 
Giovanni Bajo