argp: argp.doc prints incorrectly when it starts with "\v" [BZ #19038]

  Thanks Joseph for the review.
Here is a patch with the required changes.
Please let me know if more changes are needed in it.

argp/argp-help.c: added validation for leading \v in the doc string.
argp/tst-argp3.c: added test case when the doc string contains leading \v.
argp/Makefile: added tst-argp3 to tests

Overview:
argp.doc prints incorrectly when it starts with '\v'.
In argp-help.c in the function  variable  is being
initialized only once, which causes the whole documentation to be printed if
there is a leading '\v' in the doc string.

Implementation:
It needs to be initialized for every child in the doc and it needs to be printed
only in two cases.
1. There are no children and the doc does not starts with '\v'.
2. Argument  to the function  is false and the
complete doc needs to be printed.
---
 argp/Makefile    |  2 +-
 argp/argp-help.c | 10 +++++--
 argp/tst-argp3.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+), 3 deletions(-)
 create mode 100644 argp/tst-argp3.c
  

Could anyone please review this patch?
Thanks.

Girish Joshi
girishjoshi.io

On Mon, Nov 25, 2019 at 11:38 PM Girish Joshi <girish946@gmail.com> wrote:
>
> Thanks Joseph for the review.
> Here is a patch with the required changes.
> Please let me know if more changes are needed in it.
>
> argp/argp-help.c: added validation for leading \v in the doc string.
> argp/tst-argp3.c: added test case when the doc string contains leading \v.
> argp/Makefile: added tst-argp3 to tests
>
> Overview:
> argp.doc prints incorrectly when it starts with '\v'.
> In argp-help.c in the function  variable  is being
> initialized only once, which causes the whole documentation to be printed if
> there is a leading '\v' in the doc string.
>
> Implementation:
> It needs to be initialized for every child in the doc and it needs to be printed
> only in two cases.
> 1. There are no children and the doc does not starts with '\v'.
> 2. Argument  to the function  is false and the
> complete doc needs to be printed.
> ---
>  argp/Makefile    |  2 +-
>  argp/argp-help.c | 10 +++++--
>  argp/tst-argp3.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 77 insertions(+), 3 deletions(-)
>  create mode 100644 argp/tst-argp3.c
>
> diff --git a/argp/Makefile b/argp/Makefile
> index c97e4c307c..8206da8cd8 100644
> --- a/argp/Makefile
> +++ b/argp/Makefile
> @@ -27,7 +27,7 @@ routines    = $(addprefix argp-, ba fmtstream
> fs-xinl help parse pv \
>                       pvh xinl eexst)
>
>  tests        = argp-test tst-argp1 bug-argp1 tst-argp2 bug-argp2 \
> -          tst-ldbl-argp
> +          tst-ldbl-argp tst-argp3
>
>  CFLAGS-argp-help.c += $(uses-callbacks) -fexceptions
>  CFLAGS-argp-parse.c += $(uses-callbacks)
> diff --git a/argp/argp-help.c b/argp/argp-help.c
> index 85f5792bfe..68422127d2 100644
> --- a/argp/argp-help.c
> +++ b/argp/argp-help.c
> @@ -1465,10 +1465,11 @@ argp_doc (const struct argp *argp, const
> struct argp_state *state,
>    size_t inp_text_limit = 0;
>    const char *doc = dgettext (argp->argp_domain, argp->doc);
>    const struct argp_child *child = argp->children;
> +  char *vt = 0;
>
>    if (doc)
>      {
> -      char *vt = strchr (doc, '\v');
> +      vt = strchr (doc, '\v');
>        inp_text = post ? (vt ? vt + 1 : 0) : doc;
>        inp_text_limit = (!post && vt) ? (vt - doc) : 0;
>      }
> @@ -1499,7 +1500,12 @@ argp_doc (const struct argp *argp, const struct
> argp_state *state,
>        if (text == inp_text && inp_text_limit)
>      __argp_fmtstream_write (stream, inp_text, inp_text_limit);
>        else
> -    __argp_fmtstream_puts (stream, text);
> +      {
> +        if ((!vt && !child) || (text == inp_text && !first_only))
> +        {
> +          __argp_fmtstream_puts (stream, text);
> +        }
> +      }
>
>        if (__argp_fmtstream_point (stream) > __argp_fmtstream_lmargin (stream))
>      __argp_fmtstream_putc (stream, '\n');
> diff --git a/argp/tst-argp3.c b/argp/tst-argp3.c
> new file mode 100644
> index 0000000000..997072ba7d
> --- /dev/null
> +++ b/argp/tst-argp3.c
> @@ -0,0 +1,68 @@
> +/* Test for argparse with leading '\v' in the doc string.
> +  Copyright (C) 2019 Free Software Foundation, Inc.
> +
> +  This program is free software: you can redistribute it and/or modify
> +  it under the terms of the GNU General Public License as published by
> +  the Free Software Foundation; either version 3 of the License, or
> +  (at your option) any later version.
> +
> +  This program is distributed in the hope that it will be useful,
> +  but WITHOUT ANY WARRANTY; without even the implied warranty of
> +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +  GNU General Public License for more details.
> +
> +  You should have received a copy of the GNU General Public License
> +  along with this program.  If not, see <https://www.gnu.org/licenses/>.*/
> +
> +
> +#include<stdlib.h>
> +#include<argp.h>
> +
> +#include <support/capture_subprocess.h>
> +#include <support/check.h>
> +
> +
> +static char expected_success[] = "Usage: arp [OPTION...]\n\
> +\n\
> +  -?, --help                 Give this help list\n\
> +      --usage                Give a short usage message\n\
> +\n\
> +this is post_doc\n\
> +";
> +char *argv[3] = { (char *) "arp", NULL, NULL };
> +
> +static void
> +do_test_call (void)
> +{
> +  static char doc[] = "\vthis is post_doc";
> +  static struct argp argp = {NULL, NULL, NULL, doc};
> +
> +  argp_parse (&argp, 2, argv, 0, 0, NULL);
> +}
> +
> +static int
> +do_one_test (const char *expected)
> +{
> +  struct support_capture_subprocess result;
> +  result = support_capture_subprocess ((void *) &do_test_call, NULL);
> +
> +  TEST_COMPARE_STRING (result.out.buffer, expected);
> +
> +  return 0;
> +}
> +
> +
> +static int
> +do_test (void)
> +{
> +  const char *argument = "--help";
> +  argv[1] = (char *)argument;
> +  // success condition
> +  do_one_test (expected_success);
> +  return 0;
> +}
> +
> +/* This file references do_test above and contains the definition of
> +   the main function.  */
> +#include <support/test-driver.c>
> +
> --
> 2.21.0
>
>
> Girish Joshi
> girishjoshi.io
  

On 11/30/19 9:47 AM, Girish Joshi wrote:
> Could anyone please review this patch?
Do you have copyright assignment with the FSF for your glibc work?

Please have a look through:
https://sourceware.org/glibc/wiki/Contribution%20checklist

If you have any questions I'm happy to help.
  

Hello Carlos

> Do you have copyright assignment with the FSF for your glibc work?

Yes, now I have it.

Girish Joshi
girishjoshi.io
  

Hello,

If this patch is good enough, could someone please commit it?
Thanks.

On Mon, Nov 25, 2019 at 11:38 PM Girish Joshi <girish946@gmail.com> wrote:
>
> Thanks Joseph for the review.
> Here is a patch with the required changes.
> Please let me know if more changes are needed in it.
>
> argp/argp-help.c: added validation for leading \v in the doc string.
> argp/tst-argp3.c: added test case when the doc string contains leading \v.
> argp/Makefile: added tst-argp3 to tests
>
> Overview:
> argp.doc prints incorrectly when it starts with '\v'.
> In argp-help.c in the function  variable  is being
> initialized only once, which causes the whole documentation to be printed if
> there is a leading '\v' in the doc string.
>
> Implementation:
> It needs to be initialized for every child in the doc and it needs to be printed
> only in two cases.
> 1. There are no children and the doc does not starts with '\v'.
> 2. Argument  to the function  is false and the
> complete doc needs to be printed.
> ---
>  argp/Makefile    |  2 +-
>  argp/argp-help.c | 10 +++++--
>  argp/tst-argp3.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 77 insertions(+), 3 deletions(-)
>  create mode 100644 argp/tst-argp3.c
>
> diff --git a/argp/Makefile b/argp/Makefile
> index c97e4c307c..8206da8cd8 100644
> --- a/argp/Makefile
> +++ b/argp/Makefile
> @@ -27,7 +27,7 @@ routines    = $(addprefix argp-, ba fmtstream
> fs-xinl help parse pv \
>                       pvh xinl eexst)
>
>  tests        = argp-test tst-argp1 bug-argp1 tst-argp2 bug-argp2 \
> -          tst-ldbl-argp
> +          tst-ldbl-argp tst-argp3
>
>  CFLAGS-argp-help.c += $(uses-callbacks) -fexceptions
>  CFLAGS-argp-parse.c += $(uses-callbacks)
> diff --git a/argp/argp-help.c b/argp/argp-help.c
> index 85f5792bfe..68422127d2 100644
> --- a/argp/argp-help.c
> +++ b/argp/argp-help.c
> @@ -1465,10 +1465,11 @@ argp_doc (const struct argp *argp, const
> struct argp_state *state,
>    size_t inp_text_limit = 0;
>    const char *doc = dgettext (argp->argp_domain, argp->doc);
>    const struct argp_child *child = argp->children;
> +  char *vt = 0;
>
>    if (doc)
>      {
> -      char *vt = strchr (doc, '\v');
> +      vt = strchr (doc, '\v');
>        inp_text = post ? (vt ? vt + 1 : 0) : doc;
>        inp_text_limit = (!post && vt) ? (vt - doc) : 0;
>      }
> @@ -1499,7 +1500,12 @@ argp_doc (const struct argp *argp, const struct
> argp_state *state,
>        if (text == inp_text && inp_text_limit)
>      __argp_fmtstream_write (stream, inp_text, inp_text_limit);
>        else
> -    __argp_fmtstream_puts (stream, text);
> +      {
> +        if ((!vt && !child) || (text == inp_text && !first_only))
> +        {
> +          __argp_fmtstream_puts (stream, text);
> +        }
> +      }
>
>        if (__argp_fmtstream_point (stream) > __argp_fmtstream_lmargin (stream))
>      __argp_fmtstream_putc (stream, '\n');
> diff --git a/argp/tst-argp3.c b/argp/tst-argp3.c
> new file mode 100644
> index 0000000000..997072ba7d
> --- /dev/null
> +++ b/argp/tst-argp3.c
> @@ -0,0 +1,68 @@
> +/* Test for argparse with leading '\v' in the doc string.
> +  Copyright (C) 2019 Free Software Foundation, Inc.
> +
> +  This program is free software: you can redistribute it and/or modify
> +  it under the terms of the GNU General Public License as published by
> +  the Free Software Foundation; either version 3 of the License, or
> +  (at your option) any later version.
> +
> +  This program is distributed in the hope that it will be useful,
> +  but WITHOUT ANY WARRANTY; without even the implied warranty of
> +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +  GNU General Public License for more details.
> +
> +  You should have received a copy of the GNU General Public License
> +  along with this program.  If not, see <https://www.gnu.org/licenses/>.*/
> +
> +
> +#include<stdlib.h>
> +#include<argp.h>
> +
> +#include <support/capture_subprocess.h>
> +#include <support/check.h>
> +
> +
> +static char expected_success[] = "Usage: arp [OPTION...]\n\
> +\n\
> +  -?, --help                 Give this help list\n\
> +      --usage                Give a short usage message\n\
> +\n\
> +this is post_doc\n\
> +";
> +char *argv[3] = { (char *) "arp", NULL, NULL };
> +
> +static void
> +do_test_call (void)
> +{
> +  static char doc[] = "\vthis is post_doc";
> +  static struct argp argp = {NULL, NULL, NULL, doc};
> +
> +  argp_parse (&argp, 2, argv, 0, 0, NULL);
> +}
> +
> +static int
> +do_one_test (const char *expected)
> +{
> +  struct support_capture_subprocess result;
> +  result = support_capture_subprocess ((void *) &do_test_call, NULL);
> +
> +  TEST_COMPARE_STRING (result.out.buffer, expected);
> +
> +  return 0;
> +}
> +
> +
> +static int
> +do_test (void)
> +{
> +  const char *argument = "--help";
> +  argv[1] = (char *)argument;
> +  // success condition
> +  do_one_test (expected_success);
> +  return 0;
> +}
> +
> +/* This file references do_test above and contains the definition of
> +   the main function.  */
> +#include <support/test-driver.c>
> +
> --
> 2.21.0
>


Girish Joshi
girishjoshi.io
  

On 1/20/20 12:21 PM, Girish Joshi wrote:
> If this patch is good enough, could someone please commit it?
> Thanks.

The master branch is currently frozen for the 2.31 release so we'll have to wait
for master to reopens for active development but we can review this patch before
that time.

I haven't reviewed your patch yet and many of us are focused on release testing
at this point in time.

My suggestion is to repost the patch libc-alpha, clearly indicate your current
copyright status (which is that you have assignment), and the results of your
testing etc.

I would structure your patch as if it were a full and completely git commit
and post what is effetively a 'git format-patch HEAD~1' patch which includes
the commit message for review. This way the reviewer can review all parts of
the change.

In summary:
- Repost patch
  - State clarified copyright status.
  - Use 'git format-patch HEAD~1' to get a commit message and patch for review.
  - Make testing results clear.

> On Mon, Nov 25, 2019 at 11:38 PM Girish Joshi <girish946@gmail.com> wrote:
>>
>> Thanks Joseph for the review.
>> Here is a patch with the required changes.
>> Please let me know if more changes are needed in it.
>>
>> argp/argp-help.c: added validation for leading \v in the doc string.
>> argp/tst-argp3.c: added test case when the doc string contains leading \v.
>> argp/Makefile: added tst-argp3 to tests
>>
>> Overview:
>> argp.doc prints incorrectly when it starts with '\v'.
>> In argp-help.c in the function  variable  is being
>> initialized only once, which causes the whole documentation to be printed if
>> there is a leading '\v' in the doc string.
>>
>> Implementation:
>> It needs to be initialized for every child in the doc and it needs to be printed
>> only in two cases.
>> 1. There are no children and the doc does not starts with '\v'.
>> 2. Argument  to the function  is false and the
>> complete doc needs to be printed.
>> ---
>>  argp/Makefile    |  2 +-
>>  argp/argp-help.c | 10 +++++--
>>  argp/tst-argp3.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 77 insertions(+), 3 deletions(-)
>>  create mode 100644 argp/tst-argp3.c
>>
>> diff --git a/argp/Makefile b/argp/Makefile
>> index c97e4c307c..8206da8cd8 100644
>> --- a/argp/Makefile
>> +++ b/argp/Makefile
>> @@ -27,7 +27,7 @@ routines    = $(addprefix argp-, ba fmtstream
>> fs-xinl help parse pv \
>>                       pvh xinl eexst)
>>
>>  tests        = argp-test tst-argp1 bug-argp1 tst-argp2 bug-argp2 \
>> -          tst-ldbl-argp
>> +          tst-ldbl-argp tst-argp3
>>
>>  CFLAGS-argp-help.c += $(uses-callbacks) -fexceptions
>>  CFLAGS-argp-parse.c += $(uses-callbacks)
>> diff --git a/argp/argp-help.c b/argp/argp-help.c
>> index 85f5792bfe..68422127d2 100644
>> --- a/argp/argp-help.c
>> +++ b/argp/argp-help.c
>> @@ -1465,10 +1465,11 @@ argp_doc (const struct argp *argp, const
>> struct argp_state *state,
>>    size_t inp_text_limit = 0;
>>    const char *doc = dgettext (argp->argp_domain, argp->doc);
>>    const struct argp_child *child = argp->children;
>> +  char *vt = 0;
>>
>>    if (doc)
>>      {
>> -      char *vt = strchr (doc, '\v');
>> +      vt = strchr (doc, '\v');
>>        inp_text = post ? (vt ? vt + 1 : 0) : doc;
>>        inp_text_limit = (!post && vt) ? (vt - doc) : 0;
>>      }
>> @@ -1499,7 +1500,12 @@ argp_doc (const struct argp *argp, const struct
>> argp_state *state,
>>        if (text == inp_text && inp_text_limit)
>>      __argp_fmtstream_write (stream, inp_text, inp_text_limit);
>>        else
>> -    __argp_fmtstream_puts (stream, text);
>> +      {
>> +        if ((!vt && !child) || (text == inp_text && !first_only))
>> +        {
>> +          __argp_fmtstream_puts (stream, text);
>> +        }
>> +      }
>>
>>        if (__argp_fmtstream_point (stream) > __argp_fmtstream_lmargin (stream))
>>      __argp_fmtstream_putc (stream, '\n');
>> diff --git a/argp/tst-argp3.c b/argp/tst-argp3.c
>> new file mode 100644
>> index 0000000000..997072ba7d
>> --- /dev/null
>> +++ b/argp/tst-argp3.c
>> @@ -0,0 +1,68 @@
>> +/* Test for argparse with leading '\v' in the doc string.
>> +  Copyright (C) 2019 Free Software Foundation, Inc.
>> +
>> +  This program is free software: you can redistribute it and/or modify
>> +  it under the terms of the GNU General Public License as published by
>> +  the Free Software Foundation; either version 3 of the License, or
>> +  (at your option) any later version.
>> +
>> +  This program is distributed in the hope that it will be useful,
>> +  but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +  GNU General Public License for more details.
>> +
>> +  You should have received a copy of the GNU General Public License
>> +  along with this program.  If not, see <https://www.gnu.org/licenses/>.*/
>> +
>> +
>> +#include<stdlib.h>
>> +#include<argp.h>
>> +
>> +#include <support/capture_subprocess.h>
>> +#include <support/check.h>
>> +
>> +
>> +static char expected_success[] = "Usage: arp [OPTION...]\n\
>> +\n\
>> +  -?, --help                 Give this help list\n\
>> +      --usage                Give a short usage message\n\
>> +\n\
>> +this is post_doc\n\
>> +";
>> +char *argv[3] = { (char *) "arp", NULL, NULL };
>> +
>> +static void
>> +do_test_call (void)
>> +{
>> +  static char doc[] = "\vthis is post_doc";
>> +  static struct argp argp = {NULL, NULL, NULL, doc};
>> +
>> +  argp_parse (&argp, 2, argv, 0, 0, NULL);
>> +}
>> +
>> +static int
>> +do_one_test (const char *expected)
>> +{
>> +  struct support_capture_subprocess result;
>> +  result = support_capture_subprocess ((void *) &do_test_call, NULL);
>> +
>> +  TEST_COMPARE_STRING (result.out.buffer, expected);
>> +
>> +  return 0;
>> +}
>> +
>> +
>> +static int
>> +do_test (void)
>> +{
>> +  const char *argument = "--help";
>> +  argv[1] = (char *)argument;
>> +  // success condition
>> +  do_one_test (expected_success);
>> +  return 0;
>> +}
>> +
>> +/* This file references do_test above and contains the definition of
>> +   the main function.  */
>> +#include <support/test-driver.c>
>> +
>> --
>> 2.21.0
>>
> 
> 
> Girish Joshi
> girishjoshi.io
>