On Wed, Aug 18, 2021 at 01:53:14PM -0700, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@xxxxxxxxx> 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 ;-)
Yeah, I doubt that it's worth worrying about.
> I agree with Dscho's comment on the test script addition, but other
> than that this looks good to me.
Eleven other tests look just like the one I added. I really don't
think that doing it in some other way would gain us anything, but it
would be inconsistent with the rest.
> > 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
> > + 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
