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

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

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

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