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

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

-- 
2.33.0.453.gc5e41af357

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.

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