git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH v2 0/2] Reimplement rebase --merge via interactive machinery
@ 2018-11-08  6:01 Elijah Newren
  2018-11-08  6:01 ` [PATCH v2 1/2] git-rebase, sequencer: extend --quiet option for the " Elijah Newren
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Elijah Newren @ 2018-11-08  6:01 UTC (permalink / raw)
  To: git; +Cc: gitster, Johannes.Schindelin, predatoramigo, Elijah Newren

Now that the rewrite-interactive-rebases-in-C series have finally
merged to master, this series deletes git-rebase--merge.sh and
reimplements the --merge behavior on top of the interactive machinery.

Differences since v1:
  - Updated code to hook into builtin/rebase.C instead of git-rebase.sh

(No range-diff provided, because it has been months since v1, and v1
was only RFC and was only discussed at a high level.)

Elijah Newren (2):
  git-rebase, sequencer: extend --quiet option for the interactive
    machinery
  rebase: Implement --merge via git-rebase--interactive

 .gitignore                        |   1 -
 Documentation/git-rebase.txt      |  17 +---
 Makefile                          |   1 -
 builtin/rebase.c                  |  15 ++-
 git-legacy-rebase.sh              |  57 +++++------
 git-rebase--common.sh             |   2 +-
 git-rebase--merge.sh              | 164 ------------------------------
 sequencer.c                       |  23 +++--
 sequencer.h                       |   1 +
 t/t3406-rebase-message.sh         |   7 +-
 t/t3420-rebase-autostash.sh       |  78 ++------------
 t/t3421-rebase-topology-linear.sh |  10 +-
 t/t3425-rebase-topology-merges.sh |   6 +-
 t/t5407-post-rewrite-hook.sh      |   1 +
 t/t9903-bash-prompt.sh            |   2 +-
 15 files changed, 67 insertions(+), 318 deletions(-)
 delete mode 100644 git-rebase--merge.sh

-- 
2.19.1.858.g526e8fe740.dirty


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 1/2] git-rebase, sequencer: extend --quiet option for the interactive machinery
  2018-11-08  6:01 [PATCH v2 0/2] Reimplement rebase --merge via interactive machinery Elijah Newren
@ 2018-11-08  6:01 ` " Elijah Newren
  2018-11-12 15:11   ` Johannes Schindelin
  2018-11-08  6:01 ` [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive Elijah Newren
  2018-11-08  6:33 ` [PATCH v2 0/2] Reimplement rebase --merge via interactive machinery Elijah Newren
  2 siblings, 1 reply; 12+ messages in thread
From: Elijah Newren @ 2018-11-08  6:01 UTC (permalink / raw)
  To: git; +Cc: gitster, Johannes.Schindelin, predatoramigo, Elijah Newren

While 'quiet' and 'interactive' may sound like antonyms, the interactive
machinery actually has logic that implements several
interactive_rebase=implied cases (--exec, --keep-empty, --rebase-merges)
which won't pop up an editor.  The rewrite of interactive rebase in C
added a quiet option, though it only turns stats off.  Since we want to
make the interactive machinery also take over for git-rebase--merge, it
should fully implement the --quiet option.

git-rebase--interactive was already somewhat quieter than
git-rebase--merge and git-rebase--am, possibly because cherry-pick has
just traditionally been quieter.  As such, we only drop a few
informational messages -- "Rebasing (n/m)" and "Succesfully rebased..."

Also, for simplicity, remove the differences in how quiet and verbose
options were recorded.  Having one be signalled by the presence of a
"verbose" file in the state_dir, while the other was signalled by the
contents of a "quiet" file was just weirdly inconsistent.  (This
inconsistency pre-dated the rewrite into C.)  Make them consistent by
having them both key off the presence of the file.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/rebase.c      |  5 +----
 git-legacy-rebase.sh  |  2 +-
 git-rebase--common.sh |  2 +-
 sequencer.c           | 23 +++++++++++++----------
 sequencer.h           |  1 +
 5 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0ee06aa363..be004406a6 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -181,10 +181,7 @@ static int read_basic_state(struct rebase_options *opts)
 	if (get_oid(buf.buf, &opts->orig_head))
 		return error(_("invalid orig-head: '%s'"), buf.buf);
 
-	strbuf_reset(&buf);
-	if (read_one(state_dir_path("quiet", opts), &buf))
-		return -1;
-	if (buf.len)
+	if (file_exists(state_dir_path("quiet", opts)))
 		opts->flags &= ~REBASE_NO_QUIET;
 	else
 		opts->flags |= REBASE_NO_QUIET;
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index 75a08b2683..da27bfca5a 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -113,7 +113,7 @@ read_basic_state () {
 	else
 		orig_head=$(cat "$state_dir"/head)
 	fi &&
-	GIT_QUIET=$(cat "$state_dir"/quiet) &&
+	test -f "$state_dir"/quiet && GIT_QUIET=t
 	test -f "$state_dir"/verbose && verbose=t
 	test -f "$state_dir"/strategy && strategy="$(cat "$state_dir"/strategy)"
 	test -f "$state_dir"/strategy_opts &&
diff --git a/git-rebase--common.sh b/git-rebase--common.sh
index 7e39d22871..dc18c682fa 100644
--- a/git-rebase--common.sh
+++ b/git-rebase--common.sh
@@ -10,7 +10,7 @@ write_basic_state () {
 	echo "$head_name" > "$state_dir"/head-name &&
 	echo "$onto" > "$state_dir"/onto &&
 	echo "$orig_head" > "$state_dir"/orig-head &&
-	echo "$GIT_QUIET" > "$state_dir"/quiet &&
+	test t = "$GIT_QUIET" && : > "$state_dir"/quiet
 	test t = "$verbose" && : > "$state_dir"/verbose
 	test -n "$strategy" && echo "$strategy" > "$state_dir"/strategy
 	test -n "$strategy_opts" && echo "$strategy_opts" > \
diff --git a/sequencer.c b/sequencer.c
index 9e1ab3a2a7..bd8337dbf1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -150,6 +150,7 @@ static GIT_PATH_FUNC(rebase_path_refs_to_delete, "rebase-merge/refs-to-delete")
 static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
 static GIT_PATH_FUNC(rebase_path_orig_head, "rebase-merge/orig-head")
 static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
+static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet")
 static GIT_PATH_FUNC(rebase_path_signoff, "rebase-merge/signoff")
 static GIT_PATH_FUNC(rebase_path_head_name, "rebase-merge/head-name")
 static GIT_PATH_FUNC(rebase_path_onto, "rebase-merge/onto")
@@ -157,7 +158,6 @@ static GIT_PATH_FUNC(rebase_path_autostash, "rebase-merge/autostash")
 static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
 static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
 static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
-static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet")
 
 static int git_sequencer_config(const char *k, const char *v, void *cb)
 {
@@ -2307,6 +2307,9 @@ static int read_populate_opts(struct replay_opts *opts)
 		if (file_exists(rebase_path_verbose()))
 			opts->verbose = 1;
 
+		if (file_exists(rebase_path_quiet()))
+			opts->quiet = 1;
+
 		if (file_exists(rebase_path_signoff())) {
 			opts->allow_ff = 0;
 			opts->signoff = 1;
@@ -2374,9 +2377,6 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
 
 	if (quiet)
 		write_file(rebase_path_quiet(), "%s\n", quiet);
-	else
-		write_file(rebase_path_quiet(), "\n");
-
 	if (opts->verbose)
 		write_file(rebase_path_verbose(), "%s", "");
 	if (opts->strategy)
@@ -3450,10 +3450,11 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
 					fprintf(f, "%d\n", todo_list->done_nr);
 					fclose(f);
 				}
-				fprintf(stderr, "Rebasing (%d/%d)%s",
-					todo_list->done_nr,
-					todo_list->total_nr,
-					opts->verbose ? "\n" : "\r");
+				if (!opts->quiet)
+					fprintf(stderr, "Rebasing (%d/%d)%s",
+						todo_list->done_nr,
+						todo_list->total_nr,
+						opts->verbose ? "\n" : "\r");
 			}
 			unlink(rebase_path_message());
 			unlink(rebase_path_author_script());
@@ -3684,8 +3685,10 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
 		}
 		apply_autostash(opts);
 
-		fprintf(stderr, "Successfully rebased and updated %s.\n",
-			head_ref.buf);
+		if (!opts->quiet)
+			fprintf(stderr,
+				"Successfully rebased and updated %s.\n",
+				head_ref.buf);
 
 		strbuf_release(&buf);
 		strbuf_release(&head_ref);
diff --git a/sequencer.h b/sequencer.h
index 660cff5050..45fd977bef 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -39,6 +39,7 @@ struct replay_opts {
 	int allow_empty_message;
 	int keep_redundant_commits;
 	int verbose;
+	int quiet;
 
 	int mainline;
 
-- 
2.19.1.858.g526e8fe740.dirty


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive
  2018-11-08  6:01 [PATCH v2 0/2] Reimplement rebase --merge via interactive machinery Elijah Newren
  2018-11-08  6:01 ` [PATCH v2 1/2] git-rebase, sequencer: extend --quiet option for the " Elijah Newren
@ 2018-11-08  6:01 ` Elijah Newren
  2018-11-12 16:21   ` Johannes Schindelin
  2018-11-08  6:33 ` [PATCH v2 0/2] Reimplement rebase --merge via interactive machinery Elijah Newren
  2 siblings, 1 reply; 12+ messages in thread
From: Elijah Newren @ 2018-11-08  6:01 UTC (permalink / raw)
  To: git; +Cc: gitster, Johannes.Schindelin, predatoramigo, Elijah Newren

Interactive rebases are implemented in terms of cherry-pick rather than
the merge-recursive builtin, but cherry-pick also calls into the recursive
merge machinery by default and can accept special merge strategies and/or
special strategy options.  As such, there really is not any need for
having both git-rebase--merge and git-rebase--interactive anymore.

Delete git-rebase--merge.sh and have the --merge option be implemented
by the now built-in interactive machinery.

Note that this change fixes a few known test failures (see t3421).

testcase modification notes:
  t3406: --interactive and --merge had slightly different progress output
         while running; adjust a test to match
  t3420: these test precise output while running, but rebase--am,
         rebase--merge, and rebase--interactive all were built on very
         different commands (am, merge-recursive, cherry-pick), so the
         tests expected different output for each type.  Now we expect
         --merge and --interactive to have the same output.
  t3421: --interactive fixes some bugs in --merge!  Wahoo!
  t3425: topology linearization was inconsistent across flavors of rebase,
         as already noted in a TODO comment in the testcase.  This was not
         considered a bug before, so getting a different linearization due
         to switching out backends should not be considered a bug now.
  t5407: different rebase types varied slightly in how many times checkout
         or commit or equivalents were called based on a quick comparison
         of this tests and previous ones which covered different rebase
         flavors.  I think this is just attributable to this difference.
  t9903: --merge uses the interactive backend so the prompt expected is
         now REBASE-i.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 .gitignore                        |   1 -
 Documentation/git-rebase.txt      |  17 +---
 Makefile                          |   1 -
 builtin/rebase.c                  |  10 +-
 git-legacy-rebase.sh              |  55 +++++-----
 git-rebase--merge.sh              | 164 ------------------------------
 t/t3406-rebase-message.sh         |   7 +-
 t/t3420-rebase-autostash.sh       |  78 ++------------
 t/t3421-rebase-topology-linear.sh |  10 +-
 t/t3425-rebase-topology-merges.sh |   6 +-
 t/t5407-post-rewrite-hook.sh      |   1 +
 t/t9903-bash-prompt.sh            |   2 +-
 12 files changed, 50 insertions(+), 302 deletions(-)
 delete mode 100644 git-rebase--merge.sh

diff --git a/.gitignore b/.gitignore
index 0d77ea5894..910b1d2d2f 100644
--- a/.gitignore
+++ b/.gitignore
@@ -124,7 +124,6 @@
 /git-rebase--am
 /git-rebase--common
 /git-rebase--interactive
-/git-rebase--merge
 /git-rebase--preserve-merges
 /git-receive-pack
 /git-reflog
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 3407d835bd..35084f5681 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -504,15 +504,7 @@ See also INCOMPATIBLE OPTIONS below.
 INCOMPATIBLE OPTIONS
 --------------------
 
-git-rebase has many flags that are incompatible with each other,
-predominantly due to the fact that it has three different underlying
-implementations:
-
- * one based on linkgit:git-am[1] (the default)
- * one based on git-merge-recursive (merge backend)
- * one based on linkgit:git-cherry-pick[1] (interactive backend)
-
-Flags only understood by the am backend:
+The following options:
 
  * --committer-date-is-author-date
  * --ignore-date
@@ -520,15 +512,12 @@ Flags only understood by the am backend:
  * --ignore-whitespace
  * -C
 
-Flags understood by both merge and interactive backends:
+are incompatible with the following options:
 
  * --merge
  * --strategy
  * --strategy-option
  * --allow-empty-message
-
-Flags only understood by the interactive backend:
-
  * --[no-]autosquash
  * --rebase-merges
  * --preserve-merges
@@ -539,7 +528,7 @@ Flags only understood by the interactive backend:
  * --edit-todo
  * --root when used in combination with --onto
 
-Other incompatible flag pairs:
+In addition, the following pairs of options are incompatible:
 
  * --preserve-merges and --interactive
  * --preserve-merges and --signoff
diff --git a/Makefile b/Makefile
index bbfbb4292d..18e79fdea8 100644
--- a/Makefile
+++ b/Makefile
@@ -628,7 +628,6 @@ SCRIPT_LIB += git-parse-remote
 SCRIPT_LIB += git-rebase--am
 SCRIPT_LIB += git-rebase--common
 SCRIPT_LIB += git-rebase--preserve-merges
-SCRIPT_LIB += git-rebase--merge
 SCRIPT_LIB += git-sh-setup
 SCRIPT_LIB += git-sh-i18n
 
diff --git a/builtin/rebase.c b/builtin/rebase.c
index be004406a6..d55bbb9eb9 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -118,7 +118,7 @@ static void imply_interactive(struct rebase_options *opts, const char *option)
 	case REBASE_PRESERVE_MERGES:
 		break;
 	case REBASE_MERGE:
-		/* we silently *upgrade* --merge to --interactive if needed */
+		/* we now implement --merge via --interactive */
 	default:
 		opts->type = REBASE_INTERACTIVE; /* implied */
 		break;
@@ -475,10 +475,6 @@ static int run_specific_rebase(struct rebase_options *opts)
 		backend = "git-rebase--am";
 		backend_func = "git_rebase__am";
 		break;
-	case REBASE_MERGE:
-		backend = "git-rebase--merge";
-		backend_func = "git_rebase__merge";
-		break;
 	case REBASE_PRESERVE_MERGES:
 		backend = "git-rebase--preserve-merges";
 		backend_func = "git_rebase__preserve_merges";
@@ -1156,6 +1152,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		}
 	}
 
+	if (options.type == REBASE_MERGE) {
+		imply_interactive(&options, "--merge");
+	}
+
 	if (options.root && !options.onto_name)
 		imply_interactive(&options, "--root without --onto");
 
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index da27bfca5a..4abcceed06 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -218,6 +218,7 @@ then
 	state_dir="$apply_dir"
 elif test -d "$merge_dir"
 then
+	type=interactive
 	if test -d "$merge_dir"/rewritten
 	then
 		type=preserve-merges
@@ -225,10 +226,7 @@ then
 		preserve_merges=t
 	elif test -f "$merge_dir"/interactive
 	then
-		type=interactive
 		interactive_rebase=explicit
-	else
-		type=merge
 	fi
 	state_dir="$merge_dir"
 fi
@@ -469,6 +467,7 @@ then
 	test -z "$interactive_rebase" && interactive_rebase=implied
 fi
 
+actually_interactive=
 if test -n "$interactive_rebase"
 then
 	if test -z "$preserve_merges"
@@ -477,11 +476,12 @@ then
 	else
 		type=preserve-merges
 	fi
-
+	actually_interactive=t
 	state_dir="$merge_dir"
 elif test -n "$do_merge"
 then
-	type=merge
+	interactive_rebase=implied
+	type=interactive
 	state_dir="$merge_dir"
 else
 	type=am
@@ -493,21 +493,13 @@ then
 	git_format_patch_opt="$git_format_patch_opt --progress"
 fi
 
-if test -n "$git_am_opt"; then
-	incompatible_opts=$(echo " $git_am_opt " | \
-			    sed -e 's/ -q / /g' -e 's/^ \(.*\) $/\1/')
-	if test -n "$interactive_rebase"
+incompatible_opts=$(echo " $git_am_opt " | \
+		    sed -e 's/ -q / /g' -e 's/^ \(.*\) $/\1/')
+if test -n "$incompatible_opts"
+then
+	if test -n "$actually_interactive" || test "$do_merge"
 	then
-		if test -n "$incompatible_opts"
-		then
-			die "$(gettext "error: cannot combine interactive options (--interactive, --exec, --rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with am options ($incompatible_opts)")"
-		fi
-	fi
-	if test -n "$do_merge"; then
-		if test -n "$incompatible_opts"
-		then
-			die "$(gettext "error: cannot combine merge options (--merge, --strategy, --strategy-option) with am options ($incompatible_opts)")"
-		fi
+		die "$(gettext "error: cannot combine am options ($incompatible_opts) with either interactive or merge options")"
 	fi
 fi
 
@@ -672,7 +664,7 @@ require_clean_work_tree "rebase" "$(gettext "Please commit or stash them.")"
 # but this should be done only when upstream and onto are the same
 # and if this is not an interactive rebase.
 mb=$(git merge-base "$onto" "$orig_head")
-if test -z "$interactive_rebase" && test "$upstream" = "$onto" &&
+if test -z "$actually_interactive" && test "$upstream" = "$onto" &&
 	test "$mb" = "$onto" && test -z "$restrict_revision" &&
 	# linear history?
 	! (git rev-list --parents "$onto".."$orig_head" | sane_grep " .* ") > /dev/null
@@ -716,6 +708,19 @@ then
 	GIT_PAGER='' git diff --stat --summary "$mb" "$onto"
 fi
 
+if test -z "$actually_interactive" && test "$mb" = "$orig_head"
+then
+	# If the $onto is a proper descendant of the tip of the branch, then
+	# we just fast-forwarded.
+	say "$(eval_gettext "Fast-forwarded \$branch_name to \$onto_name.")"
+	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" \
+		git checkout -q "$onto^0" || die "could not detach HEAD"
+	git update-ref ORIG_HEAD $orig_head
+	move_to_original_branch
+	finish_rebase
+	exit 0
+fi
+
 test -n "$interactive_rebase" && run_specific_rebase
 
 # Detach HEAD and reset the tree
@@ -725,16 +730,6 @@ GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" \
 	git checkout -q "$onto^0" || die "could not detach HEAD"
 git update-ref ORIG_HEAD $orig_head
 
-# If the $onto is a proper descendant of the tip of the branch, then
-# we just fast-forwarded.
-if test "$mb" = "$orig_head"
-then
-	say "$(eval_gettext "Fast-forwarded \$branch_name to \$onto_name.")"
-	move_to_original_branch
-	finish_rebase
-	exit 0
-fi
-
 if test -n "$rebase_root"
 then
 	revisions="$onto..$orig_head"
diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
deleted file mode 100644
index aa2f2f0872..0000000000
--- a/git-rebase--merge.sh
+++ /dev/null
@@ -1,164 +0,0 @@
-# This shell script fragment is sourced by git-rebase to implement
-# its merge-based non-interactive mode that copes well with renamed
-# files.
-#
-# Copyright (c) 2010 Junio C Hamano.
-#
-
-prec=4
-
-read_state () {
-	onto_name=$(cat "$state_dir"/onto_name) &&
-	end=$(cat "$state_dir"/end) &&
-	msgnum=$(cat "$state_dir"/msgnum)
-}
-
-continue_merge () {
-	test -d "$state_dir" || die "$state_dir directory does not exist"
-
-	unmerged=$(git ls-files -u)
-	if test -n "$unmerged"
-	then
-		echo "You still have unmerged paths in your index"
-		echo "did you forget to use git add?"
-		die "$resolvemsg"
-	fi
-
-	cmt=$(cat "$state_dir/current")
-	if ! git diff-index --quiet --ignore-submodules HEAD --
-	then
-		if ! git commit ${gpg_sign_opt:+"$gpg_sign_opt"} $signoff $allow_empty_message \
-			--no-verify -C "$cmt"
-		then
-			echo "Commit failed, please do not call \"git commit\""
-			echo "directly, but instead do one of the following: "
-			die "$resolvemsg"
-		fi
-		if test -z "$GIT_QUIET"
-		then
-			printf "Committed: %0${prec}d " $msgnum
-		fi
-		echo "$cmt $(git rev-parse HEAD^0)" >> "$state_dir/rewritten"
-	else
-		if test -z "$GIT_QUIET"
-		then
-			printf "Already applied: %0${prec}d " $msgnum
-		fi
-	fi
-	test -z "$GIT_QUIET" &&
-	GIT_PAGER='' git log --format=%s -1 "$cmt"
-
-	# onto the next patch:
-	msgnum=$(($msgnum + 1))
-	echo "$msgnum" >"$state_dir/msgnum"
-}
-
-call_merge () {
-	msgnum="$1"
-	echo "$msgnum" >"$state_dir/msgnum"
-	cmt="$(cat "$state_dir/cmt.$msgnum")"
-	echo "$cmt" > "$state_dir/current"
-	git update-ref REBASE_HEAD "$cmt"
-	hd=$(git rev-parse --verify HEAD)
-	cmt_name=$(git symbolic-ref HEAD 2> /dev/null || echo HEAD)
-	eval GITHEAD_$cmt='"${cmt_name##refs/heads/}~$(($end - $msgnum))"'
-	eval GITHEAD_$hd='$onto_name'
-	export GITHEAD_$cmt GITHEAD_$hd
-	if test -n "$GIT_QUIET"
-	then
-		GIT_MERGE_VERBOSITY=1 && export GIT_MERGE_VERBOSITY
-	fi
-	test -z "$strategy" && strategy=recursive
-	# If cmt doesn't have a parent, don't include it as a base
-	base=$(git rev-parse --verify --quiet $cmt^)
-	eval 'git merge-$strategy' $strategy_opts $base ' -- "$hd" "$cmt"'
-	rv=$?
-	case "$rv" in
-	0)
-		unset GITHEAD_$cmt GITHEAD_$hd
-		return
-		;;
-	1)
-		git rerere $allow_rerere_autoupdate
-		die "$resolvemsg"
-		;;
-	2)
-		echo "Strategy: $strategy failed, try another" 1>&2
-		die "$resolvemsg"
-		;;
-	*)
-		die "Unknown exit code ($rv) from command:" \
-			"git merge-$strategy $cmt^ -- HEAD $cmt"
-		;;
-	esac
-}
-
-finish_rb_merge () {
-	move_to_original_branch
-	if test -s "$state_dir"/rewritten
-	then
-		git notes copy --for-rewrite=rebase <"$state_dir"/rewritten
-		hook="$(git rev-parse --git-path hooks/post-rewrite)"
-		test -x "$hook" && "$hook" rebase <"$state_dir"/rewritten
-	fi
-	say All done.
-}
-
-git_rebase__merge () {
-
-case "$action" in
-continue)
-	read_state
-	continue_merge
-	while test "$msgnum" -le "$end"
-	do
-		call_merge "$msgnum"
-		continue_merge
-	done
-	finish_rb_merge
-	return
-	;;
-skip)
-	read_state
-	git rerere clear
-	msgnum=$(($msgnum + 1))
-	while test "$msgnum" -le "$end"
-	do
-		call_merge "$msgnum"
-		continue_merge
-	done
-	finish_rb_merge
-	return
-	;;
-show-current-patch)
-	exec git show REBASE_HEAD --
-	;;
-esac
-
-mkdir -p "$state_dir"
-echo "$onto_name" > "$state_dir/onto_name"
-write_basic_state
-rm -f "$(git rev-parse --git-path REBASE_HEAD)"
-
-msgnum=0
-for cmt in $(git rev-list --reverse --no-merges "$revisions")
-do
-	msgnum=$(($msgnum + 1))
-	echo "$cmt" > "$state_dir/cmt.$msgnum"
-done
-
-echo 1 >"$state_dir/msgnum"
-echo $msgnum >"$state_dir/end"
-
-end=$msgnum
-msgnum=1
-
-while test "$msgnum" -le "$end"
-do
-	call_merge "$msgnum"
-	continue_merge
-done
-
-finish_rb_merge
-
-}
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 0392e36d23..04d6c71899 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -17,14 +17,9 @@ test_expect_success 'setup' '
 	git tag start
 '
 
-cat >expect <<\EOF
-Already applied: 0001 A
-Already applied: 0002 B
-Committed: 0003 Z
-EOF
-
 test_expect_success 'rebase -m' '
 	git rebase -m master >report &&
+	>expect &&
 	sed -n -e "/^Already applied: /p" \
 		-e "/^Committed: /p" report >actual &&
 	test_cmp expect actual
diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index f355c6825a..49077200c5 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -53,41 +53,6 @@ create_expected_success_interactive () {
 	EOF
 }
 
-create_expected_success_merge () {
-	cat >expected <<-EOF
-	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
-	HEAD is now at $(git rev-parse --short feature-branch) third commit
-	First, rewinding head to replay your work on top of it...
-	Merging unrelated-onto-branch with HEAD~1
-	Merging:
-	$(git rev-parse --short unrelated-onto-branch) unrelated commit
-	$(git rev-parse --short feature-branch^) second commit
-	found 1 common ancestor:
-	$(git rev-parse --short feature-branch~2) initial commit
-	[detached HEAD $(git rev-parse --short rebased-feature-branch~1)] second commit
-	 Author: A U Thor <author@example.com>
-	 Date: Thu Apr 7 15:14:13 2005 -0700
-	 2 files changed, 2 insertions(+)
-	 create mode 100644 file1
-	 create mode 100644 file2
-	Committed: 0001 second commit
-	Merging unrelated-onto-branch with HEAD~0
-	Merging:
-	$(git rev-parse --short rebased-feature-branch~1) second commit
-	$(git rev-parse --short feature-branch) third commit
-	found 1 common ancestor:
-	$(git rev-parse --short feature-branch~1) second commit
-	[detached HEAD $(git rev-parse --short rebased-feature-branch)] third commit
-	 Author: A U Thor <author@example.com>
-	 Date: Thu Apr 7 15:15:13 2005 -0700
-	 1 file changed, 1 insertion(+)
-	 create mode 100644 file3
-	Committed: 0002 third commit
-	All done.
-	Applied autostash.
-	EOF
-}
-
 create_expected_failure_am () {
 	cat >expected <<-EOF
 	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
@@ -112,43 +77,6 @@ create_expected_failure_interactive () {
 	EOF
 }
 
-create_expected_failure_merge () {
-	cat >expected <<-EOF
-	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
-	HEAD is now at $(git rev-parse --short feature-branch) third commit
-	First, rewinding head to replay your work on top of it...
-	Merging unrelated-onto-branch with HEAD~1
-	Merging:
-	$(git rev-parse --short unrelated-onto-branch) unrelated commit
-	$(git rev-parse --short feature-branch^) second commit
-	found 1 common ancestor:
-	$(git rev-parse --short feature-branch~2) initial commit
-	[detached HEAD $(git rev-parse --short rebased-feature-branch~1)] second commit
-	 Author: A U Thor <author@example.com>
-	 Date: Thu Apr 7 15:14:13 2005 -0700
-	 2 files changed, 2 insertions(+)
-	 create mode 100644 file1
-	 create mode 100644 file2
-	Committed: 0001 second commit
-	Merging unrelated-onto-branch with HEAD~0
-	Merging:
-	$(git rev-parse --short rebased-feature-branch~1) second commit
-	$(git rev-parse --short feature-branch) third commit
-	found 1 common ancestor:
-	$(git rev-parse --short feature-branch~1) second commit
-	[detached HEAD $(git rev-parse --short rebased-feature-branch)] third commit
-	 Author: A U Thor <author@example.com>
-	 Date: Thu Apr 7 15:15:13 2005 -0700
-	 1 file changed, 1 insertion(+)
-	 create mode 100644 file3
-	Committed: 0002 third commit
-	All done.
-	Applying autostash resulted in conflicts.
-	Your changes are safe in the stash.
-	You can run "git stash pop" or "git stash drop" at any time.
-	EOF
-}
-
 testrebase () {
 	type=$1
 	dotest=$2
@@ -177,6 +105,9 @@ testrebase () {
 	test_expect_success "rebase$type --autostash: check output" '
 		test_when_finished git branch -D rebased-feature-branch &&
 		suffix=${type#\ --} && suffix=${suffix:-am} &&
+		if test ${suffix} = "merge"; then
+			suffix=interactive
+		fi &&
 		create_expected_success_$suffix &&
 		test_i18ncmp expected actual
 	'
@@ -274,6 +205,9 @@ testrebase () {
 	test_expect_success "rebase$type: check output with conflicting stash" '
 		test_when_finished git branch -D rebased-feature-branch &&
 		suffix=${type#\ --} && suffix=${suffix:-am} &&
+		if test ${suffix} = "merge"; then
+			suffix=interactive
+		fi &&
 		create_expected_failure_$suffix &&
 		test_i18ncmp expected actual
 	'
diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh
index 99b2aac921..911ef49f70 100755
--- a/t/t3421-rebase-topology-linear.sh
+++ b/t/t3421-rebase-topology-linear.sh
@@ -111,7 +111,7 @@ test_run_rebase () {
 	"
 }
 test_run_rebase success ''
-test_run_rebase failure -m
+test_run_rebase success -m
 test_run_rebase success -i
 test_run_rebase success -p
 
@@ -126,7 +126,7 @@ test_run_rebase () {
 	"
 }
 test_run_rebase success ''
-test_run_rebase failure -m
+test_run_rebase success -m
 test_run_rebase success -i
 test_run_rebase success -p
 
@@ -141,7 +141,7 @@ test_run_rebase () {
 	"
 }
 test_run_rebase success ''
-test_run_rebase failure -m
+test_run_rebase success -m
 test_run_rebase success -i
 test_run_rebase success -p
 
@@ -284,7 +284,7 @@ test_run_rebase () {
 	"
 }
 test_run_rebase success ''
-test_run_rebase failure -m
+test_run_rebase success -m
 test_run_rebase success -i
 test_run_rebase success -p
 
@@ -315,7 +315,7 @@ test_run_rebase () {
 	"
 }
 test_run_rebase success ''
-test_run_rebase failure -m
+test_run_rebase success -m
 test_run_rebase success -i
 test_run_rebase failure -p
 
diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
index 846f85c27e..cd505c0711 100755
--- a/t/t3425-rebase-topology-merges.sh
+++ b/t/t3425-rebase-topology-merges.sh
@@ -72,7 +72,7 @@ test_run_rebase () {
 }
 #TODO: make order consistent across all flavors of rebase
 test_run_rebase success 'e n o' ''
-test_run_rebase success 'e n o' -m
+test_run_rebase success 'n o e' -m
 test_run_rebase success 'n o e' -i
 
 test_run_rebase () {
@@ -89,7 +89,7 @@ test_run_rebase () {
 }
 #TODO: make order consistent across all flavors of rebase
 test_run_rebase success 'd e n o' ''
-test_run_rebase success 'd e n o' -m
+test_run_rebase success 'd n o e' -m
 test_run_rebase success 'd n o e' -i
 
 test_run_rebase () {
@@ -106,7 +106,7 @@ test_run_rebase () {
 }
 #TODO: make order consistent across all flavors of rebase
 test_run_rebase success 'd e n o' ''
-test_run_rebase success 'd e n o' -m
+test_run_rebase success 'd n o e' -m
 test_run_rebase success 'd n o e' -i
 
 test_expect_success "rebase -p is no-op in non-linear history" "
diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
index 9b2a274c71..145c61251d 100755
--- a/t/t5407-post-rewrite-hook.sh
+++ b/t/t5407-post-rewrite-hook.sh
@@ -120,6 +120,7 @@ test_expect_success 'git rebase -m --skip' '
 	git rebase --continue &&
 	echo rebase >expected.args &&
 	cat >expected.data <<-EOF &&
+	$(git rev-parse C) $(git rev-parse HEAD^)
 	$(git rev-parse D) $(git rev-parse HEAD)
 	EOF
 	verify_hook_input
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index 81a5179e28..5cadedb2a9 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -180,7 +180,7 @@ test_expect_success 'prompt - interactive rebase' '
 '
 
 test_expect_success 'prompt - rebase merge' '
-	printf " (b2|REBASE-m 1/3)" >expected &&
+	printf " (b2|REBASE-i 1/3)" >expected &&
 	git checkout b2 &&
 	test_when_finished "git checkout master" &&
 	test_must_fail git rebase --merge b1 b2 &&
-- 
2.19.1.858.g526e8fe740.dirty


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/2] Reimplement rebase --merge via interactive machinery
  2018-11-08  6:01 [PATCH v2 0/2] Reimplement rebase --merge via interactive machinery Elijah Newren
  2018-11-08  6:01 ` [PATCH v2 1/2] git-rebase, sequencer: extend --quiet option for the " Elijah Newren
  2018-11-08  6:01 ` [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive Elijah Newren
@ 2018-11-08  6:33 ` Elijah Newren
  2 siblings, 0 replies; 12+ messages in thread
From: Elijah Newren @ 2018-11-08  6:33 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Junio C Hamano, Johannes Schindelin, Pratik Karki, Phillip Wood

On Wed, Nov 7, 2018 at 10:02 PM Elijah Newren <newren@gmail.com> wrote:
>
> Now that the rewrite-interactive-rebases-in-C series have finally
> merged to master, this series deletes git-rebase--merge.sh and
> reimplements the --merge behavior on top of the interactive machinery.
>
> Differences since v1:
>   - Updated code to hook into builtin/rebase.C instead of git-rebase.sh
>
> (No range-diff provided, because it has been months since v1, and v1
> was only RFC and was only discussed at a high level.)

Actually, that's not correct; it's been so long that I forgot.  Dscho
and Phillip both reviewed it and I updated my series at the time with
their suggestions, but didn't re-submit because it depended on so many
other series and conflicted with the rebase-in-C work.  So other
differences include the changes I made to address their feedback,
but...even if I dug up the old v1 and created a range-diff against it
I'm not so sure it'd be helpful.  If anyone thinks it would be, holler
and I'll generate it.

Original series here:
https://public-inbox.org/git/20180607171344.23331-1-newren@gmail.com/

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] git-rebase, sequencer: extend --quiet option for the interactive machinery
  2018-11-08  6:01 ` [PATCH v2 1/2] git-rebase, sequencer: extend --quiet option for the " Elijah Newren
@ 2018-11-12 15:11   ` Johannes Schindelin
  0 siblings, 0 replies; 12+ messages in thread
From: Johannes Schindelin @ 2018-11-12 15:11 UTC (permalink / raw)
  To: Elijah Newren; +Cc: git, gitster, predatoramigo

Hi Elijah,

On Wed, 7 Nov 2018, Elijah Newren wrote:

> While 'quiet' and 'interactive' may sound like antonyms, the interactive
> machinery actually has logic that implements several
> interactive_rebase=implied cases (--exec, --keep-empty, --rebase-merges)
> which won't pop up an editor.  The rewrite of interactive rebase in C
> added a quiet option, though it only turns stats off.  Since we want to
> make the interactive machinery also take over for git-rebase--merge, it
> should fully implement the --quiet option.
> 
> git-rebase--interactive was already somewhat quieter than
> git-rebase--merge and git-rebase--am, possibly because cherry-pick has
> just traditionally been quieter.  As such, we only drop a few
> informational messages -- "Rebasing (n/m)" and "Succesfully rebased..."

Makes sense.

> Also, for simplicity, remove the differences in how quiet and verbose
> options were recorded.  Having one be signalled by the presence of a
> "verbose" file in the state_dir, while the other was signalled by the
> contents of a "quiet" file was just weirdly inconsistent.  (This
> inconsistency pre-dated the rewrite into C.)  Make them consistent by
> having them both key off the presence of the file.

I am slightly concerned that some creative power user could have written
scripts that rely on this behavior.

But only *slightly* concerned.

The patch looks correct.

Ciao,
Dscho

> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  builtin/rebase.c      |  5 +----
>  git-legacy-rebase.sh  |  2 +-
>  git-rebase--common.sh |  2 +-
>  sequencer.c           | 23 +++++++++++++----------
>  sequencer.h           |  1 +
>  5 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 0ee06aa363..be004406a6 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -181,10 +181,7 @@ static int read_basic_state(struct rebase_options *opts)
>  	if (get_oid(buf.buf, &opts->orig_head))
>  		return error(_("invalid orig-head: '%s'"), buf.buf);
>  
> -	strbuf_reset(&buf);
> -	if (read_one(state_dir_path("quiet", opts), &buf))
> -		return -1;
> -	if (buf.len)
> +	if (file_exists(state_dir_path("quiet", opts)))
>  		opts->flags &= ~REBASE_NO_QUIET;
>  	else
>  		opts->flags |= REBASE_NO_QUIET;
> diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
> index 75a08b2683..da27bfca5a 100755
> --- a/git-legacy-rebase.sh
> +++ b/git-legacy-rebase.sh
> @@ -113,7 +113,7 @@ read_basic_state () {
>  	else
>  		orig_head=$(cat "$state_dir"/head)
>  	fi &&
> -	GIT_QUIET=$(cat "$state_dir"/quiet) &&
> +	test -f "$state_dir"/quiet && GIT_QUIET=t
>  	test -f "$state_dir"/verbose && verbose=t
>  	test -f "$state_dir"/strategy && strategy="$(cat "$state_dir"/strategy)"
>  	test -f "$state_dir"/strategy_opts &&
> diff --git a/git-rebase--common.sh b/git-rebase--common.sh
> index 7e39d22871..dc18c682fa 100644
> --- a/git-rebase--common.sh
> +++ b/git-rebase--common.sh
> @@ -10,7 +10,7 @@ write_basic_state () {
>  	echo "$head_name" > "$state_dir"/head-name &&
>  	echo "$onto" > "$state_dir"/onto &&
>  	echo "$orig_head" > "$state_dir"/orig-head &&
> -	echo "$GIT_QUIET" > "$state_dir"/quiet &&
> +	test t = "$GIT_QUIET" && : > "$state_dir"/quiet
>  	test t = "$verbose" && : > "$state_dir"/verbose
>  	test -n "$strategy" && echo "$strategy" > "$state_dir"/strategy
>  	test -n "$strategy_opts" && echo "$strategy_opts" > \
> diff --git a/sequencer.c b/sequencer.c
> index 9e1ab3a2a7..bd8337dbf1 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -150,6 +150,7 @@ static GIT_PATH_FUNC(rebase_path_refs_to_delete, "rebase-merge/refs-to-delete")
>  static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
>  static GIT_PATH_FUNC(rebase_path_orig_head, "rebase-merge/orig-head")
>  static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
> +static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet")
>  static GIT_PATH_FUNC(rebase_path_signoff, "rebase-merge/signoff")
>  static GIT_PATH_FUNC(rebase_path_head_name, "rebase-merge/head-name")
>  static GIT_PATH_FUNC(rebase_path_onto, "rebase-merge/onto")
> @@ -157,7 +158,6 @@ static GIT_PATH_FUNC(rebase_path_autostash, "rebase-merge/autostash")
>  static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
>  static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
>  static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
> -static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet")
>  
>  static int git_sequencer_config(const char *k, const char *v, void *cb)
>  {
> @@ -2307,6 +2307,9 @@ static int read_populate_opts(struct replay_opts *opts)
>  		if (file_exists(rebase_path_verbose()))
>  			opts->verbose = 1;
>  
> +		if (file_exists(rebase_path_quiet()))
> +			opts->quiet = 1;
> +
>  		if (file_exists(rebase_path_signoff())) {
>  			opts->allow_ff = 0;
>  			opts->signoff = 1;
> @@ -2374,9 +2377,6 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
>  
>  	if (quiet)
>  		write_file(rebase_path_quiet(), "%s\n", quiet);
> -	else
> -		write_file(rebase_path_quiet(), "\n");
> -
>  	if (opts->verbose)
>  		write_file(rebase_path_verbose(), "%s", "");
>  	if (opts->strategy)
> @@ -3450,10 +3450,11 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
>  					fprintf(f, "%d\n", todo_list->done_nr);
>  					fclose(f);
>  				}
> -				fprintf(stderr, "Rebasing (%d/%d)%s",
> -					todo_list->done_nr,
> -					todo_list->total_nr,
> -					opts->verbose ? "\n" : "\r");
> +				if (!opts->quiet)
> +					fprintf(stderr, "Rebasing (%d/%d)%s",
> +						todo_list->done_nr,
> +						todo_list->total_nr,
> +						opts->verbose ? "\n" : "\r");
>  			}
>  			unlink(rebase_path_message());
>  			unlink(rebase_path_author_script());
> @@ -3684,8 +3685,10 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
>  		}
>  		apply_autostash(opts);
>  
> -		fprintf(stderr, "Successfully rebased and updated %s.\n",
> -			head_ref.buf);
> +		if (!opts->quiet)
> +			fprintf(stderr,
> +				"Successfully rebased and updated %s.\n",
> +				head_ref.buf);
>  
>  		strbuf_release(&buf);
>  		strbuf_release(&head_ref);
> diff --git a/sequencer.h b/sequencer.h
> index 660cff5050..45fd977bef 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -39,6 +39,7 @@ struct replay_opts {
>  	int allow_empty_message;
>  	int keep_redundant_commits;
>  	int verbose;
> +	int quiet;
>  
>  	int mainline;
>  
> -- 
> 2.19.1.858.g526e8fe740.dirty
> 
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive
  2018-11-08  6:01 ` [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive Elijah Newren
@ 2018-11-12 16:21   ` Johannes Schindelin
  2018-11-12 18:21     ` Phillip Wood
                       ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Johannes Schindelin @ 2018-11-12 16:21 UTC (permalink / raw)
  To: Elijah Newren; +Cc: git, gitster, predatoramigo

Hi Elijah,

On Wed, 7 Nov 2018, Elijah Newren wrote:

> Interactive rebases are implemented in terms of cherry-pick rather than
> the merge-recursive builtin, but cherry-pick also calls into the recursive
> merge machinery by default and can accept special merge strategies and/or
> special strategy options.  As such, there really is not any need for
> having both git-rebase--merge and git-rebase--interactive anymore.
> 
> Delete git-rebase--merge.sh and have the --merge option be implemented
> by the now built-in interactive machinery.

Okay.

> Note that this change fixes a few known test failures (see t3421).

Nice!

> testcase modification notes:
>   t3406: --interactive and --merge had slightly different progress output
>          while running; adjust a test to match
>   t3420: these test precise output while running, but rebase--am,
>          rebase--merge, and rebase--interactive all were built on very
>          different commands (am, merge-recursive, cherry-pick), so the
>          tests expected different output for each type.  Now we expect
>          --merge and --interactive to have the same output.
>   t3421: --interactive fixes some bugs in --merge!  Wahoo!
>   t3425: topology linearization was inconsistent across flavors of rebase,
>          as already noted in a TODO comment in the testcase.  This was not
>          considered a bug before, so getting a different linearization due
>          to switching out backends should not be considered a bug now.

Ideally, the test would be fixed, then. If it fails for other reasons than
a real regression, it is not a very good regression test, is it.

>   t5407: different rebase types varied slightly in how many times checkout
>          or commit or equivalents were called based on a quick comparison
>          of this tests and previous ones which covered different rebase
>          flavors.  I think this is just attributable to this difference.

This concerns me.

In bigger repositories (no, I am not talking about Linux kernel sized
ones, I consider those small-ish) there are a ton of files, and checkout
and commit (and even more so reset) sadly do not have a runtime complexity
growing with the number of modified files, but with the number of tracked
files (and some commands even with the number of worktree files).

So a larger number of commit/checkout operations makes me expect
performance regressions.

In this light, could you elaborate a bit on the differences you see
between rebase -i and rebase -m?

>   t9903: --merge uses the interactive backend so the prompt expected is
>          now REBASE-i.

We should be able to make that test pass, still, by writing out a special
file (e.g. $state_dir/opt_m) and testing for that. Users are oddly upset
when their expectations are broken... (and I actually agree with them.)

> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 3407d835bd..35084f5681 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -504,15 +504,7 @@ See also INCOMPATIBLE OPTIONS below.
>  INCOMPATIBLE OPTIONS
>  --------------------
>  
> -git-rebase has many flags that are incompatible with each other,
> -predominantly due to the fact that it has three different underlying
> -implementations:
> -
> - * one based on linkgit:git-am[1] (the default)
> - * one based on git-merge-recursive (merge backend)
> - * one based on linkgit:git-cherry-pick[1] (interactive backend)
> -

Could we retain this part, with `s/three/two/` and `/git-merge/d`? *Maybe*
`s/interactive backend/interactive\/merge backend/`

> -Flags only understood by the am backend:
> +The following options:
>  
>   * --committer-date-is-author-date
>   * --ignore-date
> @@ -520,15 +512,12 @@ Flags only understood by the am backend:
>   * --ignore-whitespace
>   * -C
>  
> -Flags understood by both merge and interactive backends:
> +are incompatible with the following options:
>  
>   * --merge
>   * --strategy
>   * --strategy-option
>   * --allow-empty-message
> -
> -Flags only understood by the interactive backend:
> -
>   * --[no-]autosquash
>   * --rebase-merges
>   * --preserve-merges
> @@ -539,7 +528,7 @@ Flags only understood by the interactive backend:
>   * --edit-todo
>   * --root when used in combination with --onto
>  
> -Other incompatible flag pairs:
> +In addition, the following pairs of options are incompatible:
>  
>   * --preserve-merges and --interactive
>   * --preserve-merges and --signoff

The rest of the diff is funny to read, but the post image makes a lot of
sense.

> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index be004406a6..d55bbb9eb9 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -118,7 +118,7 @@ static void imply_interactive(struct rebase_options *opts, const char *option)
>  	case REBASE_PRESERVE_MERGES:
>  		break;
>  	case REBASE_MERGE:
> -		/* we silently *upgrade* --merge to --interactive if needed */
> +		/* we now implement --merge via --interactive */
>  	default:
>  		opts->type = REBASE_INTERACTIVE; /* implied */
>  		break;
> @@ -475,10 +475,6 @@ static int run_specific_rebase(struct rebase_options *opts)
>  		backend = "git-rebase--am";
>  		backend_func = "git_rebase__am";
>  		break;
> -	case REBASE_MERGE:
> -		backend = "git-rebase--merge";
> -		backend_func = "git_rebase__merge";
> -		break;
>  	case REBASE_PRESERVE_MERGES:
>  		backend = "git-rebase--preserve-merges";
>  		backend_func = "git_rebase__preserve_merges";
> @@ -1156,6 +1152,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		}
>  	}
>  
> +	if (options.type == REBASE_MERGE) {
> +		imply_interactive(&options, "--merge");
> +	}
> +
>  	if (options.root && !options.onto_name)
>  		imply_interactive(&options, "--root without --onto");
>  

Okay, this makes sense.

> diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
> index da27bfca5a..4abcceed06 100755
> --- a/git-legacy-rebase.sh
> +++ b/git-legacy-rebase.sh
> @@ -218,6 +218,7 @@ then
>  	state_dir="$apply_dir"
>  elif test -d "$merge_dir"
>  then
> +	type=interactive
>  	if test -d "$merge_dir"/rewritten
>  	then
>  		type=preserve-merges
> @@ -225,10 +226,7 @@ then
>  		preserve_merges=t
>  	elif test -f "$merge_dir"/interactive
>  	then
> -		type=interactive
>  		interactive_rebase=explicit
> -	else
> -		type=merge
>  	fi
>  	state_dir="$merge_dir"
>  fi
> @@ -469,6 +467,7 @@ then
>  	test -z "$interactive_rebase" && interactive_rebase=implied
>  fi
>  
> +actually_interactive=
>  if test -n "$interactive_rebase"
>  then
>  	if test -z "$preserve_merges"
> @@ -477,11 +476,12 @@ then
>  	else
>  		type=preserve-merges
>  	fi
> -
> +	actually_interactive=t
>  	state_dir="$merge_dir"
>  elif test -n "$do_merge"
>  then
> -	type=merge
> +	interactive_rebase=implied
> +	type=interactive
>  	state_dir="$merge_dir"
>  else
>  	type=am
> @@ -493,21 +493,13 @@ then
>  	git_format_patch_opt="$git_format_patch_opt --progress"
>  fi
>  
> -if test -n "$git_am_opt"; then
> -	incompatible_opts=$(echo " $git_am_opt " | \
> -			    sed -e 's/ -q / /g' -e 's/^ \(.*\) $/\1/')
> -	if test -n "$interactive_rebase"
> +incompatible_opts=$(echo " $git_am_opt " | \
> +		    sed -e 's/ -q / /g' -e 's/^ \(.*\) $/\1/')

Why are we no longer guarding this behind the condition that the user
specified *any* option intended for the `am` backend?

> +if test -n "$incompatible_opts"
> +then
> +	if test -n "$actually_interactive" || test "$do_merge"
>  	then
> -		if test -n "$incompatible_opts"
> -		then
> -			die "$(gettext "error: cannot combine interactive options (--interactive, --exec, --rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with am options ($incompatible_opts)")"
> -		fi
> -	fi
> -	if test -n "$do_merge"; then
> -		if test -n "$incompatible_opts"
> -		then
> -			die "$(gettext "error: cannot combine merge options (--merge, --strategy, --strategy-option) with am options ($incompatible_opts)")"
> -		fi
> +		die "$(gettext "error: cannot combine am options ($incompatible_opts) with either interactive or merge options")"
>  	fi
>  fi
>  

The rest of this hunk makes sense.

> @@ -672,7 +664,7 @@ require_clean_work_tree "rebase" "$(gettext "Please commit or stash them.")"
>  # but this should be done only when upstream and onto are the same
>  # and if this is not an interactive rebase.
>  mb=$(git merge-base "$onto" "$orig_head")
> -if test -z "$interactive_rebase" && test "$upstream" = "$onto" &&
> +if test -z "$actually_interactive" && test "$upstream" = "$onto" &&
>  	test "$mb" = "$onto" && test -z "$restrict_revision" &&
>  	# linear history?
>  	! (git rev-list --parents "$onto".."$orig_head" | sane_grep " .* ") > /dev/null
> @@ -716,6 +708,19 @@ then
>  	GIT_PAGER='' git diff --stat --summary "$mb" "$onto"
>  fi
>  
> +if test -z "$actually_interactive" && test "$mb" = "$orig_head"
> +then
> +	# If the $onto is a proper descendant of the tip of the branch, then
> +	# we just fast-forwarded.

This comment is misleading if it comes before, rather than after, the
actual `checkout -q` command.

> +	say "$(eval_gettext "Fast-forwarded \$branch_name to \$onto_name.")"
> +	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" \
> +		git checkout -q "$onto^0" || die "could not detach HEAD"
> +	git update-ref ORIG_HEAD $orig_head
> +	move_to_original_branch
> +	finish_rebase
> +	exit 0
> +fi
> +
>  test -n "$interactive_rebase" && run_specific_rebase
>  
>  # Detach HEAD and reset the tree
> @@ -725,16 +730,6 @@ GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" \
>  	git checkout -q "$onto^0" || die "could not detach HEAD"
>  git update-ref ORIG_HEAD $orig_head

It is a pity that this hunk header hides the lines that you copied into
the following conditional before moving it before the "if interactive, run
it" line:

>  
> -# If the $onto is a proper descendant of the tip of the branch, then
> -# we just fast-forwarded.
> -if test "$mb" = "$orig_head"
> -then
> -	say "$(eval_gettext "Fast-forwarded \$branch_name to \$onto_name.")"
> -	move_to_original_branch
> -	finish_rebase
> -	exit 0
> -fi
> -
>  if test -n "$rebase_root"
>  then
>  	revisions="$onto..$orig_head"

What you did is correct, if duplicating code, and definitely the easiest
way to do it. Just move the comment, and we're good here.

> diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
> deleted file mode 100644
> [... snip ...]

Nice. Really nice!

> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> index 0392e36d23..04d6c71899 100755
> --- a/t/t3406-rebase-message.sh
> +++ b/t/t3406-rebase-message.sh
> @@ -17,14 +17,9 @@ test_expect_success 'setup' '
>  	git tag start
>  '
>  
> -cat >expect <<\EOF
> -Already applied: 0001 A
> -Already applied: 0002 B
> -Committed: 0003 Z
> -EOF
> -

As I had mentioned in the previous round: I think we can retain these
messages for the `--merge` mode. And I think we should: users of --merge
have most likely become to expect them.

The most elegant way would probably be by adding code to the sequencer
that outputs these lines if in "merge" mode (and add a flag to say that we
*are* in "merge" mode).

To that end, the `make_script()` function would not generate `# pick` but
`drop` lines, I think, so that the sequencer can see those and print the
`Already applied: <message>` lines. And a successful `TODO_PICK` would
write out `Committed: <message>`.

>  test_expect_success 'rebase -m' '
>  	git rebase -m master >report &&
> +	>expect &&
>  	sed -n -e "/^Already applied: /p" \
>  		-e "/^Committed: /p" report >actual &&
>  	test_cmp expect actual
> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
> index f355c6825a..49077200c5 100755
> --- a/t/t3420-rebase-autostash.sh
> +++ b/t/t3420-rebase-autostash.sh
> @@ -53,41 +53,6 @@ create_expected_success_interactive () {
>  	EOF
>  }
>  
> -create_expected_success_merge () {
> -	cat >expected <<-EOF
> -	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
> -	HEAD is now at $(git rev-parse --short feature-branch) third commit
> -	First, rewinding head to replay your work on top of it...
> -	Merging unrelated-onto-branch with HEAD~1
> -	Merging:
> -	$(git rev-parse --short unrelated-onto-branch) unrelated commit
> -	$(git rev-parse --short feature-branch^) second commit
> -	found 1 common ancestor:
> -	$(git rev-parse --short feature-branch~2) initial commit
> -	[detached HEAD $(git rev-parse --short rebased-feature-branch~1)] second commit
> -	 Author: A U Thor <author@example.com>
> -	 Date: Thu Apr 7 15:14:13 2005 -0700
> -	 2 files changed, 2 insertions(+)
> -	 create mode 100644 file1
> -	 create mode 100644 file2
> -	Committed: 0001 second commit
> -	Merging unrelated-onto-branch with HEAD~0
> -	Merging:
> -	$(git rev-parse --short rebased-feature-branch~1) second commit
> -	$(git rev-parse --short feature-branch) third commit
> -	found 1 common ancestor:
> -	$(git rev-parse --short feature-branch~1) second commit
> -	[detached HEAD $(git rev-parse --short rebased-feature-branch)] third commit
> -	 Author: A U Thor <author@example.com>
> -	 Date: Thu Apr 7 15:15:13 2005 -0700
> -	 1 file changed, 1 insertion(+)
> -	 create mode 100644 file3
> -	Committed: 0002 third commit
> -	All done.
> -	Applied autostash.
> -	EOF
> -}
> -
>  create_expected_failure_am () {
>  	cat >expected <<-EOF
>  	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
> @@ -112,43 +77,6 @@ create_expected_failure_interactive () {
>  	EOF
>  }
>  
> -create_expected_failure_merge () {
> -	cat >expected <<-EOF
> -	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
> -	HEAD is now at $(git rev-parse --short feature-branch) third commit
> -	First, rewinding head to replay your work on top of it...
> -	Merging unrelated-onto-branch with HEAD~1
> -	Merging:
> -	$(git rev-parse --short unrelated-onto-branch) unrelated commit
> -	$(git rev-parse --short feature-branch^) second commit
> -	found 1 common ancestor:
> -	$(git rev-parse --short feature-branch~2) initial commit
> -	[detached HEAD $(git rev-parse --short rebased-feature-branch~1)] second commit
> -	 Author: A U Thor <author@example.com>
> -	 Date: Thu Apr 7 15:14:13 2005 -0700
> -	 2 files changed, 2 insertions(+)
> -	 create mode 100644 file1
> -	 create mode 100644 file2
> -	Committed: 0001 second commit
> -	Merging unrelated-onto-branch with HEAD~0
> -	Merging:
> -	$(git rev-parse --short rebased-feature-branch~1) second commit
> -	$(git rev-parse --short feature-branch) third commit
> -	found 1 common ancestor:
> -	$(git rev-parse --short feature-branch~1) second commit
> -	[detached HEAD $(git rev-parse --short rebased-feature-branch)] third commit
> -	 Author: A U Thor <author@example.com>
> -	 Date: Thu Apr 7 15:15:13 2005 -0700
> -	 1 file changed, 1 insertion(+)
> -	 create mode 100644 file3
> -	Committed: 0002 third commit
> -	All done.
> -	Applying autostash resulted in conflicts.
> -	Your changes are safe in the stash.
> -	You can run "git stash pop" or "git stash drop" at any time.
> -	EOF
> -}
> -
>  testrebase () {
>  	type=$1
>  	dotest=$2
> @@ -177,6 +105,9 @@ testrebase () {
>  	test_expect_success "rebase$type --autostash: check output" '
>  		test_when_finished git branch -D rebased-feature-branch &&
>  		suffix=${type#\ --} && suffix=${suffix:-am} &&
> +		if test ${suffix} = "merge"; then
> +			suffix=interactive
> +		fi &&
>  		create_expected_success_$suffix &&
>  		test_i18ncmp expected actual
>  	'
> @@ -274,6 +205,9 @@ testrebase () {
>  	test_expect_success "rebase$type: check output with conflicting stash" '
>  		test_when_finished git branch -D rebased-feature-branch &&
>  		suffix=${type#\ --} && suffix=${suffix:-am} &&
> +		if test ${suffix} = "merge"; then
> +			suffix=interactive
> +		fi &&
>  		create_expected_failure_$suffix &&
>  		test_i18ncmp expected actual
>  	'

As I stated earlier, I am uncomfortable with this solution. We change
behavior rather noticeably, and the regression test tells us the same. We
need to either try to deprecate `git rebase --merge`, or we need to at
least attempt to recreate the old behavior with the sequencer.

> diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh
> index 99b2aac921..911ef49f70 100755
> --- a/t/t3421-rebase-topology-linear.sh
> +++ b/t/t3421-rebase-topology-linear.sh
> @@ -111,7 +111,7 @@ test_run_rebase () {
>  	"
>  }
>  test_run_rebase success ''
> -test_run_rebase failure -m
> +test_run_rebase success -m
>  test_run_rebase success -i
>  test_run_rebase success -p
>  
> @@ -126,7 +126,7 @@ test_run_rebase () {
>  	"
>  }
>  test_run_rebase success ''
> -test_run_rebase failure -m
> +test_run_rebase success -m
>  test_run_rebase success -i
>  test_run_rebase success -p
>  
> @@ -141,7 +141,7 @@ test_run_rebase () {
>  	"
>  }
>  test_run_rebase success ''
> -test_run_rebase failure -m
> +test_run_rebase success -m
>  test_run_rebase success -i
>  test_run_rebase success -p
>  
> @@ -284,7 +284,7 @@ test_run_rebase () {
>  	"
>  }
>  test_run_rebase success ''
> -test_run_rebase failure -m
> +test_run_rebase success -m
>  test_run_rebase success -i
>  test_run_rebase success -p
>  
> @@ -315,7 +315,7 @@ test_run_rebase () {
>  	"
>  }
>  test_run_rebase success ''
> -test_run_rebase failure -m
> +test_run_rebase success -m
>  test_run_rebase success -i
>  test_run_rebase failure -p
>  

Nice.

> diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
> index 846f85c27e..cd505c0711 100755
> --- a/t/t3425-rebase-topology-merges.sh
> +++ b/t/t3425-rebase-topology-merges.sh
> @@ -72,7 +72,7 @@ test_run_rebase () {
>  }
>  #TODO: make order consistent across all flavors of rebase
>  test_run_rebase success 'e n o' ''
> -test_run_rebase success 'e n o' -m
> +test_run_rebase success 'n o e' -m
>  test_run_rebase success 'n o e' -i
>  
>  test_run_rebase () {
> @@ -89,7 +89,7 @@ test_run_rebase () {
>  }
>  #TODO: make order consistent across all flavors of rebase
>  test_run_rebase success 'd e n o' ''
> -test_run_rebase success 'd e n o' -m
> +test_run_rebase success 'd n o e' -m
>  test_run_rebase success 'd n o e' -i
>  
>  test_run_rebase () {
> @@ -106,7 +106,7 @@ test_run_rebase () {
>  }
>  #TODO: make order consistent across all flavors of rebase
>  test_run_rebase success 'd e n o' ''
> -test_run_rebase success 'd e n o' -m
> +test_run_rebase success 'd n o e' -m
>  test_run_rebase success 'd n o e' -i
>  
>  test_expect_success "rebase -p is no-op in non-linear history" "

This is a bit unfortunate. I wonder if we could do something like this, to
retain the current behavior:

-- snip --
diff --git a/sequencer.c b/sequencer.c
index 9e1ab3a2a7..5018957e49 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4394,7 +4394,8 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
 	revs.reverse = 1;
 	revs.right_only = 1;
 	revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
-	revs.topo_order = 1;
+	if (!(flags & TODO_LIST_NO_TOPO_ORDER))
+		revs.topo_order = 1;
 
 	revs.pretty_given = 1;
 	git_config_get_string("rebase.instructionFormat", &format);
-- snap -

(and then pass TODO_LIST_NO_TOPO_ORDER if in "merge" mode)?

> diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
> index 9b2a274c71..145c61251d 100755
> --- a/t/t5407-post-rewrite-hook.sh
> +++ b/t/t5407-post-rewrite-hook.sh
> @@ -120,6 +120,7 @@ test_expect_success 'git rebase -m --skip' '
>  	git rebase --continue &&
>  	echo rebase >expected.args &&
>  	cat >expected.data <<-EOF &&
> +	$(git rev-parse C) $(git rev-parse HEAD^)
>  	$(git rev-parse D) $(git rev-parse HEAD)
>  	EOF
>  	verify_hook_input

This suggests to me that we call `commit` too many times. I think we
really need to address this without changing the test. This is a change in
behavior.

> diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
> index 81a5179e28..5cadedb2a9 100755
> --- a/t/t9903-bash-prompt.sh
> +++ b/t/t9903-bash-prompt.sh
> @@ -180,7 +180,7 @@ test_expect_success 'prompt - interactive rebase' '
>  '
>  
>  test_expect_success 'prompt - rebase merge' '
> -	printf " (b2|REBASE-m 1/3)" >expected &&
> +	printf " (b2|REBASE-i 1/3)" >expected &&

As I said, this should be easily addressed by having a tell-tale in
$state_dir/. Come to think of it, we must have had one, right? Let's see
how the bash-prompt does it.

*clicketyclick*

Ah, it is the *absence* of the `interactive` file... Which makes me wonder
whether we should not simply retain that behavior for `git rebase
--merge`?

>  	git checkout b2 &&
>  	test_when_finished "git checkout master" &&
>  	test_must_fail git rebase --merge b1 b2 &&
> -- 
> 2.19.1.858.g526e8fe740.dirty
> 
> 

Thank you for this pleasant read. I think there is still quite a bit of
work to do, but you already did most of it.

Out of curiosity, do you have a public repository with these patches in a
branch? (I might have an hour to play with it tonight...)

Thanks!
Dscho

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive
  2018-11-12 16:21   ` Johannes Schindelin
@ 2018-11-12 18:21     ` Phillip Wood
  2018-11-13  9:18       ` Johannes Schindelin
  2018-11-14 23:06       ` Elijah Newren
  2018-11-13 16:06     ` Elijah Newren
  2018-11-14 23:03     ` Elijah Newren
  2 siblings, 2 replies; 12+ messages in thread
From: Phillip Wood @ 2018-11-12 18:21 UTC (permalink / raw)
  To: Johannes Schindelin, Elijah Newren; +Cc: git, gitster, predatoramigo

Hi Elijah

It's good to see these patches progressing, I've just got a couple of
comments related to Johannes' points below

On 12/11/2018 16:21, Johannes Schindelin wrote:
> Hi Elijah,
> 
> On Wed, 7 Nov 2018, Elijah Newren wrote:
> 
>> Interactive rebases are implemented in terms of cherry-pick rather than
>> the merge-recursive builtin, but cherry-pick also calls into the recursive
>> merge machinery by default and can accept special merge strategies and/or
>> special strategy options.  As such, there really is not any need for
>> having both git-rebase--merge and git-rebase--interactive anymore.
>>
>> Delete git-rebase--merge.sh and have the --merge option be implemented
>> by the now built-in interactive machinery.
> 
> Okay.
> 
>> Note that this change fixes a few known test failures (see t3421).
> 
> Nice!
> 
>> testcase modification notes:
>>   t3406: --interactive and --merge had slightly different progress output
>>          while running; adjust a test to match
>>   t3420: these test precise output while running, but rebase--am,
>>          rebase--merge, and rebase--interactive all were built on very
>>          different commands (am, merge-recursive, cherry-pick), so the
>>          tests expected different output for each type.  Now we expect
>>          --merge and --interactive to have the same output.
>>   t3421: --interactive fixes some bugs in --merge!  Wahoo!
>>   t3425: topology linearization was inconsistent across flavors of rebase,
>>          as already noted in a TODO comment in the testcase.  This was not
>>          considered a bug before, so getting a different linearization due
>>          to switching out backends should not be considered a bug now.
> 
> Ideally, the test would be fixed, then. If it fails for other reasons than
> a real regression, it is not a very good regression test, is it.
> 
>>   t5407: different rebase types varied slightly in how many times checkout
>>          or commit or equivalents were called based on a quick comparison
>>          of this tests and previous ones which covered different rebase
>>          flavors.  I think this is just attributable to this difference.
> 
> This concerns me.
> 
> In bigger repositories (no, I am not talking about Linux kernel sized
> ones, I consider those small-ish) there are a ton of files, and checkout
> and commit (and even more so reset) sadly do not have a runtime complexity
> growing with the number of modified files, but with the number of tracked
> files (and some commands even with the number of worktree files).
> 
> So a larger number of commit/checkout operations makes me expect
> performance regressions.
> 
> In this light, could you elaborate a bit on the differences you see
> between rebase -i and rebase -m?
> 
>>   t9903: --merge uses the interactive backend so the prompt expected is
>>          now REBASE-i.
> 
> We should be able to make that test pass, still, by writing out a special
> file (e.g. $state_dir/opt_m) and testing for that. Users are oddly upset
> when their expectations are broken... (and I actually agree with them.)
> 
>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
>> index 3407d835bd..35084f5681 100644
>> --- a/Documentation/git-rebase.txt
>> +++ b/Documentation/git-rebase.txt
>> @@ -504,15 +504,7 @@ See also INCOMPATIBLE OPTIONS below.
>>  INCOMPATIBLE OPTIONS
>>  --------------------
>>  
>> -git-rebase has many flags that are incompatible with each other,
>> -predominantly due to the fact that it has three different underlying
>> -implementations:
>> -
>> - * one based on linkgit:git-am[1] (the default)
>> - * one based on git-merge-recursive (merge backend)
>> - * one based on linkgit:git-cherry-pick[1] (interactive backend)
>> -
> 
> Could we retain this part, with `s/three/two/` and `/git-merge/d`? *Maybe*
> `s/interactive backend/interactive\/merge backend/`

I was hoping we could get rid of that, I'm not sure how useful it is to
users.

> 
>> -Flags only understood by the am backend:
>> +The following options:
>>  
>>   * --committer-date-is-author-date
>>   * --ignore-date
>> @@ -520,15 +512,12 @@ Flags only understood by the am backend:
>>   * --ignore-whitespace
>>   * -C
>>  
>> -Flags understood by both merge and interactive backends:
>> +are incompatible with the following options:
>>  
>>   * --merge
>>   * --strategy
>>   * --strategy-option
>>   * --allow-empty-message
>> -
>> -Flags only understood by the interactive backend:
>> -

It's nice to see this being simplified

>>   * --[no-]autosquash
>>   * --rebase-merges
>>   * --preserve-merges
>> @@ -539,7 +528,7 @@ Flags only understood by the interactive backend:
>>   * --edit-todo
>>   * --root when used in combination with --onto
>>  
>> -Other incompatible flag pairs:
>> +In addition, the following pairs of options are incompatible:
>>  
>>   * --preserve-merges and --interactive
>>   * --preserve-merges and --signoff
> 
> The rest of the diff is funny to read, but the post image makes a lot of
> sense.
> 
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index be004406a6..d55bbb9eb9 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -118,7 +118,7 @@ static void imply_interactive(struct rebase_options *opts, const char *option)
>>  	case REBASE_PRESERVE_MERGES:
>>  		break;
>>  	case REBASE_MERGE:
>> -		/* we silently *upgrade* --merge to --interactive if needed */
>> +		/* we now implement --merge via --interactive */
>>  	default:
>>  		opts->type = REBASE_INTERACTIVE; /* implied */
>>  		break;
>> @@ -475,10 +475,6 @@ static int run_specific_rebase(struct rebase_options *opts)
>>  		backend = "git-rebase--am";
>>  		backend_func = "git_rebase__am";
>>  		break;
>> -	case REBASE_MERGE:
>> -		backend = "git-rebase--merge";
>> -		backend_func = "git_rebase__merge";
>> -		break;
>>  	case REBASE_PRESERVE_MERGES:
>>  		backend = "git-rebase--preserve-merges";
>>  		backend_func = "git_rebase__preserve_merges";
>> @@ -1156,6 +1152,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>  		}
>>  	}
>>  
>> +	if (options.type == REBASE_MERGE) {
>> +		imply_interactive(&options, "--merge");
>> +	}
>> +
>>  	if (options.root && !options.onto_name)
>>  		imply_interactive(&options, "--root without --onto");
>>  
> 
> Okay, this makes sense.
> 
>> diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
>> index da27bfca5a..4abcceed06 100755
>> --- a/git-legacy-rebase.sh
>> +++ b/git-legacy-rebase.sh
>> @@ -218,6 +218,7 @@ then
>>  	state_dir="$apply_dir"
>>  elif test -d "$merge_dir"
>>  then
>> +	type=interactive
>>  	if test -d "$merge_dir"/rewritten
>>  	then
>>  		type=preserve-merges
>> @@ -225,10 +226,7 @@ then
>>  		preserve_merges=t
>>  	elif test -f "$merge_dir"/interactive
>>  	then
>> -		type=interactive
>>  		interactive_rebase=explicit
>> -	else
>> -		type=merge
>>  	fi
>>  	state_dir="$merge_dir"
>>  fi
>> @@ -469,6 +467,7 @@ then
>>  	test -z "$interactive_rebase" && interactive_rebase=implied
>>  fi
>>  
>> +actually_interactive=
>>  if test -n "$interactive_rebase"
>>  then
>>  	if test -z "$preserve_merges"
>> @@ -477,11 +476,12 @@ then
>>  	else
>>  		type=preserve-merges
>>  	fi
>> -
>> +	actually_interactive=t
>>  	state_dir="$merge_dir"
>>  elif test -n "$do_merge"
>>  then
>> -	type=merge
>> +	interactive_rebase=implied
>> +	type=interactive
>>  	state_dir="$merge_dir"
>>  else
>>  	type=am
>> @@ -493,21 +493,13 @@ then
>>  	git_format_patch_opt="$git_format_patch_opt --progress"
>>  fi
>>  
>> -if test -n "$git_am_opt"; then
>> -	incompatible_opts=$(echo " $git_am_opt " | \
>> -			    sed -e 's/ -q / /g' -e 's/^ \(.*\) $/\1/')
>> -	if test -n "$interactive_rebase"
>> +incompatible_opts=$(echo " $git_am_opt " | \
>> +		    sed -e 's/ -q / /g' -e 's/^ \(.*\) $/\1/')
> 
> Why are we no longer guarding this behind the condition that the user
> specified *any* option intended for the `am` backend?

I was confused by this as well, what if the user asks for 'rebase
--exec=<cmd> --ignore-whitespace'?

> 
>> +if test -n "$incompatible_opts"
>> +then
>> +	if test -n "$actually_interactive" || test "$do_merge"
>>  	then
>> -		if test -n "$incompatible_opts"
>> -		then
>> -			die "$(gettext "error: cannot combine interactive options (--interactive, --exec, --rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with am options ($incompatible_opts)")"
>> -		fi
>> -	fi
>> -	if test -n "$do_merge"; then
>> -		if test -n "$incompatible_opts"
>> -		then
>> -			die "$(gettext "error: cannot combine merge options (--merge, --strategy, --strategy-option) with am options ($incompatible_opts)")"
>> -		fi
>> +		die "$(gettext "error: cannot combine am options ($incompatible_opts) with either interactive or merge options")"
>>  	fi
>>  fi
>>  

If you want to change the error message here, I think you need to change
the corresponding message in builtin/rebase.c

Best Wishes

Phillip

> 
> The rest of this hunk makes sense.
> 
>> @@ -672,7 +664,7 @@ require_clean_work_tree "rebase" "$(gettext "Please commit or stash them.")"
>>  # but this should be done only when upstream and onto are the same
>>  # and if this is not an interactive rebase.
>>  mb=$(git merge-base "$onto" "$orig_head")
>> -if test -z "$interactive_rebase" && test "$upstream" = "$onto" &&
>> +if test -z "$actually_interactive" && test "$upstream" = "$onto" &&
>>  	test "$mb" = "$onto" && test -z "$restrict_revision" &&
>>  	# linear history?
>>  	! (git rev-list --parents "$onto".."$orig_head" | sane_grep " .* ") > /dev/null
>> @@ -716,6 +708,19 @@ then
>>  	GIT_PAGER='' git diff --stat --summary "$mb" "$onto"
>>  fi
>>  
>> +if test -z "$actually_interactive" && test "$mb" = "$orig_head"
>> +then
>> +	# If the $onto is a proper descendant of the tip of the branch, then
>> +	# we just fast-forwarded.
> 
> This comment is misleading if it comes before, rather than after, the
> actual `checkout -q` command.
> 
>> +	say "$(eval_gettext "Fast-forwarded \$branch_name to \$onto_name.")"
>> +	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" \
>> +		git checkout -q "$onto^0" || die "could not detach HEAD"
>> +	git update-ref ORIG_HEAD $orig_head
>> +	move_to_original_branch
>> +	finish_rebase
>> +	exit 0
>> +fi
>> +
>>  test -n "$interactive_rebase" && run_specific_rebase
>>  
>>  # Detach HEAD and reset the tree
>> @@ -725,16 +730,6 @@ GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" \
>>  	git checkout -q "$onto^0" || die "could not detach HEAD"
>>  git update-ref ORIG_HEAD $orig_head
> 
> It is a pity that this hunk header hides the lines that you copied into
> the following conditional before moving it before the "if interactive, run
> it" line:
> 
>>  
>> -# If the $onto is a proper descendant of the tip of the branch, then
>> -# we just fast-forwarded.
>> -if test "$mb" = "$orig_head"
>> -then
>> -	say "$(eval_gettext "Fast-forwarded \$branch_name to \$onto_name.")"
>> -	move_to_original_branch
>> -	finish_rebase
>> -	exit 0
>> -fi
>> -
>>  if test -n "$rebase_root"
>>  then
>>  	revisions="$onto..$orig_head"
> 
> What you did is correct, if duplicating code, and definitely the easiest
> way to do it. Just move the comment, and we're good here.
> 
>> diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
>> deleted file mode 100644
>> [... snip ...]
> 
> Nice. Really nice!
> 
>> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
>> index 0392e36d23..04d6c71899 100755
>> --- a/t/t3406-rebase-message.sh
>> +++ b/t/t3406-rebase-message.sh
>> @@ -17,14 +17,9 @@ test_expect_success 'setup' '
>>  	git tag start
>>  '
>>  
>> -cat >expect <<\EOF
>> -Already applied: 0001 A
>> -Already applied: 0002 B
>> -Committed: 0003 Z
>> -EOF
>> -
> 
> As I had mentioned in the previous round: I think we can retain these
> messages for the `--merge` mode. And I think we should: users of --merge
> have most likely become to expect them.
> 
> The most elegant way would probably be by adding code to the sequencer
> that outputs these lines if in "merge" mode (and add a flag to say that we
> *are* in "merge" mode).
> 
> To that end, the `make_script()` function would not generate `# pick` but
> `drop` lines, I think, so that the sequencer can see those and print the
> `Already applied: <message>` lines. And a successful `TODO_PICK` would
> write out `Committed: <message>`.
> 
>>  test_expect_success 'rebase -m' '
>>  	git rebase -m master >report &&
>> +	>expect &&
>>  	sed -n -e "/^Already applied: /p" \
>>  		-e "/^Committed: /p" report >actual &&
>>  	test_cmp expect actual
>> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
>> index f355c6825a..49077200c5 100755
>> --- a/t/t3420-rebase-autostash.sh
>> +++ b/t/t3420-rebase-autostash.sh
>> @@ -53,41 +53,6 @@ create_expected_success_interactive () {
>>  	EOF
>>  }
>>  
>> -create_expected_success_merge () {
>> -	cat >expected <<-EOF
>> -	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
>> -	HEAD is now at $(git rev-parse --short feature-branch) third commit
>> -	First, rewinding head to replay your work on top of it...
>> -	Merging unrelated-onto-branch with HEAD~1
>> -	Merging:
>> -	$(git rev-parse --short unrelated-onto-branch) unrelated commit
>> -	$(git rev-parse --short feature-branch^) second commit
>> -	found 1 common ancestor:
>> -	$(git rev-parse --short feature-branch~2) initial commit
>> -	[detached HEAD $(git rev-parse --short rebased-feature-branch~1)] second commit
>> -	 Author: A U Thor <author@example.com>
>> -	 Date: Thu Apr 7 15:14:13 2005 -0700
>> -	 2 files changed, 2 insertions(+)
>> -	 create mode 100644 file1
>> -	 create mode 100644 file2
>> -	Committed: 0001 second commit
>> -	Merging unrelated-onto-branch with HEAD~0
>> -	Merging:
>> -	$(git rev-parse --short rebased-feature-branch~1) second commit
>> -	$(git rev-parse --short feature-branch) third commit
>> -	found 1 common ancestor:
>> -	$(git rev-parse --short feature-branch~1) second commit
>> -	[detached HEAD $(git rev-parse --short rebased-feature-branch)] third commit
>> -	 Author: A U Thor <author@example.com>
>> -	 Date: Thu Apr 7 15:15:13 2005 -0700
>> -	 1 file changed, 1 insertion(+)
>> -	 create mode 100644 file3
>> -	Committed: 0002 third commit
>> -	All done.
>> -	Applied autostash.
>> -	EOF
>> -}
>> -
>>  create_expected_failure_am () {
>>  	cat >expected <<-EOF
>>  	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
>> @@ -112,43 +77,6 @@ create_expected_failure_interactive () {
>>  	EOF
>>  }
>>  
>> -create_expected_failure_merge () {
>> -	cat >expected <<-EOF
>> -	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
>> -	HEAD is now at $(git rev-parse --short feature-branch) third commit
>> -	First, rewinding head to replay your work on top of it...
>> -	Merging unrelated-onto-branch with HEAD~1
>> -	Merging:
>> -	$(git rev-parse --short unrelated-onto-branch) unrelated commit
>> -	$(git rev-parse --short feature-branch^) second commit
>> -	found 1 common ancestor:
>> -	$(git rev-parse --short feature-branch~2) initial commit
>> -	[detached HEAD $(git rev-parse --short rebased-feature-branch~1)] second commit
>> -	 Author: A U Thor <author@example.com>
>> -	 Date: Thu Apr 7 15:14:13 2005 -0700
>> -	 2 files changed, 2 insertions(+)
>> -	 create mode 100644 file1
>> -	 create mode 100644 file2
>> -	Committed: 0001 second commit
>> -	Merging unrelated-onto-branch with HEAD~0
>> -	Merging:
>> -	$(git rev-parse --short rebased-feature-branch~1) second commit
>> -	$(git rev-parse --short feature-branch) third commit
>> -	found 1 common ancestor:
>> -	$(git rev-parse --short feature-branch~1) second commit
>> -	[detached HEAD $(git rev-parse --short rebased-feature-branch)] third commit
>> -	 Author: A U Thor <author@example.com>
>> -	 Date: Thu Apr 7 15:15:13 2005 -0700
>> -	 1 file changed, 1 insertion(+)
>> -	 create mode 100644 file3
>> -	Committed: 0002 third commit
>> -	All done.
>> -	Applying autostash resulted in conflicts.
>> -	Your changes are safe in the stash.
>> -	You can run "git stash pop" or "git stash drop" at any time.
>> -	EOF
>> -}
>> -
>>  testrebase () {
>>  	type=$1
>>  	dotest=$2
>> @@ -177,6 +105,9 @@ testrebase () {
>>  	test_expect_success "rebase$type --autostash: check output" '
>>  		test_when_finished git branch -D rebased-feature-branch &&
>>  		suffix=${type#\ --} && suffix=${suffix:-am} &&
>> +		if test ${suffix} = "merge"; then
>> +			suffix=interactive
>> +		fi &&
>>  		create_expected_success_$suffix &&
>>  		test_i18ncmp expected actual
>>  	'
>> @@ -274,6 +205,9 @@ testrebase () {
>>  	test_expect_success "rebase$type: check output with conflicting stash" '
>>  		test_when_finished git branch -D rebased-feature-branch &&
>>  		suffix=${type#\ --} && suffix=${suffix:-am} &&
>> +		if test ${suffix} = "merge"; then
>> +			suffix=interactive
>> +		fi &&
>>  		create_expected_failure_$suffix &&
>>  		test_i18ncmp expected actual
>>  	'
> 
> As I stated earlier, I am uncomfortable with this solution. We change
> behavior rather noticeably, and the regression test tells us the same. We
> need to either try to deprecate `git rebase --merge`, or we need to at
> least attempt to recreate the old behavior with the sequencer.
> 
>> diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh
>> index 99b2aac921..911ef49f70 100755
>> --- a/t/t3421-rebase-topology-linear.sh
>> +++ b/t/t3421-rebase-topology-linear.sh
>> @@ -111,7 +111,7 @@ test_run_rebase () {
>>  	"
>>  }
>>  test_run_rebase success ''
>> -test_run_rebase failure -m
>> +test_run_rebase success -m
>>  test_run_rebase success -i
>>  test_run_rebase success -p
>>  
>> @@ -126,7 +126,7 @@ test_run_rebase () {
>>  	"
>>  }
>>  test_run_rebase success ''
>> -test_run_rebase failure -m
>> +test_run_rebase success -m
>>  test_run_rebase success -i
>>  test_run_rebase success -p
>>  
>> @@ -141,7 +141,7 @@ test_run_rebase () {
>>  	"
>>  }
>>  test_run_rebase success ''
>> -test_run_rebase failure -m
>> +test_run_rebase success -m
>>  test_run_rebase success -i
>>  test_run_rebase success -p
>>  
>> @@ -284,7 +284,7 @@ test_run_rebase () {
>>  	"
>>  }
>>  test_run_rebase success ''
>> -test_run_rebase failure -m
>> +test_run_rebase success -m
>>  test_run_rebase success -i
>>  test_run_rebase success -p
>>  
>> @@ -315,7 +315,7 @@ test_run_rebase () {
>>  	"
>>  }
>>  test_run_rebase success ''
>> -test_run_rebase failure -m
>> +test_run_rebase success -m
>>  test_run_rebase success -i
>>  test_run_rebase failure -p
>>  
> 
> Nice.
> 
>> diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
>> index 846f85c27e..cd505c0711 100755
>> --- a/t/t3425-rebase-topology-merges.sh
>> +++ b/t/t3425-rebase-topology-merges.sh
>> @@ -72,7 +72,7 @@ test_run_rebase () {
>>  }
>>  #TODO: make order consistent across all flavors of rebase
>>  test_run_rebase success 'e n o' ''
>> -test_run_rebase success 'e n o' -m
>> +test_run_rebase success 'n o e' -m
>>  test_run_rebase success 'n o e' -i
>>  
>>  test_run_rebase () {
>> @@ -89,7 +89,7 @@ test_run_rebase () {
>>  }
>>  #TODO: make order consistent across all flavors of rebase
>>  test_run_rebase success 'd e n o' ''
>> -test_run_rebase success 'd e n o' -m
>> +test_run_rebase success 'd n o e' -m
>>  test_run_rebase success 'd n o e' -i
>>  
>>  test_run_rebase () {
>> @@ -106,7 +106,7 @@ test_run_rebase () {
>>  }
>>  #TODO: make order consistent across all flavors of rebase
>>  test_run_rebase success 'd e n o' ''
>> -test_run_rebase success 'd e n o' -m
>> +test_run_rebase success 'd n o e' -m
>>  test_run_rebase success 'd n o e' -i
>>  
>>  test_expect_success "rebase -p is no-op in non-linear history" "
> 
> This is a bit unfortunate. I wonder if we could do something like this, to
> retain the current behavior:
> 
> -- snip --
> diff --git a/sequencer.c b/sequencer.c
> index 9e1ab3a2a7..5018957e49 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4394,7 +4394,8 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
>  	revs.reverse = 1;
>  	revs.right_only = 1;
>  	revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
> -	revs.topo_order = 1;
> +	if (!(flags & TODO_LIST_NO_TOPO_ORDER))
> +		revs.topo_order = 1;
>  
>  	revs.pretty_given = 1;
>  	git_config_get_string("rebase.instructionFormat", &format);
> -- snap -
> 
> (and then pass TODO_LIST_NO_TOPO_ORDER if in "merge" mode)?
> 
>> diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
>> index 9b2a274c71..145c61251d 100755
>> --- a/t/t5407-post-rewrite-hook.sh
>> +++ b/t/t5407-post-rewrite-hook.sh
>> @@ -120,6 +120,7 @@ test_expect_success 'git rebase -m --skip' '
>>  	git rebase --continue &&
>>  	echo rebase >expected.args &&
>>  	cat >expected.data <<-EOF &&
>> +	$(git rev-parse C) $(git rev-parse HEAD^)
>>  	$(git rev-parse D) $(git rev-parse HEAD)
>>  	EOF
>>  	verify_hook_input
> 
> This suggests to me that we call `commit` too many times. I think we
> really need to address this without changing the test. This is a change in
> behavior.
> 
>> diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
>> index 81a5179e28..5cadedb2a9 100755
>> --- a/t/t9903-bash-prompt.sh
>> +++ b/t/t9903-bash-prompt.sh
>> @@ -180,7 +180,7 @@ test_expect_success 'prompt - interactive rebase' '
>>  '
>>  
>>  test_expect_success 'prompt - rebase merge' '
>> -	printf " (b2|REBASE-m 1/3)" >expected &&
>> +	printf " (b2|REBASE-i 1/3)" >expected &&
> 
> As I said, this should be easily addressed by having a tell-tale in
> $state_dir/. Come to think of it, we must have had one, right? Let's see
> how the bash-prompt does it.
> 
> *clicketyclick*
> 
> Ah, it is the *absence* of the `interactive` file... Which makes me wonder
> whether we should not simply retain that behavior for `git rebase
> --merge`?
> 
>>  	git checkout b2 &&
>>  	test_when_finished "git checkout master" &&
>>  	test_must_fail git rebase --merge b1 b2 &&
>> -- 
>> 2.19.1.858.g526e8fe740.dirty
>>
>>
> 
> Thank you for this pleasant read. I think there is still quite a bit of
> work to do, but you already did most of it.
> 
> Out of curiosity, do you have a public repository with these patches in a
> branch? (I might have an hour to play with it tonight...)
> 
> Thanks!
> Dscho
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive
  2018-11-12 18:21     ` Phillip Wood
@ 2018-11-13  9:18       ` Johannes Schindelin
  2018-11-14 23:06       ` Elijah Newren
  1 sibling, 0 replies; 12+ messages in thread
From: Johannes Schindelin @ 2018-11-13  9:18 UTC (permalink / raw)
  To: phillip.wood; +Cc: Elijah Newren, git, gitster, predatoramigo

Hi Phillip,

On Mon, 12 Nov 2018, Phillip Wood wrote:

> It's good to see these patches progressing, I've just got a couple of
> comments related to Johannes' points below
> 
> On 12/11/2018 16:21, Johannes Schindelin wrote:
> > Hi Elijah,
> > 
> > On Wed, 7 Nov 2018, Elijah Newren wrote:
> > 
> >> Interactive rebases are implemented in terms of cherry-pick rather than
> >> the merge-recursive builtin, but cherry-pick also calls into the recursive
> >> merge machinery by default and can accept special merge strategies and/or
> >> special strategy options.  As such, there really is not any need for
> >> having both git-rebase--merge and git-rebase--interactive anymore.
> >>
> >> Delete git-rebase--merge.sh and have the --merge option be implemented
> >> by the now built-in interactive machinery.
> > 
> > Okay.
> > 
> >> Note that this change fixes a few known test failures (see t3421).
> > 
> > Nice!
> > 
> >> testcase modification notes:
> >>   t3406: --interactive and --merge had slightly different progress output
> >>          while running; adjust a test to match
> >>   t3420: these test precise output while running, but rebase--am,
> >>          rebase--merge, and rebase--interactive all were built on very
> >>          different commands (am, merge-recursive, cherry-pick), so the
> >>          tests expected different output for each type.  Now we expect
> >>          --merge and --interactive to have the same output.
> >>   t3421: --interactive fixes some bugs in --merge!  Wahoo!
> >>   t3425: topology linearization was inconsistent across flavors of rebase,
> >>          as already noted in a TODO comment in the testcase.  This was not
> >>          considered a bug before, so getting a different linearization due
> >>          to switching out backends should not be considered a bug now.
> > 
> > Ideally, the test would be fixed, then. If it fails for other reasons than
> > a real regression, it is not a very good regression test, is it.
> > 
> >>   t5407: different rebase types varied slightly in how many times checkout
> >>          or commit or equivalents were called based on a quick comparison
> >>          of this tests and previous ones which covered different rebase
> >>          flavors.  I think this is just attributable to this difference.
> > 
> > This concerns me.
> > 
> > In bigger repositories (no, I am not talking about Linux kernel sized
> > ones, I consider those small-ish) there are a ton of files, and checkout
> > and commit (and even more so reset) sadly do not have a runtime complexity
> > growing with the number of modified files, but with the number of tracked
> > files (and some commands even with the number of worktree files).
> > 
> > So a larger number of commit/checkout operations makes me expect
> > performance regressions.
> > 
> > In this light, could you elaborate a bit on the differences you see
> > between rebase -i and rebase -m?
> > 
> >>   t9903: --merge uses the interactive backend so the prompt expected is
> >>          now REBASE-i.
> > 
> > We should be able to make that test pass, still, by writing out a special
> > file (e.g. $state_dir/opt_m) and testing for that. Users are oddly upset
> > when their expectations are broken... (and I actually agree with them.)
> > 
> >> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> >> index 3407d835bd..35084f5681 100644
> >> --- a/Documentation/git-rebase.txt
> >> +++ b/Documentation/git-rebase.txt
> >> @@ -504,15 +504,7 @@ See also INCOMPATIBLE OPTIONS below.
> >>  INCOMPATIBLE OPTIONS
> >>  --------------------
> >>  
> >> -git-rebase has many flags that are incompatible with each other,
> >> -predominantly due to the fact that it has three different underlying
> >> -implementations:
> >> -
> >> - * one based on linkgit:git-am[1] (the default)
> >> - * one based on git-merge-recursive (merge backend)
> >> - * one based on linkgit:git-cherry-pick[1] (interactive backend)
> >> -
> > 
> > Could we retain this part, with `s/three/two/` and `/git-merge/d`? *Maybe*
> > `s/interactive backend/interactive\/merge backend/`
> 
> I was hoping we could get rid of that, I'm not sure how useful it is to
> users.

That's a good point. If the commit message makes the case for that (it is
an implementation detail that confuses users), I am fine with removing it,
too.

Thanks,
Dscho

> 
> > 
> >> -Flags only understood by the am backend:
> >> +The following options:
> >>  
> >>   * --committer-date-is-author-date
> >>   * --ignore-date
> >> @@ -520,15 +512,12 @@ Flags only understood by the am backend:
> >>   * --ignore-whitespace
> >>   * -C
> >>  
> >> -Flags understood by both merge and interactive backends:
> >> +are incompatible with the following options:
> >>  
> >>   * --merge
> >>   * --strategy
> >>   * --strategy-option
> >>   * --allow-empty-message
> >> -
> >> -Flags only understood by the interactive backend:
> >> -
> 
> It's nice to see this being simplified
> 
> >>   * --[no-]autosquash
> >>   * --rebase-merges
> >>   * --preserve-merges
> >> @@ -539,7 +528,7 @@ Flags only understood by the interactive backend:
> >>   * --edit-todo
> >>   * --root when used in combination with --onto
> >>  
> >> -Other incompatible flag pairs:
> >> +In addition, the following pairs of options are incompatible:
> >>  
> >>   * --preserve-merges and --interactive
> >>   * --preserve-merges and --signoff
> > 
> > The rest of the diff is funny to read, but the post image makes a lot of
> > sense.
> > 
> >> diff --git a/builtin/rebase.c b/builtin/rebase.c
> >> index be004406a6..d55bbb9eb9 100644
> >> --- a/builtin/rebase.c
> >> +++ b/builtin/rebase.c
> >> @@ -118,7 +118,7 @@ static void imply_interactive(struct rebase_options *opts, const char *option)
> >>  	case REBASE_PRESERVE_MERGES:
> >>  		break;
> >>  	case REBASE_MERGE:
> >> -		/* we silently *upgrade* --merge to --interactive if needed */
> >> +		/* we now implement --merge via --interactive */
> >>  	default:
> >>  		opts->type = REBASE_INTERACTIVE; /* implied */
> >>  		break;
> >> @@ -475,10 +475,6 @@ static int run_specific_rebase(struct rebase_options *opts)
> >>  		backend = "git-rebase--am";
> >>  		backend_func = "git_rebase__am";
> >>  		break;
> >> -	case REBASE_MERGE:
> >> -		backend = "git-rebase--merge";
> >> -		backend_func = "git_rebase__merge";
> >> -		break;
> >>  	case REBASE_PRESERVE_MERGES:
> >>  		backend = "git-rebase--preserve-merges";
> >>  		backend_func = "git_rebase__preserve_merges";
> >> @@ -1156,6 +1152,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> >>  		}
> >>  	}
> >>  
> >> +	if (options.type == REBASE_MERGE) {
> >> +		imply_interactive(&options, "--merge");
> >> +	}
> >> +
> >>  	if (options.root && !options.onto_name)
> >>  		imply_interactive(&options, "--root without --onto");
> >>  
> > 
> > Okay, this makes sense.
> > 
> >> diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
> >> index da27bfca5a..4abcceed06 100755
> >> --- a/git-legacy-rebase.sh
> >> +++ b/git-legacy-rebase.sh
> >> @@ -218,6 +218,7 @@ then
> >>  	state_dir="$apply_dir"
> >>  elif test -d "$merge_dir"
> >>  then
> >> +	type=interactive
> >>  	if test -d "$merge_dir"/rewritten
> >>  	then
> >>  		type=preserve-merges
> >> @@ -225,10 +226,7 @@ then
> >>  		preserve_merges=t
> >>  	elif test -f "$merge_dir"/interactive
> >>  	then
> >> -		type=interactive
> >>  		interactive_rebase=explicit
> >> -	else
> >> -		type=merge
> >>  	fi
> >>  	state_dir="$merge_dir"
> >>  fi
> >> @@ -469,6 +467,7 @@ then
> >>  	test -z "$interactive_rebase" && interactive_rebase=implied
> >>  fi
> >>  
> >> +actually_interactive=
> >>  if test -n "$interactive_rebase"
> >>  then
> >>  	if test -z "$preserve_merges"
> >> @@ -477,11 +476,12 @@ then
> >>  	else
> >>  		type=preserve-merges
> >>  	fi
> >> -
> >> +	actually_interactive=t
> >>  	state_dir="$merge_dir"
> >>  elif test -n "$do_merge"
> >>  then
> >> -	type=merge
> >> +	interactive_rebase=implied
> >> +	type=interactive
> >>  	state_dir="$merge_dir"
> >>  else
> >>  	type=am
> >> @@ -493,21 +493,13 @@ then
> >>  	git_format_patch_opt="$git_format_patch_opt --progress"
> >>  fi
> >>  
> >> -if test -n "$git_am_opt"; then
> >> -	incompatible_opts=$(echo " $git_am_opt " | \
> >> -			    sed -e 's/ -q / /g' -e 's/^ \(.*\) $/\1/')
> >> -	if test -n "$interactive_rebase"
> >> +incompatible_opts=$(echo " $git_am_opt " | \
> >> +		    sed -e 's/ -q / /g' -e 's/^ \(.*\) $/\1/')
> > 
> > Why are we no longer guarding this behind the condition that the user
> > specified *any* option intended for the `am` backend?
> 
> I was confused by this as well, what if the user asks for 'rebase
> --exec=<cmd> --ignore-whitespace'?
> 
> > 
> >> +if test -n "$incompatible_opts"
> >> +then
> >> +	if test -n "$actually_interactive" || test "$do_merge"
> >>  	then
> >> -		if test -n "$incompatible_opts"
> >> -		then
> >> -			die "$(gettext "error: cannot combine interactive options (--interactive, --exec, --rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with am options ($incompatible_opts)")"
> >> -		fi
> >> -	fi
> >> -	if test -n "$do_merge"; then
> >> -		if test -n "$incompatible_opts"
> >> -		then
> >> -			die "$(gettext "error: cannot combine merge options (--merge, --strategy, --strategy-option) with am options ($incompatible_opts)")"
> >> -		fi
> >> +		die "$(gettext "error: cannot combine am options ($incompatible_opts) with either interactive or merge options")"
> >>  	fi
> >>  fi
> >>  
> 
> If you want to change the error message here, I think you need to change
> the corresponding message in builtin/rebase.c
> 
> Best Wishes
> 
> Phillip
> 
> > 
> > The rest of this hunk makes sense.
> > 
> >> @@ -672,7 +664,7 @@ require_clean_work_tree "rebase" "$(gettext "Please commit or stash them.")"
> >>  # but this should be done only when upstream and onto are the same
> >>  # and if this is not an interactive rebase.
> >>  mb=$(git merge-base "$onto" "$orig_head")
> >> -if test -z "$interactive_rebase" && test "$upstream" = "$onto" &&
> >> +if test -z "$actually_interactive" && test "$upstream" = "$onto" &&
> >>  	test "$mb" = "$onto" && test -z "$restrict_revision" &&
> >>  	# linear history?
> >>  	! (git rev-list --parents "$onto".."$orig_head" | sane_grep " .* ") > /dev/null
> >> @@ -716,6 +708,19 @@ then
> >>  	GIT_PAGER='' git diff --stat --summary "$mb" "$onto"
> >>  fi
> >>  
> >> +if test -z "$actually_interactive" && test "$mb" = "$orig_head"
> >> +then
> >> +	# If the $onto is a proper descendant of the tip of the branch, then
> >> +	# we just fast-forwarded.
> > 
> > This comment is misleading if it comes before, rather than after, the
> > actual `checkout -q` command.
> > 
> >> +	say "$(eval_gettext "Fast-forwarded \$branch_name to \$onto_name.")"
> >> +	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" \
> >> +		git checkout -q "$onto^0" || die "could not detach HEAD"
> >> +	git update-ref ORIG_HEAD $orig_head
> >> +	move_to_original_branch
> >> +	finish_rebase
> >> +	exit 0
> >> +fi
> >> +
> >>  test -n "$interactive_rebase" && run_specific_rebase
> >>  
> >>  # Detach HEAD and reset the tree
> >> @@ -725,16 +730,6 @@ GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" \
> >>  	git checkout -q "$onto^0" || die "could not detach HEAD"
> >>  git update-ref ORIG_HEAD $orig_head
> > 
> > It is a pity that this hunk header hides the lines that you copied into
> > the following conditional before moving it before the "if interactive, run
> > it" line:
> > 
> >>  
> >> -# If the $onto is a proper descendant of the tip of the branch, then
> >> -# we just fast-forwarded.
> >> -if test "$mb" = "$orig_head"
> >> -then
> >> -	say "$(eval_gettext "Fast-forwarded \$branch_name to \$onto_name.")"
> >> -	move_to_original_branch
> >> -	finish_rebase
> >> -	exit 0
> >> -fi
> >> -
> >>  if test -n "$rebase_root"
> >>  then
> >>  	revisions="$onto..$orig_head"
> > 
> > What you did is correct, if duplicating code, and definitely the easiest
> > way to do it. Just move the comment, and we're good here.
> > 
> >> diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
> >> deleted file mode 100644
> >> [... snip ...]
> > 
> > Nice. Really nice!
> > 
> >> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> >> index 0392e36d23..04d6c71899 100755
> >> --- a/t/t3406-rebase-message.sh
> >> +++ b/t/t3406-rebase-message.sh
> >> @@ -17,14 +17,9 @@ test_expect_success 'setup' '
> >>  	git tag start
> >>  '
> >>  
> >> -cat >expect <<\EOF
> >> -Already applied: 0001 A
> >> -Already applied: 0002 B
> >> -Committed: 0003 Z
> >> -EOF
> >> -
> > 
> > As I had mentioned in the previous round: I think we can retain these
> > messages for the `--merge` mode. And I think we should: users of --merge
> > have most likely become to expect them.
> > 
> > The most elegant way would probably be by adding code to the sequencer
> > that outputs these lines if in "merge" mode (and add a flag to say that we
> > *are* in "merge" mode).
> > 
> > To that end, the `make_script()` function would not generate `# pick` but
> > `drop` lines, I think, so that the sequencer can see those and print the
> > `Already applied: <message>` lines. And a successful `TODO_PICK` would
> > write out `Committed: <message>`.
> > 
> >>  test_expect_success 'rebase -m' '
> >>  	git rebase -m master >report &&
> >> +	>expect &&
> >>  	sed -n -e "/^Already applied: /p" \
> >>  		-e "/^Committed: /p" report >actual &&
> >>  	test_cmp expect actual
> >> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
> >> index f355c6825a..49077200c5 100755
> >> --- a/t/t3420-rebase-autostash.sh
> >> +++ b/t/t3420-rebase-autostash.sh
> >> @@ -53,41 +53,6 @@ create_expected_success_interactive () {
> >>  	EOF
> >>  }
> >>  
> >> -create_expected_success_merge () {
> >> -	cat >expected <<-EOF
> >> -	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
> >> -	HEAD is now at $(git rev-parse --short feature-branch) third commit
> >> -	First, rewinding head to replay your work on top of it...
> >> -	Merging unrelated-onto-branch with HEAD~1
> >> -	Merging:
> >> -	$(git rev-parse --short unrelated-onto-branch) unrelated commit
> >> -	$(git rev-parse --short feature-branch^) second commit
> >> -	found 1 common ancestor:
> >> -	$(git rev-parse --short feature-branch~2) initial commit
> >> -	[detached HEAD $(git rev-parse --short rebased-feature-branch~1)] second commit
> >> -	 Author: A U Thor <author@example.com>
> >> -	 Date: Thu Apr 7 15:14:13 2005 -0700
> >> -	 2 files changed, 2 insertions(+)
> >> -	 create mode 100644 file1
> >> -	 create mode 100644 file2
> >> -	Committed: 0001 second commit
> >> -	Merging unrelated-onto-branch with HEAD~0
> >> -	Merging:
> >> -	$(git rev-parse --short rebased-feature-branch~1) second commit
> >> -	$(git rev-parse --short feature-branch) third commit
> >> -	found 1 common ancestor:
> >> -	$(git rev-parse --short feature-branch~1) second commit
> >> -	[detached HEAD $(git rev-parse --short rebased-feature-branch)] third commit
> >> -	 Author: A U Thor <author@example.com>
> >> -	 Date: Thu Apr 7 15:15:13 2005 -0700
> >> -	 1 file changed, 1 insertion(+)
> >> -	 create mode 100644 file3
> >> -	Committed: 0002 third commit
> >> -	All done.
> >> -	Applied autostash.
> >> -	EOF
> >> -}
> >> -
> >>  create_expected_failure_am () {
> >>  	cat >expected <<-EOF
> >>  	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
> >> @@ -112,43 +77,6 @@ create_expected_failure_interactive () {
> >>  	EOF
> >>  }
> >>  
> >> -create_expected_failure_merge () {
> >> -	cat >expected <<-EOF
> >> -	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
> >> -	HEAD is now at $(git rev-parse --short feature-branch) third commit
> >> -	First, rewinding head to replay your work on top of it...
> >> -	Merging unrelated-onto-branch with HEAD~1
> >> -	Merging:
> >> -	$(git rev-parse --short unrelated-onto-branch) unrelated commit
> >> -	$(git rev-parse --short feature-branch^) second commit
> >> -	found 1 common ancestor:
> >> -	$(git rev-parse --short feature-branch~2) initial commit
> >> -	[detached HEAD $(git rev-parse --short rebased-feature-branch~1)] second commit
> >> -	 Author: A U Thor <author@example.com>
> >> -	 Date: Thu Apr 7 15:14:13 2005 -0700
> >> -	 2 files changed, 2 insertions(+)
> >> -	 create mode 100644 file1
> >> -	 create mode 100644 file2
> >> -	Committed: 0001 second commit
> >> -	Merging unrelated-onto-branch with HEAD~0
> >> -	Merging:
> >> -	$(git rev-parse --short rebased-feature-branch~1) second commit
> >> -	$(git rev-parse --short feature-branch) third commit
> >> -	found 1 common ancestor:
> >> -	$(git rev-parse --short feature-branch~1) second commit
> >> -	[detached HEAD $(git rev-parse --short rebased-feature-branch)] third commit
> >> -	 Author: A U Thor <author@example.com>
> >> -	 Date: Thu Apr 7 15:15:13 2005 -0700
> >> -	 1 file changed, 1 insertion(+)
> >> -	 create mode 100644 file3
> >> -	Committed: 0002 third commit
> >> -	All done.
> >> -	Applying autostash resulted in conflicts.
> >> -	Your changes are safe in the stash.
> >> -	You can run "git stash pop" or "git stash drop" at any time.
> >> -	EOF
> >> -}
> >> -
> >>  testrebase () {
> >>  	type=$1
> >>  	dotest=$2
> >> @@ -177,6 +105,9 @@ testrebase () {
> >>  	test_expect_success "rebase$type --autostash: check output" '
> >>  		test_when_finished git branch -D rebased-feature-branch &&
> >>  		suffix=${type#\ --} && suffix=${suffix:-am} &&
> >> +		if test ${suffix} = "merge"; then
> >> +			suffix=interactive
> >> +		fi &&
> >>  		create_expected_success_$suffix &&
> >>  		test_i18ncmp expected actual
> >>  	'
> >> @@ -274,6 +205,9 @@ testrebase () {
> >>  	test_expect_success "rebase$type: check output with conflicting stash" '
> >>  		test_when_finished git branch -D rebased-feature-branch &&
> >>  		suffix=${type#\ --} && suffix=${suffix:-am} &&
> >> +		if test ${suffix} = "merge"; then
> >> +			suffix=interactive
> >> +		fi &&
> >>  		create_expected_failure_$suffix &&
> >>  		test_i18ncmp expected actual
> >>  	'
> > 
> > As I stated earlier, I am uncomfortable with this solution. We change
> > behavior rather noticeably, and the regression test tells us the same. We
> > need to either try to deprecate `git rebase --merge`, or we need to at
> > least attempt to recreate the old behavior with the sequencer.
> > 
> >> diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh
> >> index 99b2aac921..911ef49f70 100755
> >> --- a/t/t3421-rebase-topology-linear.sh
> >> +++ b/t/t3421-rebase-topology-linear.sh
> >> @@ -111,7 +111,7 @@ test_run_rebase () {
> >>  	"
> >>  }
> >>  test_run_rebase success ''
> >> -test_run_rebase failure -m
> >> +test_run_rebase success -m
> >>  test_run_rebase success -i
> >>  test_run_rebase success -p
> >>  
> >> @@ -126,7 +126,7 @@ test_run_rebase () {
> >>  	"
> >>  }
> >>  test_run_rebase success ''
> >> -test_run_rebase failure -m
> >> +test_run_rebase success -m
> >>  test_run_rebase success -i
> >>  test_run_rebase success -p
> >>  
> >> @@ -141,7 +141,7 @@ test_run_rebase () {
> >>  	"
> >>  }
> >>  test_run_rebase success ''
> >> -test_run_rebase failure -m
> >> +test_run_rebase success -m
> >>  test_run_rebase success -i
> >>  test_run_rebase success -p
> >>  
> >> @@ -284,7 +284,7 @@ test_run_rebase () {
> >>  	"
> >>  }
> >>  test_run_rebase success ''
> >> -test_run_rebase failure -m
> >> +test_run_rebase success -m
> >>  test_run_rebase success -i
> >>  test_run_rebase success -p
> >>  
> >> @@ -315,7 +315,7 @@ test_run_rebase () {
> >>  	"
> >>  }
> >>  test_run_rebase success ''
> >> -test_run_rebase failure -m
> >> +test_run_rebase success -m
> >>  test_run_rebase success -i
> >>  test_run_rebase failure -p
> >>  
> > 
> > Nice.
> > 
> >> diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
> >> index 846f85c27e..cd505c0711 100755
> >> --- a/t/t3425-rebase-topology-merges.sh
> >> +++ b/t/t3425-rebase-topology-merges.sh
> >> @@ -72,7 +72,7 @@ test_run_rebase () {
> >>  }
> >>  #TODO: make order consistent across all flavors of rebase
> >>  test_run_rebase success 'e n o' ''
> >> -test_run_rebase success 'e n o' -m
> >> +test_run_rebase success 'n o e' -m
> >>  test_run_rebase success 'n o e' -i
> >>  
> >>  test_run_rebase () {
> >> @@ -89,7 +89,7 @@ test_run_rebase () {
> >>  }
> >>  #TODO: make order consistent across all flavors of rebase
> >>  test_run_rebase success 'd e n o' ''
> >> -test_run_rebase success 'd e n o' -m
> >> +test_run_rebase success 'd n o e' -m
> >>  test_run_rebase success 'd n o e' -i
> >>  
> >>  test_run_rebase () {
> >> @@ -106,7 +106,7 @@ test_run_rebase () {
> >>  }
> >>  #TODO: make order consistent across all flavors of rebase
> >>  test_run_rebase success 'd e n o' ''
> >> -test_run_rebase success 'd e n o' -m
> >> +test_run_rebase success 'd n o e' -m
> >>  test_run_rebase success 'd n o e' -i
> >>  
> >>  test_expect_success "rebase -p is no-op in non-linear history" "
> > 
> > This is a bit unfortunate. I wonder if we could do something like this, to
> > retain the current behavior:
> > 
> > -- snip --
> > diff --git a/sequencer.c b/sequencer.c
> > index 9e1ab3a2a7..5018957e49 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -4394,7 +4394,8 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
> >  	revs.reverse = 1;
> >  	revs.right_only = 1;
> >  	revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
> > -	revs.topo_order = 1;
> > +	if (!(flags & TODO_LIST_NO_TOPO_ORDER))
> > +		revs.topo_order = 1;
> >  
> >  	revs.pretty_given = 1;
> >  	git_config_get_string("rebase.instructionFormat", &format);
> > -- snap -
> > 
> > (and then pass TODO_LIST_NO_TOPO_ORDER if in "merge" mode)?
> > 
> >> diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
> >> index 9b2a274c71..145c61251d 100755
> >> --- a/t/t5407-post-rewrite-hook.sh
> >> +++ b/t/t5407-post-rewrite-hook.sh
> >> @@ -120,6 +120,7 @@ test_expect_success 'git rebase -m --skip' '
> >>  	git rebase --continue &&
> >>  	echo rebase >expected.args &&
> >>  	cat >expected.data <<-EOF &&
> >> +	$(git rev-parse C) $(git rev-parse HEAD^)
> >>  	$(git rev-parse D) $(git rev-parse HEAD)
> >>  	EOF
> >>  	verify_hook_input
> > 
> > This suggests to me that we call `commit` too many times. I think we
> > really need to address this without changing the test. This is a change in
> > behavior.
> > 
> >> diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
> >> index 81a5179e28..5cadedb2a9 100755
> >> --- a/t/t9903-bash-prompt.sh
> >> +++ b/t/t9903-bash-prompt.sh
> >> @@ -180,7 +180,7 @@ test_expect_success 'prompt - interactive rebase' '
> >>  '
> >>  
> >>  test_expect_success 'prompt - rebase merge' '
> >> -	printf " (b2|REBASE-m 1/3)" >expected &&
> >> +	printf " (b2|REBASE-i 1/3)" >expected &&
> > 
> > As I said, this should be easily addressed by having a tell-tale in
> > $state_dir/. Come to think of it, we must have had one, right? Let's see
> > how the bash-prompt does it.
> > 
> > *clicketyclick*
> > 
> > Ah, it is the *absence* of the `interactive` file... Which makes me wonder
> > whether we should not simply retain that behavior for `git rebase
> > --merge`?
> > 
> >>  	git checkout b2 &&
> >>  	test_when_finished "git checkout master" &&
> >>  	test_must_fail git rebase --merge b1 b2 &&
> >> -- 
> >> 2.19.1.858.g526e8fe740.dirty
> >>
> >>
> > 
> > Thank you for this pleasant read. I think there is still quite a bit of
> > work to do, but you already did most of it.
> > 
> > Out of curiosity, do you have a public repository with these patches in a
> > branch? (I might have an hour to play with it tonight...)
> > 
> > Thanks!
> > Dscho
> > 
> 
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive
  2018-11-12 16:21   ` Johannes Schindelin
  2018-11-12 18:21     ` Phillip Wood
@ 2018-11-13 16:06     ` Elijah Newren
  2018-11-14 23:03     ` Elijah Newren
  2 siblings, 0 replies; 12+ messages in thread
From: Elijah Newren @ 2018-11-13 16:06 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Git Mailing List, Junio C Hamano, Pratik Karki

Hi Dscho,

Thanks for the detailed review!  I'll try to get back to all your
comments, but for now I feel bad that I didn't see and respond to at
least one sooner...

On Mon, Nov 12, 2018 at 8:21 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Thank you for this pleasant read. I think there is still quite a bit of
> work to do, but you already did most of it.
>
> Out of curiosity, do you have a public repository with these patches in a
> branch? (I might have an hour to play with it tonight...)

https://github.com/newren/git/tree/rebase-new-default, but ignore the
last two commits; they are separate and haven't been brought up to
date despite having been minimally rebased.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive
  2018-11-12 16:21   ` Johannes Schindelin
  2018-11-12 18:21     ` Phillip Wood
  2018-11-13 16:06     ` Elijah Newren
@ 2018-11-14 23:03     ` Elijah Newren
  2018-11-15 12:27       ` Johannes Schindelin
  2 siblings, 1 reply; 12+ messages in thread
From: Elijah Newren @ 2018-11-14 23:03 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Git Mailing List, Junio C Hamano, Pratik Karki

Hi Dscho,

On Mon, Nov 12, 2018 at 8:21 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> >   t3425: topology linearization was inconsistent across flavors of rebase,
> >          as already noted in a TODO comment in the testcase.  This was not
> >          considered a bug before, so getting a different linearization due
> >          to switching out backends should not be considered a bug now.
>
> Ideally, the test would be fixed, then. If it fails for other reasons than
> a real regression, it is not a very good regression test, is it.

I agree, it's not the best regression test.  It'd be better if it
defined and tested for "the correct behavior", but I suspect it has
some value in that it's is guaranteeing that the rebase flavors at
least give a "somewhat reasonable" result.  Sadly, It just allowed all
three rebase types to have slightly different "somewhat reasonable"
answers.

>
> >   t5407: different rebase types varied slightly in how many times checkout
> >          or commit or equivalents were called based on a quick comparison
> >          of this tests and previous ones which covered different rebase
> >          flavors.  I think this is just attributable to this difference.
>
> This concerns me.
>
> In bigger repositories (no, I am not talking about Linux kernel sized
> ones, I consider those small-ish) there are a ton of files, and checkout
> and commit (and even more so reset) sadly do not have a runtime complexity
> growing with the number of modified files, but with the number of tracked
> files (and some commands even with the number of worktree files).
>
> So a larger number of commit/checkout operations makes me expect
> performance regressions.
>
> In this light, could you elaborate a bit on the differences you see
> between rebase -i and rebase -m?

I wrote this comment months ago and don't remember the full details.
From the wording and as best I remember, I suspect I was at least
partially annoyed by the fact that these and other regression tests
for rebase seemed to not be testing for "correct" behavior, but
"existing" behavior.  That wouldn't be so bad, except that existing
behavior differed on the exact same test setup for different rebase
backends and the differences in behavior were checked and enforced in
the regression tests.  So, it became a matter of taste as to how much
things should be made identical and to which backend, or whether it
was more important to at least just consolidate the code first and
keep the results at least reasonable.  At the time, I figured getting
fewer backends, all with reasonable behavior was a bit more important,
but since this difference is worrisome to you, I will try to dig
further into it.

> >   t9903: --merge uses the interactive backend so the prompt expected is
> >          now REBASE-i.
>
> We should be able to make that test pass, still, by writing out a special
> file (e.g. $state_dir/opt_m) and testing for that. Users are oddly upset
> when their expectations are broken... (and I actually agree with them.)

I agree users are upset when expectations are broken, but why would
they expect REBASE-m?  In fact, I thought the prompt was a reflection
of what backend was in use, so that if someone reported an issue to
the git mailing list or a power user wanted to know where the control
files were located, they could look for them.  As such, I thought that
switching the prompt to REBASE-i was the right thing to do so that it
would match user expectations.  Yes, the backend changed; that's part
of the release notes.

Is there documentation somewhere that specifies the meaning of this
prompt differently than the expectations I had somehow built up?

> > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> > index 3407d835bd..35084f5681 100644
> > --- a/Documentation/git-rebase.txt
> > +++ b/Documentation/git-rebase.txt
> > @@ -504,15 +504,7 @@ See also INCOMPATIBLE OPTIONS below.
> >  INCOMPATIBLE OPTIONS
> >  --------------------
> >
> > -git-rebase has many flags that are incompatible with each other,
> > -predominantly due to the fact that it has three different underlying
> > -implementations:
> > -
> > - * one based on linkgit:git-am[1] (the default)
> > - * one based on git-merge-recursive (merge backend)
> > - * one based on linkgit:git-cherry-pick[1] (interactive backend)
> > -
>
> Could we retain this part, with `s/three/two/` and `/git-merge/d`? *Maybe*
> `s/interactive backend/interactive\/merge backend/`

Removing this was actually a suggestion by Phillip back in June at the
end of https://public-inbox.org/git/13ccb4d9-582b-d26b-f595-59cb0b7037ab@talktalk.net/,
for the purpose of removing an implementation details that users don't
need to know about.  As noted elsewhere in the thread, between you and
Phillip, I'll add some comments to the commit message about this.

> > -if test -n "$git_am_opt"; then
> > -     incompatible_opts=$(echo " $git_am_opt " | \
> > -                         sed -e 's/ -q / /g' -e 's/^ \(.*\) $/\1/')
> > -     if test -n "$interactive_rebase"
> > +incompatible_opts=$(echo " $git_am_opt " | \
> > +                 sed -e 's/ -q / /g' -e 's/^ \(.*\) $/\1/')
>
> Why are we no longer guarding this behind the condition that the user
> specified *any* option intended for the `am` backend?

The code is still correctly guarding, the diff is just confusing.
Sorry about that.  To explain, the code previously was basically:

if git_am_opt:
  if interactive:
    if incompatible_opts:
      show_error_about_interactive_and_am_incompatibilities
  if rebase-merge:
    if incompatible_opts
      show_error_about_merge_and_am_incompatibilities

which was a triply nested if, and the first (git_am_opt) and third
(incompatible_opts) were slightly redundant; the latter being a subset
of the former.  Perhaps I should have done a separate cleanup patch
before this that changed it to:

if incomptable_opts:
  if interactive:
    show_error_about_interactive_and_am_incompatibilities
  if rebase-merge:
    show_error_about_merge_and_am_incompatibilities

Before then having this patch coalesce that down to:

if incomptable_opts:
  if interactive:
    show_error_about_incompatible_options

Since it tripped up both you and Phillip, I'll add this preliminary
cleanup as a separate commit with accompanying explanation in my next
re-roll.


> > @@ -672,7 +664,7 @@ require_clean_work_tree "rebase" "$(gettext "Please commit or stash them.")"
> >  # but this should be done only when upstream and onto are the same
> >  # and if this is not an interactive rebase.
> >  mb=$(git merge-base "$onto" "$orig_head")
> > -if test -z "$interactive_rebase" && test "$upstream" = "$onto" &&
> > +if test -z "$actually_interactive" && test "$upstream" = "$onto" &&
> >       test "$mb" = "$onto" && test -z "$restrict_revision" &&
> >       # linear history?
> >       ! (git rev-list --parents "$onto".."$orig_head" | sane_grep " .* ") > /dev/null
> > @@ -716,6 +708,19 @@ then
> >       GIT_PAGER='' git diff --stat --summary "$mb" "$onto"
> >  fi
> >
> > +if test -z "$actually_interactive" && test "$mb" = "$orig_head"
> > +then
> > +     # If the $onto is a proper descendant of the tip of the branch, then
> > +     # we just fast-forwarded.
>
> This comment is misleading if it comes before, rather than after, the
> actual `checkout -q` command.
>
> > +     say "$(eval_gettext "Fast-forwarded \$branch_name to \$onto_name.")"
> > +     GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" \
> > +             git checkout -q "$onto^0" || die "could not detach HEAD"
> > +     git update-ref ORIG_HEAD $orig_head
> > +     move_to_original_branch
> > +     finish_rebase
> > +     exit 0
> > +fi
> > +
> >  test -n "$interactive_rebase" && run_specific_rebase
> >
> >  # Detach HEAD and reset the tree
> > @@ -725,16 +730,6 @@ GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" \
> >       git checkout -q "$onto^0" || die "could not detach HEAD"
> >  git update-ref ORIG_HEAD $orig_head
>
> It is a pity that this hunk header hides the lines that you copied into
> the following conditional before moving it before the "if interactive, run
> it" line:
>
> >
> > -# If the $onto is a proper descendant of the tip of the branch, then
> > -# we just fast-forwarded.
> > -if test "$mb" = "$orig_head"
> > -then
> > -     say "$(eval_gettext "Fast-forwarded \$branch_name to \$onto_name.")"
> > -     move_to_original_branch
> > -     finish_rebase
> > -     exit 0
> > -fi
> > -
> >  if test -n "$rebase_root"
> >  then
> >       revisions="$onto..$orig_head"
>
> What you did is correct, if duplicating code, and definitely the easiest
> way to do it. Just move the comment, and we're good here.

Will do.

>
> > diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
> > deleted file mode 100644
> > [... snip ...]
>
> Nice. Really nice!

:-)

> > diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> > index 0392e36d23..04d6c71899 100755
> > --- a/t/t3406-rebase-message.sh
> > +++ b/t/t3406-rebase-message.sh
> > @@ -17,14 +17,9 @@ test_expect_success 'setup' '
> >       git tag start
> >  '
> >
> > -cat >expect <<\EOF
> > -Already applied: 0001 A
> > -Already applied: 0002 B
> > -Committed: 0003 Z
> > -EOF
> > -
>
> As I had mentioned in the previous round: I think we can retain these
> messages for the `--merge` mode. And I think we should: users of --merge
> have most likely become to expect them.
>
> The most elegant way would probably be by adding code to the sequencer
> that outputs these lines if in "merge" mode (and add a flag to say that we
> *are* in "merge" mode).
>
> To that end, the `make_script()` function would not generate `# pick` but
> `drop` lines, I think, so that the sequencer can see those and print the
> `Already applied: <message>` lines. And a successful `TODO_PICK` would
> write out `Committed: <message>`.

I'll take a look, but is this a case where you want it only when
rebase previously showed them, or should it also affect other cases
such as --rebase-merges or --exec or general --interactive?

I'm hoping the latter, or that there's a good UI-level explanation for
why certain rebase flags would actually mean that users should expect
different output.  rebase seems to be loaded with cases where slight
variations on options yield very different side output and perhaps
even results mostly based on which backend it historically was
implemented on top of.  Those gratuitous inconsistencies drive this
particular user crazy.

> >  test_expect_success 'rebase -m' '
> >       git rebase -m master >report &&
> > +     >expect &&
> >       sed -n -e "/^Already applied: /p" \
> >               -e "/^Committed: /p" report >actual &&
> >       test_cmp expect actual
> > diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
> > index f355c6825a..49077200c5 100755
> > --- a/t/t3420-rebase-autostash.sh
> > +++ b/t/t3420-rebase-autostash.sh
> > @@ -53,41 +53,6 @@ create_expected_success_interactive () {
> >       EOF
> >  }
> >
> > -create_expected_success_merge () {
> > -     cat >expected <<-EOF
> > -     $(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
> > -     HEAD is now at $(git rev-parse --short feature-branch) third commit
> > -     First, rewinding head to replay your work on top of it...
> > -     Merging unrelated-onto-branch with HEAD~1
> > -     Merging:
> > -     $(git rev-parse --short unrelated-onto-branch) unrelated commit
> > -     $(git rev-parse --short feature-branch^) second commit
> > -     found 1 common ancestor:
> > -     $(git rev-parse --short feature-branch~2) initial commit
> > -     [detached HEAD $(git rev-parse --short rebased-feature-branch~1)] second commit
> > -      Author: A U Thor <author@example.com>
> > -      Date: Thu Apr 7 15:14:13 2005 -0700
> > -      2 files changed, 2 insertions(+)
> > -      create mode 100644 file1
> > -      create mode 100644 file2
> > -     Committed: 0001 second commit
> > -     Merging unrelated-onto-branch with HEAD~0
> > -     Merging:
> > -     $(git rev-parse --short rebased-feature-branch~1) second commit
> > -     $(git rev-parse --short feature-branch) third commit
> > -     found 1 common ancestor:
> > -     $(git rev-parse --short feature-branch~1) second commit
> > -     [detached HEAD $(git rev-parse --short rebased-feature-branch)] third commit
> > -      Author: A U Thor <author@example.com>
> > -      Date: Thu Apr 7 15:15:13 2005 -0700
> > -      1 file changed, 1 insertion(+)
> > -      create mode 100644 file3
> > -     Committed: 0002 third commit
> > -     All done.
> > -     Applied autostash.
> > -     EOF
> > -}
> > -
> >  create_expected_failure_am () {
> >       cat >expected <<-EOF
> >       $(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
> > @@ -112,43 +77,6 @@ create_expected_failure_interactive () {
> >       EOF
> >  }
> >
> > -create_expected_failure_merge () {
> > -     cat >expected <<-EOF
> > -     $(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
> > -     HEAD is now at $(git rev-parse --short feature-branch) third commit
> > -     First, rewinding head to replay your work on top of it...
> > -     Merging unrelated-onto-branch with HEAD~1
> > -     Merging:
> > -     $(git rev-parse --short unrelated-onto-branch) unrelated commit
> > -     $(git rev-parse --short feature-branch^) second commit
> > -     found 1 common ancestor:
> > -     $(git rev-parse --short feature-branch~2) initial commit
> > -     [detached HEAD $(git rev-parse --short rebased-feature-branch~1)] second commit
> > -      Author: A U Thor <author@example.com>
> > -      Date: Thu Apr 7 15:14:13 2005 -0700
> > -      2 files changed, 2 insertions(+)
> > -      create mode 100644 file1
> > -      create mode 100644 file2
> > -     Committed: 0001 second commit
> > -     Merging unrelated-onto-branch with HEAD~0
> > -     Merging:
> > -     $(git rev-parse --short rebased-feature-branch~1) second commit
> > -     $(git rev-parse --short feature-branch) third commit
> > -     found 1 common ancestor:
> > -     $(git rev-parse --short feature-branch~1) second commit
> > -     [detached HEAD $(git rev-parse --short rebased-feature-branch)] third commit
> > -      Author: A U Thor <author@example.com>
> > -      Date: Thu Apr 7 15:15:13 2005 -0700
> > -      1 file changed, 1 insertion(+)
> > -      create mode 100644 file3
> > -     Committed: 0002 third commit
> > -     All done.
> > -     Applying autostash resulted in conflicts.
> > -     Your changes are safe in the stash.
> > -     You can run "git stash pop" or "git stash drop" at any time.
> > -     EOF
> > -}
> > -
> >  testrebase () {
> >       type=$1
> >       dotest=$2
> > @@ -177,6 +105,9 @@ testrebase () {
> >       test_expect_success "rebase$type --autostash: check output" '
> >               test_when_finished git branch -D rebased-feature-branch &&
> >               suffix=${type#\ --} && suffix=${suffix:-am} &&
> > +             if test ${suffix} = "merge"; then
> > +                     suffix=interactive
> > +             fi &&
> >               create_expected_success_$suffix &&
> >               test_i18ncmp expected actual
> >       '
> > @@ -274,6 +205,9 @@ testrebase () {
> >       test_expect_success "rebase$type: check output with conflicting stash" '
> >               test_when_finished git branch -D rebased-feature-branch &&
> >               suffix=${type#\ --} && suffix=${suffix:-am} &&
> > +             if test ${suffix} = "merge"; then
> > +                     suffix=interactive
> > +             fi &&
> >               create_expected_failure_$suffix &&
> >               test_i18ncmp expected actual
> >       '
>
> As I stated earlier, I am uncomfortable with this solution. We change
> behavior rather noticeably, and the regression test tells us the same. We
> need to either try to deprecate `git rebase --merge`, or we need to at
> least attempt to recreate the old behavior with the sequencer.

Yes, I understand not wanting to confuse users.  But, serious
question, are you (unintentionally) enforcing that we continue
confusing users?  I may be totally misunderstanding you or the problem
space -- you know a lot more here than I do -- but to me your comments
sound like enforcing bug compatibility over correctness.

As stated in the git-2.19 release notes (relative to my previous series),
    "git rebase" behaved slightly differently depending on which one of
     the three backends gets used; this has been documented and an
     effort to make them more uniform has begun.
If we aren't trying to make the behavior more uniform and don't have
good UI rationale for things being different, then my motivation to
work on the affected aspects of rebase plummets.

Perhaps this is attributable to a difference of worldview we have, or
perhaps I'm just missing something totally obvious.  I'm hoping it's
the latter.  But if it helps, here's the angle I viewed it from: These
regression tests showed us that even when identical operations were
being performed from the user perspective, we got very different
results with the only difference being what backend happened to be
selected (and the backends had significant overlap in the area they
could all handle, as evidenced by the same testcase being run with
each).  That seems to me like pretty obvious proof that one or more
backends was buggy.  Said another way, it looks to me like this is
another example of regression tests enforcing "one semi-reasonable
behavior" instead of "the correct behavior".


I fully admit I don't have good UI rationale for why the output from
the interactive backend is correct.  It may not be.  I also don't have
good UI rationale for why the output from the traditional merge
backend would be correct.  (And it could be that both are wrong.)
It'd be better to figure out what is correct and make the code and the
tests implement and check for that.  However, while I don't know what
correct output to show the user is in these cases, I don't see how
it's possible that both backends could have previously been correct.
If someone could clearly enunciate a reason for different options to
yield different output, I'd happily go and implement it.  One obvious
explanation could be "--interactive implies user-interactivity, which
should thus have different output" -- but that doesn't explain the
past because we have several interactive_rebase=implied cases which
then should have matched --merge's output rather than --interactive's.
Absent any such UI rationale, I'd rather at least make all the
backends implement the same semi-reasonable behavior so it's at least
consistent.  Then when someone figures out what correct is, they can
fix it all in one place.

I'm curious to hear if someone has a good reason for which messages
should be preferred and when.  I'm also curious if perhaps people
think that I'm focusing too much on consistency and not enough on
"backward compatibility".

> > diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh
> > index 99b2aac921..911ef49f70 100755
> > --- a/t/t3421-rebase-topology-linear.sh
> > +++ b/t/t3421-rebase-topology-linear.sh
> > @@ -111,7 +111,7 @@ test_run_rebase () {
> >       "
> >  }
> >  test_run_rebase success ''
> > -test_run_rebase failure -m
> > +test_run_rebase success -m
> >  test_run_rebase success -i
> >  test_run_rebase success -p
> >
> > @@ -126,7 +126,7 @@ test_run_rebase () {
> >       "
> >  }
> >  test_run_rebase success ''
> > -test_run_rebase failure -m
> > +test_run_rebase success -m
> >  test_run_rebase success -i
> >  test_run_rebase success -p
> >
> > @@ -141,7 +141,7 @@ test_run_rebase () {
> >       "
> >  }
> >  test_run_rebase success ''
> > -test_run_rebase failure -m
> > +test_run_rebase success -m
> >  test_run_rebase success -i
> >  test_run_rebase success -p
> >
> > @@ -284,7 +284,7 @@ test_run_rebase () {
> >       "
> >  }
> >  test_run_rebase success ''
> > -test_run_rebase failure -m
> > +test_run_rebase success -m
> >  test_run_rebase success -i
> >  test_run_rebase success -p
> >
> > @@ -315,7 +315,7 @@ test_run_rebase () {
> >       "
> >  }
> >  test_run_rebase success ''
> > -test_run_rebase failure -m
> > +test_run_rebase success -m
> >  test_run_rebase success -i
> >  test_run_rebase failure -p
> >
>
> Nice.

:-)

> > diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
> > index 846f85c27e..cd505c0711 100755
> > --- a/t/t3425-rebase-topology-merges.sh
> > +++ b/t/t3425-rebase-topology-merges.sh
> > @@ -72,7 +72,7 @@ test_run_rebase () {
> >  }
> >  #TODO: make order consistent across all flavors of rebase
> >  test_run_rebase success 'e n o' ''
> > -test_run_rebase success 'e n o' -m
> > +test_run_rebase success 'n o e' -m
> >  test_run_rebase success 'n o e' -i
> >
> >  test_run_rebase () {
> > @@ -89,7 +89,7 @@ test_run_rebase () {
> >  }
> >  #TODO: make order consistent across all flavors of rebase
> >  test_run_rebase success 'd e n o' ''
> > -test_run_rebase success 'd e n o' -m
> > +test_run_rebase success 'd n o e' -m
> >  test_run_rebase success 'd n o e' -i
> >
> >  test_run_rebase () {
> > @@ -106,7 +106,7 @@ test_run_rebase () {
> >  }
> >  #TODO: make order consistent across all flavors of rebase
> >  test_run_rebase success 'd e n o' ''
> > -test_run_rebase success 'd e n o' -m
> > +test_run_rebase success 'd n o e' -m
> >  test_run_rebase success 'd n o e' -i
> >
> >  test_expect_success "rebase -p is no-op in non-linear history" "
>
> This is a bit unfortunate. I wonder if we could do something like this, to
> retain the current behavior:
>
> -- snip --
> diff --git a/sequencer.c b/sequencer.c
> index 9e1ab3a2a7..5018957e49 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4394,7 +4394,8 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
>         revs.reverse = 1;
>         revs.right_only = 1;
>         revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
> -       revs.topo_order = 1;
> +       if (!(flags & TODO_LIST_NO_TOPO_ORDER))
> +               revs.topo_order = 1;
>
>         revs.pretty_given = 1;
>         git_config_get_string("rebase.instructionFormat", &format);
> -- snap -
>
> (and then pass TODO_LIST_NO_TOPO_ORDER if in "merge" mode)?

Oh, sweet, we can make these consistent?  I tried to look at this a
little bit didn't dig too far.  I love it; I'll definitely give it a
try.

...but...why only in merge mode??  am, interactive, and merge backends
gave different results, and the regression test even pointed out that
whoever wrote these tests thought it was a bug (the several TODO
comments).  You knew where to fix it, but want to match previous
behavior instead of fixing the inconsistency?


<Snipped a couple things that we already discussed since they were
brought up in the commit message>

> Thank you for this pleasant read. I think there is still quite a bit of
> work to do, but you already did most of it.

Thanks for the detailed review and the pointers.  Much appreciated.
We may still have a disconnect of some sort (backwards/bug
compatibility vs. correctness/consistency), or perhaps I'm just
misunderstanding something for part of the review.  However, I can
definitely get to work on the other parts of your review comments, and
hopefully we can talk through the other bits.

> Out of curiosity, do you have a public repository with these patches in a
> branch? (I might have an hour to play with it tonight...)

So, yesterday I pointed you at my rebase-new-default branch, but since
it has other cruft as well, I just pushed this to a new branch with
just the stuff in this patch series.  I'll keep it up at

https://github.com/newren/git/tree/rebase-merge-on-sequencer


Thanks!
Elijah

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive
  2018-11-12 18:21     ` Phillip Wood
  2018-11-13  9:18       ` Johannes Schindelin
@ 2018-11-14 23:06       ` Elijah Newren
  1 sibling, 0 replies; 12+ messages in thread
From: Elijah Newren @ 2018-11-14 23:06 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Johannes Schindelin, Git Mailing List, Junio C Hamano, Pratik Karki

Hi Phillip,

On Mon, Nov 12, 2018 at 10:21 AM Phillip Wood <phillip.wood@talktalk.net> wrote:
> >> -Flags only understood by the am backend:
> >> +The following options:
> >>
> >>   * --committer-date-is-author-date
> >>   * --ignore-date
> >> @@ -520,15 +512,12 @@ Flags only understood by the am backend:
> >>   * --ignore-whitespace
> >>   * -C
> >>
> >> -Flags understood by both merge and interactive backends:
> >> +are incompatible with the following options:
> >>
> >>   * --merge
> >>   * --strategy
> >>   * --strategy-option
> >>   * --allow-empty-message
> >> -
> >> -Flags only understood by the interactive backend:
> >> -
>
> It's nice to see this being simplified

:-)

> >> -if test -n "$git_am_opt"; then
> >> -    incompatible_opts=$(echo " $git_am_opt " | \
> >> -                        sed -e 's/ -q / /g' -e 's/^ \(.*\) $/\1/')
> >> -    if test -n "$interactive_rebase"
> >> +incompatible_opts=$(echo " $git_am_opt " | \
> >> +                sed -e 's/ -q / /g' -e 's/^ \(.*\) $/\1/')
> >
> > Why are we no longer guarding this behind the condition that the user
> > specified *any* option intended for the `am` backend?
>
> I was confused by this as well, what if the user asks for 'rebase
> --exec=<cmd> --ignore-whitespace'?

They'd still get an error message about incompatible options; see my
email to Dscho.  However, since it tripped you both up, I'll make the
clean up here a separate commit with some comments.

> >> +if test -n "$incompatible_opts"
> >> +then
> >> +    if test -n "$actually_interactive" || test "$do_merge"
> >>      then
> >> -            if test -n "$incompatible_opts"
> >> -            then
> >> -                    die "$(gettext "error: cannot combine interactive options (--interactive, --exec, --rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with am options ($incompatible_opts)")"
> >> -            fi
> >> -    fi
> >> -    if test -n "$do_merge"; then
> >> -            if test -n "$incompatible_opts"
> >> -            then
> >> -                    die "$(gettext "error: cannot combine merge options (--merge, --strategy, --strategy-option) with am options ($incompatible_opts)")"
> >> -            fi
> >> +            die "$(gettext "error: cannot combine am options ($incompatible_opts) with either interactive or merge options")"
> >>      fi
> >>  fi
> >>
>
> If you want to change the error message here, I think you need to change
> the corresponding message in builtin/rebase.c

Indeed, will fix.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive
  2018-11-14 23:03     ` Elijah Newren
@ 2018-11-15 12:27       ` Johannes Schindelin
  0 siblings, 0 replies; 12+ messages in thread
From: Johannes Schindelin @ 2018-11-15 12:27 UTC (permalink / raw)
  To: Elijah Newren; +Cc: Git Mailing List, Junio C Hamano, Pratik Karki

Hi Elijah,

On Wed, 14 Nov 2018, Elijah Newren wrote:

> On Mon, Nov 12, 2018 at 8:21 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > >   t3425: topology linearization was inconsistent across flavors of rebase,
> > >          as already noted in a TODO comment in the testcase.  This was not
> > >          considered a bug before, so getting a different linearization due
> > >          to switching out backends should not be considered a bug now.
> >
> > Ideally, the test would be fixed, then. If it fails for other reasons than
> > a real regression, it is not a very good regression test, is it.
> 
> I agree, it's not the best regression test.  It'd be better if it
> defined and tested for "the correct behavior", but I suspect it has
> some value in that it's is guaranteeing that the rebase flavors at
> least give a "somewhat reasonable" result.  Sadly, It just allowed all
> three rebase types to have slightly different "somewhat reasonable"
> answers.

True. I hope, though, that we can address this relatively easily (by
toggling the `topo_order` flag).

> > >   t5407: different rebase types varied slightly in how many times checkout
> > >          or commit or equivalents were called based on a quick comparison
> > >          of this tests and previous ones which covered different rebase
> > >          flavors.  I think this is just attributable to this difference.
> >
> > This concerns me.
> >
> > In bigger repositories (no, I am not talking about Linux kernel sized
> > ones, I consider those small-ish) there are a ton of files, and checkout
> > and commit (and even more so reset) sadly do not have a runtime complexity
> > growing with the number of modified files, but with the number of tracked
> > files (and some commands even with the number of worktree files).
> >
> > So a larger number of commit/checkout operations makes me expect
> > performance regressions.
> >
> > In this light, could you elaborate a bit on the differences you see
> > between rebase -i and rebase -m?
> 
> I wrote this comment months ago and don't remember the full details.
> From the wording and as best I remember, I suspect I was at least
> partially annoyed by the fact that these and other regression tests
> for rebase seemed to not be testing for "correct" behavior, but
> "existing" behavior.  That wouldn't be so bad, except that existing
> behavior differed on the exact same test setup for different rebase
> backends and the differences in behavior were checked and enforced in
> the regression tests.  So, it became a matter of taste as to how much
> things should be made identical and to which backend, or whether it
> was more important to at least just consolidate the code first and
> keep the results at least reasonable.  At the time, I figured getting
> fewer backends, all with reasonable behavior was a bit more important,
> but since this difference is worrisome to you, I will try to dig
> further into it.

I agree that there is a ton of inconsistency going on, both in the
behavior of the different backends as well as in the tests and their
quality.

The best explanation I have for this is: it grew historically, and we
always have to strike a balance between strict rule enforcement and fun.

> > >   t9903: --merge uses the interactive backend so the prompt expected is
> > >          now REBASE-i.
> >
> > We should be able to make that test pass, still, by writing out a special
> > file (e.g. $state_dir/opt_m) and testing for that. Users are oddly upset
> > when their expectations are broken... (and I actually agree with them.)
> 
> I agree users are upset when expectations are broken, but why would
> they expect REBASE-m?  In fact, I thought the prompt was a reflection
> of what backend was in use, so that if someone reported an issue to
> the git mailing list or a power user wanted to know where the control
> files were located, they could look for them.  As such, I thought that
> switching the prompt to REBASE-i was the right thing to do so that it
> would match user expectations.  Yes, the backend changed; that's part
> of the release notes.

When you put it that way, I have to agree with you.

> Is there documentation somewhere that specifies the meaning of this
> prompt differently than the expectations I had somehow built up?

I was just looking from my perspective, with my experience: if I was a
heavy user of `git rebase -m` (I am not), I would stumble over this
prompt. That's all.

With your explanation, I agree that this is the best course, though.

> > > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> > > index 3407d835bd..35084f5681 100644
> > > --- a/Documentation/git-rebase.txt
> > > +++ b/Documentation/git-rebase.txt
> > > @@ -504,15 +504,7 @@ See also INCOMPATIBLE OPTIONS below.
> > >  INCOMPATIBLE OPTIONS
> > >  --------------------
> > >
> > > -git-rebase has many flags that are incompatible with each other,
> > > -predominantly due to the fact that it has three different underlying
> > > -implementations:
> > > -
> > > - * one based on linkgit:git-am[1] (the default)
> > > - * one based on git-merge-recursive (merge backend)
> > > - * one based on linkgit:git-cherry-pick[1] (interactive backend)
> > > -
> >
> > Could we retain this part, with `s/three/two/` and `/git-merge/d`? *Maybe*
> > `s/interactive backend/interactive\/merge backend/`
> 
> Removing this was actually a suggestion by Phillip back in June at the
> end of https://public-inbox.org/git/13ccb4d9-582b-d26b-f595-59cb0b7037ab@talktalk.net/,
> for the purpose of removing an implementation details that users don't
> need to know about.  As noted elsewhere in the thread, between you and
> Phillip, I'll add some comments to the commit message about this.

Right. It is not exactly a democracy over here, but I do agree that both
of your opinions combined weigh more than my single one. Besides, you have
a good point: the documentation is not so much about being technically
correct, but about being helpful to the users. And the new version is
probably more helpful than the old version.

> > > -if test -n "$git_am_opt"; then
> > > -     incompatible_opts=$(echo " $git_am_opt " | \
> > > -                         sed -e 's/ -q / /g' -e 's/^ \(.*\) $/\1/')
> > > -     if test -n "$interactive_rebase"
> > > +incompatible_opts=$(echo " $git_am_opt " | \
> > > +                 sed -e 's/ -q / /g' -e 's/^ \(.*\) $/\1/')
> >
> > Why are we no longer guarding this behind the condition that the user
> > specified *any* option intended for the `am` backend?
> 
> The code is still correctly guarding, the diff is just confusing.
> Sorry about that.  To explain, the code previously was basically:
> 
> if git_am_opt:
>   if interactive:
>     if incompatible_opts:
>       show_error_about_interactive_and_am_incompatibilities
>   if rebase-merge:
>     if incompatible_opts
>       show_error_about_merge_and_am_incompatibilities
> 
> which was a triply nested if, and the first (git_am_opt) and third
> (incompatible_opts) were slightly redundant; the latter being a subset
> of the former.

Ah, that's what I missed! Thank you for your explanation.

> Perhaps I should have done a separate cleanup patch before this that
> changed it to:
> 
> if incomptable_opts:
>   if interactive:
>     show_error_about_interactive_and_am_incompatibilities
>   if rebase-merge:
>     show_error_about_merge_and_am_incompatibilities
> 
> Before then having this patch coalesce that down to:
> 
> if incomptable_opts:
>   if interactive:
>     show_error_about_incompatible_options
> 
> Since it tripped up both you and Phillip, I'll add this preliminary
> cleanup as a separate commit with accompanying explanation in my next
> re-roll.

Thank you, much appreciated.

> > > diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> > > index 0392e36d23..04d6c71899 100755
> > > --- a/t/t3406-rebase-message.sh
> > > +++ b/t/t3406-rebase-message.sh
> > > @@ -17,14 +17,9 @@ test_expect_success 'setup' '
> > >       git tag start
> > >  '
> > >
> > > -cat >expect <<\EOF
> > > -Already applied: 0001 A
> > > -Already applied: 0002 B
> > > -Committed: 0003 Z
> > > -EOF
> > > -
> >
> > As I had mentioned in the previous round: I think we can retain these
> > messages for the `--merge` mode. And I think we should: users of --merge
> > have most likely become to expect them.
> >
> > The most elegant way would probably be by adding code to the sequencer
> > that outputs these lines if in "merge" mode (and add a flag to say that we
> > *are* in "merge" mode).
> >
> > To that end, the `make_script()` function would not generate `# pick` but
> > `drop` lines, I think, so that the sequencer can see those and print the
> > `Already applied: <message>` lines. And a successful `TODO_PICK` would
> > write out `Committed: <message>`.
> 
> I'll take a look, but is this a case where you want it only when
> rebase previously showed them, or should it also affect other cases
> such as --rebase-merges or --exec or general --interactive?

Oh, I was just interested in retaining the previous behavior. So only for
--merge, not for -r, --exec or -i.

> I'm hoping the latter, or that there's a good UI-level explanation for
> why certain rebase flags would actually mean that users should expect
> different output.

Hmm. You are right. This *is* an inconsistency, and insisting on
perpetuating it is maybe not the best idea.

> rebase seems to be loaded with cases where slight variations on options
> yield very different side output and perhaps even results mostly based
> on which backend it historically was implemented on top of.  Those
> gratuitous inconsistencies drive this particular user crazy.

If you put it that way, we can sell this change of behavior as an
improvement: we are *aligning* the output of `git rebase --merge` with the
output of `git rebase --interactive`.

I am starting to like it.

> > >  test_expect_success 'rebase -m' '
> > >       git rebase -m master >report &&
> > > +     >expect &&
> > >       sed -n -e "/^Already applied: /p" \
> > >               -e "/^Committed: /p" report >actual &&
> > >       test_cmp expect actual
> > > diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
> > > index f355c6825a..49077200c5 100755
> > > --- a/t/t3420-rebase-autostash.sh
> > > +++ b/t/t3420-rebase-autostash.sh
> > > @@ -53,41 +53,6 @@ create_expected_success_interactive () {
> > >       EOF
> > >  }
> > >
> > > -create_expected_success_merge () {
> > > -     cat >expected <<-EOF
> > > -     $(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
> > > -     HEAD is now at $(git rev-parse --short feature-branch) third commit
> > > -     First, rewinding head to replay your work on top of it...
> > > -     Merging unrelated-onto-branch with HEAD~1
> > > -     Merging:
> > > -     $(git rev-parse --short unrelated-onto-branch) unrelated commit
> > > -     $(git rev-parse --short feature-branch^) second commit
> > > -     found 1 common ancestor:
> > > -     $(git rev-parse --short feature-branch~2) initial commit
> > > -     [detached HEAD $(git rev-parse --short rebased-feature-branch~1)] second commit
> > > -      Author: A U Thor <author@example.com>
> > > -      Date: Thu Apr 7 15:14:13 2005 -0700
> > > -      2 files changed, 2 insertions(+)
> > > -      create mode 100644 file1
> > > -      create mode 100644 file2
> > > -     Committed: 0001 second commit
> > > -     Merging unrelated-onto-branch with HEAD~0
> > > -     Merging:
> > > -     $(git rev-parse --short rebased-feature-branch~1) second commit
> > > -     $(git rev-parse --short feature-branch) third commit
> > > -     found 1 common ancestor:
> > > -     $(git rev-parse --short feature-branch~1) second commit
> > > -     [detached HEAD $(git rev-parse --short rebased-feature-branch)] third commit
> > > -      Author: A U Thor <author@example.com>
> > > -      Date: Thu Apr 7 15:15:13 2005 -0700
> > > -      1 file changed, 1 insertion(+)
> > > -      create mode 100644 file3
> > > -     Committed: 0002 third commit
> > > -     All done.
> > > -     Applied autostash.
> > > -     EOF
> > > -}
> > > -
> > >  create_expected_failure_am () {
> > >       cat >expected <<-EOF
> > >       $(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
> > > @@ -112,43 +77,6 @@ create_expected_failure_interactive () {
> > >       EOF
> > >  }
> > >
> > > -create_expected_failure_merge () {
> > > -     cat >expected <<-EOF
> > > -     $(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
> > > -     HEAD is now at $(git rev-parse --short feature-branch) third commit
> > > -     First, rewinding head to replay your work on top of it...
> > > -     Merging unrelated-onto-branch with HEAD~1
> > > -     Merging:
> > > -     $(git rev-parse --short unrelated-onto-branch) unrelated commit
> > > -     $(git rev-parse --short feature-branch^) second commit
> > > -     found 1 common ancestor:
> > > -     $(git rev-parse --short feature-branch~2) initial commit
> > > -     [detached HEAD $(git rev-parse --short rebased-feature-branch~1)] second commit
> > > -      Author: A U Thor <author@example.com>
> > > -      Date: Thu Apr 7 15:14:13 2005 -0700
> > > -      2 files changed, 2 insertions(+)
> > > -      create mode 100644 file1
> > > -      create mode 100644 file2
> > > -     Committed: 0001 second commit
> > > -     Merging unrelated-onto-branch with HEAD~0
> > > -     Merging:
> > > -     $(git rev-parse --short rebased-feature-branch~1) second commit
> > > -     $(git rev-parse --short feature-branch) third commit
> > > -     found 1 common ancestor:
> > > -     $(git rev-parse --short feature-branch~1) second commit
> > > -     [detached HEAD $(git rev-parse --short rebased-feature-branch)] third commit
> > > -      Author: A U Thor <author@example.com>
> > > -      Date: Thu Apr 7 15:15:13 2005 -0700
> > > -      1 file changed, 1 insertion(+)
> > > -      create mode 100644 file3
> > > -     Committed: 0002 third commit
> > > -     All done.
> > > -     Applying autostash resulted in conflicts.
> > > -     Your changes are safe in the stash.
> > > -     You can run "git stash pop" or "git stash drop" at any time.
> > > -     EOF
> > > -}
> > > -
> > >  testrebase () {
> > >       type=$1
> > >       dotest=$2
> > > @@ -177,6 +105,9 @@ testrebase () {
> > >       test_expect_success "rebase$type --autostash: check output" '
> > >               test_when_finished git branch -D rebased-feature-branch &&
> > >               suffix=${type#\ --} && suffix=${suffix:-am} &&
> > > +             if test ${suffix} = "merge"; then
> > > +                     suffix=interactive
> > > +             fi &&
> > >               create_expected_success_$suffix &&
> > >               test_i18ncmp expected actual
> > >       '
> > > @@ -274,6 +205,9 @@ testrebase () {
> > >       test_expect_success "rebase$type: check output with conflicting stash" '
> > >               test_when_finished git branch -D rebased-feature-branch &&
> > >               suffix=${type#\ --} && suffix=${suffix:-am} &&
> > > +             if test ${suffix} = "merge"; then
> > > +                     suffix=interactive
> > > +             fi &&
> > >               create_expected_failure_$suffix &&
> > >               test_i18ncmp expected actual
> > >       '
> >
> > As I stated earlier, I am uncomfortable with this solution. We change
> > behavior rather noticeably, and the regression test tells us the same. We
> > need to either try to deprecate `git rebase --merge`, or we need to at
> > least attempt to recreate the old behavior with the sequencer.
> 
> Yes, I understand not wanting to confuse users.  But, serious
> question, are you (unintentionally) enforcing that we continue
> confusing users?  I may be totally misunderstanding you or the problem
> space -- you know a lot more here than I do -- but to me your comments
> sound like enforcing bug compatibility over correctness.

I am actually a really inappropriate person to make judgement calls on
`git rebase --merge`, as I am neither a user of it nor do I know of any. I
don't think I ever read any account of a Git for Windows user who called
`git rebase --merge`...

> As stated in the git-2.19 release notes (relative to my previous series),
>     "git rebase" behaved slightly differently depending on which one of
>      the three backends gets used; this has been documented and an
>      effort to make them more uniform has begun.
> If we aren't trying to make the behavior more uniform and don't have
> good UI rationale for things being different, then my motivation to
> work on the affected aspects of rebase plummets.
> 
> Perhaps this is attributable to a difference of worldview we have, or
> perhaps I'm just missing something totally obvious.  I'm hoping it's
> the latter.  But if it helps, here's the angle I viewed it from: These
> regression tests showed us that even when identical operations were
> being performed from the user perspective, we got very different
> results with the only difference being what backend happened to be
> selected (and the backends had significant overlap in the area they
> could all handle, as evidenced by the same testcase being run with
> each).  That seems to me like pretty obvious proof that one or more
> backends was buggy.  Said another way, it looks to me like this is
> another example of regression tests enforcing "one semi-reasonable
> behavior" instead of "the correct behavior".
> 
> 
> I fully admit I don't have good UI rationale for why the output from
> the interactive backend is correct.  It may not be.  I also don't have
> good UI rationale for why the output from the traditional merge
> backend would be correct.  (And it could be that both are wrong.)
> It'd be better to figure out what is correct and make the code and the
> tests implement and check for that.  However, while I don't know what
> correct output to show the user is in these cases, I don't see how
> it's possible that both backends could have previously been correct.
> If someone could clearly enunciate a reason for different options to
> yield different output, I'd happily go and implement it.  One obvious
> explanation could be "--interactive implies user-interactivity, which
> should thus have different output" -- but that doesn't explain the
> past because we have several interactive_rebase=implied cases which
> then should have matched --merge's output rather than --interactive's.
> Absent any such UI rationale, I'd rather at least make all the
> backends implement the same semi-reasonable behavior so it's at least
> consistent.  Then when someone figures out what correct is, they can
> fix it all in one place.
> 
> I'm curious to hear if someone has a good reason for which messages
> should be preferred and when.  I'm also curious if perhaps people
> think that I'm focusing too much on consistency and not enough on
> "backward compatibility".

It is good to have your perspective in addition to mine. As na open source
maintainer, I gravitate toward aiming for maximal backwards-compatibility
out of purely selfish reasons: I don't want to deal with so many people
complaining about changes in the software I maintain.

But you raised a good point: relying on behavior no matter how undesirable
said behavior is not doing users a big favor. It is better to face the
challenge of fixing that behavior.

> > > diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
> > > index 846f85c27e..cd505c0711 100755
> > > --- a/t/t3425-rebase-topology-merges.sh
> > > +++ b/t/t3425-rebase-topology-merges.sh
> > > @@ -72,7 +72,7 @@ test_run_rebase () {
> > >  }
> > >  #TODO: make order consistent across all flavors of rebase
> > >  test_run_rebase success 'e n o' ''
> > > -test_run_rebase success 'e n o' -m
> > > +test_run_rebase success 'n o e' -m
> > >  test_run_rebase success 'n o e' -i
> > >
> > >  test_run_rebase () {
> > > @@ -89,7 +89,7 @@ test_run_rebase () {
> > >  }
> > >  #TODO: make order consistent across all flavors of rebase
> > >  test_run_rebase success 'd e n o' ''
> > > -test_run_rebase success 'd e n o' -m
> > > +test_run_rebase success 'd n o e' -m
> > >  test_run_rebase success 'd n o e' -i
> > >
> > >  test_run_rebase () {
> > > @@ -106,7 +106,7 @@ test_run_rebase () {
> > >  }
> > >  #TODO: make order consistent across all flavors of rebase
> > >  test_run_rebase success 'd e n o' ''
> > > -test_run_rebase success 'd e n o' -m
> > > +test_run_rebase success 'd n o e' -m
> > >  test_run_rebase success 'd n o e' -i
> > >
> > >  test_expect_success "rebase -p is no-op in non-linear history" "
> >
> > This is a bit unfortunate. I wonder if we could do something like this, to
> > retain the current behavior:
> >
> > -- snip --
> > diff --git a/sequencer.c b/sequencer.c
> > index 9e1ab3a2a7..5018957e49 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -4394,7 +4394,8 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
> >         revs.reverse = 1;
> >         revs.right_only = 1;
> >         revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
> > -       revs.topo_order = 1;
> > +       if (!(flags & TODO_LIST_NO_TOPO_ORDER))
> > +               revs.topo_order = 1;
> >
> >         revs.pretty_given = 1;
> >         git_config_get_string("rebase.instructionFormat", &format);
> > -- snap -
> >
> > (and then pass TODO_LIST_NO_TOPO_ORDER if in "merge" mode)?
> 
> Oh, sweet, we can make these consistent?  I tried to look at this a
> little bit didn't dig too far.  I love it; I'll definitely give it a
> try.
> 
> ...but...why only in merge mode??  am, interactive, and merge backends
> gave different results, and the regression test even pointed out that
> whoever wrote these tests thought it was a bug (the several TODO
> comments).  You knew where to fix it, but want to match previous
> behavior instead of fixing the inconsistency?

I am afraid that we *have* to use topo_order for -r, but you know, I could
be convinced that it is a good idea to switch away from topo order for
regular -i.

> <Snipped a couple things that we already discussed since they were
> brought up in the commit message>
> 
> > Thank you for this pleasant read. I think there is still quite a bit of
> > work to do, but you already did most of it.
> 
> Thanks for the detailed review and the pointers.  Much appreciated.
> We may still have a disconnect of some sort (backwards/bug
> compatibility vs. correctness/consistency), or perhaps I'm just
> misunderstanding something for part of the review.  However, I can
> definitely get to work on the other parts of your review comments, and
> hopefully we can talk through the other bits.

I think it will get easier when other people chime in, so that we do not
go back and forth between our two opinions but instead get a broader
context in which to make decisions, so that we will hopefully end up with
a fine balance between consistency/correctness and
backwards-compatibility.

> > Out of curiosity, do you have a public repository with these patches in a
> > branch? (I might have an hour to play with it tonight...)
> 
> So, yesterday I pointed you at my rebase-new-default branch, but since
> it has other cruft as well, I just pushed this to a new branch with
> just the stuff in this patch series.  I'll keep it up at
> 
> https://github.com/newren/git/tree/rebase-merge-on-sequencer

Thank you!

I had originally planned on seeing how difficult it would be to reinstate
those messages of `rebase -m`, but in the meantime I get the impression
that I may not even want those to survive the merge of the --merge mode
into --interactive.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, back to index

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-08  6:01 [PATCH v2 0/2] Reimplement rebase --merge via interactive machinery Elijah Newren
2018-11-08  6:01 ` [PATCH v2 1/2] git-rebase, sequencer: extend --quiet option for the " Elijah Newren
2018-11-12 15:11   ` Johannes Schindelin
2018-11-08  6:01 ` [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive Elijah Newren
2018-11-12 16:21   ` Johannes Schindelin
2018-11-12 18:21     ` Phillip Wood
2018-11-13  9:18       ` Johannes Schindelin
2018-11-14 23:06       ` Elijah Newren
2018-11-13 16:06     ` Elijah Newren
2018-11-14 23:03     ` Elijah Newren
2018-11-15 12:27       ` Johannes Schindelin
2018-11-08  6:33 ` [PATCH v2 0/2] Reimplement rebase --merge via interactive machinery Elijah Newren

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox