Re: [PATCH] column: fix parsing of the '--nl' option

SZEDER Gábor [off-list ref] 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.

On Wed, Aug 18, 2021 at 01:53:14PM -0700, Junio C Hamano wrote:

SZEDER Gábor [off-list ref] writes:

quoted

'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.

quoted

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.

quoted

Signed-off-by: SZEDER Gábor <redacted>
---
 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