On Thu, 2009-10-15 at 11:54 -0400, Paul Burba wrote:
> On Thu, Oct 15, 2009 at 5:30 AM, Julian Foad <julianfoad_at_btopenworld.com> wrote:
>
> > On Thu, 2009-10-15 at 00:43 +0100, Julian Foad wrote:
> >> Paul,
> >>
> >> Reviewing r39019 for back-port, I struggled to understand the doc string
> >> of combine_with_lastrange(). It seemed to say bits and pieces about what
> >> it did internally, without saying what its purpose or promises or
> >> requirements were. I read through its implementation and its uses and
> >> re-wrote the doc string as in the attached patch. I found that the
> >> "lastrange" parameter was functionally redundant so I removed it, as it
> >> just made the interface to the function more complex.
>
> Hi Julian,
>
> Thanks for taking an in-depth look at this. You are absolutely
> correct re the LASTRANGE parameter; it has been around since the dawn
> of the merge tracking work and has been unnecessary the entire time!
> Call it code inertia and confirmation, if any was needed, of what a
> fresh set of eyes sees.
>
> I think part of the problem with combine_with_lastrange's doc string
> is that I have tried to describe CONSIDER_INHERITANCE as a flag which
> determines if two ranges intersect. We can instead view it as a flag
> which determines how two intersecting ranges (based only on the start
> and end revisions) are *combined*. Ultimately these are the same
> thing, but writing the doc string from the latter perspective is, I
> think, clearer...maybe! See my tweak of your patch and let me know
> what you think.
That looks great, with one exception: you took away my overview
paragraph so that the description, after the first line, launched
straight into the details of "if ... else if ... else if ...". It is
extremely helpful for the reader to start with a description of what the
function does in terms that are "general" in the sense of not having any
"ifs": a complete high-level description of what the function always
does no matter what the arguments are.
I think my attempt to generalize wasn't very good, and/or looked
superfluous to the rest of the description. I have replaced that
paragraph with another attempt and marked it clearly as an overview.
Feel free to tweak this if it's inaccurate.
> A few other changes:
>
> 1) Changed to REVLIST argument to RANGELIST, as that is the de facto
> name for rangelists...
>
> 2) ...and as such we don't need to paragraph about what a rangelist
> is, that is already documented in svn_mergeinfo.h.
>
> 3) Note that if we simply push NEW_RANGE onto RANGELIST we are
> actually pushing a copy of it allocated in RESULT_POOL.
>
> 4) Some trailing whitespace cleanup.
+1.
> Regardless of which version of the doc string we use for
> combine_with_lastrange() I am +1 on the rest of your patch.
Thanks.
Committed in r40061.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2408060
Received on 2009-10-16 01:35:27 CEST