{"thread":{"id":"56318","subject":"[PATCH] column: fix parsing of the '--nl' option","startedAt":"2021-08-18T09:25:32Z","lastAt":"2021-08-26T21:36:50Z","messageCount":5,"participants":["SZEDER Gábor","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"433052","messageId":"20210818092456.3045808-1-szeder.dev@gmail.com","threadId":"56318","inReplyTo":null,"subject":"[PATCH] column: fix parsing of the '--nl' option","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-08-18T09:24:56Z","receivedAt":"2021-08-18T09:25:32Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'git column's '--nl' option can be used to specify a \"string to be\nprinted at the end of each line\" (quoting the man page), but this\noption and its mandatory argument has been parsed as OPT_INTEGER since\nthe introduction of the command in 7e29b8254f (Add column layout\nskeleton and git-column, 2012-04-21). Consequently, any non-number\nargument is rejected by parse-options, and any number other than 0\nleads to segfault:\n\n $ printf \"%s\\n\" one two |git column --mode=plain --nl=foo\n error: option `nl' expects a numerical value\n $ printf \"%s\\n\" one two |git column --mode=plain --nl=42\n Segmentation fault (core dumped)\n $ printf \"%s\\n\" one two |git column --mode=plain --nl=0\n one\n two\n\nParse this option as OPT_STRING.\n\nSigned-off-by: SZEDER Gábor \n---\n Documentation/git-column.txt | 2 +-\n builtin/column.c | 2 +-\n t/t9002-column.sh | 18 ++++++++++++++++++\n 3 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-column.txt b/Documentation/git-column.txt\nindex f58e9c43e6..6cea9ab463 100644\n--- a/Documentation/git-column.txt\n+++ b/Documentation/git-column.txt\n@@ -39,7 +39,7 @@ OPTIONS\n --indent=::\n \tString to be printed at the beginning of each line.\n \n---nl=::\n+--nl=::\n \tString to be printed at the end of each line,\n \tincluding newline character.\n \ndiff --git a/builtin/column.c b/builtin/column.c\nindex 40d4b3bee2..158fdf53d9 100644\n--- a/builtin/column.c\n+++ b/builtin/column.c\n@@ -29,7 +29,7 @@ int cmd_column(int argc, const char **argv, const char *prefix)\n \t\tOPT_INTEGER(0, \"raw-mode\", &colopts, N_(\"layout to use\")),\n \t\tOPT_INTEGER(0, \"width\", &copts.width, N_(\"maximum width\")),\n \t\tOPT_STRING(0, \"indent\", &copts.indent, N_(\"string\"), N_(\"padding space on left border\")),\n-\t\tOPT_INTEGER(0, \"nl\", &copts.nl, N_(\"padding space on right border\")),\n+\t\tOPT_STRING(0, \"nl\", &copts.nl, N_(\"string\"), N_(\"padding space on right border\")),\n \t\tOPT_INTEGER(0, \"padding\", &copts.padding, N_(\"padding space between columns\")),\n \t\tOPT_END()\n \t};\ndiff --git a/t/t9002-column.sh b/t/t9002-column.sh\nindex 89983527b6..6d3dbde3fe 100755\n--- a/t/t9002-column.sh\n+++ b/t/t9002-column.sh\n@@ -42,6 +42,24 @@ EOF\n \ttest_cmp expected actual\n '\n \n+test_expect_success '--nl' '\n+\tcat >expected <<\\EOF &&\n+oneZ\n+twoZ\n+threeZ\n+fourZ\n+fiveZ\n+sixZ\n+sevenZ\n+eightZ\n+nineZ\n+tenZ\n+elevenZ\n+EOF\n+\tgit column --nl=\"Z$LF\" --mode=plain actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success '80 columns' '\n \tcat >expected <<\\EOF &&\n one two three four five six seven eight nine ten eleven\n-- \n2.33.0.453.gc5e41af357\n\n"},{"id":"433065","messageId":"nycvar.QRO.7.76.6.2108181347010.55@tvgsbejvaqbjf.bet","threadId":"56318","inReplyTo":"20210818092456.3045808-1-szeder.dev@gmail.com","subject":"Re: [PATCH] column: fix parsing of the '--nl' option","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-08-18T11:48:58Z","receivedAt":"2021-08-18T11:49:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Gábor,\n\nOn Wed, 18 Aug 2021, SZEDER Gábor wrote:\n\n> 'git column's '--nl' option can be used to specify a \"string to be\n> printed at the end of each line\" (quoting the man page), but this\n> option and its mandatory argument has been parsed as OPT_INTEGER since\n> the introduction of the command in 7e29b8254f (Add column layout\n> skeleton and git-column, 2012-04-21). Consequently, any non-number\n> argument is rejected by parse-options, and any number other than 0\n> leads to segfault:\n>\n> $ printf \"%s\\n\" one two |git column --mode=plain --nl=foo\n> error: option `nl' expects a numerical value\n> $ printf \"%s\\n\" one two |git column --mode=plain --nl=42\n> Segmentation fault (core dumped)\n> $ printf \"%s\\n\" one two |git column --mode=plain --nl=0\n> one\n> two\n>\n> Parse this option as OPT_STRING.\n\nWhoa. Nice catch. I have just one suggestion about the patch, below.\n\n> Signed-off-by: SZEDER Gábor \n> ---\n> Documentation/git-column.txt | 2 +-\n> builtin/column.c | 2 +-\n> t/t9002-column.sh | 18 ++++++++++++++++++\n> 3 files changed, 20 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/git-column.txt b/Documentation/git-column.txt\n> index f58e9c43e6..6cea9ab463 100644\n> --- a/Documentation/git-column.txt\n> +++ b/Documentation/git-column.txt\n> @@ -39,7 +39,7 @@ OPTIONS\n> --indent=::\n> \tString to be printed at the beginning of each line.\n>\n> ---nl=::\n> +--nl=::\n> \tString to be printed at the end of each line,\n> \tincluding newline character.\n>\n> diff --git a/builtin/column.c b/builtin/column.c\n> index 40d4b3bee2..158fdf53d9 100644\n> --- a/builtin/column.c\n> +++ b/builtin/column.c\n> @@ -29,7 +29,7 @@ int cmd_column(int argc, const char **argv, const char *prefix)\n> \t\tOPT_INTEGER(0, \"raw-mode\", &colopts, N_(\"layout to use\")),\n> \t\tOPT_INTEGER(0, \"width\", &copts.width, N_(\"maximum width\")),\n> \t\tOPT_STRING(0, \"indent\", &copts.indent, N_(\"string\"), N_(\"padding space on left border\")),\n> -\t\tOPT_INTEGER(0, \"nl\", &copts.nl, N_(\"padding space on right border\")),\n> +\t\tOPT_STRING(0, \"nl\", &copts.nl, N_(\"string\"), N_(\"padding space on right border\")),\n> \t\tOPT_INTEGER(0, \"padding\", &copts.padding, N_(\"padding space between columns\")),\n> \t\tOPT_END()\n> \t};\n> diff --git a/t/t9002-column.sh b/t/t9002-column.sh\n> index 89983527b6..6d3dbde3fe 100755\n> --- a/t/t9002-column.sh\n> +++ b/t/t9002-column.sh\n> @@ -42,6 +42,24 @@ EOF\n> \ttest_cmp expected actual\n> '\n>\n> +test_expect_success '--nl' '\n> +\tcat >expected <<\\EOF &&\n> +oneZ\n> +twoZ\n> +threeZ\n> +fourZ\n> +fiveZ\n> +sixZ\n> +sevenZ\n> +eightZ\n> +nineZ\n> +tenZ\n> +elevenZ\n> +EOF\n\nOr maybe we can do this instead?\n\n\tsed \"s/\\$/Z\" expected &&\n\nMuch shorter, and less error-prone (in case any `--run=[...]` option would\nchange what is in `lista` in the future).\n\nCiao,\nDscho\n\n> +\tgit column --nl=\"Z$LF\" --mode=plain actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> test_expect_success '80 columns' '\n> \tcat >expected <<\\EOF &&\n> one two three four five six seven eight nine ten eleven\n> --\n> 2.33.0.453.gc5e41af357\n>\n>\n"},{"id":"433084","messageId":"xmqqy28y4gqt.fsf@gitster.g","threadId":"56318","inReplyTo":"20210818092456.3045808-1-szeder.dev@gmail.com","subject":"Re: [PATCH] column: fix parsing of the '--nl' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-18T20:53:14Z","receivedAt":"2021-08-18T20:53:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> 'git column's '--nl' option can be used to specify a \"string to be\n> printed at the end of each line\" (quoting the man page), but this\n> option and its mandatory argument has been parsed as OPT_INTEGER since\n> the introduction of the command in 7e29b8254f (Add column layout\n> skeleton and git-column, 2012-04-21). Consequently, any non-number\n> argument is rejected by parse-options, and any number other than 0\n> leads to segfault:\n>\n> $ printf \"%s\\n\" one two |git column --mode=plain --nl=foo\n> error: option `nl' expects a numerical value\n> $ printf \"%s\\n\" one two |git column --mode=plain --nl=42\n> Segmentation fault (core dumped)\n> $ printf \"%s\\n\" one two |git column --mode=plain --nl=0\n> one\n> two\n\n... and another thing to notice is that number 0 would have meant\n\"use LF\" due to columns.c::print_columns()\n\n\tnopts.nl = opts && opts->nl ? opts->nl : \"\\n\";\n\nwhich is the same as the default, so it is not likely that people\nhave (mistakenly) used to trigger NUL terminated records, or\nanything fancy like that.\n\n> Parse this option as OPT_STRING.\n\nSo a possible \"regression\" by this fix could be that those who took\nadvantage of the fact that --nl=0 is an absolute no-op would\nsuddenly start seeing their output terminated with a digit \"0\". I\nwould have to say that it is not all that likely ;-)\n\nI agree with Dscho's comment on the test script addition, but other\nthan that this looks good to me.\n\n> Signed-off-by: SZEDER Gábor \n> ---\n> Documentation/git-column.txt | 2 +-\n> builtin/column.c | 2 +-\n> t/t9002-column.sh | 18 ++++++++++++++++++\n> 3 files changed, 20 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/git-column.txt b/Documentation/git-column.txt\n> index f58e9c43e6..6cea9ab463 100644\n> --- a/Documentation/git-column.txt\n> +++ b/Documentation/git-column.txt\n> @@ -39,7 +39,7 @@ OPTIONS\n> --indent=::\n> \tString to be printed at the beginning of each line.\n> \n> ---nl=::\n> +--nl=::\n> \tString to be printed at the end of each line,\n> \tincluding newline character.\n> \n> diff --git a/builtin/column.c b/builtin/column.c\n> index 40d4b3bee2..158fdf53d9 100644\n> --- a/builtin/column.c\n> +++ b/builtin/column.c\n> @@ -29,7 +29,7 @@ int cmd_column(int argc, const char **argv, const char *prefix)\n> \t\tOPT_INTEGER(0, \"raw-mode\", &colopts, N_(\"layout to use\")),\n> \t\tOPT_INTEGER(0, \"width\", &copts.width, N_(\"maximum width\")),\n> \t\tOPT_STRING(0, \"indent\", &copts.indent, N_(\"string\"), N_(\"padding space on left border\")),\n> -\t\tOPT_INTEGER(0, \"nl\", &copts.nl, N_(\"padding space on right border\")),\n> +\t\tOPT_STRING(0, \"nl\", &copts.nl, N_(\"string\"), N_(\"padding space on right border\")),\n> \t\tOPT_INTEGER(0, \"padding\", &copts.padding, N_(\"padding space between columns\")),\n> \t\tOPT_END()\n> \t};\n> diff --git a/t/t9002-column.sh b/t/t9002-column.sh\n> index 89983527b6..6d3dbde3fe 100755\n> --- a/t/t9002-column.sh\n> +++ b/t/t9002-column.sh\n> @@ -42,6 +42,24 @@ EOF\n> \ttest_cmp expected actual\n> '\n> \n> +test_expect_success '--nl' '\n> +\tcat >expected <<\\EOF &&\n> +oneZ\n> +twoZ\n> +threeZ\n> +fourZ\n> +fiveZ\n> +sixZ\n> +sevenZ\n> +eightZ\n> +nineZ\n> +tenZ\n> +elevenZ\n> +EOF\n> +\tgit column --nl=\"Z$LF\" --mode=plain actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> test_expect_success '80 columns' '\n> \tcat >expected <<\\EOF &&\n> one two three four five six seven eight nine ten eleven\n"},{"id":"433882","messageId":"20210826212529.GF2257957@szeder.dev","threadId":"56318","inReplyTo":"xmqqy28y4gqt.fsf@gitster.g","subject":"Re: [PATCH] column: fix parsing of the '--nl' option","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-08-26T21:25:29Z","receivedAt":"2021-08-26T21:25:34Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Aug 18, 2021 at 01:53:14PM -0700, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n> \n> > 'git column's '--nl' option can be used to specify a \"string to be\n> > printed at the end of each line\" (quoting the man page), but this\n> > option and its mandatory argument has been parsed as OPT_INTEGER since\n> > the introduction of the command in 7e29b8254f (Add column layout\n> > skeleton and git-column, 2012-04-21). Consequently, any non-number\n> > argument is rejected by parse-options, and any number other than 0\n> > leads to segfault:\n> >\n> > $ printf \"%s\\n\" one two |git column --mode=plain --nl=foo\n> > error: option `nl' expects a numerical value\n> > $ printf \"%s\\n\" one two |git column --mode=plain --nl=42\n> > Segmentation fault (core dumped)\n> > $ printf \"%s\\n\" one two |git column --mode=plain --nl=0\n> > one\n> > two\n> \n> ... and another thing to notice is that number 0 would have meant\n> \"use LF\" due to columns.c::print_columns()\n> \n> \tnopts.nl = opts && opts->nl ? opts->nl : \"\\n\";\n> \n> which is the same as the default, so it is not likely that people\n> have (mistakenly) used to trigger NUL terminated records, or\n> anything fancy like that.\n> \n> > Parse this option as OPT_STRING.\n> \n> So a possible \"regression\" by this fix could be that those who took\n> advantage of the fact that --nl=0 is an absolute no-op would\n> suddenly start seeing their output terminated with a digit \"0\". I\n> would have to say that it is not all that likely ;-)\n\nYeah, I doubt that it's worth worrying about.\n\n> I agree with Dscho's comment on the test script addition, but other\n> than that this looks good to me.\n\nEleven other tests look just like the one I added. I really don't\nthink that doing it in some other way would gain us anything, but it\nwould be inconsistent with the rest.\n\n> > Signed-off-by: SZEDER Gábor \n> > ---\n> > Documentation/git-column.txt | 2 +-\n> > builtin/column.c | 2 +-\n> > t/t9002-column.sh | 18 ++++++++++++++++++\n> > 3 files changed, 20 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/Documentation/git-column.txt b/Documentation/git-column.txt\n> > index f58e9c43e6..6cea9ab463 100644\n> > --- a/Documentation/git-column.txt\n> > +++ b/Documentation/git-column.txt\n> > @@ -39,7 +39,7 @@ OPTIONS\n> > --indent=::\n> > \tString to be printed at the beginning of each line.\n> > \n> > ---nl=::\n> > +--nl=::\n> > \tString to be printed at the end of each line,\n> > \tincluding newline character.\n> > \n> > diff --git a/builtin/column.c b/builtin/column.c\n> > index 40d4b3bee2..158fdf53d9 100644\n> > --- a/builtin/column.c\n> > +++ b/builtin/column.c\n> > @@ -29,7 +29,7 @@ int cmd_column(int argc, const char **argv, const char *prefix)\n> > \t\tOPT_INTEGER(0, \"raw-mode\", &colopts, N_(\"layout to use\")),\n> > \t\tOPT_INTEGER(0, \"width\", &copts.width, N_(\"maximum width\")),\n> > \t\tOPT_STRING(0, \"indent\", &copts.indent, N_(\"string\"), N_(\"padding space on left border\")),\n> > -\t\tOPT_INTEGER(0, \"nl\", &copts.nl, N_(\"padding space on right border\")),\n> > +\t\tOPT_STRING(0, \"nl\", &copts.nl, N_(\"string\"), N_(\"padding space on right border\")),\n> > \t\tOPT_INTEGER(0, \"padding\", &copts.padding, N_(\"padding space between columns\")),\n> > \t\tOPT_END()\n> > \t};\n> > diff --git a/t/t9002-column.sh b/t/t9002-column.sh\n> > index 89983527b6..6d3dbde3fe 100755\n> > --- a/t/t9002-column.sh\n> > +++ b/t/t9002-column.sh\n> > @@ -42,6 +42,24 @@ EOF\n> > \ttest_cmp expected actual\n> > '\n> > \n> > +test_expect_success '--nl' '\n> > +\tcat >expected <<\\EOF &&\n> > +oneZ\n> > +twoZ\n> > +threeZ\n> > +fourZ\n> > +fiveZ\n> > +sixZ\n> > +sevenZ\n> > +eightZ\n> > +nineZ\n> > +tenZ\n> > +elevenZ\n> > +EOF\n> > +\tgit column --nl=\"Z$LF\" --mode=plain actual &&\n> > +\ttest_cmp expected actual\n> > +'\n> > +\n> > test_expect_success '80 columns' '\n> > \tcat >expected <<\\EOF &&\n> > one two three four five six seven eight nine ten eleven\n"},{"id":"433884","messageId":"xmqqbl5jdh1w.fsf@gitster.g","threadId":"56318","inReplyTo":"20210826212529.GF2257957@szeder.dev","subject":"Re: [PATCH] column: fix parsing of the '--nl' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-26T21:36:43Z","receivedAt":"2021-08-26T21:36:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> Eleven other tests look just like the one I added. I really don't\n> think that doing it in some other way would gain us anything, but it\n> would be inconsistent with the rest.\n\nFair enough.\n"}]}