Re: svn commit: r25510 - trunk/subversion/tests/cmdline

Paul Burba wrote:
>> -----Original Message-----
>> From: Paul Burba [mailto:pburba@collab.net]
>> Sent: Monday, June 25, 2007 9:26 AM
>> To: Lieven Govaerts; Mark Phippard
>> Cc: dev@subversion.tigris.org; hwright@tigris.org; Daniel Rall
>> Subject: Re: svn commit: r25510 - trunk/subversion/tests/cmdline
>
> <snip>
>
>> Log.c:do_merged_log() - Local variable "svn_boolean_t
>> changed" is not initialized before being passed by reference
>> to check_history(). Great, except that check_history doesn't
>> always modify that arg:
>>
>> /* Set INFO->HIST to the next history for the path *if* there
>> is history
>> * available and INFO->HISTORY_REV is equal to or greater
>> than CURRENT.
>> *
>> * *CHANGED is set to TRUE if the path has history in the
>> CURRENT revision,
>>>>>> * otherwise it is not touched. <<<<
>> *
>> * If we do need to get the next history revision for the path, call
>> * get_history to do it -- see it for details.
>> */
>
> Hyrum,
>
> Just thinking out loud, but is it really wise to have functions like
> check_history() that take a predicate argument but only sometimes
> modifies it? Doesn't seem we do that anywhere else that I can recall
> (though I haven't made an exhaustive search).

I'm not sure. check_history() has been largely unchanged since its
introduction in r14722, so it seems that we've been doing this for quite
some time.

check_history() is used to determine if any of a group of paths has been
changed in a given revision, and is called in a loop over the paths.
The reason that check_history() doesn't modify changed if the answer is
FALSE is so that previous values of TRUE don't get overwritten. In
effect, this implements a logical OR.

If updating the function, and introducing local variables in the path
loops seems like a more readable solution, feel free to do so.

-Hyrum