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

2 messages, 2 authors, 2018-08-24 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2018-08-08 19:02:42

Pratik Karki [off-list ref] 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.

quoted hunk

@@ -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.

quoted hunk

 	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. */

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

From: Johannes Schindelin <hidden>
Date: 2018-08-24 16:21:22

Hi Junio,

On Wed, 8 Aug 2018, Junio C Hamano wrote:

Pratik Karki [off-list ref] writes:

quoted

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.

You might think it is a rhetorical question, but obviously it is not, as
your reaction testifies.

But certainly there are more important options and less important options!
The most important options are those that are frequently used.

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.

I'd rather keep it.

Remember, a story is easier to read than a dull academic treatise. I want
to have a little bit of a personal touch when I stumble over these commit
messages again. And I know I will.

quoted

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".

Oh, don't we now? ;-)

(This *was* a rhetorical question, as *I* use this tense all the time, and
unless you have quietly rewritten my commit messages without my knowledge
nor consent, the Git commit history is full of these instances.)

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.

To be quite frank with you, I hoped for a review that would focus a teeny
tiny bit on the correctness of the code.

If you want to continue to nit-pick the commit messages, that's fine, of
course, but do understand that I am not really prepared to change a whole
lot there, unless you point out outright errors or false statements. Those
naturally need fixing.

Also, please note that I will now *definitely* focus on bug fixes, as I am
really eager to get those speed improvements into Git for Windows v2.19.0.

And I don't know whether I have said this publicly yet: I will send the
next iterations of Pratik's patch series. He is busy with exams (GSoC
really caters for US schedules, students who are in countries with very
different university schedules are a bit out of luck here), and I really
want these patches.

Ciao,
Dscho

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help