git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Nov 13, 2018, 13:05 UTC
Message-ID
<xmqqlg5xunzh.fsf@gitster-ct.c.googlers.com>
In-Reply-To
<dc36a450680b1854abbb9bb06180001ce68c3f3b.1542112703.git.gitgitgadget@gmail.com>

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

Show 10 quoted lines
> 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.
Show 18 quoted lines
> @@ -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.

Show 7 quoted lines
> +		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.
Previous: Johannes Schindelin via GitGitGadgetNext: Phillip Wood
Message 3 of 26 in “rebase: understand -C again, refactor”
  1. 0/1 rebase: understand -C again, refactorJohannes Schindelin via GitGitGadget, Nov 13, 2018
  2. 1/1 rebase: really just passthru the `git am` optionsJohannes Schindelin via GitGitGadget, Nov 13, 2018
  3. Junio C HamanoNov 13, 2018
  4. Phillip WoodNov 13, 2018
  5. Johannes SchindelinNov 13, 2018
  6. Phillip WoodNov 13, 2018
  7. rebase-in-C stability for 2.20Ævar Arnfjörð Bjarmason, Nov 13, 2018
  8. Stefan BellerNov 14, 2018
  9. 0/2 rebase.useBuiltin doc & test modeÆvar Arnfjörð Bjarmason, Nov 14, 2018
  10. Johannes SchindelinNov 14, 2018
  11. 1/2 rebase doc: document rebase.useBuiltinÆvar Arnfjörð Bjarmason, Nov 14, 2018
  12. 2/2 tests: add a special setup where rebase.useBuiltin is offÆvar Arnfjörð Bjarmason, Nov 14, 2018
  13. Elijah NewrenNov 14, 2018
  14. Junio C HamanoNov 14, 2018
  15. Johannes SchindelinNov 14, 2018
  16. Jeff KingNov 14, 2018
  17. Johannes SchindelinNov 14, 2018
  18. 0/2 rebase: understand -C again, refactorJohannes Schindelin via GitGitGadget, Nov 14, 2018
  19. 1/2 rebase: really just passthru the `git am` optionsJohannes Schindelin via GitGitGadget, Nov 14, 2018
  20. 2/2 rebase: validate -C<n> and --whitespace=<mode> parameters earlyJohannes Schindelin via GitGitGadget, Nov 14, 2018
  21. Phillip WoodNov 14, 2018
  22. Johannes SchindelinNov 14, 2018
  23. Ævar Arnfjörð BjarmasonNov 19, 2018
  24. Johannes SchindelinNov 20, 2018
  25. rebase: mark a test as failing with rebase.useBuiltin=falseÆvar Arnfjörð Bjarmason, Nov 20, 2018
  26. Johannes SchindelinNov 20, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.