git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Elijah Newren <newren@gmail.com>, Elijah Newren <newren@gmail.com>
Subject: [PATCH 2/4] t6423: more involved directory rename test
Date: Thu, 15 Oct 2020 20:46:28 +0000	[thread overview]
Message-ID: <c9637d70caed8a265096fa701833eca3ed52d0f3.1602794791.git.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.879.git.git.1602794790.gitgitgadget@gmail.com>

From: Elijah Newren <newren@gmail.com>

Add a new testcase modelled on a real world repository example that
served multiple purposes:
  * it uncovered a bug in the current directory rename detection
    implementation.
  * it is a good test of needing to do directory rename detection for
    a series of commits instead of just one (and uses rebase instead
    of just merge like all the other tests in this testfile).
  * it is an excellent stress test for some of the optimizations in
    my new merge-ort engine

I can expand on the final item later when I have submitted more of
merge-ort, but the bug is the main immediate concern.  It arises as
follows:

  * dir/subdir/ has several files
  * almost all files in dir/subdir/ are renamed to folder/subdir/
  * one of the files in dir/subdir/ is renamed to folder/subdir/newsubdir/
  * If the other side of history (that doesn't do the renames) adds a
    new file to dir/subdir/, where should it be placed after the merge?

The most obvious two choices are: (1) leave the new file in dir/subdir/,
don't make it follow the rename, and (2) move the new file to
folder/subdir/, following the rename of most the files.  However,
there's a possible third choice here: (3) move the new file to
folder/subdir/newsubdir/.  The choice reinforce the fact that
merge.directoryRenames=conflict is a good default, but when the merge
machinery needs to stick it somewhere and notify the user of the
possibility that they might want to place it elsewhere.  Surprisingly,
the current code would always choose (3), while the real world
repository was clearly expecting (2) -- move the file along with where
the herd of files was going, not with the special exception.

The problem here is that for the majority of the file renames,
   dir/subdir/ -> folder/subdir/
is actually represented as
   dir/ -> folder/
This directory rename would have a big weight associated with it since
most the files followed that rename.  However, we always consult the
most immediate directory first, and there is only one rename rule for
it:
   dir/subdir/ -> folder/subdir/newsubdir/
Since this rule is the only one for mapping from dir/subdir/, it
automatically wins and that directory rename was followed instead of the
desired dir/subdir/ -> folder/subdir/.

Unfortunately, the fix is a bit involved so for now just add the
testcase documenting the issue.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t6423-merge-rename-directories.sh | 195 ++++++++++++++++++++++++++++
 1 file changed, 195 insertions(+)

diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
index f7ecbb886d..31aea47522 100755
--- a/t/t6423-merge-rename-directories.sh
+++ b/t/t6423-merge-rename-directories.sh
@@ -4227,6 +4227,201 @@ test_expect_success '12e: Rename/merge subdir into the root, variant 2' '
 	)
 '
 
+# Testcase 12f, Rebase of patches with big directory rename
+#   Commit O:
+#              dir/subdir/{a,b,c,d,e_O,Makefile_TOP_O}
+#              dir/subdir/tweaked/{f,g,h,Makefile_SUB_O}
+#              dir/unchanged/<LOTS OF FILES>
+#   Commit A:
+#     (Remove f & g, move e into newsubdir, rename dir/->folder/, modify files)
+#              folder/subdir/{a,b,c,d,Makefile_TOP_A}
+#              folder/subdir/newsubdir/e_A
+#              folder/subdir/tweaked/{h,Makefile_SUB_A}
+#              folder/unchanged/<LOTS OF FILES>
+#   Commit B1:
+#     (add newfile.{c,py}, modify underscored files)
+#              dir/{a,b,c,d,e_B1,Makefile_TOP_B1,newfile.c}
+#              dir/tweaked/{f,g,h,Makefile_SUB_B1,newfile.py}
+#              dir/unchanged/<LOTS OF FILES>
+#   Commit B2:
+#     (Modify e further, add newfile.rs)
+#              dir/{a,b,c,d,e_B2,Makefile_TOP_B1,newfile.c,newfile.rs}
+#              dir/tweaked/{f,g,h,Makefile_SUB_B1,newfile.py}
+#              dir/unchanged/<LOTS OF FILES>
+#   Expected:
+#          B1-picked:
+#              folder/subdir/{a,b,c,d,Makefile_TOP_Merge1,newfile.c}
+#              folder/subdir/newsubdir/e_Merge1
+#              folder/subdir/tweaked/{h,Makefile_SUB_Merge1,newfile.py}
+#              folder/unchanged/<LOTS OF FILES>
+#          B2-picked:
+#              folder/subdir/{a,b,c,d,Makefile_TOP_Merge1,newfile.c,newfile.rs}
+#              folder/subdir/newsubdir/e_Merge2
+#              folder/subdir/tweaked/{h,Makefile_SUB_Merge1,newfile.py}
+#              folder/unchanged/<LOTS OF FILES>
+#
+# Notes: This testcase happens to exercise lots of the
+#        optimization-specific codepaths in merge-ort, and also
+#        demonstrated a failing of the directory rename detection algorithm
+#        in merge-recursive; newfile.c should not get pushed into
+#        folder/subdir/newsubdir/, yet merge-recursive put it there because
+#        the rename of dir/subdir/{a,b,c,d} -> folder/subdir/{a,b,c,d}
+#        looks like
+#            dir/ -> folder/,
+#        whereas the rename of dir/subdir/e -> folder/subdir/newsubdir/e
+#        looks like
+#            dir/subdir/ -> folder/subdir/newsubdir/
+#        and if we note that newfile.c is found in dir/subdir/, we might
+#        overlook the dir/ -> folder/ rule that has more weight.
+
+test_setup_12f () {
+	test_create_repo 12f &&
+	(
+		cd 12f &&
+
+		mkdir -p dir/unchanged &&
+		mkdir -p dir/subdir/tweaked &&
+		echo a >dir/subdir/a &&
+		echo b >dir/subdir/b &&
+		echo c >dir/subdir/c &&
+		echo d >dir/subdir/d &&
+		test_seq 1 10 >dir/subdir/e &&
+		test_seq 10 20 >dir/subdir/Makefile &&
+		echo f >dir/subdir/tweaked/f &&
+		echo g >dir/subdir/tweaked/g &&
+		echo h >dir/subdir/tweaked/h &&
+		test_seq 20 30 >dir/subdir/tweaked/Makefile &&
+		for i in `test_seq 1 88`; do
+			echo content $i >dir/unchanged/file_$i
+		done &&
+		git add . &&
+		git commit -m "O" &&
+
+		git branch O &&
+		git branch A &&
+		git branch B &&
+
+		git switch A &&
+		git rm dir/subdir/tweaked/f dir/subdir/tweaked/g &&
+		test_seq 2 10 >dir/subdir/e &&
+		test_seq 11 20 >dir/subdir/Makefile &&
+		test_seq 21 30 >dir/subdir/tweaked/Makefile &&
+		mkdir dir/subdir/newsubdir &&
+		git mv dir/subdir/e dir/subdir/newsubdir/ &&
+		git mv dir folder &&
+		git add . &&
+		git commit -m "A" &&
+
+		git switch B &&
+		mkdir dir/subdir/newsubdir/ &&
+		echo c code >dir/subdir/newfile.c &&
+		echo python code >dir/subdir/newsubdir/newfile.py &&
+		test_seq 1 11 >dir/subdir/e &&
+		test_seq 10 21 >dir/subdir/Makefile &&
+		test_seq 20 31 >dir/subdir/tweaked/Makefile &&
+		git add . &&
+		git commit -m "B1" &&
+
+		echo rust code >dir/subdir/newfile.rs &&
+		test_seq 1 12 >dir/subdir/e &&
+		git add . &&
+		git commit -m "B2"
+	)
+}
+
+test_expect_failure '12f: Trivial directory resolve, caching, all kinds of fun' '
+	test_setup_12f &&
+	(
+		cd 12f &&
+
+		git checkout A^0 &&
+		git branch Bmod B &&
+
+		git -c merge.directoryRenames=true rebase A Bmod &&
+
+		echo Checking the pick of B1... &&
+
+		test_must_fail git rev-parse Bmod~1:dir &&
+
+		git ls-tree -r Bmod~1 >out &&
+		test_line_count = 98 out &&
+
+		git diff --name-status A Bmod~1 >actual &&
+		q_to_tab >expect <<-\EOF &&
+		MQfolder/subdir/Makefile
+		AQfolder/subdir/newfile.c
+		MQfolder/subdir/newsubdir/e
+		AQfolder/subdir/newsubdir/newfile.py
+		MQfolder/subdir/tweaked/Makefile
+		EOF
+		test_cmp expect actual &&
+
+		# Three-way merged files
+		test_seq  2 11 >e_Merge1 &&
+		test_seq 11 21 >Makefile_TOP &&
+		test_seq 21 31 >Makefile_SUB &&
+		git hash-object >expect      \
+			e_Merge1             \
+			Makefile_TOP         \
+			Makefile_SUB         &&
+		git rev-parse >actual              \
+			Bmod~1:folder/subdir/newsubdir/e     \
+			Bmod~1:folder/subdir/Makefile        \
+			Bmod~1:folder/subdir/tweaked/Makefile &&
+		test_cmp expect actual &&
+
+		# New files showed up at the right location with right contents
+		git rev-parse >expect                \
+			B~1:dir/subdir/newfile.c            \
+			B~1:dir/subdir/newsubdir/newfile.py &&
+		git rev-parse >actual                      \
+			Bmod~1:folder/subdir/newfile.c            \
+			Bmod~1:folder/subdir/newsubdir/newfile.py &&
+		test_cmp expect actual &&
+
+		# Removed files
+		test_path_is_missing folder/subdir/tweaked/f &&
+		test_path_is_missing folder/subdir/tweaked/g &&
+
+		# Unchanged files or directories
+		git rev-parse >actual        \
+			Bmod~1:folder/subdir/a          \
+			Bmod~1:folder/subdir/b          \
+			Bmod~1:folder/subdir/c          \
+			Bmod~1:folder/subdir/d          \
+			Bmod~1:folder/unchanged         \
+			Bmod~1:folder/subdir/tweaked/h &&
+		git rev-parse >expect          \
+			O:dir/subdir/a         \
+			O:dir/subdir/b         \
+			O:dir/subdir/c         \
+			O:dir/subdir/d         \
+			O:dir/unchanged        \
+			O:dir/subdir/tweaked/h &&
+		test_cmp expect actual &&
+
+		echo Checking the pick of B2... &&
+
+		test_must_fail git rev-parse Bmod:dir &&
+
+		git ls-tree -r Bmod >out &&
+		test_line_count = 99 out &&
+
+		git diff --name-status Bmod~1 Bmod >actual &&
+		q_to_tab >expect <<-\EOF &&
+		AQfolder/subdir/newfile.rs
+		MQfolder/subdir/newsubdir/e
+		EOF
+		test_cmp expect actual &&
+
+		# Three-way merged file
+		test_seq  2 12 >e_Merge2 &&
+		git hash-object e_Merge2 >expect &&
+		git rev-parse Bmod:folder/subdir/newsubdir/e >actual &&
+		test_cmp expect actual
+	)
+'
+
 ###########################################################################
 # SECTION 13: Checking informational and conflict messages
 #
-- 
gitgitgadget


  parent reply	other threads:[~2020-10-15 20:46 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-15 20:46 [PATCH 0/4] Directory rename detection testcases and rules Elijah Newren via GitGitGadget
2020-10-15 20:46 ` [PATCH 1/4] directory-rename-detection.txt: update references to regression tests Elijah Newren via GitGitGadget
2020-10-15 20:46 ` Elijah Newren via GitGitGadget [this message]
2020-10-15 20:57   ` [PATCH 2/4] t6423: more involved directory rename test Eric Sunshine
2020-10-15 20:46 ` [PATCH 3/4] t6423: update directory rename detection tests with new rule Elijah Newren via GitGitGadget
2020-10-15 20:46 ` [PATCH 4/4] t6423: more involved rules for renaming directories into each other Elijah Newren via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c9637d70caed8a265096fa701833eca3ed52d0f3.1602794791.git.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=newren@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).