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

[Daniel Rall]

On Thu, 13 Apr 2006, Madan S. wrote:

> 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...

This style of development is good for a branch, but less so on the
trunk.  Self-sustaining incremental changes are reasonable on the
trunk. :)

> > > 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.

Is that at odds with the existing purpose of this function?  It seems
for validation and data cleansing-related ATM.

> > > +    """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...

We'd follow up this commit with something along those lines.

> >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?

Ideally, a function documents its parameters, return value,
pre-conditions, post-conditions, and assumptions.
-- 

Daniel Rall
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: not available
Url : /pipermail/svnmerge/attachments/20060413/cf5b4420/attachment.pgp