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.
> Signed-off-by: SZEDER Gábor <szeder.dev@xxxxxxxxx>
> ---
> 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
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
>
>
