You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Madan U S <ma...@collab.net> on 2006/04/12 10:33:35 UTC
[PATCH][SVNMERGE] Make get_default_head() more generic(get_heads()).
This is a preparatory step for 'svnmerge log'.
[[[
Make the get_default_head() function more generic.
* contrib/client-side/svnmerge.py
(get_default_head): Removed.
(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().
]]]
Re: [PATCH][SVNMERGE] Make get_default_head() more generic(get_heads()).
Posted by Daniel Rall <dl...@collab.net>.
On Thu, 13 Apr 2006, Madan S. wrote:
> On Thu, 13 Apr 2006 21:55:42 +0530, Daniel Rall
> <dl...@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
RE: Re: [PATCH][SVNMERGE] Make get_default_head() more generic(get_heads()).
Posted by Madan U S <ma...@collab.net>.
On Thu, 13 Apr 2006 21:55:42 +0530, Daniel Rall
<dl...@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.
Re: [PATCH][SVNMERGE] Make get_default_head() more generic(get_heads()).
Posted by Daniel Rall <dl...@collab.net>.
Though I'd like to be able to say otherwise, I can't say that this
patch makes sense to apply by itself. :-)
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.
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().
> ]]]
>
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.
> + """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().
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.
> 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).
> + # 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.
> 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.
>
> assert opts["head-path"][0] == '/'
> opts["head-url"] = get_repo_root(branch_dir) + opts["head-path"]