Re: [PATCH] column: fix parsing of the '--nl' option

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Wed, Aug 18, 2021 at 01:53:14PM -0700, Junio C Hamano wrote:
> 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 ;-)

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@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