Re: [PATCH 01/11] builtin rebase: support --onto

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

Pratik Karki <predatoramigo@gmail.com> writes:

> The `--onto` option is important, as it allows to rebase a range of
> commits onto a different base commit (which gave the command its odd
> name: "rebase").

Is there anything unimportant?  A rhetorical question, of course.

The quite casual and natural use of "to rebase" as a verb in the
first sentence contradicts with what the parenthetical "its odd
name" comment says.  Perhaps drop everything after "(which..."?

i.e.

	The `--onto` option allows to rebase a range of commits onto
	a different base commit.  Port the support for the option to
	the C re-implementation.

> This commit introduces options parsing so that different options can
> be added in future commits.

We usually do not say "This commit does X", or (worse) "I do X in
this commit".  Instead, order the codebase to be like so, e.g.
"Support command line options by adding a call to parse_options();
later commits will add more options by building on top." or
something like that.

> @@ -318,13 +334,22 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  			BUG("sane_execvp() returned???");
>  	}
>  
> -	if (argc != 2)
> -		die(_("Usage: %s <base>"), argv[0]);
> +	if (argc == 2 && !strcmp(argv[1], "-h"))
> +		usage_with_options(builtin_rebase_usage,
> +				   builtin_rebase_options);
> +
>  	prefix = setup_git_directory();
>  	trace_repo_setup(prefix);
>  	setup_work_tree();
>
>  	git_config(git_default_config, NULL);
> +	argc = parse_options(argc, argv, prefix,
> +			     builtin_rebase_options,
> +			     builtin_rebase_usage, 0);
> +
> +	if (argc > 2)
> +		usage_with_options(builtin_rebase_usage,
> +				   builtin_rebase_options);

OK.  This correctly calls the parser after repository setup.

>  	switch (options.type) {
>  	case REBASE_MERGE:
> @@ -343,10 +368,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	}
>  
>  	if (!options.root) {
> -		if (argc < 2)
> +		if (argc < 1)
>  			die("TODO: handle @{upstream}");
>  		else {
> -			options.upstream_name = argv[1];
> +			options.upstream_name = argv[0];
>  			argc--;
>  			argv++;
>  			if (!strcmp(options.upstream_name, "-"))
> @@ -377,7 +402,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	 * orig_head -- commit object name of tip of the branch before rebasing
>  	 * head_name -- refs/heads/<that-branch> or "detached HEAD"
>  	 */
> -	if (argc > 1)
> +	if (argc > 0)
>  		 die("TODO: handle switch_to");
>  	else {
>  		/* Do not need to switch branches, we are already on it. */