{"thread":{"id":"29605","subject":"[PATCH] column: Fix an incorrect parse of the 'nodense' option token","startedAt":"2012-02-11T18:41:17Z","lastAt":"2012-02-14T18:28:34Z","messageCount":3,"participants":["Ramsay Jones","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"184469","messageId":"4F36B64D.4030000@ramsay1.demon.co.uk","threadId":"29605","inReplyTo":null,"subject":"[PATCH] column: Fix an incorrect parse of the 'nodense' option token","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2012-02-11T18:41:17Z","receivedAt":"2012-02-11T18:41:17Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nThe parse_option() function always saw the 'nodense' token as\n'dense', since it stripped the 'no' off the input argument string\nearlier when checking for the presence of the 'color' token.\n\nIn order to fix the parse, we use local variables (within the loop)\ninitialised to the function input arguments instead, which allows\neach token check to be independent.\n\nAlso, add some 'nodense' tests to t/t9002-column.sh.\n\nSigned-off-by: Ramsay Jones \n---\n\nHi Nguyen,\n\nWhile writing the earlier \"column: Fix some compiler and sparse warnings\"\npatch a few days ago, I had a feeling that a bug lurked in the parse_option()\ncode, but I just could not put my finger on the problem; so I just sent\nwhat I had.\n\nThis morning I suddenly realised what was bothering me ... As a first\nstep, I duplicated the 'dense' tests in t9002-column.sh and changed the\n'dense' token to 'nodense' and the tests still passed!\n\nNote that the code supports the 'nocolor' token, but it is not documented\nin config.txt.\n\nIf I understand correctly (and it's quite possible that I don't!), the\n'nodense' token has the same meaning as the absence of the 'dense' token\n(and I assume a similar comment applies to the '[no]color' tokens). If that\nis the case, then maybe a better solution would be to simply remove the\n'nodense' and 'nocolor' tokens. I will leave it to you to decide which\nsolution is more appropriate.\n\nATB,\nRamsay Jones\n\n column.c | 13 +++++++------\n t/t9002-column.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 6 deletions(-)\n\ndiff --git a/column.c b/column.c\nindex 98328cf..6df7640 100644\n--- a/column.c\n+++ b/column.c\n@@ -313,19 +313,20 @@ static int parse_option(const char *arg, int len,\n \tint i;\n \n \tfor (i = 0; i < ARRAY_SIZE(opts); i++) {\n-\t\tint set = 1, name_len;\n+\t\tint set = 1, arg_len = len, name_len;\n+\t\tconst char *arg_str = arg;\n \n \t\tif (opts[i].type == OPTION) {\n-\t\t\tif (len > 2 && !strncmp(arg, \"no\", 2)) {\n-\t\t\t\targ += 2;\n-\t\t\t\tlen -= 2;\n+\t\t\tif (arg_len > 2 && !strncmp(arg_str, \"no\", 2)) {\n+\t\t\t\targ_str += 2;\n+\t\t\t\targ_len -= 2;\n \t\t\t\tset = 0;\n \t\t\t}\n \t\t}\n \n \t\tname_len = strlen(opts[i].name);\n-\t\tif (len != name_len ||\n-\t\t strncmp(arg, opts[i].name, name_len))\n+\t\tif (arg_len != name_len ||\n+\t\t strncmp(arg_str, opts[i].name, name_len))\n \t\t\tcontinue;\n \n \t\tswitch (opts[i].type) {\ndiff --git a/t/t9002-column.sh b/t/t9002-column.sh\nindex 23d340e..fe7a30e 100755\n--- a/t/t9002-column.sh\n+++ b/t/t9002-column.sh\n@@ -71,6 +71,19 @@ EOF\n \ttest_cmp expected actual\n '\n \n+test_expect_success '20 columns, nodense' '\n+\tcat >expected <<\\EOF &&\n+one seven\n+two eight\n+three nine\n+four ten\n+five eleven\n+six\n+EOF\n+\tgit column --mode=column,nodense < lista > actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success '20 columns, dense' '\n \tcat >expected <<\\EOF &&\n one five nine\n@@ -121,6 +134,19 @@ EOF\n \ttest_cmp expected actual\n '\n \n+test_expect_success '20 columns, row first, nodense' '\n+\tcat >expected <<\\EOF &&\n+one two\n+three four\n+five six\n+seven eight\n+nine ten\n+eleven\n+EOF\n+\tgit column --mode=row,nodense actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success '20 columns, row first, dense' '\n \tcat >expected <<\\EOF &&\n one two three\n-- \n1.7.9\n"},{"id":"184499","messageId":"CACsJy8B7=6OYBV-HVK3Cq4_cTdPvm_1MvVv=U2R=4yQw9t5X9Q@mail.gmail.com","threadId":"29605","inReplyTo":"4F36B64D.4030000@ramsay1.demon.co.uk","subject":"Re: [PATCH] column: Fix an incorrect parse of the 'nodense' option token","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-12T03:37:50Z","receivedAt":"2012-02-12T03:37:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Feb 12, 2012 at 1:41 AM, Ramsay Jones\n wrote:\n>\n> The parse_option() function always saw the 'nodense' token as\n> 'dense', since it stripped the 'no' off the input argument string\n> earlier when checking for the presence of the 'color' token.\n>\n> In order to fix the parse, we use local variables (within the loop)\n> initialised to the function input arguments instead, which allows\n> each token check to be independent.\n>\n> Also, add some 'nodense' tests to t/t9002-column.sh.\n>\n> Signed-off-by: Ramsay Jones \n\nAcked-by: Nguyen Thai Ngoc Duy \n\n\n> Note that the code supports the 'nocolor' token, but it is not documented\n> in config.txt.\n>\n> If I understand correctly (and it's quite possible that I don't!), the\n> 'nodense' token has the same meaning as the absence of the 'dense' token\n> (and I assume a similar comment applies to the '[no]color' tokens). If that\n> is the case, then maybe a better solution would be to simply remove the\n> 'nodense' and 'nocolor' tokens. I will leave it to you to decide which\n> solution is more appropriate.\n\nIt's about overriding config. If you set \"dense\" by default in\ncolumn.ui but do not want it in this particular run, you can say\n--column=nodense.\n\nThe [no]color is for plumbing only. If a command produces colored\noutput, \"color\" is required to calculate text length correctly.\nOverriding it with \"nocolor\" would break the layout badly so it's no\nuse there. It does not make sense (to me) for users to put \"color\" in\ncolumn.ui. Which is why it's not mentioned in document.\n-- \nDuy\n"},{"id":"184665","messageId":"4F3AA7D2.9040702@ramsay1.demon.co.uk","threadId":"29605","inReplyTo":"CACsJy8B7=6OYBV-HVK3Cq4_cTdPvm_1MvVv=U2R=4yQw9t5X9Q@mail.gmail.com","subject":"Re: [PATCH] column: Fix an incorrect parse of the 'nodense' option token","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2012-02-14T18:28:34Z","receivedAt":"2012-02-14T18:28:34Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Nguyen Thai Ngoc Duy wrote:\n> It's about overriding config. If you set \"dense\" by default in\n> column.ui but do not want it in this particular run, you can say\n> --column=nodense.\n\nAh, OK, I missed that (obvious in retrospect!). Thanks.\n\n> The [no]color is for plumbing only. If a command produces colored\n> output, \"color\" is required to calculate text length correctly.\n> Overriding it with \"nocolor\" would break the layout badly so it's no\n> use there. It does not make sense (to me) for users to put \"color\" in\n> column.ui. Which is why it's not mentioned in document.\n\nEr... but 'color' is documented with column.ui in config.txt.\nI'm obviously (still) being dense! :-D\n\nATB,\nRamsay Jones\n"}]}