On Thu, 13 Apr 2006 21:55:42 +0530, Daniel Rall
<dlr@collab.net> wrote:
> Though I'd like to be able to say otherwise, I can't
> say that this
> patch makes sense to apply by itself.
true...
> 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.
hmm, am thinking one logical step per commit... I dont
mind making this part of a bigger patch though...
> > 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().
> > ]]]
agree on comments about the log... will do. thanks.
> > 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.
simple, 'svn status' needs a list of all heads to list
status... so, it would call get_heads() with the
get_only_default parameter as False.
> > + """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().
cool. would do that... over the long run, this also
requires a major change in inline documentation...
>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.
why? how would that be useful?
> > 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).
oh, it does... only that the error message becomes more
elaborate...
> > + # 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.
I side you on this... I think so too. thx for the
reassurance.
> > 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.
no, no... you are right... I should change the usage of
the current call to get_heads().. er...
get_merge_sources()
thanks for the review,
Regards,
Madan.
Received on Thu Apr 13 20:15:26 2006