Though I'd like to be able to say otherwise, I can't say that this
patch makes sense to apply by itself. :-)
Because get_default_head() has a very specific purpose, this change
results in less cohesive code. Let's apply this as part of your
larger change set. Code review inlined below.
On Wed, 12 Apr 2006, Madan S. wrote:
>
> This is a preparatory step for 'svnmerge log'.
>
>
> [[[
> Make the get_default_head() function more generic.
The summary should state how it's more generic, preferably by stating
what it does.
> * contrib/client-side/svnmerge.py
> (get_default_head): Removed.
This function was replaced by get_heads(), not simply removed. Log
message could state as much.
> (get_heads): New function similar to get_default_head(), but can return
> all the available head values. Also takes an extra parameter
> `get_only_default'. If `get_only_default' is True, expect only one
> head (error out if not), and return it. If `get_only_default' is
> False, return all the available heads as a list.
> (main): Modify call of get_default_head() to get_heads().
> ]]]
>
Content-Description: smdiff.txt
> Index: contrib/client-side/svnmerge.py
> ===================================================================
> --- contrib/client-side/svnmerge.py (revision 19320)
> +++ contrib/client-side/svnmerge.py (working copy)
> @@ -749,9 +749,10 @@
> messages.append('')
> return longest_sep.join(messages)
>
> -def get_default_head(branch_dir, branch_props):
> - """Return the default head for branch_dir (given its branch_props). Error
> - out if there is ambiguity."""
> +def get_heads(branch_dir, branch_props, get_only_default=False):
As you allude to below, I'm a little uneasy about the get_only_default
parameter. However, it's hard to be certain whether it makes sense or
not without seeing how this function is used in your 'svnmerge.py
status' implementation.
> + """Return all the heads for branch_dir (given its branch_props). If
> + the get_only_default parameter is True, expect only one head (error
> + out if there is ambiguity) and return it."""
We should take this function rename opportunity to start calling these
"sources" rather than "heads" (I've heard quite a few other people
mention this). Along those lines, I'd like to see this function named
get_sources(), or better yet, get_merge_sources().
Also, it looks like this function can never return an empty array --
it always has 1 or more elements. You should probably document as
much.
> if not branch_props:
> error("no integration info available")
>
> @@ -764,11 +765,17 @@
> if props.has_key(directory):
> del props[directory]
>
> - if len(props) > 1:
> + if get_only_default and len(props) > 1:
> error('multiple heads found. '
> 'Explicit head argument (-S/--head) required.')
>
> - return props.keys()[0]
This section of the patch probably won't apply anymore, since your
"multiple heads found" patch (r19338).
> + # Is this correct? Return a list/string depending on the
> + # value of get_only_default. I think the return value of
> + # any function should always be of the same type.
> + if get_only_default:
> + return props.keys()[0]
> + else:
> + return props.keys()
Depending on this function's usage, it'd likely be cleaner to always
return a list, and leave it up to the caller to interpret the result.
> def check_old_prop_version(branch_dir, props):
> """Check if props (of branch_dir) are svnmerge properties in old format,
> @@ -1683,7 +1690,7 @@
> if not opts["revision"]:
> opts["revision"] = "1-" + cf_rev
> else:
> - opts["head-path"] = get_default_head(branch_dir, branch_props)
> + opts["head-path"] = get_heads(branch_dir, branch_props, True)
opts["head-path"] = get_merge_sources(branch_dir, branch_props)[0]
...but this would probably be different in your 'svnmerge.py status' change set.
>
> assert opts["head-path"][0] == '/'
> opts["head-url"] = get_repo_root(branch_dir) + opts["head-path"]
- application/pgp-signature attachment: stored
Received on Thu Apr 13 18:26:34 2006