Re: [PATCH] column: fix parsing of the '--nl' option
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 18, 2021, 20:53 UTC
- Message-ID
- <xmqqy28y4gqt.fsf@gitster.g>
- In-Reply-To
- <20210818092456.3045808-1-szeder.dev@gmail.com>
SZEDER Gábor <szeder.dev@gmail.com> writes:
Show 15 quoted lines
> '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.
Show 62 quoted lines
> 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