Code review comment for lp:~laurynas-biveinis/percona-server/bug1012715-5.1

Alexey -

Thanks for the review

> - many lines exceed the 80 line width limit
> - it is a good practice to end replication test cases with "--source
> include/rpl_end.inc"
> - Yoda notation in comparisons ("0 == ...")

Noted.

> - variable declarations in the middle of a block is C99 (i.e. more
> Windows incompatibilities)

Isn't ha_innodb.cc C++? But I will fix them in any case.

> - unnecessary (char *) cast for the first argument in bzero()
> - I don't think we actually need IO_CACHE in
> innobase_do_overwrite_relay_log_info(). it's basically an enhanced
> version of fwrite() & friends, whereas we only want a single write
> of buff to the file.
> - no spaces around '=' sign in many places
> - no braces in single-statement if()s

Noted.

> - in general, I would suggest to fix all of the above by leaving the
> (incorrectly formatted) code alone and not moving it into a
> separate function. That would save me about half an hour on reading
> and commenting on changes that are not really changes but rather
> moving the code around, and allow me to focus on what's really been
> changed.
> - same goes for fname -> info_fname renaming

There are two places where I extracted new functions from existing code: in ha_innodb.cc and in trx0sys.c. Which one of them, or both, are you referring to? The trx0sys.c one I can revert, but IMHO it is easy to review too. Re. ha_innodb.cc changes, I have extracted innobase_do_overwrite_relay_log_info() because I needed to call it from another function as well and I don't see a good alternative to making a new function: if I copy pasted the code, I'd still need to adjust it heavily due to different local var context and the end result would be very close to innobase_do_overwrite_relay_log_info() anyway then. And I did the fname/pos variable rename & split, because these variables being repurposed five times is way beyond my pain threshold and extracted from innobase_setup() they stop working anyway.

Is there any way I can make the review easier with separate function? A separate MP with no functional changes perhaps?