git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* RFC: rebase inconsistency in 2.18 -- course of action?
@ 2018-06-07  4:58 Elijah Newren
  2018-06-07  5:04 ` [PATCH] t3401: add directory rename testcases for rebase and am Elijah Newren
                   ` (8 more replies)
  0 siblings, 9 replies; 47+ messages in thread
From: Elijah Newren @ 2018-06-07  4:58 UTC (permalink / raw)
  To: Git Mailing List, Stefan Beller, Johannes Schindelin, Alban Gruin

Hi everyone,

We've got a small rebase problem; I'm curious for thoughts about the
best course of action.  I'll provide several options below.

In short, while interactive-based and merge-based rebases handle
directory rename detection fine, am-based rebases do not.  This
inconsistency may frustrate or surprise users.

=== Demonstration Example ===

Let's say we have a repo with three commits -- an original commit
('O'), and two branches based off of it, A and B with an extra commit
each:

  O: has files l, x/a, x/b, x/c
  A: renames l -> lt, x/ -> y/
  B: modifies l, adds x/d

If I checkout B and run

  git rebase --interactive A

and don't bother editing the list of commits at all but just save,
then I end up with lt, y/{a, b, c, d} as expected.  However, if I run

  git rebase A

then I end up with lt, y/{a, b, c} and x/d; d is in the wrong place.

=== Problem explanation ===

am-based rebases suffer from a reduced ability to detect directory
renames upstream, which is fundamental to the fact that it throws away
information about the history: in particular, it dispenses with the
original commits involved by turning them into patches, and without
the knowledge of the original commits we cannot determine a proper
merge base.

The am-based rebase will proceed after generating patches by
commit-wise first trying git-apply, and if that fails it will attempt
a 3-way merge.  Since am has no knowledge of original commits, it
instead reconstructs a provisional merge base using
build_fake_ancestor() which will only contain the original versions of
the files modified in the patch.  Without the full list of files in
the real merge base, renames of any missing files cannot be detected.
Directory rename detection works by looking at individual file renames
and deducing when a full directory has been renamed.

Trying to modify build_fake_ancestor() to instead create a merge base
that includes common file information by looking for a commit that
contained all the same blobs as the pre-image of the patch would
require a very expensive search through history, and may choose a
wrong commit from among the several it finds.  (Also, regular am
outside of rebase may not find any commit that matches, forcing it to
somehow choose a near-fit).  Further, this would only work when the
fallback to a 3-way merge is triggered (i.e. when git-apply fails),
which may not happen for some patches. (For example, if the l->lt
change were not in the example, then git-apply would succeed and
incorrectly place d in x/, so we'd never even hit the 3-way fallback.)

In short, the am machinery simply doesn't have the necessary
information to properly detect directory renames, except when other
files involved in the renames upstream were also modified on the side
with the new file additions.

=== Possible solutions ===

1. Just accept it as a known shortcoming.

2. Revert directory rename detection from master...again.

   (Please, please, let's not pick this one.)

3. Claim it's not a bug at all.  In particular, the git-rebase(1) manpage
   states for the `--merge` option:

       Use merging strategies to rebase.  When the recursive (default)
       merge strategy is used, this allows rebase to be aware of
       renames on the upstream side.

   While "renames" is precisely why rebase --merge was added in commit
   58634dbff822 back on 2006-06-21, am-based rebases gained the
   ability to handle normal (non-directory) renames back in commit
   18cdf802ca6e from 2008-11-10.  So folks might be used to and expect
   their simple rebases to handle renames the same as --merge or
   --interactive rebases do.

4. Edit the 2.18 release notes.  Point out that directory renames can
   be detected by cherry-pick, merge, and rebases with either the -i
   or -m options (or any rebase option that implies either -i or -m);
   but not always by other rebases.  Folks may want to set pull.rebase
   to 'interactive' and just get used to immediately saving the
   popped-up list of commits and exiting to avoid this problem.

   Also, there's no good way to avoid this problem when using git-am
   directly.  At best, we can tell users to carefully select which
   older commit base to apply the patches on top of and then
   immediately run rebase --merge afterward.

5. Add an --am option to git-rebase, and make it not be the default
   (though make it be triggered by am-specific options like
   --whitespace or -C).  For the default, use either:

     5a. do_merge=t

     5b. interactive_rebase=implied

   As a slight aside, since there are only a few small changes
   necessary to make git-rebase--interactive handle ALL functionality
   from git-rebase--merge, we could take 5b one step further and
   delete git-rebase--merge and have one less rebase type.

   Either 5a or 5b may have a minor performance hit here, though
   personally I don't see this as a major factor.  But I'm happy to
   discuss at a high level if folks are curious (exact figures would
   depend heavily on the repo, size of commits, etc.)

6. Since there's not much time left before 2.18, do (1) and (4) for
   now, and (5) for 2.19.  Continue doing (1) or (3) for git-am itself.

7. Other solutions I haven't thought of?

=== Final Notes ===

I have patches implementing 5b, including the bonus removal of
git-rebase--merge.  However:

  * while my patches apply cleanly on master or next, there are lots
    of minor conflicts for the final two patches with ag/rebase-p in
    pu (in fact, even master conflicts with ag/rebase-p).  So I'm
    curious if I should wait off on sending these patches until that
    topic advances.

  * My first 9 patches are 7 independent fixes and cleanups, so I'll
    send those out shortly.


Thanks for reading this far,
Elijah

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

* [PATCH] t3401: add directory rename testcases for rebase and am
  2018-06-07  4:58 RFC: rebase inconsistency in 2.18 -- course of action? Elijah Newren
@ 2018-06-07  5:04 ` Elijah Newren
  2018-06-07  5:05 ` [PATCH] apply: fix grammar error in comment Elijah Newren
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 47+ messages in thread
From: Elijah Newren @ 2018-06-07  5:04 UTC (permalink / raw)
  To: git; +Cc: Elijah Newren

Add a simple directory rename testcase, in conjunction with each of the
types of rebases:
  git-rebase--interactive
  git-rebase--am
  git-rebase--merge
and also use the same testcase for
  git am --3way

This demonstrates an inconsistency between the different rebase backends
and which can detect the directory rename.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t3401-rebase-and-am-rename.sh | 105 ++++++++++++++++++++++++++++++++
 1 file changed, 105 insertions(+)
 create mode 100755 t/t3401-rebase-and-am-rename.sh

diff --git a/t/t3401-rebase-and-am-rename.sh b/t/t3401-rebase-and-am-rename.sh
new file mode 100755
index 0000000000..8f832957fc
--- /dev/null
+++ b/t/t3401-rebase-and-am-rename.sh
@@ -0,0 +1,105 @@
+#!/bin/sh
+
+test_description='git rebase + directory rename tests'
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
+test_expect_success 'setup testcase' '
+	test_create_repo dir-rename &&
+	(
+		cd dir-rename &&
+
+		mkdir x &&
+		test_seq  1 10 >x/a &&
+		test_seq 11 20 >x/b &&
+		test_seq 21 30 >x/c &&
+		test_write_lines a b c d e f g h i >l &&
+		git add x l &&
+		git commit -m "Initial" &&
+
+		git branch O &&
+		git branch A &&
+		git branch B &&
+
+		git checkout A &&
+		git mv x y &&
+		git mv l letters &&
+		git commit -m "Rename x to y, l to letters" &&
+
+		git checkout B &&
+		echo j >>l &&
+		test_seq 31 40 >x/d &&
+		git add l x/d &&
+		git commit -m "Modify l, add x/d"
+	)
+'
+
+test_expect_success 'rebase --interactive: directory rename detected' '
+	(
+		cd dir-rename &&
+
+		git checkout B^0 &&
+
+		set_fake_editor &&
+		FAKE_LINES="1" git rebase --interactive A &&
+
+		git ls-files -s >out &&
+		test_line_count = 5 out &&
+
+		test_path_is_file y/d &&
+		test_path_is_missing x/d
+	)
+'
+
+test_expect_failure 'rebase (am): directory rename detected' '
+	(
+		cd dir-rename &&
+
+		git checkout B^0 &&
+
+		git rebase A &&
+
+		git ls-files -s >out &&
+		test_line_count = 5 out &&
+
+		test_path_is_file y/d &&
+		test_path_is_missing x/d
+	)
+'
+
+test_expect_success 'rebase --merge: directory rename detected' '
+	(
+		cd dir-rename &&
+
+		git checkout B^0 &&
+
+		git rebase --merge A &&
+
+		git ls-files -s >out &&
+		test_line_count = 5 out &&
+
+		test_path_is_file y/d &&
+		test_path_is_missing x/d
+	)
+'
+
+test_expect_failure 'am: directory rename detected' '
+	(
+		cd dir-rename &&
+
+		git checkout A^0 &&
+
+		git format-patch -1 B &&
+
+		git am --3way 0001*.patch &&
+
+		git ls-files -s >out &&
+		test_line_count = 5 out &&
+
+		test_path_is_file y/d &&
+		test_path_is_missing x/d
+	)
+'
+
+test_done
-- 
2.18.0.rc0.46.g9cee8fce43


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

* [PATCH] apply: fix grammar error in comment
  2018-06-07  4:58 RFC: rebase inconsistency in 2.18 -- course of action? Elijah Newren
  2018-06-07  5:04 ` [PATCH] t3401: add directory rename testcases for rebase and am Elijah Newren
@ 2018-06-07  5:05 ` Elijah Newren
  2018-06-07  5:05 ` [PATCH] t5407: fix test to cover intended arguments Elijah Newren
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 47+ messages in thread
From: Elijah Newren @ 2018-06-07  5:05 UTC (permalink / raw)
  To: git; +Cc: Elijah Newren

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 apply.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/apply.c b/apply.c
index d79e61591b..85f2c92740 100644
--- a/apply.c
+++ b/apply.c
@@ -4053,7 +4053,7 @@ static int preimage_oid_in_gitlink_patch(struct patch *p, struct object_id *oid)
 	return get_oid_hex(p->old_sha1_prefix, oid);
 }
 
-/* Build an index that contains the just the files needed for a 3way merge */
+/* Build an index that contains just the files needed for a 3way merge */
 static int build_fake_ancestor(struct apply_state *state, struct patch *list)
 {
 	struct patch *patch;
-- 
2.18.0.rc0.46.g9cee8fce43


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

* [PATCH] t5407: fix test to cover intended arguments
  2018-06-07  4:58 RFC: rebase inconsistency in 2.18 -- course of action? Elijah Newren
  2018-06-07  5:04 ` [PATCH] t3401: add directory rename testcases for rebase and am Elijah Newren
  2018-06-07  5:05 ` [PATCH] apply: fix grammar error in comment Elijah Newren
@ 2018-06-07  5:05 ` Elijah Newren
  2018-06-07  5:06 ` [PATCH] git-rebase--merge: modernize "git-$cmd" to "git $cmd" Elijah Newren
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 47+ messages in thread
From: Elijah Newren @ 2018-06-07  5:05 UTC (permalink / raw)
  To: git; +Cc: Elijah Newren

Test 8 in t5407 appears to be an accidental exact duplicate of of test 5;
the testcode is identical and has identical repo state, but the test
description is different and suggests that rebase -m followed by rebase
--skip was what was actually supposed to be tested.  Modify the test to
include the -m option.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t5407-post-rewrite-hook.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
index 7a48236e87..9b2a274c71 100755
--- a/t/t5407-post-rewrite-hook.sh
+++ b/t/t5407-post-rewrite-hook.sh
@@ -113,7 +113,7 @@ test_expect_success 'git rebase -m' '
 test_expect_success 'git rebase -m --skip' '
 	git reset --hard D &&
 	clear_hook_input &&
-	test_must_fail git rebase --onto A B &&
+	test_must_fail git rebase -m --onto A B &&
 	test_must_fail git rebase --skip &&
 	echo D > foo &&
 	git add foo &&
-- 
2.18.0.rc0.46.g9cee8fce43


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

* [PATCH] git-rebase--merge: modernize "git-$cmd" to "git $cmd"
  2018-06-07  4:58 RFC: rebase inconsistency in 2.18 -- course of action? Elijah Newren
                   ` (2 preceding siblings ...)
  2018-06-07  5:05 ` [PATCH] t5407: fix test to cover intended arguments Elijah Newren
@ 2018-06-07  5:06 ` Elijah Newren
  2018-06-07  5:24   ` Elijah Newren
  2018-06-07  5:06 ` [PATCH 1/2] t3422: new testcases for checking when incompatible options passed Elijah Newren
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 47+ messages in thread
From: Elijah Newren @ 2018-06-07  5:06 UTC (permalink / raw)
  To: git; +Cc: Elijah Newren

<Comments for after diffstat:>
I tend to think git-rebase--merge is less popular than the other rebase
types, leading to it being more overlooked and less well tested than the
other ones.  This git-$cmd usage seems to support that argument.

Anyway, this patch may be irrelevant if others agree with my goal to
delete git-rebase--merge and implement --merge on top of the --interactive
machinery, but sending it along in case others don't agree with that goal.
</Comments>

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 git-rebase--merge.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index cf4c042214..aa2f2f0872 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -71,7 +71,7 @@ call_merge () {
 	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"'
+	eval 'git merge-$strategy' $strategy_opts $base ' -- "$hd" "$cmt"'
 	rv=$?
 	case "$rv" in
 	0)
@@ -88,7 +88,7 @@ call_merge () {
 		;;
 	*)
 		die "Unknown exit code ($rv) from command:" \
-			"git-merge-$strategy $cmt^ -- HEAD $cmt"
+			"git merge-$strategy $cmt^ -- HEAD $cmt"
 		;;
 	esac
 }
-- 
2.18.0.rc0.46.g9cee8fce43


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

* [PATCH 1/2] t3422: new testcases for checking when incompatible options passed
  2018-06-07  4:58 RFC: rebase inconsistency in 2.18 -- course of action? Elijah Newren
                   ` (3 preceding siblings ...)
  2018-06-07  5:06 ` [PATCH] git-rebase--merge: modernize "git-$cmd" to "git $cmd" Elijah Newren
@ 2018-06-07  5:06 ` Elijah Newren
  2018-06-07  5:06   ` [PATCH 2/2] git-rebase: error out " Elijah Newren
  2018-06-17  5:58   ` [RFC PATCH v2 0/7] Document/fix/warn about rebase incompatibilities and inconsistences Elijah Newren
  2018-06-07  5:07 ` [PATCH] git-rebase.sh: handle keep-empty like all other options Elijah Newren
                   ` (3 subsequent siblings)
  8 siblings, 2 replies; 47+ messages in thread
From: Elijah Newren @ 2018-06-07  5:06 UTC (permalink / raw)
  To: git; +Cc: Elijah Newren

git rebase is split into three types: am, merge, and interactive.  Various
options imply different types, and which mode we are using determine which
sub-script (git-rebase--$type) is executed to finish the work.  Not all
options work with all types, so add tests for combinations where we expect
to receive an error rather than having options be silently ignored.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t3422-rebase-incompatible-options.sh | 69 ++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100755 t/t3422-rebase-incompatible-options.sh

diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
new file mode 100755
index 0000000000..04cdae921b
--- /dev/null
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -0,0 +1,69 @@
+#!/bin/sh
+
+test_description='test if rebase detects and aborts on incompatible options'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	test_seq 2 9 >foo &&
+	git add foo &&
+	git commit -m orig &&
+
+	git branch A &&
+	git branch B &&
+
+	git checkout A &&
+	test_seq 1 9 >foo &&
+	git add foo &&
+	git commit -m A &&
+
+	git checkout B &&
+	# This is indented with HT SP HT.
+	echo "	 	foo();" >>foo &&
+	git add foo &&
+	git commit -m B
+'
+
+#
+# Rebase has lots of useful options like --whitepsace=fix, which are
+# actually all built in terms of flags to git-am.  Since neither
+# --merge nor --interactive (nor any options that imply those two) use
+# git-am, using them together will result in flags like --whitespace=fix
+# being ignored.  Make sure rebase warns the user and aborts instead.
+#
+
+test_run_rebase () {
+	opt=$1
+	shift
+	test_expect_failure "$opt incompatible with --merge" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --merge A
+	"
+
+	test_expect_failure "$opt incompatible with --strategy=ours" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --strategy=ours A
+	"
+
+	test_expect_failure "$opt incompatible with --strategy-option=ours" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --strategy=ours A
+	"
+
+	test_expect_failure "$opt incompatible with --interactive" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --interactive A
+	"
+
+	test_expect_failure "$opt incompatible with --exec" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --exec 'true' A
+	"
+
+}
+
+test_run_rebase --whitespace=fix
+test_run_rebase --ignore-whitespace
+test_run_rebase --committer-date-is-author-date
+test_run_rebase -C4
+
+test_done
-- 
2.18.0.rc0.46.g9cee8fce43


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

* [PATCH 2/2] git-rebase: error out when incompatible options passed
  2018-06-07  5:06 ` [PATCH 1/2] t3422: new testcases for checking when incompatible options passed Elijah Newren
@ 2018-06-07  5:06   ` " Elijah Newren
  2018-06-10 19:40     ` Phillip Wood
  2018-06-17  5:58   ` [RFC PATCH v2 0/7] Document/fix/warn about rebase incompatibilities and inconsistences Elijah Newren
  1 sibling, 1 reply; 47+ messages in thread
From: Elijah Newren @ 2018-06-07  5:06 UTC (permalink / raw)
  To: git; +Cc: Elijah Newren

git rebase has three different types: am, merge, and interactive, all of
which are implemented in terms of separate scripts.  am builds on git-am,
merge builds on git-merge-recursive, and interactive builds on
git-cherry-pick.  We make use of features in those lower-level commands in
the different rebase types, but those features don't exist in all of the
lower level commands so we have a range of incompatibilities.  Previously,
we just accepted nearly any argument and silently ignored whichever ones
weren't implemented for the type of rebase specified.  Change this so the
incompatibilities are documented, included in the testsuite, and tested
for at runtime with an appropriate error message shown.

Some exceptions I left out:

  * --merge and --interactive are technically incompatible since they are
    supposed to run different underlying scripts, but with a few small
    changes, --interactive can do everything that --merge can.  In fact,
    I'll shortly be sending another patch to remove git-rebase--merge and
    reimplement it on top of git-rebase--interactive.

  * One could argue that --interactive and --quiet are incompatible since
    --interactive doesn't implement a --quiet mode (perhaps since
    cherry-pick itself does not implement one).  However, the interactive
    mode is more quiet than the other modes in general with progress
    messages, so one could argue that it's already quiet.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt           | 15 +++++++++++++--
 git-rebase.sh                          | 17 +++++++++++++++++
 t/t3422-rebase-incompatible-options.sh | 10 +++++-----
 3 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 0e20a66e73..451252c173 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -243,6 +243,10 @@ leave out at most one of A and B, in which case it defaults to HEAD.
 --keep-empty::
 	Keep the commits that do not change anything from its
 	parents in the result.
++
+This uses the `--interactive` machinery internally, and as such,
+anything that is incompatible with --interactive is incompatible
+with this option.
 
 --allow-empty-message::
 	By default, rebasing commits with an empty message will fail.
@@ -324,6 +328,8 @@ which makes little sense.
 	and after each change.  When fewer lines of surrounding
 	context exist they all must match.  By default no context is
 	ever ignored.
+	Incompatible with the --merge and --interactive options, or
+	anything that implies those options or their machinery.
 
 -f::
 --force-rebase::
@@ -355,13 +361,15 @@ default is `--no-fork-point`, otherwise the default is `--fork-point`.
 --whitespace=<option>::
 	These flag are passed to the 'git apply' program
 	(see linkgit:git-apply[1]) that applies the patch.
-	Incompatible with the --interactive option.
+	Incompatible with the --merge and --interactive options, or
+	anything that implies those options or their machinery.
 
 --committer-date-is-author-date::
 --ignore-date::
 	These flags are passed to 'git am' to easily change the dates
 	of the rebased commits (see linkgit:git-am[1]).
-	Incompatible with the --interactive option.
+	Incompatible with the --merge and --interactive options, or
+	anything that implies those options or their machinery.
 
 --signoff::
 	Add a Signed-off-by: trailer to all the rebased commits. Note
@@ -400,6 +408,9 @@ The `--rebase-merges` mode is similar in spirit to `--preserve-merges`, but
 in contrast to that option works well in interactive rebases: commits can be
 reordered, inserted and dropped at will.
 +
+This uses the `--interactive` machinery internally, but it can be run
+without an explicit `--interactive`.
++
 It is currently only possible to recreate the merge commits using the
 `recursive` merge strategy; Different merge strategies can be used only via
 explicit `exec git merge -s <strategy> [...]` commands.
diff --git a/git-rebase.sh b/git-rebase.sh
index 40be59ecc4..f1dbecba18 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -503,6 +503,23 @@ 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//'`
+	if test -n "$interactive_rebase"
+	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
+	fi
+fi
+
 if test -n "$signoff"
 then
 	test -n "$preserve_merges" &&
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 04cdae921b..66a83363bf 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -34,27 +34,27 @@ test_expect_success 'setup' '
 test_run_rebase () {
 	opt=$1
 	shift
-	test_expect_failure "$opt incompatible with --merge" "
+	test_expect_success "$opt incompatible with --merge" "
 		git checkout B^0 &&
 		test_must_fail git rebase $opt --merge A
 	"
 
-	test_expect_failure "$opt incompatible with --strategy=ours" "
+	test_expect_success "$opt incompatible with --strategy=ours" "
 		git checkout B^0 &&
 		test_must_fail git rebase $opt --strategy=ours A
 	"
 
-	test_expect_failure "$opt incompatible with --strategy-option=ours" "
+	test_expect_success "$opt incompatible with --strategy-option=ours" "
 		git checkout B^0 &&
 		test_must_fail git rebase $opt --strategy=ours A
 	"
 
-	test_expect_failure "$opt incompatible with --interactive" "
+	test_expect_success "$opt incompatible with --interactive" "
 		git checkout B^0 &&
 		test_must_fail git rebase $opt --interactive A
 	"
 
-	test_expect_failure "$opt incompatible with --exec" "
+	test_expect_success "$opt incompatible with --exec" "
 		git checkout B^0 &&
 		test_must_fail git rebase $opt --exec 'true' A
 	"
-- 
2.18.0.rc0.46.g9cee8fce43


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

* [PATCH] git-rebase.sh: handle keep-empty like all other options
  2018-06-07  4:58 RFC: rebase inconsistency in 2.18 -- course of action? Elijah Newren
                   ` (4 preceding siblings ...)
  2018-06-07  5:06 ` [PATCH 1/2] t3422: new testcases for checking when incompatible options passed Elijah Newren
@ 2018-06-07  5:07 ` Elijah Newren
  2018-06-10 19:26   ` Phillip Wood
  2018-06-07  5:08 ` [PATCH 1/2] t3418: add testcase showing problems with rebase -i and strategy options Elijah Newren
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 47+ messages in thread
From: Elijah Newren @ 2018-06-07  5:07 UTC (permalink / raw)
  To: git; +Cc: Elijah Newren

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 git-rebase.sh | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 40be59ecc4..a56b286372 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -276,6 +276,7 @@ do
 		;;
 	--keep-empty)
 		keep_empty=yes
+		test -z "$interactive_rebase" && interactive_rebase=implied
 		;;
 	--allow-empty-message)
 		allow_empty_message=--allow-empty-message
@@ -480,11 +481,6 @@ then
 	test -z "$interactive_rebase" && interactive_rebase=implied
 fi
 
-if test -n "$keep_empty"
-then
-	test -z "$interactive_rebase" && interactive_rebase=implied
-fi
-
 if test -n "$interactive_rebase"
 then
 	type=interactive
-- 
2.18.0.rc0.46.g9cee8fce43


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

* [PATCH 1/2] t3418: add testcase showing problems with rebase -i and strategy options
  2018-06-07  4:58 RFC: rebase inconsistency in 2.18 -- course of action? Elijah Newren
                   ` (5 preceding siblings ...)
  2018-06-07  5:07 ` [PATCH] git-rebase.sh: handle keep-empty like all other options Elijah Newren
@ 2018-06-07  5:08 ` Elijah Newren
  2018-06-07  5:08   ` [PATCH 2/2] Fix use of strategy options with interactive rebases Elijah Newren
  2018-06-07 17:13 ` [RFC PATCH 0/3] Send more rebases through interactive machinery Elijah Newren
  2018-06-11 17:44 ` RFC: rebase inconsistency in 2.18 -- course of action? Junio C Hamano
  8 siblings, 1 reply; 47+ messages in thread
From: Elijah Newren @ 2018-06-07  5:08 UTC (permalink / raw)
  To: git; +Cc: Elijah Newren

We are not passing the same args to merge strategies when we are doing an
--interactive rebase as we do with a --merge rebase.  The merge strategy
should not need to be aware of which type of rebase is in effect.  Add a
testcase which checks for the appropriate args.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t3418-rebase-continue.sh | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 03bf1b8a3b..872022106f 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -74,6 +74,38 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
 	test -f funny.was.run
 '
 
+test_expect_failure 'rebase -i --continue handles merge strategy and options' '
+	rm -fr .git/rebase-* &&
+	git reset --hard commit-new-file-F2-on-topic-branch &&
+	test_commit "commit-new-file-F3-on-topic-branch-for-dash-i" F3 32 &&
+	test_when_finished "rm -fr test-bin funny.was.run funny.args" &&
+	mkdir test-bin &&
+	cat >test-bin/git-merge-funny <<-EOF &&
+	#!$SHELL_PATH
+	echo "\$@" >>funny.args
+	case "\$1" in --opt) ;; *) exit 2 ;; esac
+	case "\$2" in --foo) ;; *) exit 2 ;; esac
+	case "\$4" in --) ;; *) exit 2 ;; esac
+	shift 2 &&
+	>funny.was.run &&
+	exec git merge-recursive "\$@"
+	EOF
+	chmod +x test-bin/git-merge-funny &&
+	(
+		PATH=./test-bin:$PATH
+		test_must_fail git rebase -i -s funny -Xopt -Xfoo master topic
+	) &&
+	test -f funny.was.run &&
+	rm funny.was.run &&
+	echo "Resolved" >F2 &&
+	git add F2 &&
+	(
+		PATH=./test-bin:$PATH
+		git rebase --continue
+	) &&
+	test -f funny.was.run
+'
+
 test_expect_success 'rebase passes merge strategy options correctly' '
 	rm -fr .git/rebase-* &&
 	git reset --hard commit-new-file-F3-on-topic-branch &&
-- 
2.18.0.rc0.46.g9cee8fce43


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

* [PATCH 2/2] Fix use of strategy options with interactive rebases
  2018-06-07  5:08 ` [PATCH 1/2] t3418: add testcase showing problems with rebase -i and strategy options Elijah Newren
@ 2018-06-07  5:08   ` Elijah Newren
  0 siblings, 0 replies; 47+ messages in thread
From: Elijah Newren @ 2018-06-07  5:08 UTC (permalink / raw)
  To: git; +Cc: Elijah Newren

git-rebase.sh wrote stuff like
  '--ours'  '--renormalize'
to .git/rebase-merge/strategy_opts.  Note the double spaces.

git-rebase--interactive uses sequencer.c to parse that file, and
sequencer.c used split_cmdline() to get the individual strategy options.
After splitting, sequencer.c prefixed each "option" with a double dash,
so, concatenating all its options would result in:
  -- --ours -- --renormalize

So, when it ended up calling try_merge_strategy(), that in turn would run
  git merge-$strategy -- --ours -- --renormalize $merge_base -- $head $remote

instead of the expected
  git merge-$strategy --ours --renormalize $merge_base -- $head $remote

Remove the extra spaces so that split_cmdline() will work as expected.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 git-rebase.sh              | 2 +-
 sequencer.c                | 7 ++++++-
 t/t3418-rebase-continue.sh | 2 +-
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 40be59ecc4..224d5ebc29 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -316,7 +316,7 @@ do
 		do_merge=t
 		;;
 	--strategy-option=*)
-		strategy_opts="$strategy_opts $(git rev-parse --sq-quote "--${1#--strategy-option=}")"
+		strategy_opts="$strategy_opts $(git rev-parse --sq-quote "--${1#--strategy-option=}" | sed -e s/^.//)"
 		do_merge=t
 		test -z "$strategy" && strategy=recursive
 		;;
diff --git a/sequencer.c b/sequencer.c
index cca968043e..a2843e3906 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2203,6 +2203,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
 static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)
 {
 	int i;
+	char *strategy_opts_string;
 
 	strbuf_reset(buf);
 	if (!read_oneliner(buf, rebase_path_strategy(), 0))
@@ -2211,7 +2212,11 @@ static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)
 	if (!read_oneliner(buf, rebase_path_strategy_opts(), 0))
 		return;
 
-	opts->xopts_nr = split_cmdline(buf->buf, (const char ***)&opts->xopts);
+	strategy_opts_string = buf->buf;
+	if (*strategy_opts_string == ' ')
+		strategy_opts_string++;
+	opts->xopts_nr = split_cmdline(strategy_opts_string,
+				       (const char ***)&opts->xopts);
 	for (i = 0; i < opts->xopts_nr; i++) {
 		const char *arg = opts->xopts[i];
 
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 872022106f..7ca6cbc415 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -74,7 +74,7 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
 	test -f funny.was.run
 '
 
-test_expect_failure 'rebase -i --continue handles merge strategy and options' '
+test_expect_success 'rebase -i --continue handles merge strategy and options' '
 	rm -fr .git/rebase-* &&
 	git reset --hard commit-new-file-F2-on-topic-branch &&
 	test_commit "commit-new-file-F3-on-topic-branch-for-dash-i" F3 32 &&
-- 
2.18.0.rc0.46.g9cee8fce43


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

* Re: [PATCH] git-rebase--merge: modernize "git-$cmd" to "git $cmd"
  2018-06-07  5:06 ` [PATCH] git-rebase--merge: modernize "git-$cmd" to "git $cmd" Elijah Newren
@ 2018-06-07  5:24   ` Elijah Newren
  0 siblings, 0 replies; 47+ messages in thread
From: Elijah Newren @ 2018-06-07  5:24 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Elijah Newren

On Wed, Jun 6, 2018 at 10:06 PM, Elijah Newren <newren@gmail.com> wrote:
> <Comments for after diffstat:>
> I tend to think git-rebase--merge is less popular than the other rebase
> types, leading to it being more overlooked and less well tested than the
> other ones.  This git-$cmd usage seems to support that argument.
>
> Anyway, this patch may be irrelevant if others agree with my goal to
> delete git-rebase--merge and implement --merge on top of the --interactive
> machinery, but sending it along in case others don't agree with that goal.
> </Comments>

I would forget to pull that out of the commit message area and put
that next to the diffstat in the email.  Whoops.

Oh, well.

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

* [RFC PATCH 0/3] Send more rebases through interactive machinery
  2018-06-07  4:58 RFC: rebase inconsistency in 2.18 -- course of action? Elijah Newren
                   ` (6 preceding siblings ...)
  2018-06-07  5:08 ` [PATCH 1/2] t3418: add testcase showing problems with rebase -i and strategy options Elijah Newren
@ 2018-06-07 17:13 ` Elijah Newren
  2018-06-07 17:13   ` [RFC PATCH 1/3] git-rebase, sequencer: add a --quiet option for the " Elijah Newren
                     ` (2 more replies)
  2018-06-11 17:44 ` RFC: rebase inconsistency in 2.18 -- course of action? Junio C Hamano
  8 siblings, 3 replies; 47+ messages in thread
From: Elijah Newren @ 2018-06-07 17:13 UTC (permalink / raw)
  To: git; +Cc: gitster, Johannes.Schindelin, alban.gruin, Elijah Newren

This series:

  * deletes git-rebase--merge, making git-rebase--interactive handle
    those cases instead
  * adds an --am option to git rebase, and changes the rebase default
    from using git-rebase--am to git-rebase--interactive, fixing
    directory rename detection for default rebases.

However:

  * this series has several minor conflicts with ag/rebase-p in pu.
    But it's just an RFC for now; I can re-roll on top of that after
    getting some feedback.

  * this series depends on the other preparatory fixups under
      https://public-inbox.org/git/CABPp-BGxaroePB6aKWAkZeADLB7VE3y1CPy2RyNwpn=+C01g3A@mail.gmail.com/
    To get this series with the preparatory fixups:

Fetch-It-Via: git fetch https://github.com/newren/git rebase-new-default

Elijah Newren (3):
  git-rebase, sequencer: add a --quiet option for the interactive
    machinery
  rebase: Implement --merge via git-rebase--interactive
  git-rebase.sh: make git-rebase--interactive the default

 .gitignore                        |   1 -
 Documentation/git-rebase.txt      |  24 +++--
 Makefile                          |   1 -
 git-rebase--interactive.sh        |  14 ++-
 git-rebase--merge.sh              | 164 ------------------------------
 git-rebase.sh                     |  75 ++++++++------
 sequencer.c                       |  19 ++--
 sequencer.h                       |   1 +

Yes, there are a significant number of testcase changes below.  See
the relevant commit messages where I explain why each was changed.

 t/t3400-rebase.sh                 |   7 +-
 t/t3401-rebase-and-am-rename.sh   |  18 +++-
 t/t3404-rebase-interactive.sh     |   2 +-
 t/t3406-rebase-message.sh         |   7 +-
 t/t3407-rebase-abort.sh           |  28 ++++-
 t/t3420-rebase-autostash.sh       |  89 +++-------------
 t/t3421-rebase-topology-linear.sh |  10 +-
 t/t3425-rebase-topology-merges.sh |  15 +--
 t/t5407-post-rewrite-hook.sh      |   5 +-
 t/t5520-pull.sh                   |   9 +-
 t/t9903-bash-prompt.sh            |  12 ++-
 19 files changed, 180 insertions(+), 321 deletions(-)
 delete mode 100644 git-rebase--merge.sh

-- 
2.18.0.rc1.12.g2996b9442d

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

* [RFC PATCH 1/3] git-rebase, sequencer: add a --quiet option for the interactive machinery
  2018-06-07 17:13 ` [RFC PATCH 0/3] Send more rebases through interactive machinery Elijah Newren
@ 2018-06-07 17:13   ` " Elijah Newren
  2018-06-09 21:45     ` Johannes Schindelin
  2018-06-07 17:13   ` [RFC PATCH 2/3] rebase: Implement --merge via git-rebase--interactive Elijah Newren
  2018-06-07 17:13   ` [RFC PATCH 3/3] git-rebase.sh: make git-rebase--interactive the default Elijah Newren
  2 siblings, 1 reply; 47+ messages in thread
From: Elijah Newren @ 2018-06-07 17:13 UTC (permalink / raw)
  To: git; +Cc: gitster, Johannes.Schindelin, alban.gruin, 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.  Further, we want to make the interactive
machinery also take over for git-rebase--merge and become the default
merge strategy, so it makes sense for these other cases to have a 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..."

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 git-rebase--interactive.sh |  9 +++++++--
 git-rebase.sh              |  2 +-
 sequencer.c                | 19 +++++++++++++------
 sequencer.h                |  1 +
 4 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 06a7b79307..1f2401f702 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -139,8 +139,12 @@ mark_action_done () {
 	if test "$last_count" != "$new_count"
 	then
 		last_count=$new_count
-		eval_gettext "Rebasing (\$new_count/\$total)"; printf "\r"
-		test -z "$verbose" || echo
+		if test -z "$GIT_QUIET"
+		then
+			eval_gettext "Rebasing (\$new_count/\$total)";
+			printf "\r"
+			test -z "$verbose" || echo
+		fi
 	fi
 }
 
@@ -713,6 +717,7 @@ Commit or stash your changes, and then run
 		"$hook" rebase < "$rewritten_list"
 		true # we don't care if this hook failed
 	fi &&
+		test -z "$GIT_QUIET" &&
 		warn "$(eval_gettext "Successfully rebased and updated \$head_name.")"
 
 	return 1 # not failure; just to break the do_rest loop
diff --git a/git-rebase.sh b/git-rebase.sh
index 7d1612b31b..b639c0d4fe 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -136,7 +136,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 a2843e3906..d418d40bee 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -143,6 +143,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")
@@ -2251,6 +2252,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;
@@ -3171,10 +3175,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());
@@ -3385,8 +3390,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 c5787c6b56..da652a421f 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -33,6 +33,7 @@ struct replay_opts {
 	int allow_empty_message;
 	int keep_redundant_commits;
 	int verbose;
+	int quiet;
 
 	int mainline;
 
-- 
2.18.0.rc1.12.g2996b9442d


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

* [RFC PATCH 2/3] rebase: Implement --merge via git-rebase--interactive
  2018-06-07 17:13 ` [RFC PATCH 0/3] Send more rebases through interactive machinery Elijah Newren
  2018-06-07 17:13   ` [RFC PATCH 1/3] git-rebase, sequencer: add a --quiet option for the " Elijah Newren
@ 2018-06-07 17:13   ` Elijah Newren
  2018-06-09 22:04     ` Johannes Schindelin
  2018-06-07 17:13   ` [RFC PATCH 3/3] git-rebase.sh: make git-rebase--interactive the default Elijah Newren
  2 siblings, 1 reply; 47+ messages in thread
From: Elijah Newren @ 2018-06-07 17:13 UTC (permalink / raw)
  To: git; +Cc: gitster, Johannes.Schindelin, alban.gruin, 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 call in to
git-rebase--interactive.  Add one new variable ($actually_interactive) to
help keep cases separate, and adjust the detection of the special
circumstances under which a rebase boils down to a simple fast forward so
that it keeps working with the new structure.

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: tests of 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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 .gitignore                        |   1 -
 Documentation/git-rebase.txt      |  16 +--
 Makefile                          |   1 -
 git-rebase--interactive.sh        |   5 +-
 git-rebase--merge.sh              | 164 ------------------------------
 git-rebase.sh                     |  49 ++++-----
 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 +
 11 files changed, 55 insertions(+), 283 deletions(-)
 delete mode 100644 git-rebase--merge.sh

diff --git a/.gitignore b/.gitignore
index 388cc4beee..747be69d10 100644
--- a/.gitignore
+++ b/.gitignore
@@ -118,7 +118,6 @@
 /git-rebase--am
 /git-rebase--helper
 /git-rebase--interactive
-/git-rebase--merge
 /git-receive-pack
 /git-reflog
 /git-remote
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 451252c173..28d1658d7a 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -275,6 +275,10 @@ branch on top of the <upstream> branch.  Because of this, when a merge
 conflict happens, the side reported as 'ours' is the so-far rebased
 series, starting with <upstream>, and 'theirs' is the working branch.  In
 other words, the sides are swapped.
++
+This uses the `--interactive` machinery internally, and as such,
+anything that is incompatible with --interactive is incompatible
+with this option.
 
 -s <strategy>::
 --strategy=<strategy>::
@@ -328,8 +332,8 @@ which makes little sense.
 	and after each change.  When fewer lines of surrounding
 	context exist they all must match.  By default no context is
 	ever ignored.
-	Incompatible with the --merge and --interactive options, or
-	anything that implies those options or their machinery.
+	Incompatible with the --interactive option, or anything that
+	uses the `--interactive` machinery.
 
 -f::
 --force-rebase::
@@ -361,15 +365,15 @@ default is `--no-fork-point`, otherwise the default is `--fork-point`.
 --whitespace=<option>::
 	These flag are passed to the 'git apply' program
 	(see linkgit:git-apply[1]) that applies the patch.
-	Incompatible with the --merge and --interactive options, or
-	anything that implies those options or their machinery.
+	Incompatible with the --interactive option, or anything that
+	uses the `--interactive` machinery.
 
 --committer-date-is-author-date::
 --ignore-date::
 	These flags are passed to 'git am' to easily change the dates
 	of the rebased commits (see linkgit:git-am[1]).
-	Incompatible with the --merge and --interactive options, or
-	anything that implies those options or their machinery.
+	Incompatible with the --interactive option, or anything that
+	uses the `--interactive` machinery.
 
 --signoff::
 	Add a Signed-off-by: trailer to all the rebased commits. Note
diff --git a/Makefile b/Makefile
index 1d27f36365..8d24aaf638 100644
--- a/Makefile
+++ b/Makefile
@@ -620,7 +620,6 @@ SCRIPT_LIB += git-mergetool--lib
 SCRIPT_LIB += git-parse-remote
 SCRIPT_LIB += git-rebase--am
 SCRIPT_LIB += git-rebase--interactive
-SCRIPT_LIB += git-rebase--merge
 SCRIPT_LIB += git-sh-setup
 SCRIPT_LIB += git-sh-i18n
 
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 1f2401f702..dcc4a26a78 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -885,7 +885,10 @@ init_basic_state () {
 	mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")"
 	rm -f "$(git rev-parse --git-path REBASE_HEAD)"
 
-	: > "$state_dir"/interactive || die "$(gettext "Could not mark as interactive")"
+	if test -n "$actually_interactive"
+	then
+		: > "$state_dir"/interactive || die "$(gettext "Could not mark as interactive")"
+	fi
 	write_basic_state
 }
 
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/git-rebase.sh b/git-rebase.sh
index b639c0d4fe..5ac1dee30b 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -239,12 +239,10 @@ then
 	state_dir="$apply_dir"
 elif test -d "$merge_dir"
 then
+	type=interactive
 	if test -f "$merge_dir"/interactive
 	then
-		type=interactive
 		interactive_rebase=explicit
-	else
-		type=merge
 	fi
 	state_dir="$merge_dir"
 fi
@@ -481,13 +479,16 @@ then
 	test -z "$interactive_rebase" && interactive_rebase=implied
 fi
 
+actually_interactive=
 if test -n "$interactive_rebase"
 then
 	type=interactive
+	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
@@ -501,17 +502,11 @@ fi
 
 if test -n "$git_am_opt"; then
 	incompatible_opts=`echo "$git_am_opt" | sed -e 's/ -q//'`
-	if test -n "$interactive_rebase"
+	if test -n "$incompatible_opts"
 	then
-		if test -n "$incompatible_opts"
+		if test -n "$actually_interactive" || test "$do_merge"
 		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)")"
+			die "$(gettext "error: cannot combine am options ($incompatible_opts) with either interactive or merge options")"
 		fi
 	fi
 fi
@@ -660,7 +655,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 "$type" != interactive && 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
@@ -704,6 +699,22 @@ then
 	GIT_PAGER='' git diff --stat --summary "$mb" "$onto"
 fi
 
+if test -z "$actually_interactive"
+then
+	# 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.")"
+		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
+fi
+
 test "$type" = interactive && run_specific_rebase
 
 # Detach HEAD and reset the tree
@@ -713,16 +724,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/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 e243700660..c465713672 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
-- 
2.18.0.rc1.12.g2996b9442d


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

* [RFC PATCH 3/3] git-rebase.sh: make git-rebase--interactive the default
  2018-06-07 17:13 ` [RFC PATCH 0/3] Send more rebases through interactive machinery Elijah Newren
  2018-06-07 17:13   ` [RFC PATCH 1/3] git-rebase, sequencer: add a --quiet option for the " Elijah Newren
  2018-06-07 17:13   ` [RFC PATCH 2/3] rebase: Implement --merge via git-rebase--interactive Elijah Newren
@ 2018-06-07 17:13   ` Elijah Newren
  2018-06-09 22:11     ` Johannes Schindelin
  2 siblings, 1 reply; 47+ messages in thread
From: Elijah Newren @ 2018-06-07 17:13 UTC (permalink / raw)
  To: git; +Cc: gitster, Johannes.Schindelin, alban.gruin, Elijah Newren

am-based rebases suffer from an reduced ability to detect directory
renames upstream, which is fundamental to the fact that it throws away
information about the history: in particular, it dispenses with the
original commits involved by turning them into patches, and without the
knowledge of the original commits we cannot determine a proper merge base.

The am-based rebase will proceed by first trying git-apply, and, only if
it fails, will try to reconstruct a provisional merge base using
build_fake_ancestor().  This fake ancestor will ONLY contain the files
modified in the patch; without the full list of files in the real merge
base, renames of any missing files cannot be detected.  Directory rename
detection works by looking at individual file renames and deducing when a
full directory has been renamed.

Trying to modify build_fake_ancestor() to instead create a merge base that
includes common file information by looking for a commit that contained
all the same blobs as the pre-image of the patch would require a very
expensive search through history, may find the wrong commit, and outside
of rebase may not find any commit that matches.

But we had all the relevant information to start.  So, instead of
attempting to fix am which just doesn't have the relevant information,
instead note its strength and weaknesses, and change the default rebase
machinery to interactive since it does not suffer from the same problems.

Documentation updates:

  Several documentation updates were made as part of previous patches to
  note which sets of options were incompatible.  However, adding a new
  --am option allows us to make it clear which options imply this
  machinery and simplify the discussion of incompatible sets of options
  significantly.

testcase modification nodes:
  t3400: git-rebase--interactive.sh explicitly captures output and
         adds die "could not detach HEAD"; adjust to match
  t3401: fixes directory rename detection problems for rebase by default,
         though --am still fails on it
  t3404: testcase said it was for a non-interactive rebase, so add --am
  t3407: instead of having one test per rebase type, also add an extra one
         for the default rebase type.  That'll duplicate --merge for now,
         but will make switching default machinery again in the future
         clearer, if we ever choose to do so.  Also, previously there was
         a conspicuously missing test for all --quit combinations.
  t3420: command output varies by type of rebase and this test script
         has lots of helper functions providing the appropriate
         expectation.  Make sure we call the relevant one.
  t3425: Similar to t3407, add default rebase type to list of types to
         test.  See also comments about t3425 in the switchover of
         --merge to the interactive machinery.
  t5407: This suite has tests for different rebase types, so specify some
         are --am ones.
  t5520: interactive rebases capture and reprint informational message
         about conflicts to stdout rather than stderr.  Also, patches for
         interactive rebases are stored differently under .git than for
         am rebases.
  t9903: Add another testcase for am rebases.  Prompt for default rebases
         is now REBASE-m.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt      | 26 ++++++++++++--------------
 git-rebase.sh                     | 30 +++++++++++++++++++++---------
 t/t3400-rebase.sh                 |  7 ++++---
 t/t3401-rebase-and-am-rename.sh   | 18 +++++++++++++++++-
 t/t3404-rebase-interactive.sh     |  2 +-
 t/t3407-rebase-abort.sh           | 28 +++++++++++++++++++++++++++-
 t/t3420-rebase-autostash.sh       | 15 ++++++++++-----
 t/t3425-rebase-topology-merges.sh |  9 ++++++---
 t/t5407-post-rewrite-hook.sh      |  4 ++--
 t/t5520-pull.sh                   |  9 ++++++---
 t/t9903-bash-prompt.sh            | 12 +++++++++++-
 11 files changed, 117 insertions(+), 43 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 28d1658d7a..6cfb7479fd 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -240,13 +240,16 @@ leave out at most one of A and B, in which case it defaults to HEAD.
 	original branch. The index and working tree are also left
 	unchanged as a result.
 
+--am:
+	Use git-am internally to rebase.  This may run faster, but have
+	problems with rename detection on the upstream side.
+	Incompatible with the --interactive option, or anything that
+	uses the `--interactive` machinery.
+
 --keep-empty::
 	Keep the commits that do not change anything from its
 	parents in the result.
-+
-This uses the `--interactive` machinery internally, and as such,
-anything that is incompatible with --interactive is incompatible
-with this option.
+	This uses the `--interactive` machinery internally.
 
 --allow-empty-message::
 	By default, rebasing commits with an empty message will fail.
@@ -268,7 +271,7 @@ with this option.
 --merge::
 	Use merging strategies to rebase.  When the recursive (default) merge
 	strategy is used, this allows rebase to be aware of renames on the
-	upstream side.
+	upstream side.  This is the default.
 +
 Note that a rebase merge works by replaying each commit from the working
 branch on top of the <upstream> branch.  Because of this, when a merge
@@ -276,9 +279,7 @@ conflict happens, the side reported as 'ours' is the so-far rebased
 series, starting with <upstream>, and 'theirs' is the working branch.  In
 other words, the sides are swapped.
 +
-This uses the `--interactive` machinery internally, and as such,
-anything that is incompatible with --interactive is incompatible
-with this option.
+This uses the `--interactive` machinery internally.
 
 -s <strategy>::
 --strategy=<strategy>::
@@ -332,8 +333,7 @@ which makes little sense.
 	and after each change.  When fewer lines of surrounding
 	context exist they all must match.  By default no context is
 	ever ignored.
-	Incompatible with the --interactive option, or anything that
-	uses the `--interactive` machinery.
+	Implies --am.
 
 -f::
 --force-rebase::
@@ -365,15 +365,13 @@ default is `--no-fork-point`, otherwise the default is `--fork-point`.
 --whitespace=<option>::
 	These flag are passed to the 'git apply' program
 	(see linkgit:git-apply[1]) that applies the patch.
-	Incompatible with the --interactive option, or anything that
-	uses the `--interactive` machinery.
+	Implies --am.
 
 --committer-date-is-author-date::
 --ignore-date::
 	These flags are passed to 'git am' to easily change the dates
 	of the rebased commits (see linkgit:git-am[1]).
-	Incompatible with the --interactive option, or anything that
-	uses the `--interactive` machinery.
+	Implies --am.
 
 --signoff::
 	Add a Signed-off-by: trailer to all the rebased commits. Note
diff --git a/git-rebase.sh b/git-rebase.sh
index 5ac1dee30b..c8c3d0d05a 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -21,7 +21,8 @@ r,rebase-merges?   try to rebase merges instead of skipping them
 p,preserve-merges! try to recreate merges instead of ignoring them
 s,strategy=!       use the given merge strategy
 no-ff!             cherry-pick all commits, even if unchanged
-m,merge!           use merging strategies to rebase
+am                 use git-am to rebase
+m,merge!           use merging strategies to rebase; this is the default
 i,interactive!     let the user edit the list of commits to rebase
 x,exec=!           add exec lines after each commit of the editable list
 k,keep-empty	   preserve empty commits during rebase
@@ -69,6 +70,7 @@ unset restrict_revision
 cmd=
 strategy=
 strategy_opts=
+use_am=
 do_merge=
 merge_dir="$GIT_DIR"/rebase-merge
 apply_dir="$GIT_DIR"/rebase-apply
@@ -311,6 +313,9 @@ do
 	--no-fork-point)
 		fork_point=
 		;;
+	--am)
+		use_am=t
+		;;
 	--merge)
 		do_merge=t
 		;;
@@ -479,6 +484,7 @@ then
 	test -z "$interactive_rebase" && interactive_rebase=implied
 fi
 
+interactive_only_opts=`echo "$git_am_opt" | sed -e 's/ -q//'`
 actually_interactive=
 if test -n "$interactive_rebase"
 then
@@ -490,9 +496,18 @@ then
 	interactive_rebase=implied
 	type=interactive
 	state_dir="$merge_dir"
-else
+elif test -n "$use_am"
+then
 	type=am
 	state_dir="$apply_dir"
+elif test -n "$interactive_only_opts"
+then
+	type=am
+	state_dir="$apply_dir"
+else
+	interactive_rebase=implied
+	type=interactive
+	state_dir="$merge_dir"
 fi
 
 if test -t 2 && test -z "$GIT_QUIET"
@@ -500,14 +515,11 @@ 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//'`
-	if test -n "$incompatible_opts"
+if test -n "$interactive_only_opts"
+then
+	if test -n "$actually_interactive" || test "$do_merge"
 	then
-		if test -n "$actually_interactive" || test "$do_merge"
-		then
-			die "$(gettext "error: cannot combine am options ($incompatible_opts) with either interactive or merge options")"
-		fi
+		die "$(gettext "error: cannot combine am options ($interactive_only_opts) with either interactive or merge options")"
 	fi
 fi
 
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 72d9564747..03713572d6 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -136,8 +136,9 @@ test_expect_success 'setup: recover' '
 test_expect_success 'Show verbose error when HEAD could not be detached' '
 	>B &&
 	test_must_fail git rebase topic 2>output.err >output.out &&
-	test_i18ngrep "The following untracked working tree files would be overwritten by checkout:" output.err &&
-	test_i18ngrep B output.err
+	test_i18ngrep "The following untracked working tree files would be overwritten by checkout:" output.out &&
+	test_i18ngrep B output.out &&
+	test_i18ngrep "could not detach HEAD" output.err
 '
 rm -f B
 
@@ -289,7 +290,7 @@ test_expect_success 'rebase--am.sh and --show-current-patch' '
 		git tag two &&
 		test_must_fail git rebase --onto init HEAD^ &&
 		GIT_TRACE=1 git rebase --show-current-patch >/dev/null 2>stderr &&
-		grep "show.*$(git rev-parse two)" stderr
+		grep "show REBASE_HEAD" stderr
 	)
 '
 
diff --git a/t/t3401-rebase-and-am-rename.sh b/t/t3401-rebase-and-am-rename.sh
index ec87439bc6..6b54031201 100755
--- a/t/t3401-rebase-and-am-rename.sh
+++ b/t/t3401-rebase-and-am-rename.sh
@@ -54,7 +54,7 @@ test_expect_success 'rebase --interactive: directory rename detected' '
 	)
 '
 
-test_expect_failure 'rebase (am): directory rename detected' '
+test_expect_success 'rebase (default): directory rename detected' '
 	(
 		cd dir-rename &&
 
@@ -70,6 +70,22 @@ test_expect_failure 'rebase (am): directory rename detected' '
 	)
 '
 
+test_expect_failure 'rebase --am: directory rename detected' '
+	(
+		cd dir-rename &&
+
+		git checkout B^0 &&
+
+		git rebase --am A &&
+
+		git ls-files -s >out &&
+		test_line_count = 5 out &&
+
+		test_path_is_file y/d &&
+		test_path_is_missing x/d
+	)
+'
+
 test_expect_success 'rebase --merge: directory rename detected' '
 	(
 		cd dir-rename &&
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index c65826ddac..116060204e 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -975,7 +975,7 @@ test_expect_success C_LOCALE_OUTPUT 'rebase --edit-todo does not work on non-int
 	git reset --hard &&
 	git checkout conflict-branch &&
 	set_fake_editor &&
-	test_must_fail git rebase --onto HEAD~2 HEAD~ &&
+	test_must_fail git rebase --am --onto HEAD~2 HEAD~ &&
 	test_must_fail git rebase --edit-todo &&
 	git rebase --abort
 '
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 910f218284..33084b17f5 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -96,14 +96,28 @@ testrebase() {
 	'
 }
 
-testrebase "" .git/rebase-apply
+testrebase " --am" .git/rebase-apply
+testrebase "" .git/rebase-merge
 testrebase " --merge" .git/rebase-merge
+testrebase " --interactive" .git/rebase-merge
 
 test_expect_success 'rebase --quit' '
 	cd "$work_dir" &&
 	# Clean up the state from the previous one
 	git reset --hard pre-rebase &&
 	test_must_fail git rebase master &&
+	test_path_is_dir .git/rebase-merge &&
+	head_before=$(git rev-parse HEAD) &&
+	git rebase --quit &&
+	test $(git rev-parse HEAD) = $head_before &&
+	test ! -d .git/rebase-merge
+'
+
+test_expect_success 'rebase --am --quit' '
+	cd "$work_dir" &&
+	# Clean up the state from the previous one
+	git reset --hard pre-rebase &&
+	test_must_fail git rebase --am master &&
 	test_path_is_dir .git/rebase-apply &&
 	head_before=$(git rev-parse HEAD) &&
 	git rebase --quit &&
@@ -123,4 +137,16 @@ test_expect_success 'rebase --merge --quit' '
 	test ! -d .git/rebase-merge
 '
 
+test_expect_success 'rebase --interactive --quit' '
+	cd "$work_dir" &&
+	# Clean up the state from the previous one
+	git reset --hard pre-rebase &&
+	test_must_fail git rebase --interactive master &&
+	test_path_is_dir .git/rebase-merge &&
+	head_before=$(git rev-parse HEAD) &&
+	git rebase --quit &&
+	test $(git rev-parse HEAD) = $head_before &&
+	test ! -d .git/rebase-merge
+'
+
 test_done
diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index c465713672..9af0d27435 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -104,8 +104,10 @@ 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=${type#\ --} &&
+		if test ${suffix} = "am"; then
+			suffix=am
+		else
 			suffix=interactive
 		fi &&
 		create_expected_success_$suffix &&
@@ -204,8 +206,10 @@ 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=${type#\ --} &&
+		if test ${suffix} = "am"; then
+			suffix=am
+		else
 			suffix=interactive
 		fi &&
 		create_expected_failure_$suffix &&
@@ -235,7 +239,8 @@ test_expect_success "rebase: noop rebase" '
 	git checkout feature-branch
 '
 
-testrebase "" .git/rebase-apply
+testrebase " --am" .git/rebase-apply
+testrebase "" .git/rebase-merge
 testrebase " --merge" .git/rebase-merge
 testrebase " --interactive" .git/rebase-merge
 
diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
index cd505c0711..8fd14c86d3 100755
--- a/t/t3425-rebase-topology-merges.sh
+++ b/t/t3425-rebase-topology-merges.sh
@@ -71,7 +71,8 @@ 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' --am
+test_run_rebase success 'n o e' ''
 test_run_rebase success 'n o e' -m
 test_run_rebase success 'n o e' -i
 
@@ -88,7 +89,8 @@ 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' --am
+test_run_rebase success 'd n o e' ''
 test_run_rebase success 'd n o e' -m
 test_run_rebase success 'd n o e' -i
 
@@ -105,7 +107,8 @@ 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' --am
+test_run_rebase success 'd n o e' ''
 test_run_rebase success 'd n o e' -m
 test_run_rebase success 'd n o e' -i
 
diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
index 145c61251d..f5b5f4f04c 100755
--- a/t/t5407-post-rewrite-hook.sh
+++ b/t/t5407-post-rewrite-hook.sh
@@ -71,7 +71,7 @@ test_expect_success 'git rebase' '
 test_expect_success 'git rebase --skip' '
 	git reset --hard D &&
 	clear_hook_input &&
-	test_must_fail git rebase --onto A B &&
+	test_must_fail git rebase --am --onto A B &&
 	test_must_fail git rebase --skip &&
 	echo D > foo &&
 	git add foo &&
@@ -86,7 +86,7 @@ test_expect_success 'git rebase --skip' '
 test_expect_success 'git rebase --skip the last one' '
 	git reset --hard F &&
 	clear_hook_input &&
-	test_must_fail git rebase --onto D A &&
+	test_must_fail git rebase --am --onto D A &&
 	git rebase --skip &&
 	echo rebase >expected.args &&
 	cat >expected.data <<-EOF &&
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 59c4b778d3..d7c915a209 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -305,7 +305,7 @@ test_expect_success '--rebase with conflicts shows advice' '
 	test_tick &&
 	git commit -m "Create conflict" seq.txt &&
 	test_must_fail git pull --rebase . seq 2>err >out &&
-	test_i18ngrep "Resolve all conflicts manually" out
+	test_i18ngrep "Resolve all conflicts manually" err
 '
 
 test_expect_success 'failed --rebase shows advice' '
@@ -319,7 +319,7 @@ test_expect_success 'failed --rebase shows advice' '
 	git checkout -f -b fails-to-rebase HEAD^ &&
 	test_commit v2-without-cr file "2" file2-lf &&
 	test_must_fail git pull --rebase . diverging 2>err >out &&
-	test_i18ngrep "Resolve all conflicts manually" out
+	test_i18ngrep "Resolve all conflicts manually" err
 '
 
 test_expect_success '--rebase fails with multiple branches' '
@@ -671,7 +671,10 @@ test_expect_success 'setup for avoiding reapplying old patches' '
 test_expect_success 'git pull --rebase does not reapply old patches' '
 	(cd dst &&
 	 test_must_fail git pull --rebase &&
-	 test 1 = $(find .git/rebase-apply -name "000*" | wc -l)
+	 cat .git/rebase-merge/done .git/rebase-merge/git-rebase-todo >work &&
+	 grep -v -e \# -e ^$ work >patches &&
+	 test_line_count = 1 patches &&
+	 rm -f work patches
 	)
 '
 
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index c3b89ae783..29a83ae8cf 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -192,10 +192,20 @@ test_expect_success 'prompt - rebase merge' '
 	test_cmp expected "$actual"
 '
 
-test_expect_success 'prompt - rebase' '
+test_expect_success 'prompt - rebase am' '
 	printf " (b2|REBASE 1/3)" >expected &&
 	git checkout b2 &&
 	test_when_finished "git checkout master" &&
+	test_must_fail git rebase --am b1 b2 &&
+	test_when_finished "git rebase --abort" &&
+	__git_ps1 >"$actual" &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - rebase' '
+	printf " (b2|REBASE-m 1/3)" >expected &&
+	git checkout b2 &&
+	test_when_finished "git checkout master" &&
 	test_must_fail git rebase b1 b2 &&
 	test_when_finished "git rebase --abort" &&
 	__git_ps1 >"$actual" &&
-- 
2.18.0.rc1.12.g2996b9442d


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

* Re: [RFC PATCH 1/3] git-rebase, sequencer: add a --quiet option for the interactive machinery
  2018-06-07 17:13   ` [RFC PATCH 1/3] git-rebase, sequencer: add a --quiet option for the " Elijah Newren
@ 2018-06-09 21:45     ` Johannes Schindelin
  2018-06-09 23:05       ` Elijah Newren
  0 siblings, 1 reply; 47+ messages in thread
From: Johannes Schindelin @ 2018-06-09 21:45 UTC (permalink / raw)
  To: Elijah Newren; +Cc: git, gitster, alban.gruin

Hi Elijah,

On Thu, 7 Jun 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.  Further, we want to make the interactive
> machinery also take over for git-rebase--merge and become the default
> merge strategy, so it makes sense for these other cases to have a 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. As long as it is coordinated with Alban and Pratik, as both
of their GSoC projects are affected by this.

In particular Pratik's project, I think, would actually *benefit* from
your work, as it might even make it possible to turn all modes but
--preserve-merges into pure builtin code, which would be awesome.

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 06a7b79307..1f2401f702 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -139,8 +139,12 @@ mark_action_done () {
>  	if test "$last_count" != "$new_count"
>  	then
>  		last_count=$new_count
> -		eval_gettext "Rebasing (\$new_count/\$total)"; printf "\r"
> -		test -z "$verbose" || echo
> +		if test -z "$GIT_QUIET"
> +		then
> +			eval_gettext "Rebasing (\$new_count/\$total)";
> +			printf "\r"
> +			test -z "$verbose" || echo
> +		fi
>  	fi
>  }
>  
> @@ -713,6 +717,7 @@ Commit or stash your changes, and then run
>  		"$hook" rebase < "$rewritten_list"
>  		true # we don't care if this hook failed
>  	fi &&
> +		test -z "$GIT_QUIET" &&
>  		warn "$(eval_gettext "Successfully rebased and updated \$head_name.")"

In general, I tried the statements to return success at all times. That
means that

		test -n "$GIT_QUIET" ||

would be better in this case.

> diff --git a/git-rebase.sh b/git-rebase.sh
> index 7d1612b31b..b639c0d4fe 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -136,7 +136,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

Maybe it would be better to `echo t` into that file? That way, scripts
that used the value in that file would continue to work. (But maybe there
were no scripts that could use it, as only the interactive rebase allows
scripting, and it did not handle that flag before?)

The rest looks obviously good.

Thank you!
Dscho

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

* Re: [RFC PATCH 2/3] rebase: Implement --merge via git-rebase--interactive
  2018-06-07 17:13   ` [RFC PATCH 2/3] rebase: Implement --merge via git-rebase--interactive Elijah Newren
@ 2018-06-09 22:04     ` Johannes Schindelin
  2018-06-10  1:14       ` Elijah Newren
  0 siblings, 1 reply; 47+ messages in thread
From: Johannes Schindelin @ 2018-06-09 22:04 UTC (permalink / raw)
  To: Elijah Newren; +Cc: git, gitster, alban.gruin

Hi Elijah,

On Thu, 7 Jun 2018, Elijah Newren wrote:

> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 451252c173..28d1658d7a 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -275,6 +275,10 @@ branch on top of the <upstream> branch.  Because of this, when a merge
>  conflict happens, the side reported as 'ours' is the so-far rebased
>  series, starting with <upstream>, and 'theirs' is the working branch.  In
>  other words, the sides are swapped.
> ++
> +This uses the `--interactive` machinery internally, and as such,
> +anything that is incompatible with --interactive is incompatible
> +with this option.

I am not sure I like this change, as it describes an implementation detail
that users should neither know, nor care about, nor rely on.

> @@ -328,8 +332,8 @@ which makes little sense.
>  	and after each change.  When fewer lines of surrounding
>  	context exist they all must match.  By default no context is
>  	ever ignored.
> -	Incompatible with the --merge and --interactive options, or
> -	anything that implies those options or their machinery.
> +	Incompatible with the --interactive option, or anything that
> +	uses the `--interactive` machinery.
>  
>  -f::
>  --force-rebase::
> @@ -361,15 +365,15 @@ default is `--no-fork-point`, otherwise the default is `--fork-point`.
>  --whitespace=<option>::
>  	These flag are passed to the 'git apply' program
>  	(see linkgit:git-apply[1]) that applies the patch.
> -	Incompatible with the --merge and --interactive options, or
> -	anything that implies those options or their machinery.
> +	Incompatible with the --interactive option, or anything that
> +	uses the `--interactive` machinery.
>  
>  --committer-date-is-author-date::
>  --ignore-date::
>  	These flags are passed to 'git am' to easily change the dates
>  	of the rebased commits (see linkgit:git-am[1]).
> -	Incompatible with the --merge and --interactive options, or
> -	anything that implies those options or their machinery.
> +	Incompatible with the --interactive option, or anything that
> +	uses the `--interactive` machinery.
>  
>  --signoff::
>  	Add a Signed-off-by: trailer to all the rebased commits. Note

For above-mentioned reason, I'd suggest dropping these three hunks.

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 1f2401f702..dcc4a26a78 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -885,7 +885,10 @@ init_basic_state () {
>  	mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")"
>  	rm -f "$(git rev-parse --git-path REBASE_HEAD)"
>  
> -	: > "$state_dir"/interactive || die "$(gettext "Could not mark as interactive")"
> +	if test -n "$actually_interactive"
> +	then
> +		: > "$state_dir"/interactive || die "$(gettext "Could not mark as interactive")"
> +	fi

Do we really care at this stage? IOW what breaks if we still write that
file, even in --merge mode?

> diff --git a/git-rebase.sh b/git-rebase.sh
> index b639c0d4fe..5ac1dee30b 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -239,12 +239,10 @@ then
>  	state_dir="$apply_dir"
>  elif test -d "$merge_dir"
>  then
> +	type=interactive
>  	if test -f "$merge_dir"/interactive
>  	then
> -		type=interactive
>  		interactive_rebase=explicit
> -	else
> -		type=merge
>  	fi
>  	state_dir="$merge_dir"
>  fi

This makes me think even more that we don't care...

> @@ -481,13 +479,16 @@ then
>  	test -z "$interactive_rebase" && interactive_rebase=implied
>  fi
>  
> +actually_interactive=
>  if test -n "$interactive_rebase"
>  then
>  	type=interactive
> +	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
> @@ -501,17 +502,11 @@ fi
>  
>  if test -n "$git_am_opt"; then
>  	incompatible_opts=`echo "$git_am_opt" | sed -e 's/ -q//'`
> -	if test -n "$interactive_rebase"
> +	if test -n "$incompatible_opts"

Did you not mean to turn this into a test for actually_interactve ||
do_merge?

>  	then
> -		if test -n "$incompatible_opts"
> +		if test -n "$actually_interactive" || test "$do_merge"

This could now be combined with the previous if (and the `-n` could be
added to the latter test):

	if test -n "$actually_interactive" -o -n "$do_merge" &&
		test -n "$incompatible_opts"

The indentation would change, but this hunk is already confusing to read,
anyway, so...

>  		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)")"
> +			die "$(gettext "error: cannot combine am options ($incompatible_opts) with either interactive or merge options")"
>  		fi
>  	fi
>  fi
> @@ -660,7 +655,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 "$type" != interactive && 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
> @@ -704,6 +699,22 @@ then
>  	GIT_PAGER='' git diff --stat --summary "$mb" "$onto"
>  fi
>  
> +if test -z "$actually_interactive"
> +then
> +	# If the $onto is a proper descendant of the tip of the branch, then
> +	# we just fast-forwarded.
> +	if test "$mb" = "$orig_head"
> +	then

Likewise, this would be easier to read as

	if test -z "$actually_interactive" && test "$mb" = "$orig_head"

Also: how certain are we that "$mb" does not start with a dash? We may
have to use the `test a"$mb" = a"$orig_head"` trick here... But I guess
this is moved code, so if it is buggy, it was buggy before.

> +		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
> +fi
> +
>  test "$type" = interactive && run_specific_rebase
>  
>  # Detach HEAD and reset the tree
> @@ -713,16 +724,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/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

This might be called a regression... I don't know any user of `git rebase
-m`, but I guess if I was one, I would like to keep seeing those messages.

It *should* be relatively easy to tell the sequencer.c to issue these
messages, I think. But it would be more work.

> 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

Neat!

> 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

Nice!

>  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

This is a bit sad, because it seems to suggest that we now do more
unnecessary work here, or is my reading incorrect?

Thanks,
Dscho

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

* Re: [RFC PATCH 3/3] git-rebase.sh: make git-rebase--interactive the default
  2018-06-07 17:13   ` [RFC PATCH 3/3] git-rebase.sh: make git-rebase--interactive the default Elijah Newren
@ 2018-06-09 22:11     ` Johannes Schindelin
  2018-06-10  1:31       ` Elijah Newren
  0 siblings, 1 reply; 47+ messages in thread
From: Johannes Schindelin @ 2018-06-09 22:11 UTC (permalink / raw)
  To: Elijah Newren; +Cc: git, gitster, alban.gruin

Hi Elijah,

On Thu, 7 Jun 2018, Elijah Newren wrote:

> am-based rebases suffer from an reduced ability to detect directory
> renames upstream, which is fundamental to the fact that it throws away
> information about the history: in particular, it dispenses with the
> original commits involved by turning them into patches, and without the
> knowledge of the original commits we cannot determine a proper merge base.
> 
> The am-based rebase will proceed by first trying git-apply, and, only if
> it fails, will try to reconstruct a provisional merge base using
> build_fake_ancestor().  This fake ancestor will ONLY contain the files
> modified in the patch; without the full list of files in the real merge
> base, renames of any missing files cannot be detected.  Directory rename
> detection works by looking at individual file renames and deducing when a
> full directory has been renamed.
> 
> Trying to modify build_fake_ancestor() to instead create a merge base that
> includes common file information by looking for a commit that contained
> all the same blobs as the pre-image of the patch would require a very
> expensive search through history, may find the wrong commit, and outside
> of rebase may not find any commit that matches.
> 
> But we had all the relevant information to start.  So, instead of
> attempting to fix am which just doesn't have the relevant information,
> instead note its strength and weaknesses, and change the default rebase
> machinery to interactive since it does not suffer from the same problems.

I'll let Eric comment on the grammar, and I'll comment on the idea behind
this commit instead.

IMHO `git rebase -i` is still too slow to be a true replacement for `git
rebase --am` for the cases where it serves the user well. Maybe we should
work on making `rebase -i` faster, first?

I imagine, for example, that it might make *tons* of sense to avoid
writing out the index and worktree files all the time. That was necessary
in the shell script version because if the ridiculous limitations we
subjected ourselves to, such as: no object-oriented state worth
mentioning, only string-based processing, etc. But we could now start to
do everything in memory (*maybe* write out the new blob/tree/commit
objects immediately, but maybe not) until the time when we either have
succeeded in the rebase, or when there was a problem and we have to exit
with an error. And only then write the files and the index.

In any case, I think that the rather noticeable change of the default
would probably necessitate a deprecation phase.

Ciao,
Dscho

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

* Re: [RFC PATCH 1/3] git-rebase, sequencer: add a --quiet option for the interactive machinery
  2018-06-09 21:45     ` Johannes Schindelin
@ 2018-06-09 23:05       ` Elijah Newren
  0 siblings, 0 replies; 47+ messages in thread
From: Elijah Newren @ 2018-06-09 23:05 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Git Mailing List, Junio C Hamano, Alban Gruin, Pratik Karki

Hi Dscho,

On Sat, Jun 9, 2018 at 2:45 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Thu, 7 Jun 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.  Further, we want to make the interactive
>> machinery also take over for git-rebase--merge and become the default
>> merge strategy, so it makes sense for these other cases to have a 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. As long as it is coordinated with Alban and Pratik, as both
> of their GSoC projects are affected by this.

I had Alban cc'ed, and had looked briefly at the GSoC projects but
somehow missed that Pratik was also working in the area.  Adding him
to cc.

> In particular Pratik's project, I think, would actually *benefit* from
> your work, as it might even make it possible to turn all modes but
> --preserve-merges into pure builtin code, which would be awesome.

:-)

>> @@ -713,6 +717,7 @@ Commit or stash your changes, and then run
>>               "$hook" rebase < "$rewritten_list"
>>               true # we don't care if this hook failed
>>       fi &&
>> +             test -z "$GIT_QUIET" &&
>>               warn "$(eval_gettext "Successfully rebased and updated \$head_name.")"
>
> In general, I tried the statements to return success at all times. That
> means that
>
>                 test -n "$GIT_QUIET" ||
>
> would be better in this case.

Good point, I'll switch it over.

>> diff --git a/git-rebase.sh b/git-rebase.sh
>> index 7d1612b31b..b639c0d4fe 100755
>> --- a/git-rebase.sh
>> +++ b/git-rebase.sh
>> @@ -136,7 +136,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
>
> Maybe it would be better to `echo t` into that file? That way, scripts
> that used the value in that file would continue to work. (But maybe there
> were no scripts that could use it, as only the interactive rebase allows
> scripting, and it did not handle that flag before?)

Right, I don't think we had users before, and I'd rather make the code
a little more self-consistent.  In particular, since
$state_dir/verbose work based off the presence of the file rather than
the contents, I'd rather we just did the same for $state_dir/quiet.

However, there is one bug here; in order to make it like verbose, I
need to also make the following change:

diff --git a/git-rebase.sh b/git-rebase.sh
index c8c3d0d05a..8f0c7a4738 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -119,7 +119,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 &&


> The rest looks obviously good.


Thanks!
Elijah

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

* Re: [RFC PATCH 2/3] rebase: Implement --merge via git-rebase--interactive
  2018-06-09 22:04     ` Johannes Schindelin
@ 2018-06-10  1:14       ` Elijah Newren
  2018-06-11  9:54         ` Phillip Wood
  0 siblings, 1 reply; 47+ messages in thread
From: Elijah Newren @ 2018-06-10  1:14 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Git Mailing List, Junio C Hamano, Alban Gruin

Hi Dscho,

On Sat, Jun 9, 2018 at 3:04 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Thu, 7 Jun 2018, Elijah Newren wrote:
>
>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
>> index 451252c173..28d1658d7a 100644
>> --- a/Documentation/git-rebase.txt
>> +++ b/Documentation/git-rebase.txt
>> @@ -275,6 +275,10 @@ branch on top of the <upstream> branch.  Because of this, when a merge
>>  conflict happens, the side reported as 'ours' is the so-far rebased
>>  series, starting with <upstream>, and 'theirs' is the working branch.  In
>>  other words, the sides are swapped.
>> ++
>> +This uses the `--interactive` machinery internally, and as such,
>> +anything that is incompatible with --interactive is incompatible
>> +with this option.
>
> I am not sure I like this change, as it describes an implementation detail
> that users should neither know, nor care about, nor rely on.

Let me back up for just a second to see if I can point out the real
problem I'm trying to address here, which you may have a better
solution for.  What should happen if a user runs
   git rebase --merge --whitespace=fix
?

git currently silently ignores the --whitepsace=fix argument, leaving
the whitespace damage present at the end of the rebase.  Same goes for
--interactive combined with any am-specific options  (Fix proposed at
https://public-inbox.org/git/20180607050654.19663-2-newren@gmail.com/).
This silent ignoring of some options depending on which other options
were specified has caused me problems in the past.

So, while I totally agree with you that users shouldn't need to know
implementation details, they do need to know how to use commands and
which options go well together and which are mutually incompatible.
Do you have any suggestions on alternate wording that would convey the
sets of mutually incompatible options without talking about
implementation details?  Would you rather only address that in the
code and not mention it in the documentation?

See also https://public-inbox.org/git/20180607050654.19663-1-newren@gmail.com/,
which proposes testcases for these incompatibility sets.


>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index 1f2401f702..dcc4a26a78 100644
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -885,7 +885,10 @@ init_basic_state () {
>>       mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")"
>>       rm -f "$(git rev-parse --git-path REBASE_HEAD)"
>>
>> -     : > "$state_dir"/interactive || die "$(gettext "Could not mark as interactive")"
>> +     if test -n "$actually_interactive"
>> +     then
>> +             : > "$state_dir"/interactive || die "$(gettext "Could not mark as interactive")"
>> +     fi
>
> Do we really care at this stage? IOW what breaks if we still write that
> file, even in --merge mode?

Two things will change if we do that:
  * bash prompt will be different for those using git-prompt.sh
(REBASE-m vs. REBASE-i).
  * git-status output is no longer the same ('rebase in progress' vs.
'interactive rebase in progress...last command done:...pick 0dea123 my
wonderful commit')

I don't think the prompt is a big deal.  The status output might not
be either, but showing the "last command done" may be weird to someone
that never saw or edited a list of commands.  (Then again, that same
argument could be made for users of --exec, --rebase-merges, or
--keep-empty without an explicit --interactive)

Opinions on whether these two difference matter?  If others don't
think these differences are significant, I'm happy to update any
necessary testcases and just unconditionally create the
$state_dir/interactive file.


>> @@ -501,17 +502,11 @@ fi
>>
>>  if test -n "$git_am_opt"; then
>>       incompatible_opts=`echo "$git_am_opt" | sed -e 's/ -q//'`
>> -     if test -n "$interactive_rebase"
>> +     if test -n "$incompatible_opts"
>
> Did you not mean to turn this into a test for actually_interactve ||
> do_merge?
>
>>       then
>> -             if test -n "$incompatible_opts"
>> +             if test -n "$actually_interactive" || test "$do_merge"
>
> This could now be combined with the previous if (and the `-n` could be
> added to the latter test):
>
>         if test -n "$actually_interactive" -o -n "$do_merge" &&
>                 test -n "$incompatible_opts"
>
> The indentation would change, but this hunk is already confusing to read,
> anyway, so...

I'm happy to switch the order of the nesting as you suggest and agree
that it would make it easier to read.  However, I hesitate to combine
the two if-lines.  When I read your combined suggested line, I parsed
it as follows (using invalid pseduo-syntax just to convey grouping):

  ( -n "$actually_interactive ) || ( -n "$do_merge" && -n "$incompatible_opts" )

Granted, I parsed it wrong, putting the parentheses in the wrong
places, and bash parses it as you intended.  While you may have
precedence and left- vs. right- associativity rules down pat, I
clearly didn't.  If we combine the lines, I'll probably mis-read them
again when I see them in a year or more.

>> @@ -704,6 +699,22 @@ then
>>       GIT_PAGER='' git diff --stat --summary "$mb" "$onto"
>>  fi
>>
>> +if test -z "$actually_interactive"
>> +then
>> +     # If the $onto is a proper descendant of the tip of the branch, then
>> +     # we just fast-forwarded.
>> +     if test "$mb" = "$orig_head"
>> +     then
>
> Likewise, this would be easier to read as
>
>         if test -z "$actually_interactive" && test "$mb" = "$orig_head"

Good point, I'll fix that up.

> Also: how certain are we that "$mb" does not start with a dash? We may
> have to use the `test a"$mb" = a"$orig_head"` trick here... But I guess
> this is moved code, so if it is buggy, it was buggy before.

From earlier in the file,
mb=$(git merge-base ...)

So, unless we're expecting the output of git-merge-base to change in
the future to include leading dashes, we shouldn't hit any issues.  I
can make the change you suggest if you're worried, though.

>> 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
>
> This might be called a regression... I don't know any user of `git rebase
> -m`, but I guess if I was one, I would like to keep seeing those messages.
>
> It *should* be relatively easy to tell the sequencer.c to issue these
> messages, I think. But it would be more work.

You may well be right.  Here's where my thinking came from...

am-based, interactive-based, and merge-based rebases have lots of
little ways in which they have differed, this being just one of them.
It was sometimes hard making a judgement call when writing this series
about whether any given deviation was a difference I wanted to smooth
over or a difference I wanted to perpetuate between various flags.
Further, if it was a difference I wanted to smooth over, then I had to
decide which of the current behaviors was 'correct'.

In this particular case, I basically went off perceived usage.
am-based rebases have lots of special flags relevant to it only
(--whitespace, -C, etc.) and is the default, so it clearly has lots of
users.  interactive-based rebases are pretty prominent too, and have
very specific special capabilities the other modes don't.  In
contrast, merge-based rebases can't do a single thing that interactive
can't; and unless you're using a special merge strategy, for the last
10 years merge-based rebases haven't been able to do anything a normal
am-based rebase couldn't.  merge-based rebases were added in mid-2006
to handle renames, but am-based rebases gained that ability at the end
of 2008.  Basically, rebase -m was dormant and useless...until
directory rename detection became a thing this cycle.  (Also, in
config options and documentation merge tends to be overlooked; just a
single example is that pull.rebase can be set to interactive, but not
to merge.)

Anyway, with this in mind, I didn't think those extra messages were
all that important.  If others disagree I can look into adding them --
that'd also make the --quiet mode more useful for interactive, since
there'd be more messages for it to strip out.

>>  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
>
> This is a bit sad, because it seems to suggest that we now do more
> unnecessary work here, or is my reading incorrect?

I agree that it's a bit sad.  I spent a while looking at this testcase
and puzzling over what it meant, and my commit message pointed out
that I wasn't quite sure where it came from:

      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.

It would be nice to avoid the extra work, but I'm worried tackling
that might cause me to step on toes of folks doing the rewrite of
interactive rebases from shell to C.  Maybe I should just add a TODO
note in the testcase, similar to the one in t3425 near a few lines I
touched in this patch?


Thanks for the detailed feedback and suggestions!

Elijah

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

* Re: [RFC PATCH 3/3] git-rebase.sh: make git-rebase--interactive the default
  2018-06-09 22:11     ` Johannes Schindelin
@ 2018-06-10  1:31       ` Elijah Newren
  2018-06-17 21:44         ` Johannes Schindelin
  0 siblings, 1 reply; 47+ messages in thread
From: Elijah Newren @ 2018-06-10  1:31 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Git Mailing List, Junio C Hamano, Alban Gruin

Hi Dscho,

On Sat, Jun 9, 2018 at 3:11 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Thu, 7 Jun 2018, Elijah Newren wrote:
>
>> am-based rebases suffer from an reduced ability to detect directory
>> renames upstream, which is fundamental to the fact that it throws away
>> information about the history: in particular, it dispenses with the
>> original commits involved by turning them into patches, and without the
>> knowledge of the original commits we cannot determine a proper merge base.
>>
>> The am-based rebase will proceed by first trying git-apply, and, only if
>> it fails, will try to reconstruct a provisional merge base using
>> build_fake_ancestor().  This fake ancestor will ONLY contain the files
>> modified in the patch; without the full list of files in the real merge
>> base, renames of any missing files cannot be detected.  Directory rename
>> detection works by looking at individual file renames and deducing when a
>> full directory has been renamed.
>>
>> Trying to modify build_fake_ancestor() to instead create a merge base that
>> includes common file information by looking for a commit that contained
>> all the same blobs as the pre-image of the patch would require a very
>> expensive search through history, may find the wrong commit, and outside
>> of rebase may not find any commit that matches.
>>
>> But we had all the relevant information to start.  So, instead of
>> attempting to fix am which just doesn't have the relevant information,
>> instead note its strength and weaknesses, and change the default rebase
>> machinery to interactive since it does not suffer from the same problems.
>
> I'll let Eric comment on the grammar, and I'll comment on the idea behind
> this commit instead.

Going to dump the hard job on Eric, eh?  ;-)

> IMHO `git rebase -i` is still too slow to be a true replacement for `git
> rebase --am` for the cases where it serves the user well. Maybe we should
> work on making `rebase -i` faster, first?

That sounds fair.

> I imagine, for example, that it might make *tons* of sense to avoid
> writing out the index and worktree files all the time. That was necessary
> in the shell script version because if the ridiculous limitations we
> subjected ourselves to, such as: no object-oriented state worth
> mentioning, only string-based processing, etc. But we could now start to
> do everything in memory (*maybe* write out the new blob/tree/commit
> objects immediately, but maybe not) until the time when we either have
> succeeded in the rebase, or when there was a problem and we have to exit
> with an error. And only then write the files and the index.

Hmm...are you still planning on using cherry-pick (internally rather
than forking, of course)?  Because cherry-pick uses the
merge-recursive machinery, and the merge-recursive machinery doesn't
have a nice way of avoiding writing to the working tree or index.
Fixing that is on my radar; see the first block of
https://public-inbox.org/git/CABPp-BG2fZHm3s-yrzxyGj3Eh+O7_LHLz5pgstHhG2drigSyRA@mail.gmail.com/
(reading up until "At this point, I'd rather just fix the design flaw
rather than complicate the code further.")

However, also covered in my plans is a few things to speed up the
merge-recursive machinery, which should provide some other performance
benefits for interactive rebases.

> In any case, I think that the rather noticeable change of the default
> would probably necessitate a deprecation phase.

Why is it a "rather noticable change of the default"?  If we popped up
the editor and asked the user to edit the list of options, I'd agree,
or if folks thought that it was significantly slower by a big enough
margin (though you already suggested waiting and making sure we don't
do that).  What else remains that qualifies?

(Okay, the default behavior to just skip empty patches rather than
halt the rebase and ask the user to advise is different, but we could
fix that up too.  Is there anything else?)

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

* Re: [PATCH] git-rebase.sh: handle keep-empty like all other options
  2018-06-07  5:07 ` [PATCH] git-rebase.sh: handle keep-empty like all other options Elijah Newren
@ 2018-06-10 19:26   ` Phillip Wood
  2018-06-11 14:42     ` Elijah Newren
  0 siblings, 1 reply; 47+ messages in thread
From: Phillip Wood @ 2018-06-10 19:26 UTC (permalink / raw)
  To: Elijah Newren, git

Hi Elijah
On 07/06/18 06:07, Elijah Newren wrote:
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>   git-rebase.sh | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/git-rebase.sh b/git-rebase.sh
> index 40be59ecc4..a56b286372 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -276,6 +276,7 @@ do
>   		;;
>   	--keep-empty)
>   		keep_empty=yes
> +		test -z "$interactive_rebase" && interactive_rebase=implied

I think you need to wait until all the options have been parsed before 
setting the implied interactive rebase in case the user specifies has 
'--keep-empty' in an alias and specifies '--no-keep-empty' with some am 
options on the command line.

Best Wishes

Phillip
>   		;;
>   	--allow-empty-message)
>   		allow_empty_message=--allow-empty-message
> @@ -480,11 +481,6 @@ then
>   	test -z "$interactive_rebase" && interactive_rebase=implied
>   fi
>   
> -if test -n "$keep_empty"
> -then
> -	test -z "$interactive_rebase" && interactive_rebase=implied
> -fi
> -
>   if test -n "$interactive_rebase"
>   then
>   	type=interactive
> 


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

* Re: [PATCH 2/2] git-rebase: error out when incompatible options passed
  2018-06-07  5:06   ` [PATCH 2/2] git-rebase: error out " Elijah Newren
@ 2018-06-10 19:40     ` Phillip Wood
  2018-06-11 15:19       ` Elijah Newren
  2018-06-11 15:49       ` Elijah Newren
  0 siblings, 2 replies; 47+ messages in thread
From: Phillip Wood @ 2018-06-10 19:40 UTC (permalink / raw)
  To: Elijah Newren, git

On 07/06/18 06:06, Elijah Newren wrote:
> git rebase has three different types: am, merge, and interactive, all of
> which are implemented in terms of separate scripts.  am builds on git-am,
> merge builds on git-merge-recursive, and interactive builds on
> git-cherry-pick.  We make use of features in those lower-level commands in
> the different rebase types, but those features don't exist in all of the
> lower level commands so we have a range of incompatibilities.  Previously,
> we just accepted nearly any argument and silently ignored whichever ones
> weren't implemented for the type of rebase specified.  Change this so the
> incompatibilities are documented, included in the testsuite, and tested
> for at runtime with an appropriate error message shown.

I think this is a great improvement, it has always bothered me that 
rebase silently ignored the am options when they're given with 
interactive ones.
> 
> Some exceptions I left out:
> 
>    * --merge and --interactive are technically incompatible since they are
>      supposed to run different underlying scripts, but with a few small
>      changes, --interactive can do everything that --merge can.  In fact,
>      I'll shortly be sending another patch to remove git-rebase--merge and
>      reimplement it on top of git-rebase--interactive.

Excellent I've wondered about doing that but never got round to it. One 
thing I was slightly concerned about was that someone maybe parsing the 
output of git-rebase--merge and they'll get a nasty shock when that 
output changes as a result of using the sequencer.

> 
>    * One could argue that --interactive and --quiet are incompatible since
>      --interactive doesn't implement a --quiet mode (perhaps since
>      cherry-pick itself does not implement one).  However, the interactive
>      mode is more quiet than the other modes in general with progress
>      messages, so one could argue that it's already quiet.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>   Documentation/git-rebase.txt           | 15 +++++++++++++--
>   git-rebase.sh                          | 17 +++++++++++++++++
>   t/t3422-rebase-incompatible-options.sh | 10 +++++-----
>   3 files changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 0e20a66e73..451252c173 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -243,6 +243,10 @@ leave out at most one of A and B, in which case it defaults to HEAD.
>   --keep-empty::
>   	Keep the commits that do not change anything from its
>   	parents in the result.
> ++
> +This uses the `--interactive` machinery internally, and as such,
> +anything that is incompatible with --interactive is incompatible
> +with this option.
>   
>   --allow-empty-message::
>   	By default, rebasing commits with an empty message will fail.
> @@ -324,6 +328,8 @@ which makes little sense.
>   	and after each change.  When fewer lines of surrounding
>   	context exist they all must match.  By default no context is
>   	ever ignored.
> +	Incompatible with the --merge and --interactive options, or
> +	anything that implies those options or their machinery.

struct replay_opts has an allow_empty_message member so I'm not sure 
that's true.

>   -f::
>   --force-rebase::
> @@ -355,13 +361,15 @@ default is `--no-fork-point`, otherwise the default is `--fork-point`.
>   --whitespace=<option>::
>   	These flag are passed to the 'git apply' program
>   	(see linkgit:git-apply[1]) that applies the patch.
> -	Incompatible with the --interactive option.
> +	Incompatible with the --merge and --interactive options, or
> +	anything that implies those options or their machinery.

I wonder if it is better just to list the incompatible options it might 
be a bit long but it would be nicer for the user than them having to 
work out which options imply --interactive.

>   --committer-date-is-author-date::
>   --ignore-date::
>   	These flags are passed to 'git am' to easily change the dates
>   	of the rebased commits (see linkgit:git-am[1]).
> -	Incompatible with the --interactive option.
> +	Incompatible with the --merge and --interactive options, or
> +	anything that implies those options or their machinery.
>   
>   --signoff::
>   	Add a Signed-off-by: trailer to all the rebased commits. Note
> @@ -400,6 +408,9 @@ The `--rebase-merges` mode is similar in spirit to `--preserve-merges`, but
>   in contrast to that option works well in interactive rebases: commits can be
>   reordered, inserted and dropped at will.
>   +
> +This uses the `--interactive` machinery internally, but it can be run
> +without an explicit `--interactive`.
> ++

Without more context it's hard to judge but I'm not sure this adds 
anything useful

>   It is currently only possible to recreate the merge commits using the
>   `recursive` merge strategy; Different merge strategies can be used only via
>   explicit `exec git merge -s <strategy> [...]` commands.
> diff --git a/git-rebase.sh b/git-rebase.sh
> index 40be59ecc4..f1dbecba18 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -503,6 +503,23 @@ 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//'`

I think the style guide recommends $() over ``

> +	if test -n "$interactive_rebase"
> +	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
> +	fi
> +fi
> +
>   if test -n "$signoff"
>   then
>   	test -n "$preserve_merges" &&
> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
> index 04cdae921b..66a83363bf 100755
> --- a/t/t3422-rebase-incompatible-options.sh
> +++ b/t/t3422-rebase-incompatible-options.sh
> @@ -34,27 +34,27 @@ test_expect_success 'setup' '
>   test_run_rebase () {
>   	opt=$1
>   	shift
> -	test_expect_failure "$opt incompatible with --merge" "
> +	test_expect_success "$opt incompatible with --merge" "
>   		git checkout B^0 &&
>   		test_must_fail git rebase $opt --merge A
>   	"
>   
> -	test_expect_failure "$opt incompatible with --strategy=ours" "
> +	test_expect_success "$opt incompatible with --strategy=ours" "
>   		git checkout B^0 &&
>   		test_must_fail git rebase $opt --strategy=ours A
>   	"
>   
> -	test_expect_failure "$opt incompatible with --strategy-option=ours" "
> +	test_expect_success "$opt incompatible with --strategy-option=ours" "
>   		git checkout B^0 &&
>   		test_must_fail git rebase $opt --strategy=ours A
>   	"
>   
> -	test_expect_failure "$opt incompatible with --interactive" "
> +	test_expect_success "$opt incompatible with --interactive" "
>   		git checkout B^0 &&
>   		test_must_fail git rebase $opt --interactive A
>   	"
>   
> -	test_expect_failure "$opt incompatible with --exec" "
> +	test_expect_success "$opt incompatible with --exec" "
>   		git checkout B^0 &&
>   		test_must_fail git rebase $opt --exec 'true' A
>   	"
> 


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

* Re: [RFC PATCH 2/3] rebase: Implement --merge via git-rebase--interactive
  2018-06-10  1:14       ` Elijah Newren
@ 2018-06-11  9:54         ` Phillip Wood
  0 siblings, 0 replies; 47+ messages in thread
From: Phillip Wood @ 2018-06-11  9:54 UTC (permalink / raw)
  To: Elijah Newren, Johannes Schindelin
  Cc: Git Mailing List, Junio C Hamano, Alban Gruin

Hi Elijah

On 10/06/18 02:14, Elijah Newren wrote:
> 
> Hi Dscho,
> 
> On Sat, Jun 9, 2018 at 3:04 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>> On Thu, 7 Jun 2018, Elijah Newren wrote:
>>
>>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
>>> index 451252c173..28d1658d7a 100644
>>> --- a/Documentation/git-rebase.txt
>>> +++ b/Documentation/git-rebase.txt
>>> @@ -275,6 +275,10 @@ branch on top of the <upstream> branch.  Because of this, when a merge
>>>  conflict happens, the side reported as 'ours' is the so-far rebased
>>>  series, starting with <upstream>, and 'theirs' is the working branch.  In
>>>  other words, the sides are swapped.
>>> ++
>>> +This uses the `--interactive` machinery internally, and as such,
>>> +anything that is incompatible with --interactive is incompatible
>>> +with this option.
>>
>> I am not sure I like this change, as it describes an implementation detail
>> that users should neither know, nor care about, nor rely on.
> 
> Let me back up for just a second to see if I can point out the real
> problem I'm trying to address here, which you may have a better
> solution for.  What should happen if a user runs
>    git rebase --merge --whitespace=fix
> ?
> 
> git currently silently ignores the --whitepsace=fix argument, leaving
> the whitespace damage present at the end of the rebase.  Same goes for
> --interactive combined with any am-specific options  (Fix proposed at
> https://public-inbox.org/git/20180607050654.19663-2-newren@gmail.com/).
> This silent ignoring of some options depending on which other options
> were specified has caused me problems in the past.
> 
> So, while I totally agree with you that users shouldn't need to know
> implementation details, they do need to know how to use commands and
> which options go well together and which are mutually incompatible.
> Do you have any suggestions on alternate wording that would convey the
> sets of mutually incompatible options without talking about
> implementation details?  Would you rather only address that in the
> code and not mention it in the documentation?
> 
> See also https://public-inbox.org/git/20180607050654.19663-1-newren@gmail.com/,
> which proposes testcases for these incompatibility sets.
> 
> 
>>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>>> index 1f2401f702..dcc4a26a78 100644
>>> --- a/git-rebase--interactive.sh
>>> +++ b/git-rebase--interactive.sh
>>> @@ -885,7 +885,10 @@ init_basic_state () {
>>>       mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")"
>>>       rm -f "$(git rev-parse --git-path REBASE_HEAD)"
>>>
>>> -     : > "$state_dir"/interactive || die "$(gettext "Could not mark as interactive")"
>>> +     if test -n "$actually_interactive"
>>> +     then
>>> +             : > "$state_dir"/interactive || die "$(gettext "Could not mark as interactive")"
>>> +     fi
>>
>> Do we really care at this stage? IOW what breaks if we still write that
>> file, even in --merge mode?
> 
> Two things will change if we do that:
>   * bash prompt will be different for those using git-prompt.sh
> (REBASE-m vs. REBASE-i).
>   * git-status output is no longer the same ('rebase in progress' vs.
> 'interactive rebase in progress...last command done:...pick 0dea123 my
> wonderful commit')
> 
> I don't think the prompt is a big deal.  The status output might not
> be either, but showing the "last command done" may be weird to someone
> that never saw or edited a list of commands.  (Then again, that same
> argument could be made for users of --exec, --rebase-merges, or
> --keep-empty without an explicit --interactive)
> 
> Opinions on whether these two difference matter?  If others don't
> think these differences are significant, I'm happy to update any
> necessary testcases and just unconditionally create the
> $state_dir/interactive file.

I'm inclined to agree that it should just write the file
unconditionally. As you point out it already does this for other things
that aren't "interactive" as far as the user is concerned. Someone could
always add an file to indicate the "real" mode later that would work for
all the implicit interactive rebase cases if it was important to them.

Best Wishes

Phillip

> 
>>> @@ -501,17 +502,11 @@ fi
>>>
>>>  if test -n "$git_am_opt"; then
>>>       incompatible_opts=`echo "$git_am_opt" | sed -e 's/ -q//'`
>>> -     if test -n "$interactive_rebase"
>>> +     if test -n "$incompatible_opts"
>>
>> Did you not mean to turn this into a test for actually_interactve ||
>> do_merge?
>>
>>>       then
>>> -             if test -n "$incompatible_opts"
>>> +             if test -n "$actually_interactive" || test "$do_merge"
>>
>> This could now be combined with the previous if (and the `-n` could be
>> added to the latter test):
>>
>>         if test -n "$actually_interactive" -o -n "$do_merge" &&
>>                 test -n "$incompatible_opts"
>>
>> The indentation would change, but this hunk is already confusing to read,
>> anyway, so...
> 
> I'm happy to switch the order of the nesting as you suggest and agree
> that it would make it easier to read.  However, I hesitate to combine
> the two if-lines.  When I read your combined suggested line, I parsed
> it as follows (using invalid pseduo-syntax just to convey grouping):
> 
>   ( -n "$actually_interactive ) || ( -n "$do_merge" && -n "$incompatible_opts" )
> 
> Granted, I parsed it wrong, putting the parentheses in the wrong
> places, and bash parses it as you intended.  While you may have
> precedence and left- vs. right- associativity rules down pat, I
> clearly didn't.  If we combine the lines, I'll probably mis-read them
> again when I see them in a year or more.
> 
>>> @@ -704,6 +699,22 @@ then
>>>       GIT_PAGER='' git diff --stat --summary "$mb" "$onto"
>>>  fi
>>>
>>> +if test -z "$actually_interactive"
>>> +then
>>> +     # If the $onto is a proper descendant of the tip of the branch, then
>>> +     # we just fast-forwarded.
>>> +     if test "$mb" = "$orig_head"
>>> +     then
>>
>> Likewise, this would be easier to read as
>>
>>         if test -z "$actually_interactive" && test "$mb" = "$orig_head"
> 
> Good point, I'll fix that up.
> 
>> Also: how certain are we that "$mb" does not start with a dash? We may
>> have to use the `test a"$mb" = a"$orig_head"` trick here... But I guess
>> this is moved code, so if it is buggy, it was buggy before.
> 
> From earlier in the file,
> mb=$(git merge-base ...)
> 
> So, unless we're expecting the output of git-merge-base to change in
> the future to include leading dashes, we shouldn't hit any issues.  I
> can make the change you suggest if you're worried, though.
> 
>>> 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
>>
>> This might be called a regression... I don't know any user of `git rebase
>> -m`, but I guess if I was one, I would like to keep seeing those messages.
>>
>> It *should* be relatively easy to tell the sequencer.c to issue these
>> messages, I think. But it would be more work.
> 
> You may well be right.  Here's where my thinking came from...
> 
> am-based, interactive-based, and merge-based rebases have lots of
> little ways in which they have differed, this being just one of them.
> It was sometimes hard making a judgement call when writing this series
> about whether any given deviation was a difference I wanted to smooth
> over or a difference I wanted to perpetuate between various flags.
> Further, if it was a difference I wanted to smooth over, then I had to
> decide which of the current behaviors was 'correct'.
> 
> In this particular case, I basically went off perceived usage.
> am-based rebases have lots of special flags relevant to it only
> (--whitespace, -C, etc.) and is the default, so it clearly has lots of
> users.  interactive-based rebases are pretty prominent too, and have
> very specific special capabilities the other modes don't.  In
> contrast, merge-based rebases can't do a single thing that interactive
> can't; and unless you're using a special merge strategy, for the last
> 10 years merge-based rebases haven't been able to do anything a normal
> am-based rebase couldn't.  merge-based rebases were added in mid-2006
> to handle renames, but am-based rebases gained that ability at the end
> of 2008.  Basically, rebase -m was dormant and useless...until
> directory rename detection became a thing this cycle.  (Also, in
> config options and documentation merge tends to be overlooked; just a
> single example is that pull.rebase can be set to interactive, but not
> to merge.)
> 
> Anyway, with this in mind, I didn't think those extra messages were
> all that important.  If others disagree I can look into adding them --
> that'd also make the --quiet mode more useful for interactive, since
> there'd be more messages for it to strip out.
> 
>>>  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
>>
>> This is a bit sad, because it seems to suggest that we now do more
>> unnecessary work here, or is my reading incorrect?
> 
> I agree that it's a bit sad.  I spent a while looking at this testcase
> and puzzling over what it meant, and my commit message pointed out
> that I wasn't quite sure where it came from:
> 
>       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.
> 
> It would be nice to avoid the extra work, but I'm worried tackling
> that might cause me to step on toes of folks doing the rewrite of
> interactive rebases from shell to C.  Maybe I should just add a TODO
> note in the testcase, similar to the one in t3425 near a few lines I
> touched in this patch?
> 
> 
> Thanks for the detailed feedback and suggestions!
> 
> Elijah
> 


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

* Re: [PATCH] git-rebase.sh: handle keep-empty like all other options
  2018-06-10 19:26   ` Phillip Wood
@ 2018-06-11 14:42     ` Elijah Newren
  2018-06-11 15:16       ` Phillip Wood
  0 siblings, 1 reply; 47+ messages in thread
From: Elijah Newren @ 2018-06-11 14:42 UTC (permalink / raw)
  To: phillip.wood; +Cc: Git Mailing List

Hi Phillip,

On Sun, Jun 10, 2018 at 12:26 PM, Phillip Wood
<phillip.wood@talktalk.net> wrote:
> On 07/06/18 06:07, Elijah Newren wrote:
>>
>> Signed-off-by: Elijah Newren <newren@gmail.com>
>> ---
>>   git-rebase.sh | 6 +-----
>>   1 file changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/git-rebase.sh b/git-rebase.sh
>> index 40be59ecc4..a56b286372 100755
>> --- a/git-rebase.sh
>> +++ b/git-rebase.sh
>> @@ -276,6 +276,7 @@ do
>>                 ;;
>>         --keep-empty)
>>                 keep_empty=yes
>> +               test -z "$interactive_rebase" &&
>> interactive_rebase=implied
>
>
> I think you need to wait until all the options have been parsed before
> setting the implied interactive rebase in case the user specifies has
> '--keep-empty' in an alias and specifies '--no-keep-empty' with some am
> options on the command line.

Ah, indeed you are right.  Let's drop this patch then.

However, we have a bigger problem with empty commits, in that there
are really three modes rather than two:
  * Automatically drop empty commits (default for am-based rebase)
  * Automatically keep empty commits (as done with --keep-empty)
  * Halt the rebase and tell the user how to specify if they want to
keep it (default for interactive rebases)

Currently, only the first option is available to am-based rebases, and
only the second two options are available to interactive-based
rebases.  But if we want to make all three available to
interactive-based rebases, what should the command line option look
like?  --empty={drop,ask,keep}?

(And deprecate but continue to support --[no-]keep-empty?)

And should the two rebase modes really have a different default?  What
should the default be?

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

* Re: [PATCH] git-rebase.sh: handle keep-empty like all other options
  2018-06-11 14:42     ` Elijah Newren
@ 2018-06-11 15:16       ` Phillip Wood
  2018-06-11 16:08         ` Elijah Newren
  0 siblings, 1 reply; 47+ messages in thread
From: Phillip Wood @ 2018-06-11 15:16 UTC (permalink / raw)
  To: Elijah Newren, phillip.wood; +Cc: Git Mailing List

Hi Elijah

On 11/06/18 15:42, Elijah Newren wrote:
> Hi Phillip,
> 
> On Sun, Jun 10, 2018 at 12:26 PM, Phillip Wood
> <phillip.wood@talktalk.net> wrote:
>> On 07/06/18 06:07, Elijah Newren wrote:
>>>
>>> Signed-off-by: Elijah Newren <newren@gmail.com>
>>> ---
>>>    git-rebase.sh | 6 +-----
>>>    1 file changed, 1 insertion(+), 5 deletions(-)
>>>
>>> diff --git a/git-rebase.sh b/git-rebase.sh
>>> index 40be59ecc4..a56b286372 100755
>>> --- a/git-rebase.sh
>>> +++ b/git-rebase.sh
>>> @@ -276,6 +276,7 @@ do
>>>                  ;;
>>>          --keep-empty)
>>>                  keep_empty=yes
>>> +               test -z "$interactive_rebase" &&
>>> interactive_rebase=implied
>>
>>
>> I think you need to wait until all the options have been parsed before
>> setting the implied interactive rebase in case the user specifies has
>> '--keep-empty' in an alias and specifies '--no-keep-empty' with some am
>> options on the command line.
> 
> Ah, indeed you are right.  Let's drop this patch then.
> 
> However, we have a bigger problem with empty commits, in that there
> are really three modes rather than two:
>    * Automatically drop empty commits (default for am-based rebase)
>    * Automatically keep empty commits (as done with --keep-empty)
>    * Halt the rebase and tell the user how to specify if they want to
> keep it (default for interactive rebases)
> 
> Currently, only the first option is available to am-based rebases, and
> only the second two options are available to interactive-based
> rebases.  

I'm not sure that's the case, my understanding is that for an 
interactive rebase unless you give '--keep-empty' then any empty commits 
will be commented out in the todo list and therefore dropped unless 
they're uncommented when editing the list. The third case happens when a 
commit becomes empty when it's rebased (i.e. the original commit is not 
empty), I'm not sure what the am backend does for this. cherry-pick has 
'--keep-redundant-commits' for this case but that has never been added 
to rebase.

Best Wishes

Phillip

But if we want to make all three available to
> interactive-based rebases, what should the command line option look
> like?  --empty={drop,ask,keep}?
> 
> (And deprecate but continue to support --[no-]keep-empty?)
> 
> And should the two rebase modes really have a different default?  What
> should the default be?
> 


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

* Re: [PATCH 2/2] git-rebase: error out when incompatible options passed
  2018-06-10 19:40     ` Phillip Wood
@ 2018-06-11 15:19       ` Elijah Newren
  2018-06-11 15:59         ` Phillip Wood
  2018-06-11 15:49       ` Elijah Newren
  1 sibling, 1 reply; 47+ messages in thread
From: Elijah Newren @ 2018-06-11 15:19 UTC (permalink / raw)
  To: phillip.wood; +Cc: Git Mailing List

Hi Phillip

On Sun, Jun 10, 2018 at 12:40 PM, Phillip Wood
<phillip.wood@talktalk.net> wrote:

>>   Documentation/git-rebase.txt           | 15 +++++++++++++--
>>   git-rebase.sh                          | 17 +++++++++++++++++
>>   t/t3422-rebase-incompatible-options.sh | 10 +++++-----
>>   3 files changed, 35 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
>> index 0e20a66e73..451252c173 100644
>> --- a/Documentation/git-rebase.txt
>> +++ b/Documentation/git-rebase.txt
>> @@ -243,6 +243,10 @@ leave out at most one of A and B, in which case it
>> defaults to HEAD.
>>   --keep-empty::
>>         Keep the commits that do not change anything from its
>>         parents in the result.
>> ++
>> +This uses the `--interactive` machinery internally, and as such,
>> +anything that is incompatible with --interactive is incompatible
>> +with this option.
>>     --allow-empty-message::
>>         By default, rebasing commits with an empty message will fail.
>> @@ -324,6 +328,8 @@ which makes little sense.
>>         and after each change.  When fewer lines of surrounding
>>         context exist they all must match.  By default no context is
>>         ever ignored.
>> +       Incompatible with the --merge and --interactive options, or
>> +       anything that implies those options or their machinery.
>
>
> struct replay_opts has an allow_empty_message member so I'm not sure that's
> true.

I think you were confused by the way the patch broke up.  The jump to
line 328 means that this comment is about the -C option, not the
--allow-empty-message option.

However, I probably should add a comment next to the
--allow-empty-message option, to not the reverse is true, i.e. that
it's incompatible with am-based rebases.  (git-rebase--am.sh ignores
the allow_empty_message variable set in git-rebase.sh, unlike
git-rebase--interactive.sh and git-rebase--merge.sh)

>>   -f::
>>   --force-rebase::
>> @@ -355,13 +361,15 @@ default is `--no-fork-point`, otherwise the default
>> is `--fork-point`.
>>   --whitespace=<option>::
>>         These flag are passed to the 'git apply' program
>>         (see linkgit:git-apply[1]) that applies the patch.
>> -       Incompatible with the --interactive option.
>> +       Incompatible with the --merge and --interactive options, or
>> +       anything that implies those options or their machinery.
>
>
> I wonder if it is better just to list the incompatible options it might be a
> bit long but it would be nicer for the user than them having to work out
> which options imply --interactive.

That could work.  Would this be done at the end of the 'OPTIONS'
section of the manpage?  Should I create an 'INCOMPATIBLE OPTIONS'
section that follows the 'OPTIONS' section?

>>   --committer-date-is-author-date::
>>   --ignore-date::
>>         These flags are passed to 'git am' to easily change the dates
>>         of the rebased commits (see linkgit:git-am[1]).
>> -       Incompatible with the --interactive option.
>> +       Incompatible with the --merge and --interactive options, or
>> +       anything that implies those options or their machinery.
>>     --signoff::
>>         Add a Signed-off-by: trailer to all the rebased commits. Note
>> @@ -400,6 +408,9 @@ The `--rebase-merges` mode is similar in spirit to
>> `--preserve-merges`, but
>>   in contrast to that option works well in interactive rebases: commits
>> can be
>>   reordered, inserted and dropped at will.
>>   +
>> +This uses the `--interactive` machinery internally, but it can be run
>> +without an explicit `--interactive`.
>> ++
>
> Without more context it's hard to judge but I'm not sure this adds anything
> useful

Hmm, yeah.  I noted that --exec had similar wording, noted that
--preserve-merges had something along the same lines but as a warning,
and didn't see the similar wording for --rebase-merges -- I somehow
missed the paragraph right above where I added these lines.  Oops.
Anyway, I'll pull it out.

>>   It is currently only possible to recreate the merge commits using the
>>   `recursive` merge strategy; Different merge strategies can be used only
>> via
>>   explicit `exec git merge -s <strategy> [...]` commands.
>> diff --git a/git-rebase.sh b/git-rebase.sh
>> index 40be59ecc4..f1dbecba18 100755
>> --- a/git-rebase.sh
>> +++ b/git-rebase.sh
>> @@ -503,6 +503,23 @@ 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//'`
>
>
> I think the style guide recommends $() over ``

Will fix.


Thanks for taking a look!

Elijah

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

* Re: [PATCH 2/2] git-rebase: error out when incompatible options passed
  2018-06-10 19:40     ` Phillip Wood
  2018-06-11 15:19       ` Elijah Newren
@ 2018-06-11 15:49       ` Elijah Newren
  2018-06-12  9:45         ` Phillip Wood
  1 sibling, 1 reply; 47+ messages in thread
From: Elijah Newren @ 2018-06-11 15:49 UTC (permalink / raw)
  To: phillip.wood; +Cc: Git Mailing List

Another thing I missed...

On Sun, Jun 10, 2018 at 12:40 PM, Phillip Wood
<phillip.wood@talktalk.net> wrote:
> On 07/06/18 06:06, Elijah Newren wrote:

>> Some exceptions I left out:
>>
>>    * --merge and --interactive are technically incompatible since they are
>>      supposed to run different underlying scripts, but with a few small
>>      changes, --interactive can do everything that --merge can.  In fact,
>>      I'll shortly be sending another patch to remove git-rebase--merge and
>>      reimplement it on top of git-rebase--interactive.
>
> Excellent I've wondered about doing that but never got round to it. One
> thing I was slightly concerned about was that someone maybe parsing the
> output of git-rebase--merge and they'll get a nasty shock when that output
> changes as a result of using the sequencer.

I can see the minor worry, but I think upon inspection it's not
something that concerns me, for a few reasons:

In terms of use, given that rebase --merge was introduced to handle
renames in mid-2006, but plain rebase has been able to handle them
since late 2008 (though directory renames changes that again), the
utility of rebase --merge has been somewhat limited.  I think that
limits the exposure.  But to address the 'break' more directly...

If we were to agree that we needed to support folks parsing rebase
output, that would be a really strict requirement that I think would
prevent lots of fixes.  And if so, it's one we've violated a number of
times.  For example, I certainly wasn't thinking about rebase when I
modified messages in merge-recursive.c over the years, but they'd leak
through for rebase --merge.  (Those messages would not leak through to
rebase --interactive as much, since the sequencer sets o.buffer_output
and then only conditionally shows the output.)  Also, changes that
have occurred in the past, like adding 'git gc --auto' to rebase,
modifying error messages directly found in git-rebase--merge.sh would
have been considered breaks.

Finally, looking over all the differences in output while fixing up
testcases makes me think we've done much less around designing the
output based on what we want the user to see, and more around what
minimal fixups can we do to these lower level commands that provide
useful functionality to the user?  We linearize history differently
for different rebase modes, have different entries in the reflog
depending on which mode, and we often times implement features for
just one mode and then sometimes add it to others.  In fact, I think
the primary reason that am-based and merge-based rebases had a --quiet
option and the interactive rebases didn't, is mostly attributable to
the defaults of the lower level commands these three were built on top
of (git-am vs. git-merge-recursive vs. git-cherry-pick).  The noiser
two merited a quiet option, and the option was never added for the
last one.

Anyway, that's my rationale.  I'm curious to hear if folks disagree or
see things I'm overlooking or have reasons I might be weighting
tradeoffs less than optimally.

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

* Re: [PATCH 2/2] git-rebase: error out when incompatible options passed
  2018-06-11 15:19       ` Elijah Newren
@ 2018-06-11 15:59         ` Phillip Wood
  0 siblings, 0 replies; 47+ messages in thread
From: Phillip Wood @ 2018-06-11 15:59 UTC (permalink / raw)
  To: Elijah Newren, phillip.wood; +Cc: Git Mailing List

On 11/06/18 16:19, Elijah Newren wrote:
> Hi Phillip
> 
> On Sun, Jun 10, 2018 at 12:40 PM, Phillip Wood
> <phillip.wood@talktalk.net> wrote:
> 
>>>    Documentation/git-rebase.txt           | 15 +++++++++++++--
>>>    git-rebase.sh                          | 17 +++++++++++++++++
>>>    t/t3422-rebase-incompatible-options.sh | 10 +++++-----
>>>    3 files changed, 35 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
>>> index 0e20a66e73..451252c173 100644
>>> --- a/Documentation/git-rebase.txt
>>> +++ b/Documentation/git-rebase.txt
>>> @@ -243,6 +243,10 @@ leave out at most one of A and B, in which case it
>>> defaults to HEAD.
>>>    --keep-empty::
>>>          Keep the commits that do not change anything from its
>>>          parents in the result.
>>> ++
>>> +This uses the `--interactive` machinery internally, and as such,
>>> +anything that is incompatible with --interactive is incompatible
>>> +with this option.
>>>      --allow-empty-message::
>>>          By default, rebasing commits with an empty message will fail.
>>> @@ -324,6 +328,8 @@ which makes little sense.
>>>          and after each change.  When fewer lines of surrounding
>>>          context exist they all must match.  By default no context is
>>>          ever ignored.
>>> +       Incompatible with the --merge and --interactive options, or
>>> +       anything that implies those options or their machinery.
>>
>>
>> struct replay_opts has an allow_empty_message member so I'm not sure that's
>> true.
> 
> I think you were confused by the way the patch broke up.  The jump to
> line 328 means that this comment is about the -C option, not the
> --allow-empty-message option.

Ah you're right, I missed the hunk header

> However, I probably should add a comment next to the
> --allow-empty-message option, to not the reverse is true, i.e. that
> it's incompatible with am-based rebases.  (git-rebase--am.sh ignores
> the allow_empty_message variable set in git-rebase.sh, unlike
> git-rebase--interactive.sh and git-rebase--merge.sh)

That sounds like a good idea

>>>    -f::
>>>    --force-rebase::
>>> @@ -355,13 +361,15 @@ default is `--no-fork-point`, otherwise the default
>>> is `--fork-point`.
>>>    --whitespace=<option>::
>>>          These flag are passed to the 'git apply' program
>>>          (see linkgit:git-apply[1]) that applies the patch.
>>> -       Incompatible with the --interactive option.
>>> +       Incompatible with the --merge and --interactive options, or
>>> +       anything that implies those options or their machinery.
>>
>>
>> I wonder if it is better just to list the incompatible options it might be a
>> bit long but it would be nicer for the user than them having to work out
>> which options imply --interactive.
> 
> That could work.  Would this be done at the end of the 'OPTIONS'
> section of the manpage?  Should I create an 'INCOMPATIBLE OPTIONS'
> section that follows the 'OPTIONS' section?

I think that would be the best way of doing it, maybe with a note in the 
description of the am options to check in that section to see what they 
can be safely combined with.

>>>    --committer-date-is-author-date::
>>>    --ignore-date::
>>>          These flags are passed to 'git am' to easily change the dates
>>>          of the rebased commits (see linkgit:git-am[1]).
>>> -       Incompatible with the --interactive option.
>>> +       Incompatible with the --merge and --interactive options, or
>>> +       anything that implies those options or their machinery.
>>>      --signoff::
>>>          Add a Signed-off-by: trailer to all the rebased commits. Note
>>> @@ -400,6 +408,9 @@ The `--rebase-merges` mode is similar in spirit to
>>> `--preserve-merges`, but
>>>    in contrast to that option works well in interactive rebases: commits
>>> can be
>>>    reordered, inserted and dropped at will.
>>>    +
>>> +This uses the `--interactive` machinery internally, but it can be run
>>> +without an explicit `--interactive`.
>>> ++
>>
>> Without more context it's hard to judge but I'm not sure this adds anything
>> useful
> 
> Hmm, yeah.  I noted that --exec had similar wording, noted that
> --preserve-merges had something along the same lines but as a warning,
> and didn't see the similar wording for --rebase-merges -- I somehow
> missed the paragraph right above where I added these lines.  Oops.
> Anyway, I'll pull it out.

If we can get a good description of which options are compatible with 
what then hopefully we can remove the existing references to implicit 
interactive and am, the user should only have to worry about which 
options are compatible.

>>>    It is currently only possible to recreate the merge commits using the
>>>    `recursive` merge strategy; Different merge strategies can be used only
>>> via
>>>    explicit `exec git merge -s <strategy> [...]` commands.
>>> diff --git a/git-rebase.sh b/git-rebase.sh
>>> index 40be59ecc4..f1dbecba18 100755
>>> --- a/git-rebase.sh
>>> +++ b/git-rebase.sh
>>> @@ -503,6 +503,23 @@ 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//'`
>>
>>
>> I think the style guide recommends $() over ``
> 
> Will fix.
> 
> 
> Thanks for taking a look!

Thanks for working on this, it should make things simpler for user's to 
understand

Best Wishes

Phillip
> Elijah
> 


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

* Re: [PATCH] git-rebase.sh: handle keep-empty like all other options
  2018-06-11 15:16       ` Phillip Wood
@ 2018-06-11 16:08         ` Elijah Newren
  2018-06-12  9:53           ` Phillip Wood
  0 siblings, 1 reply; 47+ messages in thread
From: Elijah Newren @ 2018-06-11 16:08 UTC (permalink / raw)
  To: phillip.wood; +Cc: Git Mailing List

Hi Phillip,

On Mon, Jun 11, 2018 at 8:16 AM, Phillip Wood <phillip.wood@talktalk.net> wrote:
> On 11/06/18 15:42, Elijah Newren wrote:

>> However, we have a bigger problem with empty commits, in that there
>> are really three modes rather than two:
>>    * Automatically drop empty commits (default for am-based rebase)
>>    * Automatically keep empty commits (as done with --keep-empty)
>>    * Halt the rebase and tell the user how to specify if they want to
>> keep it (default for interactive rebases)
>>
>> Currently, only the first option is available to am-based rebases, and
>> only the second two options are available to interactive-based
>> rebases.
>
>
> I'm not sure that's the case, my understanding is that for an interactive
> rebase unless you give '--keep-empty' then any empty commits will be
> commented out in the todo list and therefore dropped unless they're
> uncommented when editing the list.

Ah, yes, you are right; I was implicitly thinking about cases where
the commit became empty rather than when the commit started
empty...and mis-read --keep-empty (which as I learned now is only for
started-empty cases).

> The third case happens when a commit
> becomes empty when it's rebased (i.e. the original commit is not empty), I'm
> not sure what the am backend does for this.

The am backend does not halt the rebase; it simply drops the commit
and says "No changes -- Patch already applied."

It has always seemed jarring and confusing to me that one rebase mode
stops and asks the user what to do and the other assumes.  I think
both should have the same default (and have options to pick the
alternate behavior).

I'm less certain what the default should be, though.

> cherry-pick has
> '--keep-redundant-commits' for this case but that has never been added to
> rebase.

Thanks for the pointer.

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

* Re: RFC: rebase inconsistency in 2.18 -- course of action?
  2018-06-07  4:58 RFC: rebase inconsistency in 2.18 -- course of action? Elijah Newren
                   ` (7 preceding siblings ...)
  2018-06-07 17:13 ` [RFC PATCH 0/3] Send more rebases through interactive machinery Elijah Newren
@ 2018-06-11 17:44 ` Junio C Hamano
  2018-06-11 20:17   ` Elijah Newren
  8 siblings, 1 reply; 47+ messages in thread
From: Junio C Hamano @ 2018-06-11 17:44 UTC (permalink / raw)
  To: Elijah Newren
  Cc: Git Mailing List, Stefan Beller, Johannes Schindelin, Alban Gruin

Elijah Newren <newren@gmail.com> writes:

> In short, while interactive-based and merge-based rebases handle
> directory rename detection fine, am-based rebases do not.  This
> inconsistency may frustrate or surprise users.

FWIW, I actually do not quite think it is universally a good idea to
infer that I would have added the path X/F I added to my branch at
Y/F instead if I were on vacation while somebody else first moved
other paths X/A, X/B, etc. to Y/A, Y/B, etc., and I would even
imagine that I would be frustrated if my X/F, which the somebody
else wasn't even aware of, were moved to Y/F without even telling
me.  So in that sense, doing such extra and unasked-for "moves"
during a rebase may be a bug, not a feature.

In any case, I think I'll have to delay -rc2 by a few days to catch
up, so I won't be looking at any topic (including this one) that is
not about 2.18-rc regressions for a few days.  Please do not get
upset if people with RFC patches do not hear from me until the
final.

Thanks.

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

* Re: RFC: rebase inconsistency in 2.18 -- course of action?
  2018-06-11 17:44 ` RFC: rebase inconsistency in 2.18 -- course of action? Junio C Hamano
@ 2018-06-11 20:17   ` Elijah Newren
  0 siblings, 0 replies; 47+ messages in thread
From: Elijah Newren @ 2018-06-11 20:17 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Git Mailing List, Stefan Beller, Johannes Schindelin, Alban Gruin

Hi Junio,

On Mon, Jun 11, 2018 at 10:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Elijah Newren <newren@gmail.com> writes:
>
>> In short, while interactive-based and merge-based rebases handle
>> directory rename detection fine, am-based rebases do not.  This
>> inconsistency may frustrate or surprise users.
>
> FWIW, I actually do not quite think it is universally a good idea to
> infer that I would have added the path X/F I added to my branch at
> Y/F instead if I were on vacation while somebody else first moved
> other paths X/A, X/B, etc. to Y/A, Y/B, etc., and I would even
> imagine that I would be frustrated if my X/F, which the somebody
> else wasn't even aware of, were moved to Y/F without even telling
> me.  So in that sense, doing such extra and unasked-for "moves"
> during a rebase may be a bug, not a feature.

So...I'm a little confused.  Am I correct to understand that you're
arguing that directory rename detection is an anti-feature and it
shouldn't have been merged in the first place?  Or that it doesn't
give appropriate output/warning, and it should have configuration
flags?  Or are you trying to make a distinction between am-based
rebases vs.any one of rebase -i, rebase -m, cherry-pick, and merge [-s
recursive]?

I was leaning towards "this inconsistency is no big deal; we can
address it later", but now you have me wondering about a different
angle.

> In any case, I think I'll have to delay -rc2 by a few days to catch
> up, so I won't be looking at any topic (including this one) that is
> not about 2.18-rc regressions for a few days.  Please do not get
> upset if people with RFC patches do not hear from me until the
> final.

I emailed about this rebase inconsistency because I was worried that
*an* inconsistency between the various rebase types was bad.  But the
more I've dug, the more I've found they are inconsistent in lots of
ways anyway (levels and types of output, reflog entries, automatically
dropping became-empty patches vs. halting and asking the user,
accepting commits with empty commit messages vs. requiring an
--allow-empty-message flag to do so, large sets of mutually
incompatible options, etc.)  Adding one more inconsistency is feeling
less worrisome to me.  I'd still like to lessen all these
inconsistencies between rebase types, but my current opinion is that
it's not that big a deal and my patches can all wait for the 2.19
cycle to start.

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

* Re: [PATCH 2/2] git-rebase: error out when incompatible options passed
  2018-06-11 15:49       ` Elijah Newren
@ 2018-06-12  9:45         ` Phillip Wood
  0 siblings, 0 replies; 47+ messages in thread
From: Phillip Wood @ 2018-06-12  9:45 UTC (permalink / raw)
  To: Elijah Newren, phillip.wood; +Cc: Git Mailing List

On 11/06/18 16:49, Elijah Newren wrote:
> Another thing I missed...
> 
> On Sun, Jun 10, 2018 at 12:40 PM, Phillip Wood
> <phillip.wood@talktalk.net> wrote:
>> On 07/06/18 06:06, Elijah Newren wrote:
> 
>>> Some exceptions I left out:
>>>
>>>    * --merge and --interactive are technically incompatible since they are
>>>      supposed to run different underlying scripts, but with a few small
>>>      changes, --interactive can do everything that --merge can.  In fact,
>>>      I'll shortly be sending another patch to remove git-rebase--merge and
>>>      reimplement it on top of git-rebase--interactive.
>>
>> Excellent I've wondered about doing that but never got round to it. One
>> thing I was slightly concerned about was that someone maybe parsing the
>> output of git-rebase--merge and they'll get a nasty shock when that output
>> changes as a result of using the sequencer.
> 
> I can see the minor worry, but I think upon inspection it's not
> something that concerns me, for a few reasons:
> 
> In terms of use, given that rebase --merge was introduced to handle
> renames in mid-2006, but plain rebase has been able to handle them
> since late 2008 (though directory renames changes that again), the
> utility of rebase --merge has been somewhat limited.  I think that
> limits the exposure.  But to address the 'break' more directly...
> 
> If we were to agree that we needed to support folks parsing rebase
> output, that would be a really strict requirement that I think would
> prevent lots of fixes.  And if so, it's one we've violated a number of
> times.  For example, I certainly wasn't thinking about rebase when I
> modified messages in merge-recursive.c over the years, but they'd leak
> through for rebase --merge.  (Those messages would not leak through to
> rebase --interactive as much, since the sequencer sets o.buffer_output
> and then only conditionally shows the output.)  Also, changes that
> have occurred in the past, like adding 'git gc --auto' to rebase,
> modifying error messages directly found in git-rebase--merge.sh would
> have been considered breaks.
> 
> Finally, looking over all the differences in output while fixing up
> testcases makes me think we've done much less around designing the
> output based on what we want the user to see, and more around what
> minimal fixups can we do to these lower level commands that provide
> useful functionality to the user?  We linearize history differently
> for different rebase modes, have different entries in the reflog
> depending on which mode, and we often times implement features for
> just one mode and then sometimes add it to others.  In fact, I think
> the primary reason that am-based and merge-based rebases had a --quiet
> option and the interactive rebases didn't, is mostly attributable to
> the defaults of the lower level commands these three were built on top
> of (git-am vs. git-merge-recursive vs. git-cherry-pick).  The noiser
> two merited a quiet option, and the option was never added for the
> last one.
> 
> Anyway, that's my rationale.  I'm curious to hear if folks disagree or
> see things I'm overlooking or have reasons I might be weighting
> tradeoffs less than optimally.
> 

I agree that there are already plenty of inconsistencies, (it's great to
see you reducing them). If we can avoid emulating the ouput of
git-rebase--merge.sh in sequencer.c that would definitely be my
preferred option (the code is already a bit hard to follow in places
where there it's doing slightly different things for cherry-pick and
rebase -i). Hopefully no one is relying on the output, as you say it's
just whatever the underlying plumbing prints rather than designed for a
specific purpose.

Best Wishes

Phillip

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

* Re: [PATCH] git-rebase.sh: handle keep-empty like all other options
  2018-06-11 16:08         ` Elijah Newren
@ 2018-06-12  9:53           ` Phillip Wood
  0 siblings, 0 replies; 47+ messages in thread
From: Phillip Wood @ 2018-06-12  9:53 UTC (permalink / raw)
  To: Elijah Newren, phillip.wood; +Cc: Git Mailing List

On 11/06/18 17:08, Elijah Newren wrote:
> Hi Phillip,
> 
> On Mon, Jun 11, 2018 at 8:16 AM, Phillip Wood <phillip.wood@talktalk.net> wrote:
>> On 11/06/18 15:42, Elijah Newren wrote:
> 
>>> However, we have a bigger problem with empty commits, in that there
>>> are really three modes rather than two:
>>>    * Automatically drop empty commits (default for am-based rebase)
>>>    * Automatically keep empty commits (as done with --keep-empty)
>>>    * Halt the rebase and tell the user how to specify if they want to
>>> keep it (default for interactive rebases)
>>>
>>> Currently, only the first option is available to am-based rebases, and
>>> only the second two options are available to interactive-based
>>> rebases.
>>
>>
>> I'm not sure that's the case, my understanding is that for an interactive
>> rebase unless you give '--keep-empty' then any empty commits will be
>> commented out in the todo list and therefore dropped unless they're
>> uncommented when editing the list.
> 
> Ah, yes, you are right; I was implicitly thinking about cases where
> the commit became empty rather than when the commit started
> empty...and mis-read --keep-empty (which as I learned now is only for
> started-empty cases).
> 
>> The third case happens when a commit
>> becomes empty when it's rebased (i.e. the original commit is not empty), I'm
>> not sure what the am backend does for this.
> 
> The am backend does not halt the rebase; it simply drops the commit
> and says "No changes -- Patch already applied."
> 
> It has always seemed jarring and confusing to me that one rebase mode
> stops and asks the user what to do and the other assumes.  I think
> both should have the same default (and have options to pick the
> alternate behavior).
> 
> I'm less certain what the default should be, though.

I'm not really sure either, on the one hand most of the time it is
convenient for rebase just to skip over it, on the other if you have
some important information in the commit message or a note then maybe
you want rebase to stop so you can selvage that information.

> 
>> cherry-pick has
>> '--keep-redundant-commits' for this case but that has never been added to
>> rebase.

On path forwards is to always stop, and implement
--keep-redundant-commits for rebase. That would be very easy for
interactive rebases as it shares the same code as cherry-pick. I've just
had a quick look at builtin/am.c and I think it would be fairly
straightforward to add some code to check if it's rebasing and stop if
the patch is already applied.

Best Wishes

Phillip

> Thanks for the pointer.
> 


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

* [RFC PATCH v2 0/7] Document/fix/warn about rebase incompatibilities and inconsistences
  2018-06-07  5:06 ` [PATCH 1/2] t3422: new testcases for checking when incompatible options passed Elijah Newren
  2018-06-07  5:06   ` [PATCH 2/2] git-rebase: error out " Elijah Newren
@ 2018-06-17  5:58   ` Elijah Newren
  2018-06-17  5:58     ` [RFC PATCH v2 1/7] git-rebase.txt: document incompatible options Elijah Newren
                       ` (7 more replies)
  1 sibling, 8 replies; 47+ messages in thread
From: Elijah Newren @ 2018-06-17  5:58 UTC (permalink / raw)
  To: git, phillip.wood; +Cc: johannes.schindelin, gitster, Elijah Newren

git-rebase has lots of options that are mutually incompatible.  Even among
aspects of its behavior that is common to all rebase types, it has a number
of inconsistencies.  This series tries to document, fix, and/or warn users
about many of these.

I have a much higher than average expectation that folks will object
to some of these patches.  I've tried to divide them up so that any parts
we decide to drop or redo can be more easily excised.

No branch-diff; because it's a significant re-work; instead I'll comment
briefly on the individual patches...


Elijah Newren (7):
  git-rebase.txt: document incompatible options

Both Dscho (on a related patch series) and Phillip suggested changing the
documentation to avoid implementational details.  I instead made a separate
section with sets of incompatible options...but it still mentions the
different backends while doing so.  Does that seem alright?

  git-rebase.sh: update help messages a bit

Minor tweaks to `git rebase -h` output.

  t3422: new testcases for checking when incompatible options passed

The one unmodified patch from the first round.

  git-rebase: error out when incompatible options passed

Almost the same as the first round, except:
  * Documentation pulled into a separate patch (patch 1)
  * $() instead of ``

  git-rebase.txt: document behavioral inconsistencies between modes

Add another section to the documentation for aspects that ideally
should be common between all modes but are handled differently.

  git-rebase.txt: address confusion between --no-ff vs --force-rebase

This came up on the list not that long ago; fix the documentation.

  git-rebase: make --allow-empty-message the default

Address the easiest of the inconsistencies, assuming the am-based backend
has the correct default and the merge-based and interactive-based backends
are the ones that need to change.

 Documentation/git-rebase.txt           | 154 ++++++++++++++++++++-----
 git-rebase.sh                          |  25 +++-
 t/t3404-rebase-interactive.sh          |   7 +-
 t/t3405-rebase-malformed.sh            |  11 +-
 t/t3422-rebase-incompatible-options.sh |  69 +++++++++++
 5 files changed, 224 insertions(+), 42 deletions(-)
 create mode 100755 t/t3422-rebase-incompatible-options.sh

-- 
2.18.0.rc2.1.g5453d3f70b.dirty

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

* [RFC PATCH v2 1/7] git-rebase.txt: document incompatible options
  2018-06-17  5:58   ` [RFC PATCH v2 0/7] Document/fix/warn about rebase incompatibilities and inconsistences Elijah Newren
@ 2018-06-17  5:58     ` Elijah Newren
  2018-06-17 15:38       ` Phillip Wood
  2018-06-17 17:15       ` Eric Sunshine
  2018-06-17  5:58     ` [RFC PATCH v2 2/7] git-rebase.sh: update help messages a bit Elijah Newren
                       ` (6 subsequent siblings)
  7 siblings, 2 replies; 47+ messages in thread
From: Elijah Newren @ 2018-06-17  5:58 UTC (permalink / raw)
  To: git, phillip.wood; +Cc: johannes.schindelin, gitster, Elijah Newren

git rebase has many options that only work with one of its three backends.
It also has a few other pairs of incompatible options.  Document these.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt | 84 ++++++++++++++++++++++++++++++++----
 1 file changed, 76 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 0e20a66e73..adccc15284 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -243,11 +243,15 @@ leave out at most one of A and B, in which case it defaults to HEAD.
 --keep-empty::
 	Keep the commits that do not change anything from its
 	parents in the result.
++
+See also INCOMPATIBLE OPTIONS below.
 
 --allow-empty-message::
 	By default, rebasing commits with an empty message will fail.
 	This option overrides that behavior, allowing commits with empty
 	messages to be rebased.
++
+See also INCOMPATIBLE OPTIONS below.
 
 --skip::
 	Restart the rebasing process by skipping the current patch.
@@ -271,6 +275,8 @@ branch on top of the <upstream> branch.  Because of this, when a merge
 conflict happens, the side reported as 'ours' is the so-far rebased
 series, starting with <upstream>, and 'theirs' is the working branch.  In
 other words, the sides are swapped.
++
+See also INCOMPATIBLE OPTIONS below.
 
 -s <strategy>::
 --strategy=<strategy>::
@@ -280,8 +286,10 @@ other words, the sides are swapped.
 +
 Because 'git rebase' replays each commit from the working branch
 on top of the <upstream> branch using the given strategy, using
-the 'ours' strategy simply discards all patches from the <branch>,
+the 'ours' strategy simply empties all patches from the <branch>,
 which makes little sense.
++
+See also INCOMPATIBLE OPTIONS below.
 
 -X <strategy-option>::
 --strategy-option=<strategy-option>::
@@ -289,6 +297,8 @@ which makes little sense.
 	This implies `--merge` and, if no strategy has been
 	specified, `-s recursive`.  Note the reversal of 'ours' and
 	'theirs' as noted above for the `-m` option.
++
+See also INCOMPATIBLE OPTIONS below.
 
 -S[<keyid>]::
 --gpg-sign[=<keyid>]::
@@ -324,6 +334,8 @@ which makes little sense.
 	and after each change.  When fewer lines of surrounding
 	context exist they all must match.  By default no context is
 	ever ignored.
++
+See also INCOMPATIBLE OPTIONS below.
 
 -f::
 --force-rebase::
@@ -355,19 +367,22 @@ default is `--no-fork-point`, otherwise the default is `--fork-point`.
 --whitespace=<option>::
 	These flag are passed to the 'git apply' program
 	(see linkgit:git-apply[1]) that applies the patch.
-	Incompatible with the --interactive option.
++
+See also INCOMPATIBLE OPTIONS below.
 
 --committer-date-is-author-date::
 --ignore-date::
 	These flags are passed to 'git am' to easily change the dates
 	of the rebased commits (see linkgit:git-am[1]).
-	Incompatible with the --interactive option.
++
+See also INCOMPATIBLE OPTIONS below.
 
 --signoff::
 	Add a Signed-off-by: trailer to all the rebased commits. Note
 	that if `--interactive` is given then only commits marked to be
-	picked, edited or reworded will have the trailer added. Incompatible
-	with the `--preserve-merges` option.
+	picked, edited or reworded will have the trailer added.
++
+See also INCOMPATIBLE OPTIONS below.
 
 -i::
 --interactive::
@@ -378,6 +393,8 @@ default is `--no-fork-point`, otherwise the default is `--fork-point`.
 The commit list format can be changed by setting the configuration option
 rebase.instructionFormat.  A customized instruction format will automatically
 have the long commit hash prepended to the format.
++
+See also INCOMPATIBLE OPTIONS below.
 
 -r::
 --rebase-merges[=(rebase-cousins|no-rebase-cousins)]::
@@ -404,7 +421,7 @@ It is currently only possible to recreate the merge commits using the
 `recursive` merge strategy; Different merge strategies can be used only via
 explicit `exec git merge -s <strategy> [...]` commands.
 +
-See also REBASING MERGES below.
+See also REBASING MERGES and INCOMPATIBLE OPTIONS below.
 
 -p::
 --preserve-merges::
@@ -415,6 +432,8 @@ See also REBASING MERGES below.
 This uses the `--interactive` machinery internally, but combining it
 with the `--interactive` option explicitly is generally not a good
 idea unless you know what you are doing (see BUGS below).
++
+See also INCOMPATIBLE OPTIONS below.
 
 -x <cmd>::
 --exec <cmd>::
@@ -437,6 +456,8 @@ squash/fixup series.
 +
 This uses the `--interactive` machinery internally, but it can be run
 without an explicit `--interactive`.
++
+See also INCOMPATIBLE OPTIONS below.
 
 --root::
 	Rebase all commits reachable from <branch>, instead of
@@ -447,6 +468,8 @@ without an explicit `--interactive`.
 	When used together with both --onto and --preserve-merges,
 	'all' root commits will be rewritten to have <newbase> as parent
 	instead.
++
+See also INCOMPATIBLE OPTIONS below.
 
 --autosquash::
 --no-autosquash::
@@ -461,11 +484,11 @@ without an explicit `--interactive`.
 	too.  The recommended way to create fixup/squash commits is by using
 	the `--fixup`/`--squash` options of linkgit:git-commit[1].
 +
-This option is only valid when the `--interactive` option is used.
-+
 If the `--autosquash` option is enabled by default using the
 configuration variable `rebase.autoSquash`, this option can be
 used to override and disable this setting.
++
+See also INCOMPATIBLE OPTIONS below.
 
 --autostash::
 --no-autostash::
@@ -487,6 +510,51 @@ recreates the topic branch with fresh commits so it can be remerged
 successfully without needing to "revert the reversion" (see the
 link:howto/revert-a-faulty-merge.html[revert-a-faulty-merge How-To] for details).
 
+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 linkgit:git-merge-recursive[1] (merge backend)
+ * one based on linkgit:git-cherry-pick[1] (interactive backend)
+
+Flags only understood by the am backend:
+
+ * --committer-date-is-author-date
+ * --ignore-date
+ * --whitespace
+ * --ignore-whitespace
+ * -C
+
+Flags understood by both merge and interactive backends:
+
+ * --merge
+ * --strategy
+ * --strategy-option
+ * --allow-empty-message
+
+Flags only understood by the interactive backend:
+
+ * --[no-]autosquash
+ * --rebase-merges
+ * --preserve-merges
+ * --interactive
+ * --exec
+ * --keep-empty
+ * --autosquash
+ * --edit-todo
+ * --root + --onto
+
+Other incompatible flag pairs:
+
+ * --preserve-merges && --interactive
+ * --preserve-merges && --signoff
+ * --preserve-merges && --rebase-merges
+ * --rebase-merges && --strategy
+
 include::merge-strategies.txt[]
 
 NOTES
-- 
2.18.0.rc2.1.g5453d3f70b.dirty


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

* [RFC PATCH v2 2/7] git-rebase.sh: update help messages a bit
  2018-06-17  5:58   ` [RFC PATCH v2 0/7] Document/fix/warn about rebase incompatibilities and inconsistences Elijah Newren
  2018-06-17  5:58     ` [RFC PATCH v2 1/7] git-rebase.txt: document incompatible options Elijah Newren
@ 2018-06-17  5:58     ` Elijah Newren
  2018-06-17  5:58     ` [RFC PATCH v2 3/7] t3422: new testcases for checking when incompatible options passed Elijah Newren
                       ` (5 subsequent siblings)
  7 siblings, 0 replies; 47+ messages in thread
From: Elijah Newren @ 2018-06-17  5:58 UTC (permalink / raw)
  To: git, phillip.wood; +Cc: johannes.schindelin, gitster, Elijah Newren

signoff is not specific to the am-backend.  Also, re-order a few options
to make like things (e.g. strategy and strategy-option) be near each
other.
---
 git-rebase.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 40be59ecc4..bf71b7fa20 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -20,23 +20,23 @@ onto=!             rebase onto given branch instead of upstream
 r,rebase-merges?   try to rebase merges instead of skipping them
 p,preserve-merges! try to recreate merges instead of ignoring them
 s,strategy=!       use the given merge strategy
+X,strategy-option=! pass the argument through to the merge strategy
 no-ff!             cherry-pick all commits, even if unchanged
+f,force-rebase!    cherry-pick all commits, even if unchanged
 m,merge!           use merging strategies to rebase
 i,interactive!     let the user edit the list of commits to rebase
 x,exec=!           add exec lines after each commit of the editable list
 k,keep-empty	   preserve empty commits during rebase
 allow-empty-message allow rebasing commits with empty messages
-f,force-rebase!    force rebase even if branch is up to date
-X,strategy-option=! pass the argument through to the merge strategy
 stat!              display a diffstat of what changed upstream
 n,no-stat!         do not show diffstat of what changed upstream
 verify             allow pre-rebase hook to run
 rerere-autoupdate  allow rerere to update index with resolved conflicts
 root!              rebase all reachable commits up to the root(s)
 autosquash         move commits that begin with squash!/fixup! under -i
+signoff            add a Signed-off-by: line to each commit
 committer-date-is-author-date! passed to 'git am'
 ignore-date!       passed to 'git am'
-signoff            passed to 'git am'
 whitespace=!       passed to 'git apply'
 ignore-whitespace! passed to 'git apply'
 C=!                passed to 'git apply'
-- 
2.18.0.rc2.1.g5453d3f70b.dirty


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

* [RFC PATCH v2 3/7] t3422: new testcases for checking when incompatible options passed
  2018-06-17  5:58   ` [RFC PATCH v2 0/7] Document/fix/warn about rebase incompatibilities and inconsistences Elijah Newren
  2018-06-17  5:58     ` [RFC PATCH v2 1/7] git-rebase.txt: document incompatible options Elijah Newren
  2018-06-17  5:58     ` [RFC PATCH v2 2/7] git-rebase.sh: update help messages a bit Elijah Newren
@ 2018-06-17  5:58     ` Elijah Newren
  2018-06-17  5:58     ` [RFC PATCH v2 4/7] git-rebase: error out " Elijah Newren
                       ` (4 subsequent siblings)
  7 siblings, 0 replies; 47+ messages in thread
From: Elijah Newren @ 2018-06-17  5:58 UTC (permalink / raw)
  To: git, phillip.wood; +Cc: johannes.schindelin, gitster, Elijah Newren

git rebase is split into three types: am, merge, and interactive.  Various
options imply different types, and which mode we are using determine which
sub-script (git-rebase--$type) is executed to finish the work.  Not all
options work with all types, so add tests for combinations where we expect
to receive an error rather than having options be silently ignored.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t3422-rebase-incompatible-options.sh | 69 ++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100755 t/t3422-rebase-incompatible-options.sh

diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
new file mode 100755
index 0000000000..04cdae921b
--- /dev/null
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -0,0 +1,69 @@
+#!/bin/sh
+
+test_description='test if rebase detects and aborts on incompatible options'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	test_seq 2 9 >foo &&
+	git add foo &&
+	git commit -m orig &&
+
+	git branch A &&
+	git branch B &&
+
+	git checkout A &&
+	test_seq 1 9 >foo &&
+	git add foo &&
+	git commit -m A &&
+
+	git checkout B &&
+	# This is indented with HT SP HT.
+	echo "	 	foo();" >>foo &&
+	git add foo &&
+	git commit -m B
+'
+
+#
+# Rebase has lots of useful options like --whitepsace=fix, which are
+# actually all built in terms of flags to git-am.  Since neither
+# --merge nor --interactive (nor any options that imply those two) use
+# git-am, using them together will result in flags like --whitespace=fix
+# being ignored.  Make sure rebase warns the user and aborts instead.
+#
+
+test_run_rebase () {
+	opt=$1
+	shift
+	test_expect_failure "$opt incompatible with --merge" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --merge A
+	"
+
+	test_expect_failure "$opt incompatible with --strategy=ours" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --strategy=ours A
+	"
+
+	test_expect_failure "$opt incompatible with --strategy-option=ours" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --strategy=ours A
+	"
+
+	test_expect_failure "$opt incompatible with --interactive" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --interactive A
+	"
+
+	test_expect_failure "$opt incompatible with --exec" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --exec 'true' A
+	"
+
+}
+
+test_run_rebase --whitespace=fix
+test_run_rebase --ignore-whitespace
+test_run_rebase --committer-date-is-author-date
+test_run_rebase -C4
+
+test_done
-- 
2.18.0.rc2.1.g5453d3f70b.dirty


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

* [RFC PATCH v2 4/7] git-rebase: error out when incompatible options passed
  2018-06-17  5:58   ` [RFC PATCH v2 0/7] Document/fix/warn about rebase incompatibilities and inconsistences Elijah Newren
                       ` (2 preceding siblings ...)
  2018-06-17  5:58     ` [RFC PATCH v2 3/7] t3422: new testcases for checking when incompatible options passed Elijah Newren
@ 2018-06-17  5:58     ` " Elijah Newren
  2018-06-17  5:58     ` [RFC PATCH v2 5/7] git-rebase.txt: document behavioral inconsistencies between modes Elijah Newren
                       ` (3 subsequent siblings)
  7 siblings, 0 replies; 47+ messages in thread
From: Elijah Newren @ 2018-06-17  5:58 UTC (permalink / raw)
  To: git, phillip.wood; +Cc: johannes.schindelin, gitster, Elijah Newren

git rebase has three different types: am, merge, and interactive, all of
which are implemented in terms of separate scripts.  am builds on git-am,
merge builds on git-merge-recursive, and interactive builds on
git-cherry-pick.  We make use of features in those lower-level commands in
the different rebase types, but those features don't exist in all of the
lower level commands so we have a range of incompatibilities.  Previously,
we just accepted nearly any argument and silently ignored whichever ones
weren't implemented for the type of rebase specified.  Change this so the
incompatibilities are documented, included in the testsuite, and tested
for at runtime with an appropriate error message shown.

Some exceptions I left out:

  * --merge and --interactive are technically incompatible since they are
    supposed to run different underlying scripts, but with a few small
    changes, --interactive can do everything that --merge can.  In fact,
    I'll shortly be sending another patch to remove git-rebase--merge and
    reimplement it on top of git-rebase--interactive.

  * One could argue that --interactive and --quiet are incompatible since
    --interactive doesn't implement a --quiet mode (perhaps since
    cherry-pick itself does not implement one).  However, the interactive
    mode is more quiet than the other modes in general with progress
    messages, so one could argue that it's already quiet.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 git-rebase.sh                          | 17 +++++++++++++++++
 t/t3422-rebase-incompatible-options.sh | 10 +++++-----
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index bf71b7fa20..5f891214fa 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -503,6 +503,23 @@ 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//')
+	if test -n "$interactive_rebase"
+	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
+	fi
+fi
+
 if test -n "$signoff"
 then
 	test -n "$preserve_merges" &&
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 04cdae921b..66a83363bf 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -34,27 +34,27 @@ test_expect_success 'setup' '
 test_run_rebase () {
 	opt=$1
 	shift
-	test_expect_failure "$opt incompatible with --merge" "
+	test_expect_success "$opt incompatible with --merge" "
 		git checkout B^0 &&
 		test_must_fail git rebase $opt --merge A
 	"
 
-	test_expect_failure "$opt incompatible with --strategy=ours" "
+	test_expect_success "$opt incompatible with --strategy=ours" "
 		git checkout B^0 &&
 		test_must_fail git rebase $opt --strategy=ours A
 	"
 
-	test_expect_failure "$opt incompatible with --strategy-option=ours" "
+	test_expect_success "$opt incompatible with --strategy-option=ours" "
 		git checkout B^0 &&
 		test_must_fail git rebase $opt --strategy=ours A
 	"
 
-	test_expect_failure "$opt incompatible with --interactive" "
+	test_expect_success "$opt incompatible with --interactive" "
 		git checkout B^0 &&
 		test_must_fail git rebase $opt --interactive A
 	"
 
-	test_expect_failure "$opt incompatible with --exec" "
+	test_expect_success "$opt incompatible with --exec" "
 		git checkout B^0 &&
 		test_must_fail git rebase $opt --exec 'true' A
 	"
-- 
2.18.0.rc2.1.g5453d3f70b.dirty


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

* [RFC PATCH v2 5/7] git-rebase.txt: document behavioral inconsistencies between modes
  2018-06-17  5:58   ` [RFC PATCH v2 0/7] Document/fix/warn about rebase incompatibilities and inconsistences Elijah Newren
                       ` (3 preceding siblings ...)
  2018-06-17  5:58     ` [RFC PATCH v2 4/7] git-rebase: error out " Elijah Newren
@ 2018-06-17  5:58     ` Elijah Newren
  2018-06-17  5:58     ` [RFC PATCH v2 6/7] git-rebase.txt: address confusion between --no-ff vs --force-rebase Elijah Newren
                       ` (2 subsequent siblings)
  7 siblings, 0 replies; 47+ messages in thread
From: Elijah Newren @ 2018-06-17  5:58 UTC (permalink / raw)
  To: git, phillip.wood; +Cc: johannes.schindelin, gitster, Elijah Newren

There are a variety of aspects that are common to all rebases regardless
of which backend is in use; however, the behavior for these different
aspects varies in ways that could surprise users.  (In fact, it's not
clear -- to me at least -- that these differences were even desirable or
intentional.)  Document these differences.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt | 57 ++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index adccc15284..0dbfab06d0 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -555,6 +555,63 @@ Other incompatible flag pairs:
  * --preserve-merges && --rebase-merges
  * --rebase-merges && --strategy
 
+BEHAVIORAL INCONSISTENCIES
+--------------------------
+
+  * --no-ff vs. --force-rebase
+
+    These options are actually identical, though their description
+    leads people to believe they might not be.
+
+ * empty commits:
+
+    am-based rebase will drop any "empty" commits, whether the
+    commit started empty (had no changes relative to its parent to
+    start with) or ended empty (all changes were already applied
+    upstream in other commits).
+
+    merge-based rebase does the same.
+
+    interactive-based rebase will by default drop commits that
+    started empty and halt if it hits a commit that ended up empty.
+    The --keep-empty option exists for interactive rebases to allow
+    it to keep commits that started empty.
+
+  * empty commit messages:
+
+    am-based rebase will silently apply commits with empty commit
+    messages.
+
+    merge-based and interactive-based rebases will by default halt
+    on any such commits.  The --allow-empty-message option exists to
+    allow interactive-based rebases to apply such commits without
+    halting.
+
+  * directory rename detection:
+
+    merge-based and interactive-based rebases work fine with
+    directory rename detection.  am-based rebases sometimes do not.
+
+    git-am tries to avoid a full three way merge, instead calling
+    git-apply.  That prevents us from detecting renames at all,
+    which may defeat the directory rename detection.  There is a
+    fallback, though; if the initial git-apply fails and the user
+    has specified the -3 option, git-am will fall back to a three
+    way merge.  However, git-am lacks the necessary information to
+    do a "real" three way merge.  Instead, it has to use
+    build_fake_ancestor() to get a merge base that is missing files
+    whose rename may have been important to detect for directory
+    rename detection to function.
+
+    Since am-based rebases work by first generating a bunch of
+    patches (which no longer record what the original commits were
+    and thus don't have the necessary info from which we can find a
+    real merge-base), and then calling git-am, this implies that
+    am-based rebases will not always successfully detect directory
+    renames either.  merged-based rebases (rebase -m) and
+    cherry-pick-based rebases (rebase -i) are not affected by this
+    shortcoming.
+
 include::merge-strategies.txt[]
 
 NOTES
-- 
2.18.0.rc2.1.g5453d3f70b.dirty


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

* [RFC PATCH v2 6/7] git-rebase.txt: address confusion between --no-ff vs --force-rebase
  2018-06-17  5:58   ` [RFC PATCH v2 0/7] Document/fix/warn about rebase incompatibilities and inconsistences Elijah Newren
                       ` (4 preceding siblings ...)
  2018-06-17  5:58     ` [RFC PATCH v2 5/7] git-rebase.txt: document behavioral inconsistencies between modes Elijah Newren
@ 2018-06-17  5:58     ` Elijah Newren
  2018-06-17  5:58     ` [RFC PATCH v2 7/7] git-rebase: make --allow-empty-message the default Elijah Newren
  2018-06-17 15:41     ` [RFC PATCH v2 0/7] Document/fix/warn about rebase incompatibilities and inconsistences Phillip Wood
  7 siblings, 0 replies; 47+ messages in thread
From: Elijah Newren @ 2018-06-17  5:58 UTC (permalink / raw)
  To: git, phillip.wood; +Cc: johannes.schindelin, gitster, Elijah Newren

rebase was taught the --force-rebase option in commit b2f82e05de ("Teach
rebase to rebase even if upstream is up to date", 2009-02-13).  This flag
worked for the am and merge backends, but wasn't a valid option for the
interactive backend.

rebase was taught the --no-ff option for interactive rebases in commit
b499549401cb ("Teach rebase the --no-ff option.", 2010-03-24), to do the
exact same thing as --force-rebase does for non-interactive rebases.  This
commit explicitly documented the fact that --force-rebase was incompatible
with --interactive, though it made --no-ff a synonym for --force-rebase
for non-interactive rebases.  The choice of a new option was based on the
fact that "force rebase" didn't sound like an appropriate term for the
interactive machinery.

In commit 6bb4e485cff8 ("rebase: align variable names", 2011-02-06), the
separate parsing of command line options in the different rebase scripts
was removed, and whether on accident or because the author noticed that
these options did the same thing, the options became synonyms and both
were accepted by all three rebase types.

In commit 2d26d533a012 ("Documentation/git-rebase.txt: -f forces a rebase
that would otherwise be a no-op", 2014-08-12), which reworded the
description of the --force-rebase option, the (no-longer correct) sentence
stating that --force-rebase was incompatible with --interactive was
finally removed.

Finally, as explained at
https://public-inbox.org/git/98279912-0f52-969d-44a6-22242039387f@xiplink.com

    In the original discussion around this option [1], at one point I
    proposed teaching rebase--interactive to respect --force-rebase
    instead of adding a new option [2].  Ultimately --no-ff was chosen as
    the better user interface design [3], because an interactive rebase
    can't be "forced" to run.

We have accepted both --no-ff and --force-rebase as full synonyms for all
three rebase types for over seven years.  Documenting them differently
and in ways that suggest they might not be quite synonyms simply leads to
confusion.  Adjust the documentation to match reality.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt | 35 ++++++++++-------------------------
 1 file changed, 10 insertions(+), 25 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 0dbfab06d0..7a2ed9efdc 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -337,16 +337,18 @@ See also INCOMPATIBLE OPTIONS below.
 +
 See also INCOMPATIBLE OPTIONS below.
 
--f::
+--no-ff::
 --force-rebase::
-	Force a rebase even if the current branch is up to date and
-	the command without `--force` would return without doing anything.
+-f::
+	Individually replay all rebased commits instead of fast-forwarding
+	over the unchanged ones.  This ensures that the entire history of
+	the rebased branch is composed of new commits.
 +
-You may find this (or --no-ff with an interactive rebase) helpful after
-reverting a topic branch merge, as this option recreates the topic branch with
-fresh commits so it can be remerged successfully without needing to "revert
-the reversion" (see the
-link:howto/revert-a-faulty-merge.html[revert-a-faulty-merge How-To] for details).
+You may find this helpful after reverting a topic branch merge, as this option
+recreates the topic branch with fresh commits so it can be remerged
+successfully without needing to "revert the reversion" (see the
+link:howto/revert-a-faulty-merge.html[revert-a-faulty-merge How-To] for
+details).
 
 --fork-point::
 --no-fork-point::
@@ -498,18 +500,6 @@ See also INCOMPATIBLE OPTIONS below.
 	with care: the final stash application after a successful
 	rebase might result in non-trivial conflicts.
 
---no-ff::
-	With --interactive, cherry-pick all rebased commits instead of
-	fast-forwarding over the unchanged ones.  This ensures that the
-	entire history of the rebased branch is composed of new commits.
-+
-Without --interactive, this is a synonym for --force-rebase.
-+
-You may find this helpful after reverting a topic branch merge, as this option
-recreates the topic branch with fresh commits so it can be remerged
-successfully without needing to "revert the reversion" (see the
-link:howto/revert-a-faulty-merge.html[revert-a-faulty-merge How-To] for details).
-
 INCOMPATIBLE OPTIONS
 --------------------
 
@@ -558,11 +548,6 @@ Other incompatible flag pairs:
 BEHAVIORAL INCONSISTENCIES
 --------------------------
 
-  * --no-ff vs. --force-rebase
-
-    These options are actually identical, though their description
-    leads people to believe they might not be.
-
  * empty commits:
 
     am-based rebase will drop any "empty" commits, whether the
-- 
2.18.0.rc2.1.g5453d3f70b.dirty


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

* [RFC PATCH v2 7/7] git-rebase: make --allow-empty-message the default
  2018-06-17  5:58   ` [RFC PATCH v2 0/7] Document/fix/warn about rebase incompatibilities and inconsistences Elijah Newren
                       ` (5 preceding siblings ...)
  2018-06-17  5:58     ` [RFC PATCH v2 6/7] git-rebase.txt: address confusion between --no-ff vs --force-rebase Elijah Newren
@ 2018-06-17  5:58     ` Elijah Newren
  2018-06-17 15:30       ` Phillip Wood
  2018-06-17 15:41     ` [RFC PATCH v2 0/7] Document/fix/warn about rebase incompatibilities and inconsistences Phillip Wood
  7 siblings, 1 reply; 47+ messages in thread
From: Elijah Newren @ 2018-06-17  5:58 UTC (permalink / raw)
  To: git, phillip.wood; +Cc: johannes.schindelin, gitster, Elijah Newren

am-based rebases already apply commits with an empty commit message
without requiring the user to specify an extra flag.  Make merge-based and
interactive-based rebases behave the same.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt  | 10 ----------
 git-rebase.sh                 |  2 +-
 t/t3404-rebase-interactive.sh |  7 ++++---
 t/t3405-rebase-malformed.sh   | 11 +++--------
 4 files changed, 8 insertions(+), 22 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 7a2ed9efdc..a5608f481f 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -562,16 +562,6 @@ BEHAVIORAL INCONSISTENCIES
     The --keep-empty option exists for interactive rebases to allow
     it to keep commits that started empty.
 
-  * empty commit messages:
-
-    am-based rebase will silently apply commits with empty commit
-    messages.
-
-    merge-based and interactive-based rebases will by default halt
-    on any such commits.  The --allow-empty-message option exists to
-    allow interactive-based rebases to apply such commits without
-    halting.
-
   * directory rename detection:
 
     merge-based and interactive-based rebases work fine with
diff --git a/git-rebase.sh b/git-rebase.sh
index 5f891214fa..bf033da4e5 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -95,7 +95,7 @@ rebase_cousins=
 preserve_merges=
 autosquash=
 keep_empty=
-allow_empty_message=
+allow_empty_message=--allow-empty-message
 signoff=
 test "$(git config --bool rebase.autosquash)" = "true" && autosquash=t
 case "$(git config --bool commit.gpgsign)" in
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index c65826ddac..f84fa63b15 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -553,15 +553,16 @@ test_expect_success '--continue tries to commit, even for "edit"' '
 '
 
 test_expect_success 'aborted --continue does not squash commits after "edit"' '
+	test_when_finished "git rebase --abort" &&
 	old=$(git rev-parse HEAD) &&
 	test_tick &&
 	set_fake_editor &&
 	FAKE_LINES="edit 1" git rebase -i HEAD^ &&
 	echo "edited again" > file7 &&
 	git add file7 &&
-	test_must_fail env FAKE_COMMIT_MESSAGE=" " git rebase --continue &&
-	test $old = $(git rev-parse HEAD) &&
-	git rebase --abort
+	echo all the things >>conflict &&
+	test_must_fail git rebase --continue &&
+	test $old = $(git rev-parse HEAD)
 '
 
 test_expect_success 'auto-amend only edited commits after "edit"' '
diff --git a/t/t3405-rebase-malformed.sh b/t/t3405-rebase-malformed.sh
index cb7c6de84a..da94dddc86 100755
--- a/t/t3405-rebase-malformed.sh
+++ b/t/t3405-rebase-malformed.sh
@@ -77,19 +77,14 @@ test_expect_success 'rebase commit with diff in message' '
 '
 
 test_expect_success 'rebase -m commit with empty message' '
-	test_must_fail git rebase -m master empty-message-merge &&
-	git rebase --abort &&
-	git rebase -m --allow-empty-message master empty-message-merge
+	git rebase -m master empty-message-merge
 '
 
 test_expect_success 'rebase -i commit with empty message' '
 	git checkout diff-in-message &&
 	set_fake_editor &&
-	test_must_fail env FAKE_COMMIT_MESSAGE=" " FAKE_LINES="reword 1" \
-		git rebase -i HEAD^ &&
-	git rebase --abort &&
-	FAKE_COMMIT_MESSAGE=" " FAKE_LINES="reword 1" \
-		git rebase -i --allow-empty-message HEAD^
+	env FAKE_COMMIT_MESSAGE=" " FAKE_LINES="reword 1" \
+		git rebase -i HEAD^
 '
 
 test_done
-- 
2.18.0.rc2.1.g5453d3f70b.dirty


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

* Re: [RFC PATCH v2 7/7] git-rebase: make --allow-empty-message the default
  2018-06-17  5:58     ` [RFC PATCH v2 7/7] git-rebase: make --allow-empty-message the default Elijah Newren
@ 2018-06-17 15:30       ` Phillip Wood
  0 siblings, 0 replies; 47+ messages in thread
From: Phillip Wood @ 2018-06-17 15:30 UTC (permalink / raw)
  To: Elijah Newren, git, phillip.wood; +Cc: johannes.schindelin, gitster

Hi Elijah

On 17/06/18 06:58, Elijah Newren wrote:
> am-based rebases already apply commits with an empty commit message
> without requiring the user to specify an extra flag.  Make merge-based and
> interactive-based rebases behave the same.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>   Documentation/git-rebase.txt  | 10 ----------
>   git-rebase.sh                 |  2 +-
>   t/t3404-rebase-interactive.sh |  7 ++++---
>   t/t3405-rebase-malformed.sh   | 11 +++--------
>   4 files changed, 8 insertions(+), 22 deletions(-)
> 
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 7a2ed9efdc..a5608f481f 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -562,16 +562,6 @@ BEHAVIORAL INCONSISTENCIES
>       The --keep-empty option exists for interactive rebases to allow
>       it to keep commits that started empty.
>   
> -  * empty commit messages:
> -
> -    am-based rebase will silently apply commits with empty commit
> -    messages.
> -
> -    merge-based and interactive-based rebases will by default halt
> -    on any such commits.  The --allow-empty-message option exists to
> -    allow interactive-based rebases to apply such commits without
> -    halting.
> -
>     * directory rename detection:
>   
>       merge-based and interactive-based rebases work fine with
> diff --git a/git-rebase.sh b/git-rebase.sh
> index 5f891214fa..bf033da4e5 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -95,7 +95,7 @@ rebase_cousins=
>   preserve_merges=
>   autosquash=
>   keep_empty=
> -allow_empty_message=
> +allow_empty_message=--allow-empty-message

Looking at the option parsing in git-rebase.sh it appears that it does 
not check for --no-allow-empty-message so there's no way to turn off the 
default for those modes that support it. I'm not sure what to think of 
this change, I'm slightly uneasy with changing to default to be 
different from cherry-pick, though one could argue rebasing is a 
different operation and just keeping the existing messages even if they 
are empty is more appropriate and having all the rebase modes do the 
default to the same behaviour is definitely an improvement.

Best Wishes

Phillip

>   signoff=
>   test "$(git config --bool rebase.autosquash)" = "true" && autosquash=t
>   case "$(git config --bool commit.gpgsign)" in
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index c65826ddac..f84fa63b15 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -553,15 +553,16 @@ test_expect_success '--continue tries to commit, even for "edit"' '
>   '
>   
>   test_expect_success 'aborted --continue does not squash commits after "edit"' '
> +	test_when_finished "git rebase --abort" &&
>   	old=$(git rev-parse HEAD) &&
>   	test_tick &&
>   	set_fake_editor &&
>   	FAKE_LINES="edit 1" git rebase -i HEAD^ &&
>   	echo "edited again" > file7 &&
>   	git add file7 &&
> -	test_must_fail env FAKE_COMMIT_MESSAGE=" " git rebase --continue &&
> -	test $old = $(git rev-parse HEAD) &&
> -	git rebase --abort
> +	echo all the things >>conflict &&
> +	test_must_fail git rebase --continue &&
> +	test $old = $(git rev-parse HEAD)
>   '
>   
>   test_expect_success 'auto-amend only edited commits after "edit"' '
> diff --git a/t/t3405-rebase-malformed.sh b/t/t3405-rebase-malformed.sh
> index cb7c6de84a..da94dddc86 100755
> --- a/t/t3405-rebase-malformed.sh
> +++ b/t/t3405-rebase-malformed.sh
> @@ -77,19 +77,14 @@ test_expect_success 'rebase commit with diff in message' '
>   '
>   
>   test_expect_success 'rebase -m commit with empty message' '
> -	test_must_fail git rebase -m master empty-message-merge &&
> -	git rebase --abort &&
> -	git rebase -m --allow-empty-message master empty-message-merge
> +	git rebase -m master empty-message-merge
>   '
>   
>   test_expect_success 'rebase -i commit with empty message' '
>   	git checkout diff-in-message &&
>   	set_fake_editor &&
> -	test_must_fail env FAKE_COMMIT_MESSAGE=" " FAKE_LINES="reword 1" \
> -		git rebase -i HEAD^ &&
> -	git rebase --abort &&
> -	FAKE_COMMIT_MESSAGE=" " FAKE_LINES="reword 1" \
> -		git rebase -i --allow-empty-message HEAD^
> +	env FAKE_COMMIT_MESSAGE=" " FAKE_LINES="reword 1" \
> +		git rebase -i HEAD^
>   '
>   
>   test_done
> 


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

* Re: [RFC PATCH v2 1/7] git-rebase.txt: document incompatible options
  2018-06-17  5:58     ` [RFC PATCH v2 1/7] git-rebase.txt: document incompatible options Elijah Newren
@ 2018-06-17 15:38       ` Phillip Wood
  2018-06-17 17:15       ` Eric Sunshine
  1 sibling, 0 replies; 47+ messages in thread
From: Phillip Wood @ 2018-06-17 15:38 UTC (permalink / raw)
  To: Elijah Newren, git, phillip.wood; +Cc: johannes.schindelin, gitster

Hi Elijah
On 17/06/18 06:58, Elijah Newren wrote:
> git rebase has many options that only work with one of its three backends.
> It also has a few other pairs of incompatible options.  Document these.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>   Documentation/git-rebase.txt | 84 ++++++++++++++++++++++++++++++++----
>   1 file changed, 76 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 0e20a66e73..adccc15284 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -243,11 +243,15 @@ leave out at most one of A and B, in which case it defaults to HEAD.
>   --keep-empty::
>   	Keep the commits that do not change anything from its
>   	parents in the result.
> ++
> +See also INCOMPATIBLE OPTIONS below.
>   
>   --allow-empty-message::
>   	By default, rebasing commits with an empty message will fail.
>   	This option overrides that behavior, allowing commits with empty
>   	messages to be rebased.
> ++
> +See also INCOMPATIBLE OPTIONS below.
>   
>   --skip::
>   	Restart the rebasing process by skipping the current patch.
> @@ -271,6 +275,8 @@ branch on top of the <upstream> branch.  Because of this, when a merge
>   conflict happens, the side reported as 'ours' is the so-far rebased
>   series, starting with <upstream>, and 'theirs' is the working branch.  In
>   other words, the sides are swapped.
> ++
> +See also INCOMPATIBLE OPTIONS below.
>   
>   -s <strategy>::
>   --strategy=<strategy>::
> @@ -280,8 +286,10 @@ other words, the sides are swapped.
>   +
>   Because 'git rebase' replays each commit from the working branch
>   on top of the <upstream> branch using the given strategy, using
> -the 'ours' strategy simply discards all patches from the <branch>,
> +the 'ours' strategy simply empties all patches from the <branch>,
>   which makes little sense.
> ++
> +See also INCOMPATIBLE OPTIONS below.
>   
>   -X <strategy-option>::
>   --strategy-option=<strategy-option>::
> @@ -289,6 +297,8 @@ which makes little sense.
>   	This implies `--merge` and, if no strategy has been
>   	specified, `-s recursive`.  Note the reversal of 'ours' and
>   	'theirs' as noted above for the `-m` option.
> ++
> +See also INCOMPATIBLE OPTIONS below.
>   
>   -S[<keyid>]::
>   --gpg-sign[=<keyid>]::
> @@ -324,6 +334,8 @@ which makes little sense.
>   	and after each change.  When fewer lines of surrounding
>   	context exist they all must match.  By default no context is
>   	ever ignored.
> ++
> +See also INCOMPATIBLE OPTIONS below.
>   
>   -f::
>   --force-rebase::
> @@ -355,19 +367,22 @@ default is `--no-fork-point`, otherwise the default is `--fork-point`.
>   --whitespace=<option>::
>   	These flag are passed to the 'git apply' program
>   	(see linkgit:git-apply[1]) that applies the patch.
> -	Incompatible with the --interactive option.
> ++
> +See also INCOMPATIBLE OPTIONS below.
>   
>   --committer-date-is-author-date::
>   --ignore-date::
>   	These flags are passed to 'git am' to easily change the dates
>   	of the rebased commits (see linkgit:git-am[1]).
> -	Incompatible with the --interactive option.
> ++
> +See also INCOMPATIBLE OPTIONS below.
>   
>   --signoff::
>   	Add a Signed-off-by: trailer to all the rebased commits. Note
>   	that if `--interactive` is given then only commits marked to be
> -	picked, edited or reworded will have the trailer added. Incompatible
> -	with the `--preserve-merges` option.
> +	picked, edited or reworded will have the trailer added.
> ++
> +See also INCOMPATIBLE OPTIONS below.
>   
>   -i::
>   --interactive::
> @@ -378,6 +393,8 @@ default is `--no-fork-point`, otherwise the default is `--fork-point`.
>   The commit list format can be changed by setting the configuration option
>   rebase.instructionFormat.  A customized instruction format will automatically
>   have the long commit hash prepended to the format.
> ++
> +See also INCOMPATIBLE OPTIONS below.
>   
>   -r::
>   --rebase-merges[=(rebase-cousins|no-rebase-cousins)]::
> @@ -404,7 +421,7 @@ It is currently only possible to recreate the merge commits using the
>   `recursive` merge strategy; Different merge strategies can be used only via
>   explicit `exec git merge -s <strategy> [...]` commands.
>   +
> -See also REBASING MERGES below.
> +See also REBASING MERGES and INCOMPATIBLE OPTIONS below.
>   
>   -p::
>   --preserve-merges::
> @@ -415,6 +432,8 @@ See also REBASING MERGES below.
>   This uses the `--interactive` machinery internally, but combining it
>   with the `--interactive` option explicitly is generally not a good
>   idea unless you know what you are doing (see BUGS below).
> ++
> +See also INCOMPATIBLE OPTIONS below.
>   
>   -x <cmd>::
>   --exec <cmd>::
> @@ -437,6 +456,8 @@ squash/fixup series.
>   +
>   This uses the `--interactive` machinery internally, but it can be run
>   without an explicit `--interactive`.
> ++
> +See also INCOMPATIBLE OPTIONS below.
>   
>   --root::
>   	Rebase all commits reachable from <branch>, instead of
> @@ -447,6 +468,8 @@ without an explicit `--interactive`.
>   	When used together with both --onto and --preserve-merges,
>   	'all' root commits will be rewritten to have <newbase> as parent
>   	instead.
> ++
> +See also INCOMPATIBLE OPTIONS below.
>   
>   --autosquash::
>   --no-autosquash::
> @@ -461,11 +484,11 @@ without an explicit `--interactive`.
>   	too.  The recommended way to create fixup/squash commits is by using
>   	the `--fixup`/`--squash` options of linkgit:git-commit[1].
>   +
> -This option is only valid when the `--interactive` option is used.
> -+
>   If the `--autosquash` option is enabled by default using the
>   configuration variable `rebase.autoSquash`, this option can be
>   used to override and disable this setting.
> ++
> +See also INCOMPATIBLE OPTIONS below.
>   
>   --autostash::
>   --no-autostash::
> @@ -487,6 +510,51 @@ recreates the topic branch with fresh commits so it can be remerged
>   successfully without needing to "revert the reversion" (see the
>   link:howto/revert-a-faulty-merge.html[revert-a-faulty-merge How-To] for details).
>   
> +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 linkgit:git-merge-recursive[1] (merge backend)
> + * one based on linkgit:git-cherry-pick[1] (interactive backend)
> +
> +Flags only understood by the am backend:
> +
> + * --committer-date-is-author-date
> + * --ignore-date
> + * --whitespace
> + * --ignore-whitespace
> + * -C
> +
> +Flags understood by both merge and interactive backends:
> +
> + * --merge
> + * --strategy
> + * --strategy-option
> + * --allow-empty-message
> +
> +Flags only understood by the interactive backend:
> +
> + * --[no-]autosquash
> + * --rebase-merges
> + * --preserve-merges
> + * --interactive
> + * --exec
> + * --keep-empty
> + * --autosquash
> + * --edit-todo
> + * --root + --onto
> +
> +Other incompatible flag pairs:
> +
> + * --preserve-merges && --interactive
> + * --preserve-merges && --signoff
> + * --preserve-merges && --rebase-merges
> + * --rebase-merges && --strategy

Does --rebase-merges support --strategy-options?

> +
>   include::merge-strategies.txt[]
>   
>   NOTES
> 
This is a great step forward. In the long run it would be nice not to 
have to mention the backends. Once git-rebase--merge.sh is gone, the 
situation is simpler and it would be nice to reword this to say 
something like
   The --committer-date-is-author-date, --ignore-date, --whitespace,
   --ignore-whitespace and -C options are incompatible with the following
   --exec ... Also note that --rebase-merges is incompatible with
   --strategy; and --preserve-merges is incompatible with --interactive,
   --signoff, --rebase-merges.
rather than talking about am options etc.

Best Wishes

Phillip

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

* Re: [RFC PATCH v2 0/7] Document/fix/warn about rebase incompatibilities and inconsistences
  2018-06-17  5:58   ` [RFC PATCH v2 0/7] Document/fix/warn about rebase incompatibilities and inconsistences Elijah Newren
                       ` (6 preceding siblings ...)
  2018-06-17  5:58     ` [RFC PATCH v2 7/7] git-rebase: make --allow-empty-message the default Elijah Newren
@ 2018-06-17 15:41     ` Phillip Wood
  7 siblings, 0 replies; 47+ messages in thread
From: Phillip Wood @ 2018-06-17 15:41 UTC (permalink / raw)
  To: Elijah Newren, git, phillip.wood; +Cc: johannes.schindelin, gitster

Hi Elijah

On 17/06/18 06:58, Elijah Newren wrote:
> git-rebase has lots of options that are mutually incompatible.  Even among
> aspects of its behavior that is common to all rebase types, it has a number
> of inconsistencies.  This series tries to document, fix, and/or warn users
> about many of these.
> 
> I have a much higher than average expectation that folks will object
> to some of these patches.  I've tried to divide them up so that any parts
> we decide to drop or redo can be more easily excised.
> 
> No branch-diff; because it's a significant re-work; instead I'll comment
> briefly on the individual patches...

I found this series well structured and easy to follow. I've commented 
on a couple of the patches, the others seemed fine to me. It's great to 
see these inconsistencies being documented and some being eliminated.

Best Wishes

Phillip

> 
> Elijah Newren (7):
>    git-rebase.txt: document incompatible options
> 
> Both Dscho (on a related patch series) and Phillip suggested changing the
> documentation to avoid implementational details.  I instead made a separate
> section with sets of incompatible options...but it still mentions the
> different backends while doing so.  Does that seem alright?
> 
>    git-rebase.sh: update help messages a bit
> 
> Minor tweaks to `git rebase -h` output.
> 
>    t3422: new testcases for checking when incompatible options passed
> 
> The one unmodified patch from the first round.
> 
>    git-rebase: error out when incompatible options passed
> 
> Almost the same as the first round, except:
>    * Documentation pulled into a separate patch (patch 1)
>    * $() instead of ``
> 
>    git-rebase.txt: document behavioral inconsistencies between modes
> 
> Add another section to the documentation for aspects that ideally
> should be common between all modes but are handled differently.
> 
>    git-rebase.txt: address confusion between --no-ff vs --force-rebase
> 
> This came up on the list not that long ago; fix the documentation.
> 
>    git-rebase: make --allow-empty-message the default
> 
> Address the easiest of the inconsistencies, assuming the am-based backend
> has the correct default and the merge-based and interactive-based backends
> are the ones that need to change.
> 
>   Documentation/git-rebase.txt           | 154 ++++++++++++++++++++-----
>   git-rebase.sh                          |  25 +++-
>   t/t3404-rebase-interactive.sh          |   7 +-
>   t/t3405-rebase-malformed.sh            |  11 +-
>   t/t3422-rebase-incompatible-options.sh |  69 +++++++++++
>   5 files changed, 224 insertions(+), 42 deletions(-)
>   create mode 100755 t/t3422-rebase-incompatible-options.sh
> 


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

* Re: [RFC PATCH v2 1/7] git-rebase.txt: document incompatible options
  2018-06-17  5:58     ` [RFC PATCH v2 1/7] git-rebase.txt: document incompatible options Elijah Newren
  2018-06-17 15:38       ` Phillip Wood
@ 2018-06-17 17:15       ` Eric Sunshine
  1 sibling, 0 replies; 47+ messages in thread
From: Eric Sunshine @ 2018-06-17 17:15 UTC (permalink / raw)
  To: Elijah Newren; +Cc: Git List, Phillip Wood, Johannes Schindelin, Junio C Hamano

On Sun, Jun 17, 2018 at 1:59 AM Elijah Newren <newren@gmail.com> wrote:
> git rebase has many options that only work with one of its three backends.
> It also has a few other pairs of incompatible options.  Document these.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> @@ -487,6 +510,51 @@ recreates the topic branch with fresh commits so it can be remerged
> +Flags only understood by the interactive backend:
> + [...]
> + * --edit-todo
> + * --root + --onto

What does "+" mean? Is it "and"?

> +Other incompatible flag pairs:
> +
> + * --preserve-merges && --interactive
> + * --preserve-merges && --signoff
> + * --preserve-merges && --rebase-merges
> + * --rebase-merges && --strategy

It's somewhat odd seeing "programmer" notation in end-user
documentation. Perhaps replace "&&" with "and"?

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

* Re: [RFC PATCH 3/3] git-rebase.sh: make git-rebase--interactive the default
  2018-06-10  1:31       ` Elijah Newren
@ 2018-06-17 21:44         ` Johannes Schindelin
  0 siblings, 0 replies; 47+ messages in thread
From: Johannes Schindelin @ 2018-06-17 21:44 UTC (permalink / raw)
  To: Elijah Newren; +Cc: Git Mailing List, Junio C Hamano, Alban Gruin

Hi Elijah,

On Sat, 9 Jun 2018, Elijah Newren wrote:

> On Sat, Jun 9, 2018 at 3:11 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > On Thu, 7 Jun 2018, Elijah Newren wrote:
> >
> >> am-based rebases suffer from an reduced ability to detect directory
> >> renames upstream, which is fundamental to the fact that it throws
> >> away information about the history: in particular, it dispenses with
> >> the original commits involved by turning them into patches, and
> >> without the knowledge of the original commits we cannot determine a
> >> proper merge base.
> >>
> >> The am-based rebase will proceed by first trying git-apply, and, only
> >> if it fails, will try to reconstruct a provisional merge base using
> >> build_fake_ancestor().  This fake ancestor will ONLY contain the
> >> files modified in the patch; without the full list of files in the
> >> real merge base, renames of any missing files cannot be detected.
> >> Directory rename detection works by looking at individual file
> >> renames and deducing when a full directory has been renamed.
> >>
> >> Trying to modify build_fake_ancestor() to instead create a merge base
> >> that includes common file information by looking for a commit that
> >> contained all the same blobs as the pre-image of the patch would
> >> require a very expensive search through history, may find the wrong
> >> commit, and outside of rebase may not find any commit that matches.
> >>
> >> But we had all the relevant information to start.  So, instead of
> >> attempting to fix am which just doesn't have the relevant
> >> information, instead note its strength and weaknesses, and change the
> >> default rebase machinery to interactive since it does not suffer from
> >> the same problems.
> >
> > I'll let Eric comment on the grammar, and I'll comment on the idea
> > behind this commit instead.
> 
> Going to dump the hard job on Eric, eh?  ;-)

Just trying to learn how to delegate.

:0)

> > IMHO `git rebase -i` is still too slow to be a true replacement for
> > `git rebase --am` for the cases where it serves the user well. Maybe
> > we should work on making `rebase -i` faster, first?
> 
> That sounds fair.
> 
> > I imagine, for example, that it might make *tons* of sense to avoid
> > writing out the index and worktree files all the time. That was
> > necessary in the shell script version because if the ridiculous
> > limitations we subjected ourselves to, such as: no object-oriented
> > state worth mentioning, only string-based processing, etc. But we
> > could now start to do everything in memory (*maybe* write out the new
> > blob/tree/commit objects immediately, but maybe not) until the time
> > when we either have succeeded in the rebase, or when there was a
> > problem and we have to exit with an error. And only then write the
> > files and the index.
> 
> Hmm...are you still planning on using cherry-pick (internally rather
> than forking, of course)?

The sequencer side-steps the cherry-pick command by calling
merge_recursive() directly. But yes, this is what the interactive rebase
will always use.

>  Because cherry-pick uses the merge-recursive machinery, and the
>  merge-recursive machinery doesn't have a nice way of avoiding writing
>  to the working tree or index.

Exactly. I think it could be taught to perform its magic in memory. I am
not saying that it is necessarily easy to teach merge-recursive.c this
trick, but it should be possible.

> Fixing that is on my radar; see the first block of
> https://public-inbox.org/git/CABPp-BG2fZHm3s-yrzxyGj3Eh+O7_LHLz5pgstHhG2drigSyRA@mail.gmail.com/

Great!

> (reading up until "At this point, I'd rather just fix the design flaw
> rather than complicate the code further.")
> 
> However, also covered in my plans is a few things to speed up the
> merge-recursive machinery, which should provide some other performance
> benefits for interactive rebases.

I am looking forward to see this materialize.

> > In any case, I think that the rather noticeable change of the default
> > would probably necessitate a deprecation phase.
> 
> Why is it a "rather noticable change of the default"?

I was really referring to speed. But I have to admit that I do not have
any current numbers.

Another issue just hit me, though: rebase --am does not need to look at as
many Git objects as rebase --merge or rebase -i. Therefore, GVFS users
will still want to use --am wherever possible, to avoid "hydrating"
many objects during their rebase.

> If we popped up the editor and asked the user to edit the list of
> options, I'd agree, or if folks thought that it was significantly slower
> by a big enough margin (though you already suggested waiting and making
> sure we don't do that).  What else remains that qualifies?
> 
> (Okay, the default behavior to just skip empty patches rather than halt
> the rebase and ask the user to advise is different, but we could fix
> that up too.  Is there anything else?)

That is indeed a change in behavior that is rather easy to address.

As to speed: that might be harder. But then, the performance might already
be good enough. I do not have numbers (nor the time to generate them) to
back up my hunch that --am is substantially faster than --merge.

Ciao,
Dscho

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

end of thread, back to index

Thread overview: 47+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-07  4:58 RFC: rebase inconsistency in 2.18 -- course of action? Elijah Newren
2018-06-07  5:04 ` [PATCH] t3401: add directory rename testcases for rebase and am Elijah Newren
2018-06-07  5:05 ` [PATCH] apply: fix grammar error in comment Elijah Newren
2018-06-07  5:05 ` [PATCH] t5407: fix test to cover intended arguments Elijah Newren
2018-06-07  5:06 ` [PATCH] git-rebase--merge: modernize "git-$cmd" to "git $cmd" Elijah Newren
2018-06-07  5:24   ` Elijah Newren
2018-06-07  5:06 ` [PATCH 1/2] t3422: new testcases for checking when incompatible options passed Elijah Newren
2018-06-07  5:06   ` [PATCH 2/2] git-rebase: error out " Elijah Newren
2018-06-10 19:40     ` Phillip Wood
2018-06-11 15:19       ` Elijah Newren
2018-06-11 15:59         ` Phillip Wood
2018-06-11 15:49       ` Elijah Newren
2018-06-12  9:45         ` Phillip Wood
2018-06-17  5:58   ` [RFC PATCH v2 0/7] Document/fix/warn about rebase incompatibilities and inconsistences Elijah Newren
2018-06-17  5:58     ` [RFC PATCH v2 1/7] git-rebase.txt: document incompatible options Elijah Newren
2018-06-17 15:38       ` Phillip Wood
2018-06-17 17:15       ` Eric Sunshine
2018-06-17  5:58     ` [RFC PATCH v2 2/7] git-rebase.sh: update help messages a bit Elijah Newren
2018-06-17  5:58     ` [RFC PATCH v2 3/7] t3422: new testcases for checking when incompatible options passed Elijah Newren
2018-06-17  5:58     ` [RFC PATCH v2 4/7] git-rebase: error out " Elijah Newren
2018-06-17  5:58     ` [RFC PATCH v2 5/7] git-rebase.txt: document behavioral inconsistencies between modes Elijah Newren
2018-06-17  5:58     ` [RFC PATCH v2 6/7] git-rebase.txt: address confusion between --no-ff vs --force-rebase Elijah Newren
2018-06-17  5:58     ` [RFC PATCH v2 7/7] git-rebase: make --allow-empty-message the default Elijah Newren
2018-06-17 15:30       ` Phillip Wood
2018-06-17 15:41     ` [RFC PATCH v2 0/7] Document/fix/warn about rebase incompatibilities and inconsistences Phillip Wood
2018-06-07  5:07 ` [PATCH] git-rebase.sh: handle keep-empty like all other options Elijah Newren
2018-06-10 19:26   ` Phillip Wood
2018-06-11 14:42     ` Elijah Newren
2018-06-11 15:16       ` Phillip Wood
2018-06-11 16:08         ` Elijah Newren
2018-06-12  9:53           ` Phillip Wood
2018-06-07  5:08 ` [PATCH 1/2] t3418: add testcase showing problems with rebase -i and strategy options Elijah Newren
2018-06-07  5:08   ` [PATCH 2/2] Fix use of strategy options with interactive rebases Elijah Newren
2018-06-07 17:13 ` [RFC PATCH 0/3] Send more rebases through interactive machinery Elijah Newren
2018-06-07 17:13   ` [RFC PATCH 1/3] git-rebase, sequencer: add a --quiet option for the " Elijah Newren
2018-06-09 21:45     ` Johannes Schindelin
2018-06-09 23:05       ` Elijah Newren
2018-06-07 17:13   ` [RFC PATCH 2/3] rebase: Implement --merge via git-rebase--interactive Elijah Newren
2018-06-09 22:04     ` Johannes Schindelin
2018-06-10  1:14       ` Elijah Newren
2018-06-11  9:54         ` Phillip Wood
2018-06-07 17:13   ` [RFC PATCH 3/3] git-rebase.sh: make git-rebase--interactive the default Elijah Newren
2018-06-09 22:11     ` Johannes Schindelin
2018-06-10  1:31       ` Elijah Newren
2018-06-17 21:44         ` Johannes Schindelin
2018-06-11 17:44 ` RFC: rebase inconsistency in 2.18 -- course of action? Junio C Hamano
2018-06-11 20:17   ` 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