On Sun, Nov 20, 2022, 16:01 Tom Schwindl <schwindl_AT_posteo.de> wrote:
>
> Hi Alan,
>
> On Sun Nov 20, 2022 at 1:17 PM CET, Alan Potteiger wrote:
> > On Sun, Nov 20, 2022 at 2:17 AM Sebastian LaVine <mail_AT_smlavine.com> wrote:
> > >
> > > This only reads a single character, not a line as the standard states.
> > > If the user responds with 'y\n', then that '\n' is going to be read if
> > > the program takes any more input. I suggest using getline(3) instead.
> > >
> > > [0]: https://www.man7.org/linux/man-pages/man1/cp.1p.html
> > >
> >
> > My mistake. I misunderstood how the buffering works in stdio, I played
> > with it and
> > saw exactly what you mean. Switched to getline(3).
> >
> > ---
> > Makefile | 1 +
> > libutil/promptyn.c | 26 ++++++++++++++++++++++++++
> > util.h | 1 +
> > 3 files changed, 28 insertions(+)
> > create mode 100644 libutil/promptyn.c
> >
> > diff --git a/Makefile b/Makefile
> > index 3243b1c..0a5c930 100644
> > --- a/Makefile
> > +++ b/Makefile
> > _AT_@ -63,6 +63,7 @@ LIBUTILSRC =\
> > libutil/mkdirp.c\
> > libutil/mode.c\
> > libutil/parseoffset.c\
> > + libutil/promptyn.c\
> > libutil/putword.c\
> > libutil/reallocarray.c\
> > libutil/recurse.c\
> > diff --git a/libutil/promptyn.c b/libutil/promptyn.c
> > new file mode 100644
> > index 0000000..b63bfaa
> > --- /dev/null
> > +++ b/libutil/promptyn.c
> > _AT_@ -0,0 +1,26 @@
> > +/* See LICENSE file for copyright and license details. */
> > +#include <stdio.h>
> > +#include <stdarg.h>
> > +
> > +int
> > +promptyn(const char *nomsg, const char *promptfmt, ...)
> > +{
> > + va_list ap;
> > + char *linep = NULL;
> > + size_t linecapp = 1;
> > +
> > + va_start(ap, promptfmt);
> > + vfprintf(stderr, promptfmt, ap);
> > + va_end(ap);
> > +
> > + fprintf(stderr, " (y/n [n]): ");
> > + getline(&linep, &linecapp, stdin);
> > + switch (*linep) {
> > + case 'y': case 'Y':
> > + return 0;
> > + default:
> > + fprintf(stderr, "%s\n", nomsg);
> > + return 1;
> > + }
> > +}
> > +
>
> You should check if getline(3) failed before dereferencing the pointer.
> linecapp should be zero (or not initialized) since its a throw away variable.
> linep should be freed.
>
> I don't quite get why there is a variable argument list instead of just using
> a single argument which contains the prompt. I can't see the need for formatting
> in a y/n prompt. I also wouldn't use `nomsg' since it just polutes stderr.
> Additionally, you should fix your style in this diff.
>
> I'm not sure if we even _want_ to implement the `-i' option.
> Someone with more knowledge about the sbase design history could elaborate a bit
> further on this.
>
> --
> Best Regards,
> Tom Schwindl
Apologies, I've never sent patches in like this before so this is all
a bit of a learning curve for me. I appreciate your review.
I suppose it could be simplified by removing the formatting. Then
having the utilities themselves handle the prompts and responses
themselves. My intention was to try and avoid repeating code but in
such small amounts it doesn't matter.
The thought was different utilities would be asking for confirmation
on different things, `cp` is wanting confirmation on overwriting of a
file, rm is wanting confirmation on the deletion of a file, etc.
I just looked through the README and noticed cp, mv, and rm all said
they were missing -i so I thought I'd look into it. Understandable if
its not actually desired but I assumed as much if it was mentioned in
the README.
I'll fix it up and await confirmation on whether -i is desired.
Appreciate the time
Received on Sun Nov 20 2022 - 16:39:39 CET