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

[Madan U S]

On Thu, 13 Apr 2006 21:55:42 +0530, Daniel Rall 
<dlr at 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: /pipermail/svnmerge/attachments/20060413/0f811fe9/attachment.htm