pt-config-diff doesn't handle end-of-line comments

Bug #1007938 reported by Kaiwang CHEN on 2012-06-02
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Percona Toolkit moved to
Fix Released
Brian Fraser

Bug Description

option file supports end-of-line comments started with semi-comma(;) or sharp (#), but MySQLConfig treats those comments part of value.

[kc@dns1 percona-toolkit]$ cat mycnf-kc-001.txt
user=mysql ; comment
[kc@dns1 percona-toolkit]$ cat mycnf-kc-002.txt
[kc@dns1 percona-toolkit]$ perl bin/pt-config-diff mycnf-kc-001.txt mycnf-kc-002.txt
1 config difference
Variable mycnf-kc-001.txt mycnf-kc-002.txt
========================= ================ ================
user mysql ; comment mysql

I think the problem is in _parse_varvals()
=== modified file 'lib/'
--- lib/ 2012-01-19 19:46:56 +0000
+++ lib/ 2012-06-02 20:07:03 +0000
@@ -339,6 +339,7 @@
       if ( $item ) {
          # Strip leading and trailing whitespace.
          $item =~ s/^\s+//;
+ $item =~ s/[#;].*$//; # trailing comment
          $item =~ s/\s+$//;

The enclosed patch(bin/pt-config-diff not updated) should solve the problem, covered by this test
not ok 26 - end of line comment in option file
# Failed test 'end of line comment in option file'
# in t/lib/MySQLConfig.t at line 805.
# got: 'mysql ; comment'
# expected: 'mysql'
# Looks like you failed 1 test of 30.


Related branches

Kaiwang CHEN (kaiwangchen) wrote :
tags: added: comments pt-config-diff
Changed in percona-toolkit:
status: New → Triaged
Brian Fraser (fraserbn) wrote :


Thank you for the patch! It's an unexpected but welcome surprise to get one with a test case. I have just created a branch with it, and will be running tests shortly, but a bit of a heads up: It might be a bit too late to get this into the 2.1.2 release, so it'll probably have to wait until 2.1.3.

Brian Fraser (fraserbn) wrote :

Heh, we've both been corrected; turns out that MySQL only supports # as the start-of-comment character. I've updated the branch.

Kaiwang CHEN (kaiwangchen) wrote :

Yes, I neglected the manual does not define ";" as a comment starter in the middle of a line. However, it really could start a comment at the beginning of line with whitespace trimmed, as cited:
    Comment lines start with “#” or “;”. A “#” comment can start in the middle of a line as well.

I verified it by

[kc@dns1 msb_5_1_46]$ grep datadir my.sandbox.cnf;./my _print_defaults mysqld | grep datadir
;datadir = /home/kc/sandboxes/msb_5_1_46/data
[kc@dns1 msb_5_1_46]$ grep datadir my.sandbox.cnf;./my _print_defaults mysqld | grep datadir
#datadir = /home/kc/sandboxes/msb_5_1_46/data

[kc@dns1 msb_5_1_46]$ grep datadir my.sandbox.cnf;./my _print_defaults mysqld | grep datadir
datadir = ;/home/kc/sandboxes/msb_5_1_46/data
[kc@dns1 msb_5_1_46]$ grep datadir my.sandbox.cnf;./my _print_defaults mysqld | grep datadir
datadir = #/home/kc/sandboxes/msb_5_1_46/data

Additionally I found my original patch is not against the right place. Enclosed is fixed version and with more tests.

Kaiwang CHEN (kaiwangchen) wrote :

Forgive me, my patch minutes ago is problematic, it does not deal with quotes and escapes... Good job, Brian. Fantastic regexp work.

I guess you will take semi-comma comment line into consideration, it is defined, as explained previously.

tags: added: option-parsing
removed: comments
Brian Fraser (fraserbn) on 2012-11-02
Changed in percona-toolkit:
assignee: nobody → Brian Fraser (fraserbn)
status: Triaged → In Progress
Brian Fraser (fraserbn) wrote :

Yep, right you are! I've went ahead an added skips for lines starting with ; (with optional whitespace in front). Thanks for the correction!

Brian Fraser (fraserbn) on 2012-11-09
Changed in percona-toolkit:
milestone: none → 2.1.7
Changed in percona-toolkit:
milestone: 2.1.7 → 2.1.8
Changed in percona-toolkit:
importance: Undecided → Low
Brian Fraser (fraserbn) on 2012-12-05
Changed in percona-toolkit:
status: In Progress → Fix Committed
summary: - MySQLConfig doesn't support end-of-line comments
+ pt-config-diff doesn't handle end-of-line comments
Changed in percona-toolkit:
status: Fix Committed → Fix Released

Percona now uses JIRA for bug reports so this bug report is migrated to:

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers