Message ID | 20210818092456.3045808-1-szeder.dev@gmail.com (mailing list archive) |
---|---|
State | Accepted |
Commit | c93ca46cf5d0b0cd22e357a6460fa84fd4633440 |
Headers | show |
Series | column: fix parsing of the '--nl' option | expand |
Hi Gábor, On Wed, 18 Aug 2021, SZEDER Gábor wrote: > 'git column's '--nl' option can be used to specify a "string to be > printed at the end of each line" (quoting the man page), but this > option and its mandatory argument has been parsed as OPT_INTEGER since > the introduction of the command in 7e29b8254f (Add column layout > skeleton and git-column, 2012-04-21). Consequently, any non-number > argument is rejected by parse-options, and any number other than 0 > leads to segfault: > > $ printf "%s\n" one two |git column --mode=plain --nl=foo > error: option `nl' expects a numerical value > $ printf "%s\n" one two |git column --mode=plain --nl=42 > Segmentation fault (core dumped) > $ printf "%s\n" one two |git column --mode=plain --nl=0 > one > two > > Parse this option as OPT_STRING. Whoa. Nice catch. I have just one suggestion about the patch, below. > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> > --- > Documentation/git-column.txt | 2 +- > builtin/column.c | 2 +- > t/t9002-column.sh | 18 ++++++++++++++++++ > 3 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/Documentation/git-column.txt b/Documentation/git-column.txt > index f58e9c43e6..6cea9ab463 100644 > --- a/Documentation/git-column.txt > +++ b/Documentation/git-column.txt > @@ -39,7 +39,7 @@ OPTIONS > --indent=<string>:: > String to be printed at the beginning of each line. > > ---nl=<N>:: > +--nl=<string>:: > String to be printed at the end of each line, > including newline character. > > diff --git a/builtin/column.c b/builtin/column.c > index 40d4b3bee2..158fdf53d9 100644 > --- a/builtin/column.c > +++ b/builtin/column.c > @@ -29,7 +29,7 @@ int cmd_column(int argc, const char **argv, const char *prefix) > OPT_INTEGER(0, "raw-mode", &colopts, N_("layout to use")), > OPT_INTEGER(0, "width", &copts.width, N_("maximum width")), > OPT_STRING(0, "indent", &copts.indent, N_("string"), N_("padding space on left border")), > - OPT_INTEGER(0, "nl", &copts.nl, N_("padding space on right border")), > + OPT_STRING(0, "nl", &copts.nl, N_("string"), N_("padding space on right border")), > OPT_INTEGER(0, "padding", &copts.padding, N_("padding space between columns")), > OPT_END() > }; > diff --git a/t/t9002-column.sh b/t/t9002-column.sh > index 89983527b6..6d3dbde3fe 100755 > --- a/t/t9002-column.sh > +++ b/t/t9002-column.sh > @@ -42,6 +42,24 @@ EOF > test_cmp expected actual > ' > > +test_expect_success '--nl' ' > + cat >expected <<\EOF && > +oneZ > +twoZ > +threeZ > +fourZ > +fiveZ > +sixZ > +sevenZ > +eightZ > +nineZ > +tenZ > +elevenZ > +EOF Or maybe we can do this instead? sed "s/\$/Z" <lista >expected && Much shorter, and less error-prone (in case any `--run=[...]` option would change what is in `lista` in the future). Ciao, Dscho > + git column --nl="Z$LF" --mode=plain <lista >actual && > + test_cmp expected actual > +' > + > test_expect_success '80 columns' ' > cat >expected <<\EOF && > one two three four five six seven eight nine ten eleven > -- > 2.33.0.453.gc5e41af357 > >
SZEDER Gábor <szeder.dev@gmail.com> writes: > 'git column's '--nl' option can be used to specify a "string to be > printed at the end of each line" (quoting the man page), but this > option and its mandatory argument has been parsed as OPT_INTEGER since > the introduction of the command in 7e29b8254f (Add column layout > skeleton and git-column, 2012-04-21). Consequently, any non-number > argument is rejected by parse-options, and any number other than 0 > leads to segfault: > > $ printf "%s\n" one two |git column --mode=plain --nl=foo > error: option `nl' expects a numerical value > $ printf "%s\n" one two |git column --mode=plain --nl=42 > Segmentation fault (core dumped) > $ printf "%s\n" one two |git column --mode=plain --nl=0 > one > two ... and another thing to notice is that number 0 would have meant "use LF" due to columns.c::print_columns() nopts.nl = opts && opts->nl ? opts->nl : "\n"; which is the same as the default, so it is not likely that people have (mistakenly) used to trigger NUL terminated records, or anything fancy like that. > Parse this option as OPT_STRING. So a possible "regression" by this fix could be that those who took advantage of the fact that --nl=0 is an absolute no-op would suddenly start seeing their output terminated with a digit "0". I would have to say that it is not all that likely ;-) I agree with Dscho's comment on the test script addition, but other than that this looks good to me. > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> > --- > Documentation/git-column.txt | 2 +- > builtin/column.c | 2 +- > t/t9002-column.sh | 18 ++++++++++++++++++ > 3 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/Documentation/git-column.txt b/Documentation/git-column.txt > index f58e9c43e6..6cea9ab463 100644 > --- a/Documentation/git-column.txt > +++ b/Documentation/git-column.txt > @@ -39,7 +39,7 @@ OPTIONS > --indent=<string>:: > String to be printed at the beginning of each line. > > ---nl=<N>:: > +--nl=<string>:: > String to be printed at the end of each line, > including newline character. > > diff --git a/builtin/column.c b/builtin/column.c > index 40d4b3bee2..158fdf53d9 100644 > --- a/builtin/column.c > +++ b/builtin/column.c > @@ -29,7 +29,7 @@ int cmd_column(int argc, const char **argv, const char *prefix) > OPT_INTEGER(0, "raw-mode", &colopts, N_("layout to use")), > OPT_INTEGER(0, "width", &copts.width, N_("maximum width")), > OPT_STRING(0, "indent", &copts.indent, N_("string"), N_("padding space on left border")), > - OPT_INTEGER(0, "nl", &copts.nl, N_("padding space on right border")), > + OPT_STRING(0, "nl", &copts.nl, N_("string"), N_("padding space on right border")), > OPT_INTEGER(0, "padding", &copts.padding, N_("padding space between columns")), > OPT_END() > }; > diff --git a/t/t9002-column.sh b/t/t9002-column.sh > index 89983527b6..6d3dbde3fe 100755 > --- a/t/t9002-column.sh > +++ b/t/t9002-column.sh > @@ -42,6 +42,24 @@ EOF > test_cmp expected actual > ' > > +test_expect_success '--nl' ' > + cat >expected <<\EOF && > +oneZ > +twoZ > +threeZ > +fourZ > +fiveZ > +sixZ > +sevenZ > +eightZ > +nineZ > +tenZ > +elevenZ > +EOF > + git column --nl="Z$LF" --mode=plain <lista >actual && > + test_cmp expected actual > +' > + > test_expect_success '80 columns' ' > cat >expected <<\EOF && > one two three four five six seven eight nine ten eleven
On Wed, Aug 18, 2021 at 01:53:14PM -0700, Junio C Hamano wrote: > SZEDER Gábor <szeder.dev@gmail.com> writes: > > > 'git column's '--nl' option can be used to specify a "string to be > > printed at the end of each line" (quoting the man page), but this > > option and its mandatory argument has been parsed as OPT_INTEGER since > > the introduction of the command in 7e29b8254f (Add column layout > > skeleton and git-column, 2012-04-21). Consequently, any non-number > > argument is rejected by parse-options, and any number other than 0 > > leads to segfault: > > > > $ printf "%s\n" one two |git column --mode=plain --nl=foo > > error: option `nl' expects a numerical value > > $ printf "%s\n" one two |git column --mode=plain --nl=42 > > Segmentation fault (core dumped) > > $ printf "%s\n" one two |git column --mode=plain --nl=0 > > one > > two > > ... and another thing to notice is that number 0 would have meant > "use LF" due to columns.c::print_columns() > > nopts.nl = opts && opts->nl ? opts->nl : "\n"; > > which is the same as the default, so it is not likely that people > have (mistakenly) used to trigger NUL terminated records, or > anything fancy like that. > > > Parse this option as OPT_STRING. > > So a possible "regression" by this fix could be that those who took > advantage of the fact that --nl=0 is an absolute no-op would > suddenly start seeing their output terminated with a digit "0". I > would have to say that it is not all that likely ;-) Yeah, I doubt that it's worth worrying about. > I agree with Dscho's comment on the test script addition, but other > than that this looks good to me. Eleven other tests look just like the one I added. I really don't think that doing it in some other way would gain us anything, but it would be inconsistent with the rest. > > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> > > --- > > Documentation/git-column.txt | 2 +- > > builtin/column.c | 2 +- > > t/t9002-column.sh | 18 ++++++++++++++++++ > > 3 files changed, 20 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/git-column.txt b/Documentation/git-column.txt > > index f58e9c43e6..6cea9ab463 100644 > > --- a/Documentation/git-column.txt > > +++ b/Documentation/git-column.txt > > @@ -39,7 +39,7 @@ OPTIONS > > --indent=<string>:: > > String to be printed at the beginning of each line. > > > > ---nl=<N>:: > > +--nl=<string>:: > > String to be printed at the end of each line, > > including newline character. > > > > diff --git a/builtin/column.c b/builtin/column.c > > index 40d4b3bee2..158fdf53d9 100644 > > --- a/builtin/column.c > > +++ b/builtin/column.c > > @@ -29,7 +29,7 @@ int cmd_column(int argc, const char **argv, const char *prefix) > > OPT_INTEGER(0, "raw-mode", &colopts, N_("layout to use")), > > OPT_INTEGER(0, "width", &copts.width, N_("maximum width")), > > OPT_STRING(0, "indent", &copts.indent, N_("string"), N_("padding space on left border")), > > - OPT_INTEGER(0, "nl", &copts.nl, N_("padding space on right border")), > > + OPT_STRING(0, "nl", &copts.nl, N_("string"), N_("padding space on right border")), > > OPT_INTEGER(0, "padding", &copts.padding, N_("padding space between columns")), > > OPT_END() > > }; > > diff --git a/t/t9002-column.sh b/t/t9002-column.sh > > index 89983527b6..6d3dbde3fe 100755 > > --- a/t/t9002-column.sh > > +++ b/t/t9002-column.sh > > @@ -42,6 +42,24 @@ EOF > > test_cmp expected actual > > ' > > > > +test_expect_success '--nl' ' > > + cat >expected <<\EOF && > > +oneZ > > +twoZ > > +threeZ > > +fourZ > > +fiveZ > > +sixZ > > +sevenZ > > +eightZ > > +nineZ > > +tenZ > > +elevenZ > > +EOF > > + git column --nl="Z$LF" --mode=plain <lista >actual && > > + test_cmp expected actual > > +' > > + > > test_expect_success '80 columns' ' > > cat >expected <<\EOF && > > one two three four five six seven eight nine ten eleven
SZEDER Gábor <szeder.dev@gmail.com> writes: > Eleven other tests look just like the one I added. I really don't > think that doing it in some other way would gain us anything, but it > would be inconsistent with the rest. Fair enough.
diff --git a/Documentation/git-column.txt b/Documentation/git-column.txt index f58e9c43e6..6cea9ab463 100644 --- a/Documentation/git-column.txt +++ b/Documentation/git-column.txt @@ -39,7 +39,7 @@ OPTIONS --indent=<string>:: String to be printed at the beginning of each line. ---nl=<N>:: +--nl=<string>:: String to be printed at the end of each line, including newline character. diff --git a/builtin/column.c b/builtin/column.c index 40d4b3bee2..158fdf53d9 100644 --- a/builtin/column.c +++ b/builtin/column.c @@ -29,7 +29,7 @@ int cmd_column(int argc, const char **argv, const char *prefix) OPT_INTEGER(0, "raw-mode", &colopts, N_("layout to use")), OPT_INTEGER(0, "width", &copts.width, N_("maximum width")), OPT_STRING(0, "indent", &copts.indent, N_("string"), N_("padding space on left border")), - OPT_INTEGER(0, "nl", &copts.nl, N_("padding space on right border")), + OPT_STRING(0, "nl", &copts.nl, N_("string"), N_("padding space on right border")), OPT_INTEGER(0, "padding", &copts.padding, N_("padding space between columns")), OPT_END() }; diff --git a/t/t9002-column.sh b/t/t9002-column.sh index 89983527b6..6d3dbde3fe 100755 --- a/t/t9002-column.sh +++ b/t/t9002-column.sh @@ -42,6 +42,24 @@ EOF test_cmp expected actual ' +test_expect_success '--nl' ' + cat >expected <<\EOF && +oneZ +twoZ +threeZ +fourZ +fiveZ +sixZ +sevenZ +eightZ +nineZ +tenZ +elevenZ +EOF + git column --nl="Z$LF" --mode=plain <lista >actual && + test_cmp expected actual +' + test_expect_success '80 columns' ' cat >expected <<\EOF && one two three four five six seven eight nine ten eleven
'git column's '--nl' option can be used to specify a "string to be printed at the end of each line" (quoting the man page), but this option and its mandatory argument has been parsed as OPT_INTEGER since the introduction of the command in 7e29b8254f (Add column layout skeleton and git-column, 2012-04-21). Consequently, any non-number argument is rejected by parse-options, and any number other than 0 leads to segfault: $ printf "%s\n" one two |git column --mode=plain --nl=foo error: option `nl' expects a numerical value $ printf "%s\n" one two |git column --mode=plain --nl=42 Segmentation fault (core dumped) $ printf "%s\n" one two |git column --mode=plain --nl=0 one two Parse this option as OPT_STRING. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- Documentation/git-column.txt | 2 +- builtin/column.c | 2 +- t/t9002-column.sh | 18 ++++++++++++++++++ 3 files changed, 20 insertions(+), 2 deletions(-)