Re: [PATCH 1/1] rebase: really just passthru the `git am` options

[Junio C Hamano <gitster@pobox.com>]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> However, there is a much better way (that I was unaware of, at the time
> when I mentored Pratik to implement these options): OPT_PASSTHRU_ARGV.
> It is intended for exactly this use case, where command-line options
> want to be parsed into a separate `argv_array`.
>
> Let's use this feature.
>
> Incidentally, this also allows us to address a bug discovered by Phillip
> Wood, where the built-in rebase failed to understand that the `-C`
> option takes an optional argument.

The resulting code does show what is going on more clearly.  I
wonder if we can do the same for -S as well?  It needs to be passed
to other backends, so I guess it is not such a good idea.

> -		.git_am_opt = STRBUF_INIT,
> +		.git_am_opts = ARGV_ARRAY_INIT,

Yes, this is much nicer.

> @@ -856,10 +858,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		{ OPTION_STRING, 'S', "gpg-sign", &gpg_sign, N_("key-id"),
>  			N_("GPG-sign commits"),
>  			PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
> -		OPT_STRING_LIST(0, "whitespace", &whitespace,
> -				N_("whitespace"), N_("passed to 'git apply'")),
> -		OPT_SET_INT('C', NULL, &opt_c, N_("passed to 'git apply'"),
> -			    REBASE_AM),
>  		OPT_BOOL(0, "autostash", &options.autostash,
>  			 N_("automatically stash/stash pop before and after")),
>  		OPT_STRING_LIST('x', "exec", &exec, N_("exec"),
> @@ -884,6 +882,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  			 N_("rebase all reachable commits up to the root(s)")),
>  		OPT_END(),
>  	};
> +	int i;
>  
> +	for (i = 0; i < options.git_am_opts.argc; i++) {

Yes, this is very nice way to first collect and then inspect them
separately.

> +		const char *option = options.git_am_opts.argv[i];
> +		if (!strcmp(option, "--committer-date-is-author-date") ||
> +		    !strcmp(option, "--ignore-date") ||
> +		    !strcmp(option, "--whitespace=fix") ||
> +		    !strcmp(option, "--whitespace=strip"))
> +			options.flags |= REBASE_FORCE;
>  	}

Overall very nicely done.  Perhaps we'll see a test or two from
Philip?

Thanks.