[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

RE: Re: [PATCH][SVNMERGE] Make get_default_head() more generic(get_heads()).

From: Madan U S <madan_at_collab.net>
Date: 2006-04-13 20:02:56 CEST

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

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.