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
>
