Re: [GSoC][PATCH v8 18/20] stash: convert `stash--helper.c` into `stash.c`

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Paul,

On Wed, 26 Sep 2018, Paul-Sebastian Ungureanu wrote:

> Hi,
> 
> > >   @@ -1443,9 +1448,10 @@ static int push_stash(int argc, const char
> > > **argv, const char *prefix)
> > >    	OPT_END()
> > >    };
> > >   -	argc = parse_options(argc, argv, prefix, options,
> > > -			     git_stash_helper_push_usage,
> > > -			     0);
> > > +	if (argc)
> > > +		argc = parse_options(argc, argv, prefix, options,
> > > +				     git_stash_push_usage,
> > > +				     0);
> > 
> > Is this `if (argc)` guard necessary?
> 
> Yes because without it, there would be a seg fault when
> calling `git stash`. Why?
> 
> After spawning `git stash`, in `push_stash()`: `argc` would be
> 0 (and `argv` would be `NULL`).

I *think* what you want to do, then, is to pass PARSE_OPT_KEEP_ARGV0 in
the first parse_options() call. In general, this needs to be done any time
you might want to call `parse_options()` on the remaining options.

Looking forward to v10,
Dscho

> `parse_options()` calls `parse_options_start()` which does the following:
> 
> 	ctx->argc = ctx->total = argc - 1;
> 	ctx->argv = argv + 1;
> 
> So, `ctx->argc` would be `-1`. After we are back to `parse_options()`,
> `parse_options_step()` would be called. In `parse_options_step()`:
> 
> 	for (; ctx->argc; ctx->argc--, ctx->argv++)
> 
> Which is an infinite loop, because `ctx->argc` is already -1.
> 
> This check is also done for `git notes list` (function `list()`
> inside `builtin/notes.c`). Now, that I relook at it, it seems to me
> that this is a bug. Probably a better solution would be to check at the
> beginning of `parse_options()` and return early if `argc` is less then one.
> 
> > > @@ -1536,7 +1544,44 @@ int cmd_stash__helper(int argc, const char **argv,
> > > const char *prefix)
> > >    	return !!push_stash(argc, argv, prefix);
> > >    else if (!strcmp(argv[0], "save"))
> > >   		return !!save_stash(argc, argv, prefix);
> > > +	else if (*argv[0] != '-')
> > > +		usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
> > > +			      git_stash_usage, options);
> > > +
> > > +	if (strcmp(argv[0], "-p")) {
> > > +		while (++i < argc && strcmp(argv[i], "--")) {
> > > +			/*
> > > +			 * `akpqu` is a string which contains all short
> > > options,
> > > +			 * except `-m` which is verified separately.
> > > +			 */
> > > +			if ((strlen(argv[i]) == 2) && *argv[i] == '-' &&
> > > +			    strchr("akpqu", argv[i][1]))
> > 
> > I *think* this is missing the `n`.
> 
> I guess that by `n` you are referring to the short option of
> `--no-keep-index`, which is missing because it was also omitted
> in `stash.sh`. I am not sure whether it is worth adding it. In
> case `stash` will learn any other option starting with `n`, this
> might create confusion amongst users.
> 
> Best regards,
> Paul
>