git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH 00/30] Add directory rename detection to git
@ 2017-11-10 19:05 Elijah Newren
  2017-11-10 19:05 ` [PATCH 01/30] Tighten and correct a few testcases for merging and cherry-picking Elijah Newren
                   ` (30 more replies)
  0 siblings, 31 replies; 81+ messages in thread
From: Elijah Newren @ 2017-11-10 19:05 UTC (permalink / raw)
  To: git; +Cc: Elijah Newren

[This series is entirely independent of my rename detection limits series.
However, I have a separate rename detection performance series that depends
on both this series and the rename detection limits series.]

In this patchset, I introduce directory rename detection to merge-recursive,
predominantly so that when files are added to directories on one side of
history and those directories are renamed on the other side of history, the
files will end up in the proper location after a merge or cherry-pick.

However, this isn't limited to that simplistic case.  More interesting
possibilities exist, such as:

  * a file being renamed into a directory which is renamed on the other
    side of history, causing the need for a transitive rename.

  * two (or three or N) directories being merged (with no conflicts so
    long as files/directories within the merged directory have different
    names), and the "merging" being detected as a directory rename for
    each original directory.

  * not all files in a directory being renamed to the same location;
    i.e. perhaps the directory was renamed, but some files within it were
    renamed to a different location

  * a directory being renamed, which also contained a subdirectory that
    was renamed to some entirely different location.  (And perhaps the
    inner directory itself contained inner directories that were renamed
    to yet other locations).

Also, I found it useful to allow all files within the directory being
renamed to themselves be renamed and still detect the directory rename.
For example, if goal/a and goal/b are renamed to priority/alpha and
priority/bravo, we can detect that goal/ was renamed to priority/, so that
if someone adds goal/c on the other side of history, after the merge we'll
end up with priority/c.  (In the absence of a readily available
libmindread.so library that I can link to, we can't rename directly from
goal/c to priority/charlie automatically, and will need to have priority/c
suffice.)

Naturally, an attempt to do all of the above brings up all kinds of
interesting edge and corner cases, some of which result in conflicts
that cannot be represented in the index, and others of which might be
considered too complex for users to understand and resolve.  For
example:

  * An add/add/add/.../add conflict, all on one side of history (see
    testcase 9e in the new t6043, or any of the testcases in section 5)

  * Doubly, triply, or N-fold transitive renames (testcases 9c & 9d)

In order to prevent such problems, I introduce a couple basic rules that
limit when directory rename detection applies:

  1) If a subset of to-be-renamed files have a file or directory in the
     way (or would be in the way of each other), "turn off" the directory
     rename for those specific sub-paths and report the conflict to the
     user.

  2) If the other side of history did a directory rename to a path that
     your side of history renamed away, then ignore that particular
     rename from the other side of history for any implicit directory
     renames (but warn the user).

Further, there's a basic question about when directory rename detection
should be applied at all.  I have a simple rule:

  3) If a given directory still exists on both sides of a merge, we do
     not consider it to have been renamed.

Rule 3 may sound obvious at first, but it will probably arise as a
question for some users -- what if someone "mostly" moved a directory but
still left some files around, or, equivalently (from the perspective of the
three-way merge that merge-recursive performs), fully renamed a directory
in one commmit and then recreated that directory in a later commit adding
some new files and then tried to merge?  See the big comment in section 4
of the new t6043 for further discussion of this rule.

This set of rules seems to be reasonably easy to explain, is
self-consistent, allows all conflict cases to be represented without
changing any on-disk data structures or introducing new terminology or
commands for users, prevents excessively complex conflicts that users
might struggle to understand, and brings peace to the middle east.
Actually, maybe not that last one.

While I feel that this directory rename detection reduces the number of
suboptimal merges and cherry-picks that git performs, there are sadly
still a number of cases that remain suboptimal, or that even newly appear
to be not-quite-consistent with other cases.  The fact that one file
layout might trigger some of the rules above while another "slightly"
different file layout doesn't might occasionally cause some user
grumblings.  I've tried to explore and document these cases in section 8
of the new t6043-merge-rename-directories.sh

Finally, from an implementation perspective, there's another strong
advantage to the ruleset above: it means that any path to which we want
to apply an implicit directory rename will have a free and open spot
for us to move it into.  Thus, we can just adjust the diff_filepair
from an add or modify into a rename (or adjust a rename diff_filepair
to change the target a little more), and then let process_renames and
process_entry do all their magic.  That allows us to rely on all the
heavy testing already done for those code paths to handle a large
variety of edge and corner cases (e.g. D/F, rename/rename, criss-cross
merges, etc.)  The big trick is just making sure to do all the
necessary checks that we can apply directory rename detection, and then
fixing things up to put it in the expected format, with enough test
cases to make sure we actually got it into the right format.

Okay, the last paragraph had a small lie (though I didn't know that when
I originally wrote it): the fact that unpack_trees() aborts early if it
detects an untracked or dirty file would be overwritten by a merge, and
if not it immediately proceeds to start modifying the working tree before
passing control back to merge-recursive, causes some problems.  Not only
has it always made the code more complex, but the fact that
unpack_trees() doesn't understand renames means that it can't
appropriately abort early if a path involved in a rename has untracked
or dirty contents in the way of the merge.  But by the time we detect
renames, it's too late to abort early.  So we have to instead figure out
ways of emitting warnings messages and writing something sensible to the
working copy without overwriting any of their data.  This was a problem
before directory rename detection, but directory rename detection
increases the number of places where we have to worry about this.

Elijah Newren (30):
  Tighten and correct a few testcases for merging and cherry-picking
  merge-recursive: Fix logic ordering issue
  merge-recursive: Add explanation for src_entry and dst_entry

These three patches provide a few miscellaneous fixups that could be
submitted independent of this series, though the series partially
depends on the fixes in the first one, and the second fix becomes more
important with the rest of the changes in this series.

  directory rename detection: basic testcases
  directory rename detection: directory splitting testcases
  directory rename detection: testcases to avoid taking detection too
    far
  directory rename detection: partially renamed directory
    testcase/discussion
  directory rename detection: files/directories in the way of some
    renames
  directory rename detection: testcases checking which side did the
    rename
  directory rename detection: more involved edge/corner testcases
  directory rename detection: testcases exploring possibly suboptimal
    merges
  directory rename detection: miscellaneous testcases to complete
    coverage
  directory rename detection: tests for handling overwriting untracked
    files
  directory rename detection: tests for handling overwriting dirty files

These patches add testcases for directory rename detection, trying to
cover the space of possibilities as exhaustively as I can while trying
to avoid excessive overlap in testcases

  merge-recursive: Move the get_renames() function
  merge-recursive: Introduce new functions to handle rename logic
  merge-recursive: Fix leaks of allocated renames and diff_filepairs
  merge-recursive: Make !o->detect_rename codepath more obvious
  merge-recursive: Split out code for determining diff_filepairs

These four patches make small code reorganizations in preparation for
further changes, though they include some memory leak fixes.

  merge-recursive: Add a new hashmap for storing directory renames
  merge-recursive: Add get_directory_renames()
  merge-recursive: Check for directory level conflicts
  merge-recursive: Add a new hashmap for storing file collisions
  merge-recursive: Add computation of collisions due to dir rename &
    merging
  merge-recursive: Check for file level conflicts then get new name
  merge-recursive: When comparing files, don't include trees
  merge-recursive: Apply necessary modifications for directory renames

These eight patches implement the directory rename detection logic.
  
  merge-recursive: Avoid clobbering untracked files with directory
    renames
  merge-recursive: Fix overwriting dirty files involved in renames
  merge-recursive: Fix remaining directory rename + dirty overwrite
    cases

These last three deal with untracked and dirty file overwriting
headaches.  The middle patch in particular, isn't just a fix for
directory rename detection but fixes a bug in current versions of git
in overwriting dirty files that are involved in a rename.  That patch
could be backported and submitted independent of this series, but the
final patch depends heavily on it.

 merge-recursive.c                   | 1212 +++++++++++--
 merge-recursive.h                   |   17 +
 t/t3501-revert-cherry-pick.sh       |    5 +-
 t/t6043-merge-rename-directories.sh | 3277 +++++++++++++++++++++++++++++++++++
 t/t7607-merge-overwrite.sh          |    7 +-
 unpack-trees.c                      |    4 +-
 unpack-trees.h                      |    4 +
 7 files changed, 4413 insertions(+), 113 deletions(-)
 create mode 100755 t/t6043-merge-rename-directories.sh

-- 
2.15.0.5.g9567be9905

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

end of thread, other threads:[~2018-06-14 17:36 UTC | newest]

Thread overview: 81+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-10 19:05 [PATCH 00/30] Add directory rename detection to git Elijah Newren
2017-11-10 19:05 ` [PATCH 01/30] Tighten and correct a few testcases for merging and cherry-picking Elijah Newren
2017-11-13 19:32   ` Stefan Beller
2017-11-10 19:05 ` [PATCH 02/30] merge-recursive: Fix logic ordering issue Elijah Newren
2017-11-13 19:48   ` Stefan Beller
2017-11-13 22:04     ` Elijah Newren
2017-11-13 22:12       ` Stefan Beller
2017-11-13 23:39         ` Elijah Newren
2017-11-13 23:46           ` Stefan Beller
2017-11-10 19:05 ` [PATCH 03/30] merge-recursive: Add explanation for src_entry and dst_entry Elijah Newren
2017-11-13 21:06   ` Stefan Beller
2017-11-13 22:57     ` Elijah Newren
2017-11-13 23:11       ` Stefan Beller
2017-11-14  1:26   ` Junio C Hamano
2017-11-10 19:05 ` [PATCH 04/30] directory rename detection: basic testcases Elijah Newren
2017-11-13 22:04   ` Stefan Beller
2017-11-14  0:57     ` Elijah Newren
2017-11-14  1:21       ` Stefan Beller
2017-11-14  1:40         ` Elijah Newren
2017-11-14  2:03     ` Junio C Hamano
2017-11-10 19:05 ` [PATCH 05/30] directory rename detection: directory splitting testcases Elijah Newren
2017-11-13 23:20   ` Stefan Beller
2017-11-10 19:05 ` [PATCH 06/30] directory rename detection: testcases to avoid taking detection too far Elijah Newren
2017-11-13 23:25   ` Stefan Beller
2017-11-14  1:02     ` Elijah Newren
2017-11-10 19:05 ` [PATCH 07/30] directory rename detection: partially renamed directory testcase/discussion Elijah Newren
2017-11-14  0:07   ` Stefan Beller
2017-11-10 19:05 ` [PATCH 08/30] directory rename detection: files/directories in the way of some renames Elijah Newren
2017-11-14  0:15   ` Stefan Beller
2017-11-14  1:19     ` Elijah Newren
2017-11-10 19:05 ` [PATCH 09/30] directory rename detection: testcases checking which side did the rename Elijah Newren
2017-11-14  0:25   ` Stefan Beller
2017-11-14  1:30     ` Elijah Newren
2017-11-10 19:05 ` [PATCH 10/30] directory rename detection: more involved edge/corner testcases Elijah Newren
2017-11-14  0:42   ` Stefan Beller
2017-11-14 21:11     ` Elijah Newren
2017-11-14 22:47       ` Stefan Beller
2017-11-10 19:05 ` [PATCH 11/30] directory rename detection: testcases exploring possibly suboptimal merges Elijah Newren
2017-11-14 20:33   ` Stefan Beller
2017-11-14 21:42     ` Elijah Newren
2017-11-10 19:05 ` [PATCH 12/30] directory rename detection: miscellaneous testcases to complete coverage Elijah Newren
2017-11-15 20:03   ` Stefan Beller
2017-11-16 21:17     ` Elijah Newren
2017-11-10 19:05 ` [PATCH 13/30] directory rename detection: tests for handling overwriting untracked files Elijah Newren
2017-11-10 19:05 ` [PATCH 14/30] directory rename detection: tests for handling overwriting dirty files Elijah Newren
2017-11-10 19:05 ` [PATCH 15/30] merge-recursive: Move the get_renames() function Elijah Newren
2017-11-14  4:46   ` Junio C Hamano
2017-11-14 17:41     ` Elijah Newren
2017-11-15  1:20       ` Junio C Hamano
2017-11-10 19:05 ` [PATCH 16/30] merge-recursive: Introduce new functions to handle rename logic Elijah Newren
2017-11-14  4:56   ` Junio C Hamano
2017-11-14  5:14     ` Junio C Hamano
2017-11-14 18:24       ` Elijah Newren
2017-11-10 19:05 ` [PATCH 17/30] merge-recursive: Fix leaks of allocated renames and diff_filepairs Elijah Newren
2017-11-14  4:58   ` Junio C Hamano
2017-11-10 19:05 ` [PATCH 18/30] merge-recursive: Make !o->detect_rename codepath more obvious Elijah Newren
2017-11-10 19:05 ` [PATCH 19/30] merge-recursive: Split out code for determining diff_filepairs Elijah Newren
2017-11-14  5:20   ` Junio C Hamano
2017-11-10 19:05 ` [PATCH 20/30] merge-recursive: Add a new hashmap for storing directory renames Elijah Newren
2017-11-10 19:05 ` [PATCH 21/30] merge-recursive: Add get_directory_renames() Elijah Newren
2017-11-14  5:30   ` Junio C Hamano
2017-11-14 18:38     ` Elijah Newren
2017-11-10 19:05 ` [PATCH 22/30] merge-recursive: Check for directory level conflicts Elijah Newren
2017-11-10 19:05 ` [PATCH 23/30] merge-recursive: Add a new hashmap for storing file collisions Elijah Newren
2017-11-10 19:05 ` [PATCH 24/30] merge-recursive: Add computation of collisions due to dir rename & merging Elijah Newren
2018-06-10 10:56   ` René Scharfe
2018-06-10 11:03     ` René Scharfe
2018-06-10 20:44     ` Jeff King
2018-06-11 15:03     ` Elijah Newren
2018-06-14 17:36     ` Junio C Hamano
2017-11-10 19:05 ` [PATCH 25/30] merge-recursive: Check for file level conflicts then get new name Elijah Newren
2017-11-10 19:05 ` [PATCH 26/30] merge-recursive: When comparing files, don't include trees Elijah Newren
2017-11-10 19:05 ` [PATCH 27/30] merge-recursive: Apply necessary modifications for directory renames Elijah Newren
2017-11-15 20:23   ` Stefan Beller
2017-11-16  3:54     ` Elijah Newren
2017-11-10 19:05 ` [PATCH 28/30] merge-recursive: Avoid clobbering untracked files with " Elijah Newren
2017-11-10 19:05 ` [RFC PATCH 29/30] merge-recursive: Fix overwriting dirty files involved in renames Elijah Newren
2017-11-10 19:05 ` [PATCH 30/30] merge-recursive: Fix remaining directory rename + dirty overwrite cases Elijah Newren
2017-11-10 22:27 ` [PATCH 00/30] Add directory rename detection to git Philip Oakley
2017-11-10 23:26   ` Elijah Newren
2017-11-13 15:04     ` Philip Oakley

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