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

[Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>]

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