git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 00/30] Add directory rename detection to git

From
Elijah Newren <newren@gmail.com>
Date
Nov 10, 2017, 19:05 UTC
Message-ID
<20171110190550.27059-1-newren@gmail.com>

[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
Next: Elijah Newren
Message 1 of 81 in “Add directory rename detection to git”
  1. 00/30 Add directory rename detection to gitElijah Newren, Nov 10, 2017
  2. 01/30 Tighten and correct a few testcases for merging and cherry-pickingElijah Newren, Nov 10, 2017
  3. Stefan BellerNov 13, 2017
  4. 24/30 merge-recursive: Add computation of collisions due to dir rename & mergingElijah Newren, Nov 10, 2017
  5. René ScharfeJun 10, 2018
  6. René ScharfeJun 10, 2018
  7. Jeff KingJun 10, 2018
  8. Elijah NewrenJun 11, 2018
  9. Junio C HamanoJun 14, 2018
  10. 23/30 merge-recursive: Add a new hashmap for storing file collisionsElijah Newren, Nov 10, 2017
  11. 11/30 directory rename detection: testcases exploring possibly suboptimal mergesElijah Newren, Nov 10, 2017
  12. Stefan BellerNov 14, 2017
  13. Elijah NewrenNov 14, 2017
  14. 16/30 merge-recursive: Introduce new functions to handle rename logicElijah Newren, Nov 10, 2017
  15. Junio C HamanoNov 14, 2017
  16. Junio C HamanoNov 14, 2017
  17. Elijah NewrenNov 14, 2017
  18. 09/30 directory rename detection: testcases checking which side did the renameElijah Newren, Nov 10, 2017
  19. Stefan BellerNov 14, 2017
  20. Elijah NewrenNov 14, 2017
  21. 06/30 directory rename detection: testcases to avoid taking detection too farElijah Newren, Nov 10, 2017
  22. Stefan BellerNov 13, 2017
  23. Elijah NewrenNov 14, 2017
  24. 08/30 directory rename detection: files/directories in the way of some renamesElijah Newren, Nov 10, 2017
  25. Stefan BellerNov 14, 2017
  26. Elijah NewrenNov 14, 2017
  27. 05/30 directory rename detection: directory splitting testcasesElijah Newren, Nov 10, 2017
  28. Stefan BellerNov 13, 2017
  29. 07/30 directory rename detection: partially renamed directory testcase/discussionElijah Newren, Nov 10, 2017
  30. Stefan BellerNov 14, 2017
  31. 26/30 merge-recursive: When comparing files, don't include treesElijah Newren, Nov 10, 2017
  32. 29/30 merge-recursive: Fix overwriting dirty files involved in renamesElijah Newren, Nov 10, 2017
  33. 12/30 directory rename detection: miscellaneous testcases to complete coverageElijah Newren, Nov 10, 2017
  34. Stefan BellerNov 15, 2017
  35. Elijah NewrenNov 16, 2017
  36. 10/30 directory rename detection: more involved edge/corner testcasesElijah Newren, Nov 10, 2017
  37. Stefan BellerNov 14, 2017
  38. Elijah NewrenNov 14, 2017
  39. Stefan BellerNov 14, 2017
  40. 14/30 directory rename detection: tests for handling overwriting dirty filesElijah Newren, Nov 10, 2017
  41. 15/30 merge-recursive: Move the get_renames() functionElijah Newren, Nov 10, 2017
  42. Junio C HamanoNov 14, 2017
  43. Elijah NewrenNov 14, 2017
  44. Junio C HamanoNov 15, 2017
  45. 22/30 merge-recursive: Check for directory level conflictsElijah Newren, Nov 10, 2017
  46. 19/30 merge-recursive: Split out code for determining diff_filepairsElijah Newren, Nov 10, 2017
  47. Junio C HamanoNov 14, 2017
  48. 13/30 directory rename detection: tests for handling overwriting untracked filesElijah Newren, Nov 10, 2017
  49. 27/30 merge-recursive: Apply necessary modifications for directory renamesElijah Newren, Nov 10, 2017
  50. Stefan BellerNov 15, 2017
  51. Elijah NewrenNov 16, 2017
  52. 18/30 merge-recursive: Make !o->detect_rename codepath more obviousElijah Newren, Nov 10, 2017
  53. 17/30 merge-recursive: Fix leaks of allocated renames and diff_filepairsElijah Newren, Nov 10, 2017
  54. Junio C HamanoNov 14, 2017
  55. 28/30 merge-recursive: Avoid clobbering untracked files with directory renamesElijah Newren, Nov 10, 2017
  56. 30/30 merge-recursive: Fix remaining directory rename + dirty overwrite casesElijah Newren, Nov 10, 2017
  57. 21/30 merge-recursive: Add get_directory_renames()Elijah Newren, Nov 10, 2017
  58. Junio C HamanoNov 14, 2017
  59. Elijah NewrenNov 14, 2017
  60. 25/30 merge-recursive: Check for file level conflicts then get new nameElijah Newren, Nov 10, 2017
  61. 20/30 merge-recursive: Add a new hashmap for storing directory renamesElijah Newren, Nov 10, 2017
  62. 02/30 merge-recursive: Fix logic ordering issueElijah Newren, Nov 10, 2017
  63. Stefan BellerNov 13, 2017
  64. Elijah NewrenNov 13, 2017
  65. Stefan BellerNov 13, 2017
  66. Elijah NewrenNov 13, 2017
  67. Stefan BellerNov 13, 2017
  68. 04/30 directory rename detection: basic testcasesElijah Newren, Nov 10, 2017
  69. Stefan BellerNov 13, 2017
  70. Elijah NewrenNov 14, 2017
  71. Stefan BellerNov 14, 2017
  72. Elijah NewrenNov 14, 2017
  73. Junio C HamanoNov 14, 2017
  74. 03/30 merge-recursive: Add explanation for src_entry and dst_entryElijah Newren, Nov 10, 2017
  75. Stefan BellerNov 13, 2017
  76. Elijah NewrenNov 13, 2017
  77. Stefan BellerNov 13, 2017
  78. Junio C HamanoNov 14, 2017
  79. Philip OakleyNov 10, 2017
  80. Elijah NewrenNov 10, 2017
  81. Philip OakleyNov 13, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.