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.
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.
Regardless of which version of the doc string we use for
combine_with_lastrange() I am +1 on the rest of your patch.
Paul
> Having slept on it... ew, I sound really grumpy. No weasel words in
> there :-) It's difficult to document something you're totally immersed
> in, in a way that works for people who are not already there. It did say
> most of the things it needed to, in one way or another, but my re-write
> tries to say them in a "top-down" direction from overall purpose through
> conditions that are true in all cases and then down to the details of
> all variations.
>
> - Julian
>
>
>> Please review the patch if you would.
>>
>> Thanks.
>> - Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2407937
Received on 2009-10-15 17:54:23 CEST