Re: [PATCH] column: fix parsing of the '--nl' option
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Aug 18, 2021, 11:48 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2108181347010.55@tvgsbejvaqbjf.bet>
- In-Reply-To
- <20210818092456.3045808-1-szeder.dev@gmail.com>
Hi Gábor,
On Wed, 18 Aug 2021, SZEDER Gábor wrote:
Show 17 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 > > Parse this option as OPT_STRING.
Whoa. Nice catch. I have just one suggestion about the patch, below.
Show 55 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
> +EOFOr 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
Show 11 quoted lines
> + 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 > >