From: Junio C Hamano <gitster@pobox•com>
To: "SZEDER Gábor" <szeder.dev@gmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH] column: fix parsing of the '--nl' option
Date: Wed, 18 Aug 2021 13:53:14 -0700 [thread overview]
Message-ID: <xmqqy28y4gqt.fsf@gitster.g> (raw)
In-Reply-To: <20210818092456.3045808-1-szeder.dev@gmail.com> ("SZEDER Gábor"'s message of "Wed, 18 Aug 2021 11:24:56 +0200")
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
next prev parent reply other threads:[~2021-08-18 20:53 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-18 9:24 [PATCH] column: fix parsing of the '--nl' option SZEDER Gábor
2021-08-18 11:48 ` Johannes Schindelin
2021-08-18 20:53 ` Junio C Hamano [this message]
2021-08-26 21:25 ` SZEDER Gábor
2021-08-26 21:36 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqy28y4gqt.fsf@gitster.g \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=szeder.dev@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox