Before merge comments for the second patch (changes to backup stream library).
Chuck Bell wrote:
> Rafal,
>
> This patch is ok to push under the following conditions:
>
> 1) you merge the latest clone of mysql-6.0-backup (I know -- I'm nagging)
Will come as a separate patch.
> 2) I think you need to remove the dead diffs. There are a lot of spacing
> changes and things like:
>
> -*/
> +*/
>
These changes remove spaces at end of lines (which should not be there). I have
a script which does it automatically before a commit - if spaces get there
somehow, such diffs result. Removing them would be extremely tedious and time
consuming since I'd have to discover in what places there were spaces and
restore exact the same amount. I'd be grateful if we can leave it for now (I
know it is annoying when reviewing).
After long fight with bk I did this - moved all space changes to a new rename
patch. After that, the two other patches should be free from such diffs I hope.
I should find a better solution for "spurious spaces" problem but I never have
time to work on this.
> ...that occur mostly in stream_v1.h.
> 3) please check your condition statements and use ()'s more. For example, (a
> && b < c) is perfectly fine for the compiler but humans (especially this
> one) have trouble reading things like that -- extra proof when there are 4
> or more operands or if someone (like me) accidentally reorders them! Having
> ()'s around every comparison and op/operand pairs will help prevent errors.
> I think this should be fixed to make debugging and reading of the code (for
> comprehension) easier.
>
I added parentheses
> There are a few things I think should be changed but are not crucial to our
> deadline:
>
OK, let discuss them after the push then.
I also added definition of bzero() using memset in hope of avoiding windows
compilation problems.
> 1) Why did you change backup_stream->state to backup_stream->mode? State
> seemed to be much more appropriate.
> 2) I think we need (collectively) to change our practices with enums. For
> enums that are public, I think we need to either declare a scope with them
> or make them unique enough that someone else can't accidentally reuse them.
> For example, use scope::MY_ENUM or change things like TABLE_ITEM to
> BSTREAM_TABLE_ITEM. This isn't just for your patch, mine need this too! ;)
>
> Chuck
>
>> -----Original Message-----
>> From: rsomla@stripped [mailto:rsomla@stripped]
>> Sent: Tuesday, November 27, 2007 5:04 AM
>> To: commits@stripped
>> Subject: bk commit into 6.0 tree (rafal:1.2669) WL#4063
>>
>> Below is the list of changes that have just been committed
>> into a local
>> 6.0 repository of rafal. When rafal does a push these changes will
>> be propagated to the main repository and, within 24 hours after the
>> push, to the public repository.
>> For information on how to access the public repository
>> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>>
>> ChangeSet@stripped, 2007-11-27 11:03:52+01:00, rafal@quant.(none) +4 -0
>> WL#4063 (backup stream library):
>>
>> This patch introduces changes and bug fixes necessary for
>> integrating the backup stream library
>> with the backup server code.
>>
>> sql/backup/stream_v1.c@stripped, 2007-11-27 11:03:45+01:00,
>> rafal@quant.(none) +115 -96
>> - changed the way table position is stored in the
>> catalogue: as snapshot number and position within snapshot's
>> table list;
>> - added checks for return values from the bcat_* service
>> functions;
>> - since item types are stored in 2 bytes, an item list
>> separator consists of two 0x00 bytes now - updated docs
>> and code;
>> - when metadata item is read from a stream, the code
>> doesn't try to locate it in the catalogue - the upper
>> layer can do it on its own; this removes a need for
>> bcat_get_*() service functions;
>>
>>
>
