[PATCH v4 0/9] Document/fix/warn about rebase incompatibilities and inconsistencies

[Elijah Newren <newren@gmail.com>]

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.

Changes since v3 (range-diff included at the end):
  * Fix many small issues mentioned by Junio and SZEDER, including a fix
    suggested by Eric.
  * Add a new patch (#6) which introduces technical documentation on directory
    rename detection, so that...
  * The more technical portion of the docs from patch 7 (used to be patch 5
    in v3) can be moved from git-rebase.txt to the new
    technical/directory-rename-detection.txt, as suggested by Junio.
  * Also adds a new patch (#8) which adds testcases to demonstrate what the
    new technical doc discusses.

Items of particular note for reviewers:
  * I have left patch 9 (used to be patch 7) in RFC state, but
    expanded the commit message with an in-depth usability rationale
    for the change.
  * It sounded like Junio was slightly unclear about the intent of the
    wording in Patch 1.  Not sure if my answer (in email) was sufficient or
    if there are changes I should make to the patch.
  * On Patch 5 of v3 (which is now patch 7 of v4), Junio suggested a course
    of action to take with --keep-empty.  I counter-proposed with a
    --empty={drop,halt,keep} idea.  Is just deferring this issue to a later
    series okay, or are there changes I should make to the current series to
    handle this?  (If so, what changes should those be?)

Elijah Newren (9):
  git-rebase.txt: document incompatible options
  git-rebase.sh: update help messages a bit
  t3422: new testcases for checking when incompatible options passed
  git-rebase: error out when incompatible options passed
  git-rebase.txt: address confusion between --no-ff vs --force-rebase
  directory-rename-detection.txt: technical docs on abilities and
    limitations
  git-rebase.txt: document behavioral differences between modes
  t3401: add directory rename testcases for rebase and am
  git-rebase: make --allow-empty-message the default

 Documentation/git-rebase.txt                  | 135 ++++++++++++++----
 .../technical/directory-rename-detection.txt  | 115 +++++++++++++++
 git-rebase.sh                                 |  43 +++++-
 t/t3401-rebase-and-am-rename.sh               | 105 ++++++++++++++
 t/t3404-rebase-interactive.sh                 |   7 +-
 t/t3405-rebase-malformed.sh                   |  11 +-
 t/t3422-rebase-incompatible-options.sh        |  88 ++++++++++++
 7 files changed, 462 insertions(+), 42 deletions(-)
 create mode 100644 Documentation/technical/directory-rename-detection.txt
 create mode 100755 t/t3401-rebase-and-am-rename.sh
 create mode 100755 t/t3422-rebase-incompatible-options.sh

 1: 4cdf9130cc !  1: 3f454ebc5e git-rebase.txt: document incompatible options
    @@ -163,7 +163,7 @@
     +implementations:
     +
     + * one based on linkgit:git-am[1] (the default)
    -+ * one based on linkgit:git-merge-recursive[1] (merge backend)
    ++ * one based on git-merge-recursive (merge backend)
     + * one based on linkgit:git-cherry-pick[1] (interactive backend)
     +
     +Flags only understood by the am backend:
 2: e336f76c5e !  2: 31a5a071a6 git-rebase.sh: update help messages a bit
    @@ -6,6 +6,8 @@
         to make like things (e.g. strategy and strategy-option) be near each
         other.
     
    +    Signed-off-by: Elijah Newren <newren@gmail.com>
    +
     diff --git a/git-rebase.sh b/git-rebase.sh
     --- a/git-rebase.sh
     +++ b/git-rebase.sh
 3: 4ab38d8a5f !  3: 5a2b5eec79 t3422: new testcases for checking when incompatible options passed
    @@ -34,8 +34,7 @@
     +	git commit -m A &&
     +
     +	git checkout B &&
    -+	# This is indented with HT SP HT.
    -+	echo "	 	foo();" >>foo &&
    ++	echo "q qfoo();" | q_to_tab >>foo &&
     +	git add foo &&
     +	git commit -m B
     +'
    @@ -48,7 +47,7 @@
     +# being ignored.  Make sure rebase warns the user and aborts instead.
     +#
     +
    -+test_run_rebase () {
    ++test_rebase_am_only () {
     +	opt=$1
     +	shift
     +	test_expect_failure "$opt incompatible with --merge" "
    @@ -78,10 +77,10 @@
     +
     +}
     +
    -+test_run_rebase --whitespace=fix
    -+test_run_rebase --ignore-whitespace
    -+test_run_rebase --committer-date-is-author-date
    -+test_run_rebase -C4
    ++test_rebase_am_only --whitespace=fix
    ++test_rebase_am_only --ignore-whitespace
    ++test_rebase_am_only --committer-date-is-author-date
    ++test_rebase_am_only -C4
     +
     +test_expect_success '--preserve-merges incompatible with --signoff' '
     +	git checkout B^0 &&
 4: 5223954caf !  4: 1e1c83724a git-rebase: error out when incompatible options passed
    @@ -37,7 +37,8 @@
      fi
      
     +if test -n "$git_am_opt"; then
    -+	incompatible_opts=$(echo "$git_am_opt" | sed -e 's/ -q//')
    ++	incompatible_opts=$(echo " $git_am_opt " | \
    ++			    sed -e 's/ -q / /g' -e 's/^ \(.*\) $/\1/')
     +	if test -n "$interactive_rebase"
     +	then
     +		if test -n "$incompatible_opts"
    @@ -85,7 +86,7 @@
     --- a/t/t3422-rebase-incompatible-options.sh
     +++ b/t/t3422-rebase-incompatible-options.sh
     @@
    - test_run_rebase () {
    + test_rebase_am_only () {
      	opt=$1
      	shift
     -	test_expect_failure "$opt incompatible with --merge" "
 5: 96f7ba98bc < --: ---------- git-rebase.txt: document behavioral inconsistencies between modes
 6: 7bb7b380ac !  5: 51023269d3 git-rebase.txt: address confusion between --no-ff vs --force-rebase
    @@ -93,15 +93,3 @@
      INCOMPATIBLE OPTIONS
      --------------------
      
    -@@
    - 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
--: ---------- >  6: f017d45dd9 directory-rename-detection.txt: technical docs on abilities and limitations
--: ---------- >  7: 0a359df404 git-rebase.txt: document behavioral differences between modes
--: ---------- >  8: beaadceaef t3401: add directory rename testcases for rebase and am
 7: 3ed07548a6 !  9: 431b2c36d5 git-rebase: make --allow-empty-message the default
    @@ -2,13 +2,85 @@
     
         git-rebase: make --allow-empty-message the default
     
    -    All rebase backends should behave the same with empty commit messages, but
    -    currently do not.  am-based rebases already apply commits with an empty
    -    commit message without stopping or requiring the user to specify an extra
    -    flag.  Since am-based rebases are the default rebase type, and since it
    -    appears no one has ever requested a --no-allow-empty-message flag to
    -    change this behavior, make --allow-empty-message the default so that
    -    merge-based and interactive-based rebases will behave the same.
    +    rebase backends currently behave differently with empty commit messages,
    +    largely as a side-effect of the different underlying commands on which
    +    they are based.  am-based rebases apply commits with an empty commit
    +    message without stopping or requiring the user to specify an extra flag.
    +    (It is interesting to note that am-based rebases are the default rebase
    +    type, and no one has ever requested a --no-allow-empty-message flag to
    +    change this behavior.)  merge-based and interactive-based rebases (which
    +    are ultimately based on git-commit), will currently halt on any such
    +    commits and require the user to manually specify what to do with the
    +    commit and continue.
    +
    +    One possible rationale for the difference in behavior is that the purpose
    +    of an "am" based rebase is solely to transplant an existing history, while
    +    an "interactive" rebase is one whose purpose is to polish a series before
    +    making it publishable.  Thus, stopping and asking for confirmation for a
    +    possible problem is more appropriate in the latter case.  However, there
    +    are two problems with this rationale:
    +
    +      1) merge-based rebases are also non-interactive and there are multiple
    +         types of rebases that use the interactive machinery but are not
    +         explicitly interactive (e.g. when either --rebase-merges or
    +         --keep-empty are specified without --interactive).  These rebases are
    +         also used solely to transplant an existing history, and thus also
    +         should default to --allow-empty-message.
    +
    +      2) this rationale only says that the user is more accepting of stopping
    +         in the case of an explicitly interactive rebase, not that stopping
    +         for this particular reason actually makes sense.  Exploring whether
    +         it makes sense, requires backing up and analyzing the underlying
    +         commands...
    +
    +    If git-commit did not error out on empty commits by default, accidental
    +    creation of commits with empty messages would be a very common occurrence
    +    (this check has caught me many times).  Further, nearly all such empty
    +    commit messages would be considered an accidental error (as evidenced by a
    +    huge amount of documentation across version control systems and in various
    +    blog posts explaining how important commit messages are).  A simple check
    +    for what would otherwise be a common error thus made a lot of sense, and
    +    git-commit gained an --allow-empty-message flag for special case
    +    overrides.  This has made commits with empty messages very rare.
    +
    +    There are two sources for commits with empty messages for rebase (and
    +    cherry-pick): (a) commits created in git where the user previously
    +    specified --allow-empty-message to git-commit, and (b) commits imported
    +    into git from other version control systems.  In case (a), the user has
    +    already explicitly specified that there is something special about this
    +    commit that makes them not want to specify a commit message; forcing them
    +    to re-specify with every cherry-pick or rebase seems more likely to be
    +    infuriating than helpful.  In case (b), the commit is highly unlikely to
    +    have been authored by the person who has imported the history and is doing
    +    the rebase or cherry-pick, and thus the user is unlikely to be the
    +    appropriate person to write a commit message for it.  Stopping and
    +    expecting the user to modify the commit before proceeding thus seems
    +    counter-productive.
    +
    +    Further, note that while empty commit messages was a common error case for
    +    git-commit to deal with, it is a rare case for rebase (or cherry-pick).
    +    The fact that it is rare raises the question of why it would be worth
    +    checking and stopping on this particular condition and not others.  For
    +    example, why doesn't an interactive rebase automatically stop if the
    +    commit message's first line is 2000 columns long, or is missing a blank
    +    line after the first line, or has every line indented with five spaces, or
    +    any number of other myriad problems?
    +
    +    Finally, note that if a user doing an interactive rebase does have the
    +    necessary knowledge to add a message for any such commit and wants to do
    +    so, it is rather simple for them to change the appropriate line from
    +    'pick' to 'reword'.  The fact that the subject is empty in the todo list
    +    that the user edits should even serve as a way to notify them.
    +
    +    As far as I can tell, the fact that merge-based and interactive-based
    +    rebases stop on commits with empty commit messages is solely a by-product
    +    of having been based on git-commit.  It went without notice for a long
    +    time precisely because such cases are rare.  The rareness of this
    +    situation made it difficult to reason about, so when folks did eventually
    +    notice this behavior, they assumed it was there for a good reason and just
    +    added an --allow-empty-message flag.  In my opinion, stopping on such
    +    messages not desirable in any of these cases, even the (explicitly)
    +    interactive case.
     
         Signed-off-by: Elijah Newren <newren@gmail.com>
     
    @@ -16,7 +88,7 @@
     --- a/Documentation/git-rebase.txt
     +++ b/Documentation/git-rebase.txt
     @@
    -     The --keep-empty option exists for interactive rebases to allow
    +     The `--keep-empty` option exists for interactive rebases to allow
          it to keep commits that started empty.
      
     -  * empty commit messages:
    @@ -25,7 +97,7 @@
     -    messages.
     -
     -    merge-based and interactive-based rebases will by default halt
    --    on any such commits.  The --allow-empty-message option exists to
    +-    on any such commits.  The `--allow-empty-message` option exists to
     -    allow interactive-based rebases to apply such commits without
     -    halting.
     -

-- 
2.18.0.9.g678597d97e