Code review comment for lp:~sergei.glushchenko/percona-server/mysqlbinlog-stdin

Sergei,

- Wouldn't the following be a more correct fix?

--- mysys/mf_iocache2.c 2011-06-30 15:37:13 +0000
+++ mysys/mf_iocache2.c 2012-02-21 10:56:04 +0000
@@ -145,7 +145,8 @@ void my_b_seek(IO_CACHE *info,my_off_t p
   if (info->type == READ_CACHE || info->type == SEQ_READ_APPEND)
   {
     /* TODO: explain why this works if pos < info->pos_in_file */
- if ((ulonglong) offset < (ulonglong) (info->read_end - info->buffer))
+ if (offset == 0 ||
+ (ulonglong) offset < (ulonglong) (info->read_end - info->buffer))
     {
       /* The read is in the current buffer; Reuse it */
       info->read_pos = info->buffer + offset;

As in, if pos == pos_in_file, we literally don't have to do anything, including forcing a seek on next read.

- We usually prefix our test names with "percona_", and it's sometimes handy to have a bug number in the corresponding test name, so "percona_bug933969.{test,result}" would probably be better, but it's optional.

- Similarly, "bug933969.patch" looks a bit more descriptive than "mysqlbinlog.patch" to me. At least I can look up the bug without even opening the patch.

- The test case should also have "--source include/not_windows.inc" and "--source include/not_embedded.inc". We support neither Windows nor embedded, but it would be good to at least try not to break them even further. :)

- Please replace "--exec /bin/bash -c" with "--system". In which case you also don't need --short-form or "> /dev/null".

- Tests usually have SQL statements in uppercase. It's a minor thing, just to make sure all tests use a consistent style.