From: Paul-Sebastian Ungureanu Date: Tue, 25 Sep 2018 22:31:07 GMT Subject: Re: [GSoC][PATCH v8 18/20] stash: convert `stash--helper.c` into `stash.c` Message-ID: <4de1b725-8687-590a-f93f-e80843e5bf8f@gmail.com> In-Reply-To: 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`). `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