git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / 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; 130+ 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] 130+ messages in thread

end of thread, other threads:[~2019-01-22 15:37 UTC | newest]

Thread overview: 130+ 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-25 16:17   ` 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-27  7:46   ` [PATCH v2] " Elijah Newren
2018-07-12 15:49     ` Johannes Schindelin
2018-07-13 16:13       ` Elijah Newren
2018-07-14 22:20         ` Johannes Schindelin
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-20 17:09         ` Elijah Newren
2018-06-17 17:15       ` Eric Sunshine
2018-06-20 17:10         ` 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     ` [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-20 16:47         ` Elijah Newren
2018-06-17 15:41     ` [RFC PATCH v2 0/7] Document/fix/warn about rebase incompatibilities and inconsistences Phillip Wood
2018-06-21 15:00     ` [PATCH v3 0/7] Document/fix/warn about rebase incompatibilities and inconsistencies Elijah Newren
2018-06-21 15:00       ` [PATCH v3 1/7] git-rebase.txt: document incompatible options Elijah Newren
2018-06-21 19:47         ` Junio C Hamano
2018-06-21 20:33           ` Elijah Newren
2018-06-22  8:32         ` SZEDER Gábor
2018-06-21 15:00       ` [PATCH v3 2/7] git-rebase.sh: update help messages a bit Elijah Newren
2018-06-21 19:58         ` Junio C Hamano
2018-06-21 20:34           ` Elijah Newren
2018-06-21 15:00       ` [PATCH v3 3/7] t3422: new testcases for checking when incompatible options passed Elijah Newren
2018-06-21 20:04         ` Junio C Hamano
2018-06-21 20:37           ` Elijah Newren
2018-06-21 21:18           ` Eric Sunshine
2018-06-21 15:00       ` [PATCH v3 4/7] git-rebase: error out " Elijah Newren
2018-06-21 20:29         ` Junio C Hamano
2018-06-21 20:41           ` Elijah Newren
2018-06-21 15:00       ` [PATCH v3 5/7] git-rebase.txt: document behavioral inconsistencies between modes Elijah Newren
2018-06-21 20:44         ` Junio C Hamano
2018-06-21 21:25           ` Elijah Newren
2018-06-21 15:00       ` [PATCH v3 6/7] git-rebase.txt: address confusion between --no-ff vs --force-rebase Elijah Newren
2018-06-21 20:46         ` Junio C Hamano
2018-06-21 21:22           ` Elijah Newren
2018-06-21 15:00       ` [RFC PATCH v3 7/7] git-rebase: make --allow-empty-message the default Elijah Newren
2018-06-25 16:12       ` [PATCH v4 0/9] Document/fix/warn about rebase incompatibilities and inconsistencies Elijah Newren
2018-06-25 16:12         ` [PATCH v4 1/9] git-rebase.txt: document incompatible options Elijah Newren
2018-06-25 16:12         ` [PATCH v4 2/9] git-rebase.sh: update help messages a bit Elijah Newren
2018-06-25 16:12         ` [PATCH v4 3/9] t3422: new testcases for checking when incompatible options passed Elijah Newren
2018-06-26 18:17           ` Junio C Hamano
2018-06-27  6:50             ` Elijah Newren
2018-06-25 16:12         ` [PATCH v4 4/9] git-rebase: error out " Elijah Newren
2018-06-25 16:12         ` [PATCH v4 5/9] git-rebase.txt: address confusion between --no-ff vs --force-rebase Elijah Newren
2018-06-25 16:12         ` [PATCH v4 6/9] directory-rename-detection.txt: technical docs on abilities and limitations Elijah Newren
2018-06-25 16:12         ` [PATCH v4 7/9] git-rebase.txt: document behavioral differences between modes Elijah Newren
2018-06-25 16:12         ` [PATCH v4 8/9] t3401: add directory rename testcases for rebase and am Elijah Newren
2018-06-25 16:13         ` [RFC PATCH v4 9/9] git-rebase: make --allow-empty-message the default Elijah Newren
2018-06-27  7:23         ` [PATCH v5 0/9] Document/fix/warn about rebase incompatibilities and inconsistencies Elijah Newren
2018-06-27  7:23           ` [PATCH v5 1/9] git-rebase.txt: document incompatible options Elijah Newren
2018-06-27  7:23           ` [PATCH v5 2/9] git-rebase.sh: update help messages a bit Elijah Newren
2018-06-27  7:23           ` [PATCH v5 3/9] t3422: new testcases for checking when incompatible options passed Elijah Newren
2018-06-27  7:23           ` [PATCH v5 4/9] git-rebase: error out " Elijah Newren
2018-06-27  7:23           ` [PATCH v5 5/9] git-rebase.txt: address confusion between --no-ff vs --force-rebase Elijah Newren
2018-06-27  7:23           ` [PATCH v5 6/9] directory-rename-detection.txt: technical docs on abilities and limitations Elijah Newren
2018-06-27  7:23           ` [PATCH v5 7/9] git-rebase.txt: document behavioral differences between modes Elijah Newren
2018-06-27  7:23           ` [PATCH v5 8/9] t3401: add directory rename testcases for rebase and am Elijah Newren
2018-06-27  7:23           ` [RFC PATCH v5 9/9] git-rebase: make --allow-empty-message the default Elijah Newren
2018-09-12  2:42             ` 2.19.0 regression: leaving editor with empty commit message doesn't stop rebase [was: Re: [RFC PATCH v5 9/9] git-rebase: make --allow-empty-message the default] SZEDER Gábor
2018-09-12  6:21               ` Elijah Newren
2018-09-12 21:18               ` [PATCH] sequencer: fix --allow-empty-message behavior, make it smarter Elijah Newren
2018-09-13 20:24                 ` Junio C Hamano
2018-06-29 13:20           ` [PATCH v5 0/9] Document/fix/warn about rebase incompatibilities and inconsistencies 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-27  7:36   ` [PATCH v2 0/2] " Elijah Newren
2018-06-27  7:36     ` [PATCH v2 1/2] t3418: add testcase showing problems with rebase -i and strategy options Elijah Newren
2018-06-27  7:45       ` Eric Sunshine
2018-06-27  7:49         ` Elijah Newren
2018-06-27 16:54           ` Junio C Hamano
2018-06-27 17:27             ` Elijah Newren
2018-06-27 18:17               ` Johannes Sixt
2018-06-27 18:24                 ` Eric Sunshine
2018-06-27 18:34                 ` [PATCH v2 1/2] t3418: add testcase showing problems with rebase SZEDER Gábor
2018-06-27 19:20                 ` [PATCH v2 1/2] t3418: add testcase showing problems with rebase -i and strategy options Junio C Hamano
2018-06-27 19:23               ` Junio C Hamano
2018-06-27  7:36     ` [PATCH v2 2/2] Fix use of strategy options with interactive rebases Elijah Newren
2018-06-27 15:48     ` [PATCH v3 0/2] " Elijah Newren
2018-06-27 15:48       ` [PATCH v3 1/2] t3418: add testcase showing problems with rebase -i and strategy options Elijah Newren
2018-06-27 18:28         ` SZEDER Gábor
2018-07-11 10:56           ` SZEDER Gábor
2018-07-11 19:12             ` Elijah Newren
2018-06-27 15:48       ` [PATCH v3 2/2] Fix use of strategy options with interactive rebases Elijah Newren
2018-07-12 15:41         ` Johannes Schindelin
2018-07-13 15:51           ` 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-20 16:27           ` Elijah Newren
2018-06-21 10:57             ` Johannes Schindelin
2018-06-21 15:36               ` Elijah Newren
2019-01-21 15:55                 ` Johannes Schindelin
2019-01-21 19:02                   ` Elijah Newren
2019-01-22 15:37                     ` 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

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).