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

[Junio C Hamano <gitster@pobox.com>]

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