This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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
> 


-- 
Cheers,
Carlos.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]