[PATCH 0/2] Dir rename fixes

[PATCH 0/2] Dir rename fixes

[Elijah Newren via GitGitGadget]

This series improves a couple things found after looking into things Dscho
flagged:

 * clarify and slightly restructure code in the get_renamed_dir_portion()
   function
 * extend support of detecting renaming/merging of one directory into
   another to support the root directory as a target directory

First patch best viewed with a --histogram diff, which I sadly don't know
how to make gitgitgadget generate.

Elijah Newren (2):
  merge-recursive: clean up get_renamed_dir_portion()
  merge-recursive: fix merging a subdirectory into the root directory

 merge-recursive.c                   | 89 +++++++++++++++++++++--------
 t/t6043-merge-rename-directories.sh | 56 ++++++++++++++++++
 2 files changed, 121 insertions(+), 24 deletions(-)


base-commit: 08da6496b61341ec45eac36afcc8f94242763468
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-390%2Fnewren%2Fdir-rename-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-390/newren/dir-rename-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/390
-- 
gitgitgadget

[PATCH 1/2] merge-recursive: clean up get_renamed_dir_portion()

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

Dscho noted a few things making this function hard to follow.
Restructure it a bit and add comments to make it easier to follow.  The
restructurings include:

  * There was a special case if-check at the end of the function
    checking whether someone just renamed a file within its original
    directory, meaning that there could be no directory rename involved.
    That check was slightly convoluted; it could be done in a more
    straightforward fashion earlier in the function, and can be done
    more cheaply too (no call to strncmp).

  * The conditions for advancing end_of_old and end_of_new before
    calling strchr were both confusing and unnecessary.  If either
    points at a '/', then they need to be advanced in order to find the
    next '/'.  If either doesn't point at a '/', then advancing them one
    char before calling strchr() doesn't hurt.  So, just rip out the
    if conditions and advance both before calling strchr().

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c | 60 ++++++++++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 24 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 22a12cfeba..f80e48f623 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1943,8 +1943,8 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path,
 				    char **old_dir, char **new_dir)
 {
 	char *end_of_old, *end_of_new;
-	int old_len, new_len;
 
+	/* Default return values: NULL, meaning no rename */
 	*old_dir = NULL;
 	*new_dir = NULL;
 
@@ -1955,43 +1955,55 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path,
 	 *    "a/b/c/d" was renamed to "a/b/some/thing/else"
 	 * so, for this example, this function returns "a/b/c/d" in
 	 * *old_dir and "a/b/some/thing/else" in *new_dir.
-	 *
-	 * Also, if the basename of the file changed, we don't care.  We
-	 * want to know which portion of the directory, if any, changed.
+	 */
+
+	/*
+	 * If the basename of the file changed, we don't care.  We want
+	 * to know which portion of the directory, if any, changed.
 	 */
 	end_of_old = strrchr(old_path, '/');
 	end_of_new = strrchr(new_path, '/');
-
 	if (end_of_old == NULL || end_of_new == NULL)
-		return;
+		return; /* We haven't modified *old_dir or *new_dir yet. */
+
+	/* Find the first non-matching character traversing backwards */
 	while (*--end_of_new == *--end_of_old &&
 	       end_of_old != old_path &&
 	       end_of_new != new_path)
 		; /* Do nothing; all in the while loop */
+
 	/*
-	 * We've found the first non-matching character in the directory
-	 * paths.  That means the current directory we were comparing
-	 * represents the rename.  Move end_of_old and end_of_new back
-	 * to the full directory name.
+	 * If both got back to the beginning of their strings, then the
+	 * directory didn't change at all, only the basename did.
 	 */
-	if (*end_of_old == '/')
-		end_of_old++;
-	if (*end_of_old != '/')
-		end_of_new++;
-	end_of_old = strchr(end_of_old, '/');
-	end_of_new = strchr(end_of_new, '/');
+	if (end_of_old == old_path && end_of_new == new_path &&
+	    *end_of_old == *end_of_new)
+		return; /* We haven't modified *old_dir or *new_dir yet. */
 
 	/*
-	 * It may have been the case that old_path and new_path were the same
-	 * directory all along.  Don't claim a rename if they're the same.
+	 * We've found the first non-matching character in the directory
+	 * paths.  That means the current characters we were looking at
+	 * were part of the first non-matching subdir name going back from
+	 * the end of the strings.  Get the whole name by advancing both
+	 * end_of_old and end_of_new to the NEXT '/' character.  That will
+	 * represent the entire directory rename.
+	 *
+	 * The reason for the increment is cases like
+	 *    a/b/star/foo/whatever.c -> a/b/tar/foo/random.c
+	 * After dropping the basename and going back to the first
+	 * non-matching character, we're now comparing:
+	 *    a/b/s          and         a/b/
+	 * and we want to be comparing:
+	 *    a/b/star/      and         a/b/tar/
+	 * but without the pre-increment, the one on the right would stay
+	 * a/b/.
 	 */
-	old_len = end_of_old - old_path;
-	new_len = end_of_new - new_path;
+	end_of_old = strchr(++end_of_old, '/');
+	end_of_new = strchr(++end_of_new, '/');
 
-	if (old_len != new_len || strncmp(old_path, new_path, old_len)) {
-		*old_dir = xstrndup(old_path, old_len);
-		*new_dir = xstrndup(new_path, new_len);
-	}
+	/* Copy the old and new directories into *old_dir and *new_dir. */
+	*old_dir = xstrndup(old_path, end_of_old - old_path);
+	*new_dir = xstrndup(new_path, end_of_new - new_path);
 }
 
 static void remove_hashmap_entries(struct hashmap *dir_renames,
-- 
gitgitgadget

[PATCH 2/2] merge-recursive: fix merging a subdirectory into the root directory

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

We allow renaming all entries in e.g. a directory named z/ into a
directory named y/ to be detected as a z/ -> y/ rename, so that if the
other side of history adds any files to the directory z/ in the mean
time, we can provide the hint that they should be moved to y/.

There is no reason to not allow 'y/' to be the root directory, but the
code did not handle that case correctly.  Add a testcase and the
necessary special checks to support this case.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c                   | 29 +++++++++++++++
 t/t6043-merge-rename-directories.sh | 56 +++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+)

diff --git a/merge-recursive.c b/merge-recursive.c
index f80e48f623..7bd4a7cf10 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1931,6 +1931,16 @@ static char *apply_dir_rename(struct dir_rename_entry *entry,
 		return NULL;
 
 	oldlen = strlen(entry->dir);
+	if (entry->new_dir.len == 0)
+		/*
+		 * If someone renamed/merged a subdirectory into the root
+		 * directory (e.g. 'some/subdir' -> ''), then we want to
+		 * avoid returning
+		 *     '' + '/filename'
+		 * as the rename; we need to make old_path + oldlen advance
+		 * past the '/' character.
+		 */
+		oldlen++;
 	newlen = entry->new_dir.len + (strlen(old_path) - oldlen) + 1;
 	strbuf_grow(&new_path, newlen);
 	strbuf_addbuf(&new_path, &entry->new_dir);
@@ -1980,6 +1990,25 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path,
 	    *end_of_old == *end_of_new)
 		return; /* We haven't modified *old_dir or *new_dir yet. */
 
+	/*
+	 * If end_of_new got back to the beginning of its string, and
+	 * end_of_old got back to the beginning of some subdirectory, then
+	 * we have a rename/merge of a subdirectory into the root, which
+	 * needs slightly special handling.
+	 *
+	 * Note: There is no need to consider the opposite case, with a
+	 * rename/merge of the root directory into some subdirectory.
+	 * Our rule elsewhere that a directory which still exists is not
+	 * considered to have been renamed means the root directory can
+	 * never be renamed (because the root directory always exists).
+	 */
+	if (end_of_new == new_path &&
+	    end_of_old != old_path && end_of_old[-1] == '/') {
+		*old_dir = xstrndup(old_path, end_of_old-1 - old_path);
+		*new_dir = xstrndup(new_path, end_of_new - new_path);
+		return;
+	}
+
 	/*
 	 * We've found the first non-matching character in the directory
 	 * paths.  That means the current characters we were looking at
diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
index c966147d5d..b920bb0850 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -4051,6 +4051,62 @@ test_expect_success '12c-check: Moving one directory hierarchy into another w/ c
 	)
 '
 
+# Testcase 12d, Rename/merge of subdirectory into the root
+#   Commit O: a/b/{foo.c}
+#   Commit A: foo.c
+#   Commit B: a/b/{foo.c,bar.c}
+#   Expected: a/b/{foo.c,bar.c}
+
+test_expect_success '12d-setup: Rename (merge) of subdirectory into the root' '
+	test_create_repo 12d &&
+	(
+		cd 12d &&
+
+		mkdir -p a/b/subdir &&
+		test_commit a/b/subdir/foo.c &&
+
+		git branch O &&
+		git branch A &&
+		git branch B &&
+
+		git checkout A &&
+		mkdir subdir &&
+		git mv a/b/subdir/foo.c.t subdir/foo.c.t &&
+		test_tick &&
+		git commit -m "A" &&
+
+		git checkout B &&
+		test_commit a/b/bar.c
+	)
+'
+
+test_expect_success '12d-check: Rename (merge) of subdirectory into the root' '
+	(
+		cd 12d &&
+
+		git checkout A^0 &&
+
+		git -c merge.directoryRenames=true merge -s recursive B^0 &&
+
+		git ls-files -s >out &&
+		test_line_count = 2 out &&
+
+		git rev-parse >actual \
+			HEAD:subdir/foo.c.t   HEAD:bar.c.t &&
+		git rev-parse >expect \
+			O:a/b/subdir/foo.c.t  B:a/b/bar.c.t &&
+		test_cmp expect actual &&
+
+		git hash-object bar.c.t >actual &&
+		git rev-parse B:a/b/bar.c.t >expect &&
+		test_cmp expect actual &&
+
+		test_must_fail git rev-parse HEAD:a/b/subdir/foo.c.t &&
+		test_must_fail git rev-parse HEAD:a/b/bar.c.t &&
+		test_path_is_missing a/
+	)
+'
+
 ###########################################################################
 # SECTION 13: Checking informational and conflict messages
 #
-- 
gitgitgadget

Re: [PATCH 0/2] Dir rename fixes

[Johannes Schindelin]

Hi Elijah,

On Fri, 11 Oct 2019, Elijah Newren via GitGitGadget wrote:

> This series improves a couple things found after looking into things Dscho
> flagged:
>
>  * clarify and slightly restructure code in the get_renamed_dir_portion()
>    function
>
>  * extend support of detecting renaming/merging of one directory into
>    another to support the root directory as a target directory

Will have a look in a moment,

> First patch best viewed with a --histogram diff, which I sadly don't know
> how to make gitgitgadget generate.

Currently, there is no way to do that yet. It should not be too
difficult to implement code to pick up a footer in the PR description to
do that, I just don't have the time right now to do so.

Essentially, it would have to imitate the already-existing code path
that picks up the `Cc:` footer in the PR description.

If you're interested, have a look at the `parsePullRequestDescription()`
function in `lib/patch-series.ts` in
https://github.com/gitgitgadget/gitgitgadget/.

Ciao,
Dscho

> Elijah Newren (2):
>   merge-recursive: clean up get_renamed_dir_portion()
>   merge-recursive: fix merging a subdirectory into the root directory
>
>  merge-recursive.c                   | 89 +++++++++++++++++++++--------
>  t/t6043-merge-rename-directories.sh | 56 ++++++++++++++++++
>  2 files changed, 121 insertions(+), 24 deletions(-)
>
>
> base-commit: 08da6496b61341ec45eac36afcc8f94242763468
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-390%2Fnewren%2Fdir-rename-fixes-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-390/newren/dir-rename-fixes-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/390
> --
> gitgitgadget
>

Re: [PATCH 1/2] merge-recursive: clean up get_renamed_dir_portion()

[Johannes Schindelin]

Hi Elijah,

On Fri, 11 Oct 2019, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>
>
> Dscho noted a few things making this function hard to follow.
> Restructure it a bit and add comments to make it easier to follow.  The
> restructurings include:
>
>   * There was a special case if-check at the end of the function
>     checking whether someone just renamed a file within its original
>     directory, meaning that there could be no directory rename involved.
>     That check was slightly convoluted; it could be done in a more
>     straightforward fashion earlier in the function, and can be done
>     more cheaply too (no call to strncmp).
>
>   * The conditions for advancing end_of_old and end_of_new before
>     calling strchr were both confusing and unnecessary.  If either
>     points at a '/', then they need to be advanced in order to find the
>     next '/'.  If either doesn't point at a '/', then advancing them one
>     char before calling strchr() doesn't hurt.  So, just rip out the
>     if conditions and advance both before calling strchr().
>
> Signed-off-by: Elijah Newren <newren@gmail.com>

This commit message, as well as the patch, make a lot of sense to me.
Thank you for doing this!

Ciao,
Dscho

> ---
>  merge-recursive.c | 60 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 36 insertions(+), 24 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 22a12cfeba..f80e48f623 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1943,8 +1943,8 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path,
>  				    char **old_dir, char **new_dir)
>  {
>  	char *end_of_old, *end_of_new;
> -	int old_len, new_len;
>
> +	/* Default return values: NULL, meaning no rename */
>  	*old_dir = NULL;
>  	*new_dir = NULL;
>
> @@ -1955,43 +1955,55 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path,
>  	 *    "a/b/c/d" was renamed to "a/b/some/thing/else"
>  	 * so, for this example, this function returns "a/b/c/d" in
>  	 * *old_dir and "a/b/some/thing/else" in *new_dir.
> -	 *
> -	 * Also, if the basename of the file changed, we don't care.  We
> -	 * want to know which portion of the directory, if any, changed.
> +	 */
> +
> +	/*
> +	 * If the basename of the file changed, we don't care.  We want
> +	 * to know which portion of the directory, if any, changed.
>  	 */
>  	end_of_old = strrchr(old_path, '/');
>  	end_of_new = strrchr(new_path, '/');
> -
>  	if (end_of_old == NULL || end_of_new == NULL)
> -		return;
> +		return; /* We haven't modified *old_dir or *new_dir yet. */
> +
> +	/* Find the first non-matching character traversing backwards */
>  	while (*--end_of_new == *--end_of_old &&
>  	       end_of_old != old_path &&
>  	       end_of_new != new_path)
>  		; /* Do nothing; all in the while loop */
> +
>  	/*
> -	 * We've found the first non-matching character in the directory
> -	 * paths.  That means the current directory we were comparing
> -	 * represents the rename.  Move end_of_old and end_of_new back
> -	 * to the full directory name.
> +	 * If both got back to the beginning of their strings, then the
> +	 * directory didn't change at all, only the basename did.
>  	 */
> -	if (*end_of_old == '/')
> -		end_of_old++;
> -	if (*end_of_old != '/')
> -		end_of_new++;
> -	end_of_old = strchr(end_of_old, '/');
> -	end_of_new = strchr(end_of_new, '/');
> +	if (end_of_old == old_path && end_of_new == new_path &&
> +	    *end_of_old == *end_of_new)
> +		return; /* We haven't modified *old_dir or *new_dir yet. */
>
>  	/*
> -	 * It may have been the case that old_path and new_path were the same
> -	 * directory all along.  Don't claim a rename if they're the same.
> +	 * We've found the first non-matching character in the directory
> +	 * paths.  That means the current characters we were looking at
> +	 * were part of the first non-matching subdir name going back from
> +	 * the end of the strings.  Get the whole name by advancing both
> +	 * end_of_old and end_of_new to the NEXT '/' character.  That will
> +	 * represent the entire directory rename.
> +	 *
> +	 * The reason for the increment is cases like
> +	 *    a/b/star/foo/whatever.c -> a/b/tar/foo/random.c
> +	 * After dropping the basename and going back to the first
> +	 * non-matching character, we're now comparing:
> +	 *    a/b/s          and         a/b/
> +	 * and we want to be comparing:
> +	 *    a/b/star/      and         a/b/tar/
> +	 * but without the pre-increment, the one on the right would stay
> +	 * a/b/.
>  	 */
> -	old_len = end_of_old - old_path;
> -	new_len = end_of_new - new_path;
> +	end_of_old = strchr(++end_of_old, '/');
> +	end_of_new = strchr(++end_of_new, '/');
>
> -	if (old_len != new_len || strncmp(old_path, new_path, old_len)) {
> -		*old_dir = xstrndup(old_path, old_len);
> -		*new_dir = xstrndup(new_path, new_len);
> -	}
> +	/* Copy the old and new directories into *old_dir and *new_dir. */
> +	*old_dir = xstrndup(old_path, end_of_old - old_path);
> +	*new_dir = xstrndup(new_path, end_of_new - new_path);
>  }
>
>  static void remove_hashmap_entries(struct hashmap *dir_renames,
> --
> gitgitgadget
>
>

Re: [PATCH 2/2] merge-recursive: fix merging a subdirectory into the root directory

[Johannes Schindelin]

Hi Elijah,

On Fri, 11 Oct 2019, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>
>
> We allow renaming all entries in e.g. a directory named z/ into a
> directory named y/ to be detected as a z/ -> y/ rename, so that if the
> other side of history adds any files to the directory z/ in the mean
> time, we can provide the hint that they should be moved to y/.
>
> There is no reason to not allow 'y/' to be the root directory, but the
> code did not handle that case correctly.  Add a testcase and the
> necessary special checks to support this case.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>

This makes sense.

> ---
>  merge-recursive.c                   | 29 +++++++++++++++
>  t/t6043-merge-rename-directories.sh | 56 +++++++++++++++++++++++++++++

It is good to have a test case verifying this!

FWIW I frequently run into those same issues because I have to use --
quite often! -- `contrib/fast-import/import-tars.perl` in the Git for
Windows project (in the MSYS2 part thereof, to be precise), and the
`pax_global_header` and I will probably never become friends, so I often
have to move files into the top-level directory...

> diff --git a/merge-recursive.c b/merge-recursive.c
> index f80e48f623..7bd4a7cf10 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1931,6 +1931,16 @@ static char *apply_dir_rename(struct dir_rename_entry *entry,
>  		return NULL;
>
>  	oldlen = strlen(entry->dir);
> +	if (entry->new_dir.len == 0)
> +		/*
> +		 * If someone renamed/merged a subdirectory into the root
> +		 * directory (e.g. 'some/subdir' -> ''), then we want to
> +		 * avoid returning
> +		 *     '' + '/filename'
> +		 * as the rename; we need to make old_path + oldlen advance
> +		 * past the '/' character.
> +		 */
> +		oldlen++;

This makes sense to me.

>  	newlen = entry->new_dir.len + (strlen(old_path) - oldlen) + 1;
>  	strbuf_grow(&new_path, newlen);
>  	strbuf_addbuf(&new_path, &entry->new_dir);
> @@ -1980,6 +1990,25 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path,
>  	    *end_of_old == *end_of_new)
>  		return; /* We haven't modified *old_dir or *new_dir yet. */
>
> +	/*
> +	 * If end_of_new got back to the beginning of its string, and
> +	 * end_of_old got back to the beginning of some subdirectory, then
> +	 * we have a rename/merge of a subdirectory into the root, which
> +	 * needs slightly special handling.
> +	 *
> +	 * Note: There is no need to consider the opposite case, with a
> +	 * rename/merge of the root directory into some subdirectory.
> +	 * Our rule elsewhere that a directory which still exists is not
> +	 * considered to have been renamed means the root directory can
> +	 * never be renamed (because the root directory always exists).
> +	 */
> +	if (end_of_new == new_path &&
> +	    end_of_old != old_path && end_of_old[-1] == '/') {
> +		*old_dir = xstrndup(old_path, end_of_old-1 - old_path);
> +		*new_dir = xstrndup(new_path, end_of_new - new_path);

However, here we write something convoluted that essentially amounts to
`xstrdup("")`. I would rather have that simple call than the convoluted
one that would puzzle me every time I have to look at this part of the
code.

While at it, would you mind either surrounding the `-` and the `1` by
spaces, or even write `--end_of_old - old_path`?

> +		return;
> +	}
> +
>  	/*
>  	 * We've found the first non-matching character in the directory
>  	 * paths.  That means the current characters we were looking at
> diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
> index c966147d5d..b920bb0850 100755
> --- a/t/t6043-merge-rename-directories.sh
> +++ b/t/t6043-merge-rename-directories.sh
> @@ -4051,6 +4051,62 @@ test_expect_success '12c-check: Moving one directory hierarchy into another w/ c
>  	)
>  '
>
> +# Testcase 12d, Rename/merge of subdirectory into the root
> +#   Commit O: a/b/{foo.c}
> +#   Commit A: foo.c
> +#   Commit B: a/b/{foo.c,bar.c}
> +#   Expected: a/b/{foo.c,bar.c}
> +
> +test_expect_success '12d-setup: Rename (merge) of subdirectory into the root' '
> +	test_create_repo 12d &&
> +	(
> +		cd 12d &&
> +
> +		mkdir -p a/b/subdir &&
> +		test_commit a/b/subdir/foo.c &&

Why `.c`? That's a little distracting.

> +
> +		git branch O &&

Might be simpler just to use `master` subsequently and not "waste" a new
ref on that.

> +		git branch A &&

Might make more sense to create it below, via the `-b` option of `git
checkout`.

Or, for extra brownie points, via the `-c` option of `git switch`.

> +		git branch B &&

Likewise, this might want to be created below, via replacing `git
checkout B` with `git switch -c B master`.

> +
> +		git checkout A &&
> +		mkdir subdir &&
> +		git mv a/b/subdir/foo.c.t subdir/foo.c.t &&
> +		test_tick &&
> +		git commit -m "A" &&
> +
> +		git checkout B &&
> +		test_commit a/b/bar.c
> +	)
> +'
> +
> +test_expect_success '12d-check: Rename (merge) of subdirectory into the root' '

For the record: I am still a huge anti-fan of splitting `setup` test
cases from the test cases that do actual things, _unless_ it is _one_,
and _only one_, big, honking `setup` test case that is the very first
one in the test script.

Splitting things into two inevitably leads to unnecessary churn when
investigating test failures.

And that's really what test cases need to be optimized for: when they
report breakages. They need to help as much as they can to investigate
why things break. Nobody cares when test cases succeed. The ~150k test
cases that pass on every CI build: nobody is interested. When a test
case reports failure, that's when people pay attention. At least some,
including me.

The most common case for me (and every other lazy person who relies on
CI/PR builds) is when a build breaks, and then I usually get to the
trace of the actually failing test case very quickly. The previous test
case's trace, not so easy. Clicks involved. Now I lose context. Not
good.

A less common case for me is when I run test scripts locally, with `-i
-v -x`. Still, I need to scroll back to get context. And then, really, I
already lost context.

> +	(
> +		cd 12d &&
> +
> +		git checkout A^0 &&
> +
> +		git -c merge.directoryRenames=true merge -s recursive B^0 &&
> +
> +		git ls-files -s >out &&
> +		test_line_count = 2 out &&

Isn't this a bit overzealous?

> +
> +		git rev-parse >actual \
> +			HEAD:subdir/foo.c.t   HEAD:bar.c.t &&
> +		git rev-parse >expect \
> +			O:a/b/subdir/foo.c.t  B:a/b/bar.c.t &&
> +		test_cmp expect actual &&

Likewise?

> +
> +		git hash-object bar.c.t >actual &&
> +		git rev-parse B:a/b/bar.c.t >expect &&
> +		test_cmp expect actual &&

Likewise?

> +
> +		test_must_fail git rev-parse HEAD:a/b/subdir/foo.c.t &&
> +		test_must_fail git rev-parse HEAD:a/b/bar.c.t &&
> +		test_path_is_missing a/

Makes sense, but the part that I am missing is

		test_path_is_file bar.c.t

I.e. the _most_ important outcome of this test is: the rename was
detected, and the added file was correctly placed into the target
directory of the rename.

Thanks,
Dscho

> +	)
> +'
> +
>  ###########################################################################
>  # SECTION 13: Checking informational and conflict messages
>  #
> --
> gitgitgadget
>

Re: [PATCH 2/2] merge-recursive: fix merging a subdirectory into the root directory

[Elijah Newren]

Hi Dscho,

Thanks for the reviews!

On Sat, Oct 12, 2019 at 1:37 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Fri, 11 Oct 2019, Elijah Newren via GitGitGadget wrote:
>
[...]
> > @@ -1980,6 +1990,25 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path,
> >           *end_of_old == *end_of_new)
> >               return; /* We haven't modified *old_dir or *new_dir yet. */
> >
> > +     /*
> > +      * If end_of_new got back to the beginning of its string, and
> > +      * end_of_old got back to the beginning of some subdirectory, then
> > +      * we have a rename/merge of a subdirectory into the root, which
> > +      * needs slightly special handling.
> > +      *
> > +      * Note: There is no need to consider the opposite case, with a
> > +      * rename/merge of the root directory into some subdirectory.
> > +      * Our rule elsewhere that a directory which still exists is not
> > +      * considered to have been renamed means the root directory can
> > +      * never be renamed (because the root directory always exists).
> > +      */
> > +     if (end_of_new == new_path &&
> > +         end_of_old != old_path && end_of_old[-1] == '/') {
> > +             *old_dir = xstrndup(old_path, end_of_old-1 - old_path);
> > +             *new_dir = xstrndup(new_path, end_of_new - new_path);
>
> However, here we write something convoluted that essentially amounts to
> `xstrdup("")`. I would rather have that simple call than the convoluted
> one that would puzzle me every time I have to look at this part of the
> code.

Makes sense; I can switch it over.

>
> While at it, would you mind either surrounding the `-` and the `1` by
> spaces, or even write `--end_of_old - old_path`?

Sounds good to me; I'll make the change.


> > diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
> > index c966147d5d..b920bb0850 100755
> > --- a/t/t6043-merge-rename-directories.sh
> > +++ b/t/t6043-merge-rename-directories.sh
> > @@ -4051,6 +4051,62 @@ test_expect_success '12c-check: Moving one directory hierarchy into another w/ c
> >       )
> >  '
> >
> > +# Testcase 12d, Rename/merge of subdirectory into the root
> > +#   Commit O: a/b/{foo.c}
> > +#   Commit A: foo.c
> > +#   Commit B: a/b/{foo.c,bar.c}
> > +#   Expected: a/b/{foo.c,bar.c}

Note the nice explanation of the testcase setup at the beginning of
every test within this file...

> > +
> > +test_expect_success '12d-setup: Rename (merge) of subdirectory into the root' '
> > +     test_create_repo 12d &&
> > +     (
> > +             cd 12d &&
> > +
> > +             mkdir -p a/b/subdir &&
> > +             test_commit a/b/subdir/foo.c &&
>
> Why `.c`? That's a little distracting.

I can toss it.

> > +
> > +             git branch O &&
>
> Might be simpler just to use `master` subsequently and not "waste" a new
> ref on that.

I could do so, but then this makes the testcase description comment
earlier harder to read comparing "master", "A", and "B".  Having the
same length simplifies it a bit, and the triple of O, A, and B are
also used quite a bit in merge-recursive.c (e.g. in process_entry()
and merge_threeway() and other places).

Also, behaving differently for this test than the other 50+ tests in
the testfile would break the comment at the beginning of t6043 which
explains how *every* test in the file is of a certain form, using O,
A, and B.

>
> > +             git branch A &&
>
> Might make more sense to create it below, via the `-b` option of `git
> checkout`.
>
> Or, for extra brownie points, via the `-c` option of `git switch`.
>
> > +             git branch B &&
>
> Likewise, this might want to be created below, via replacing `git
> checkout B` with `git switch -c B master`.

I'm not sure I see why it'd be beneficial to switch this, though in
isolation I also don't see any drawbacks with your suggestion either.
It looks entirely reasonable, so I'd probably just do it if it weren't
for the fact that there are four dozen or so other tests in the same
file that already do it this way.  I'd rather keep the file internally
consistent, and there's a bit too much inertia for me to want to
switch all the tests over...unless you can provide a reason to
strongly prefer one style over the other?

> > +
> > +             git checkout A &&
> > +             mkdir subdir &&
> > +             git mv a/b/subdir/foo.c.t subdir/foo.c.t &&
> > +             test_tick &&
> > +             git commit -m "A" &&
> > +
> > +             git checkout B &&
> > +             test_commit a/b/bar.c
> > +     )
> > +'
> > +
> > +test_expect_success '12d-check: Rename (merge) of subdirectory into the root' '
>
> For the record: I am still a huge anti-fan of splitting `setup` test
> cases from the test cases that do actual things, _unless_ it is _one_,
> and _only one_, big, honking `setup` test case that is the very first
> one in the test script.
>
> Splitting things into two inevitably leads to unnecessary churn when
> investigating test failures.
>
> And that's really what test cases need to be optimized for: when they
> report breakages. They need to help as much as they can to investigate
> why things break. Nobody cares when test cases succeed. The ~150k test
> cases that pass on every CI build: nobody is interested. When a test
> case reports failure, that's when people pay attention. At least some,
> including me.
>
> The most common case for me (and every other lazy person who relies on
> CI/PR builds) is when a build breaks, and then I usually get to the
> trace of the actually failing test case very quickly. The previous test
> case's trace, not so easy. Clicks involved. Now I lose context. Not
> good.
>
> A less common case for me is when I run test scripts locally, with `-i
> -v -x`. Still, I need to scroll back to get context. And then, really, I
> already lost context.

I guess we have some strongly differing opinions here.  The one thing
I do agree with you on is test cases need to be optimized for when
they report breakages, but that is precisely what led me to splitting
setup and testing.  Way too many tests in the testsuite intersperse
several setup and test cases together making it really hard to
disentangle, understand what is going on, or even reverse engineer
what is relevant.  The absolute worst tests are the ones which just
keep making additional changes to some existing repo to provide extra
setup, causing all sorts of problems for skipping and resuming and
understanding (to say nothing of test prerequisites that aren't always
met).  But the ones with one big honking `setup` test case that is the
very first one in the test script can often be pretty bad too when
you've found a bug in testcase 82 and want to have a simple way to
reproduce.  It's awesome when someone can just run ./testcase.sh --ver
--imm -x --run=86,87 and reproduce the problem.  It feels sadly rare
to be able to do that in much of git.git's testsuite.  Not accreting
ever more changes into a repo to setup subsequent problems, using
entirely separate repos for different cases where testing makes any
changes, making it clear what is setup, making it clear what's the
command being tested, and making it clear what all the commands are
that are testing that the original test command produced the expected
behavior all go a long way to making things way easier to investigate.

Now, I will re-use a setup case for multiple tests and even did so in
t6043, but only when the setup really is identical (e.g. not only are
the tests expecting an identical setup, but the commands being tested
are read-only or any changes they make are unconditionally reverted as
the last step of the testcase).  Naturally, when I re-use a setup
block multiple times, I really don't want to copy the setup into each
of those tests either.

It would be really nice, though, if there were some way for me to
specify that a given "testcase" was just setup (so that they don't
even get a number in the prove output and don't count as a "test"
except maybe when they fail), and if there were some way to specify
which testcases depend on which setup blocks (so that if I want to run
just a given test, the setup test gets automatically included).


Your example of CI/PR builds makes sense to me, and I really would
like to make that nicer and improve other workflows too, especially if
it can be done without breaking my ability to investigate test
failures.  If someone can think of a clever way to do that (e.g. some
way of implementing my "It would be really nice" in the last
paragraph, and in a way that could also benefit CI/PR builds), I'd be
happy to go through and help switch things over.

> > +     (
> > +             cd 12d &&
> > +
> > +             git checkout A^0 &&
> > +
> > +             git -c merge.directoryRenames=true merge -s recursive B^0 &&
> > +
> > +             git ls-files -s >out &&
> > +             test_line_count = 2 out &&
>
> Isn't this a bit overzealous?
>
> > +
> > +             git rev-parse >actual \
> > +                     HEAD:subdir/foo.c.t   HEAD:bar.c.t &&
> > +             git rev-parse >expect \
> > +                     O:a/b/subdir/foo.c.t  B:a/b/bar.c.t &&
> > +             test_cmp expect actual &&
>
> Likewise?
>
> > +
> > +             git hash-object bar.c.t >actual &&
> > +             git rev-parse B:a/b/bar.c.t >expect &&
> > +             test_cmp expect actual &&
>
> Likewise?
>
> > +
> > +             test_must_fail git rev-parse HEAD:a/b/subdir/foo.c.t &&
> > +             test_must_fail git rev-parse HEAD:a/b/bar.c.t &&
> > +             test_path_is_missing a/
>
> Makes sense, but the part that I am missing is
>
>                 test_path_is_file bar.c.t
>
> I.e. the _most_ important outcome of this test is: the rename was
> detected, and the added file was correctly placed into the target
> directory of the rename.

That's a useful thing to add to the test, I'll add it.  (It's kind of
included in the 'git hash-object bar.c.t' half a dozen or so lines
above, but this line helps document the expectation a bit better.)

I'd be very reticent to include only this test_path_is_file check, as
it makes no guarantee that it has the right contents or that we didn't
also keep around another copy in a/b/bar.c.t, etc.  I understand my
checks above look overzealous, but merge-recursive has been a bit of a
pain to understand and deal with at times, and I've been bitten one
too many times with tests (of merge-recursive and elsewhere in
git.git) that passed but still did the wrong thing because they only
tested one way in which the test problem failed in the past rather
than specifying what correct behavior is.  And my "overzealousness" in
the merge-recursive tests really have caught and prevented bugs would
have been missed otherwise.


Oh, and I think there's another place in the code that needs to be
tweaked to make sure we handle renaming subdirectories into the root
directory that I missed (and just wasn't tested by this testcase), so
I'll check into it and if so fix the code and add another testcase,
and include the fixups I agreed to above and send out a v2.  Probably
won't get to it until the middle of next week, though.


Thanks again for the review and food for thought.  The CI/PR builds
sound particularly interesting; I'd really like to make those better
for you if it can be done in a way that doesn't break command line
builds with clear documentation of setup and expectations while
supporting test skipping and post-facto investigation (even without
--immediate or re-running), etc.  Hmm...

Re: [PATCH 2/2] merge-recursive: fix merging a subdirectory into the root directory

[Johannes Schindelin]

Hi Elijah,

On Sat, 12 Oct 2019, Elijah Newren wrote:

> On Sat, Oct 12, 2019 at 1:37 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> [...]
> > For the record: I am still a huge anti-fan of splitting `setup` test
> > cases from the test cases that do actual things, _unless_ it is
> > _one_, and _only one_, big, honking `setup` test case that is the
> > very first one in the test script.
> >
> > Splitting things into two inevitably leads to unnecessary churn when
> > investigating test failures.
> >
> > And that's really what test cases need to be optimized for: when
> > they report breakages. They need to help as much as they can to
> > investigate why things break. Nobody cares when test cases succeed.
> > The ~150k test cases that pass on every CI build: nobody is
> > interested. When a test case reports failure, that's when people pay
> > attention. At least some, including me.
> >
> > The most common case for me (and every other lazy person who relies
> > on CI/PR builds) is when a build breaks, and then I usually get to
> > the trace of the actually failing test case very quickly. The
> > previous test case's trace, not so easy. Clicks involved. Now I lose
> > context. Not good.
> >
> > A less common case for me is when I run test scripts locally, with
> > `-i -v -x`. Still, I need to scroll back to get context. And then,
> > really, I already lost context.
>
> I guess we have some strongly differing opinions here.

And probably experiences, too.

I literally debug something like a dozen per week test failures that are
reported via Azure Pipelines. And yes, I ran into some snags even with
your two-parter test cases in the past, and they did not help me. They
increased my burden of figuring out what is going wrong.

> The one thing I do agree with you on is test cases need to be
> optimized for when they report breakages, but that is precisely what
> led me to splitting setup and testing.

To me, it is so not helpful _not_ to see the output of a `setup` that
succeeded, and only the output of the actual test that actually failed.

It removes context.

I need to understand the scenario where the breakage happens, and the
only way I can understand is when I understand the context.

So the context needs to be as close as possible.

> Way too many tests in the testsuite intersperse several setup and test
> cases together making it really hard to disentangle, understand what
> is going on, or even reverse engineer what is relevant.  The absolute
> worst tests are the ones which just keep making additional changes to
> some existing repo to provide extra setup, causing all sorts of
> problems for skipping and resuming and understanding (to say nothing
> of test prerequisites that aren't always met).

I agree with this sentiment, and have to point out that this is yet
another fallout of the way our test suite is implemented. If you look
e.g. at JUnit, there are no "setup test cases". There are specific setup
steps that you can define, there is even a teardown step you can define,
and those individual test cases? They can run in parallel, or
randomized, and they run in their own sandbox, to make sure that they
don't rely on side effects of unrelated test cases.

We don't do that. We don't enforce the absence of side effects, and
therefore we have a megaton of them.

But note how this actually speaks _against_ separating the setup from
the test? Because then you _explicitly_ want those test cases to rely on
one another. Which flies in the _face_ of trying to disentangling them.

> But the ones with one big honking `setup` test case that is the very
> first one in the test script can often be pretty bad too when you've
> found a bug in testcase 82 and want to have a simple way to reproduce.

Indeed. One of the first things I try when `--run=82` fails is
`--run=1,82`. That's the best we can do in the absence of a clean design
like JUnit and its `@Before` method.

> It's awesome when someone can just run ./testcase.sh --ver --imm -x
> --run=86,87 and reproduce the problem.

But of course, often you would need `--run=1,86,87`. And it is totally
not obvious to _anybody_ who wants to contribute a new feature and whose
CI/PR build fails in one of your test cases.

> It feels sadly rare to be able to do that in much of git.git's
> testsuite.  Not accreting ever more changes into a repo to setup
> subsequent problems, using entirely separate repos for different cases
> where testing makes any changes, making it clear what is setup, making
> it clear what's the command being tested, and making it clear what all
> the commands are that are testing that the original test command
> produced the expected behavior all go a long way to making things way
> easier to investigate.

Sorry, I disagree. By squirreling away your setup phase into what looks
like an independent test case, the code is not more obvious, but less
so.

> Now, I will re-use a setup case for multiple tests and even did so in
> t6043, but only when the setup really is identical (e.g. not only are
> the tests expecting an identical setup, but the commands being tested
> are read-only or any changes they make are unconditionally reverted as
> the last step of the testcase).  Naturally, when I re-use a setup
> block multiple times, I really don't want to copy the setup into each
> of those tests either.
>
> It would be really nice, though, if there were some way for me to
> specify that a given "testcase" was just setup (so that they don't
> even get a number in the prove output and don't count as a "test"
> except maybe when they fail), and if there were some way to specify
> which testcases depend on which setup blocks (so that if I want to run
> just a given test, the setup test gets automatically included).
>
>
> Your example of CI/PR builds makes sense to me, and I really would
> like to make that nicer and improve other workflows too, especially if
> it can be done without breaking my ability to investigate test
> failures.  If someone can think of a clever way to do that (e.g. some
> way of implementing my "It would be really nice" in the last
> paragraph, and in a way that could also benefit CI/PR builds), I'd be
> happy to go through and help switch things over.

My suggestion: do not rip apart commands that belong together. The setup
phase is often more important to understand than the actually failing
command. _Especially_ when the setup test case reports success, but
actually failed to set things up as intended (which I encountered more
times than I care to count).

> > > +     (
> > > +             cd 12d &&
> > > +
> > > +             git checkout A^0 &&
> > > +
> > > +             git -c merge.directoryRenames=true merge -s recursive B^0 &&
> > > +
> > > +             git ls-files -s >out &&
> > > +             test_line_count = 2 out &&
> >
> > Isn't this a bit overzealous?
> >
> > > +
> > > +             git rev-parse >actual \
> > > +                     HEAD:subdir/foo.c.t   HEAD:bar.c.t &&
> > > +             git rev-parse >expect \
> > > +                     O:a/b/subdir/foo.c.t  B:a/b/bar.c.t &&
> > > +             test_cmp expect actual &&
> >
> > Likewise?
> >
> > > +
> > > +             git hash-object bar.c.t >actual &&
> > > +             git rev-parse B:a/b/bar.c.t >expect &&
> > > +             test_cmp expect actual &&
> >
> > Likewise?
> >
> > > +
> > > +             test_must_fail git rev-parse HEAD:a/b/subdir/foo.c.t &&
> > > +             test_must_fail git rev-parse HEAD:a/b/bar.c.t &&
> > > +             test_path_is_missing a/
> >
> > Makes sense, but the part that I am missing is
> >
> >                 test_path_is_file bar.c.t
> >
> > I.e. the _most_ important outcome of this test is: the rename was
> > detected, and the added file was correctly placed into the target
> > directory of the rename.
>
> That's a useful thing to add to the test, I'll add it.  (It's kind of
> included in the 'git hash-object bar.c.t' half a dozen or so lines
> above, but this line helps document the expectation a bit better.)
>
> I'd be very reticent to include only this test_path_is_file check, as
> it makes no guarantee that it has the right contents or that we didn't
> also keep around another copy in a/b/bar.c.t, etc.r

I agree that it has to strike a balance. There are multiple aspects you
need to consider:

- It needs to be easy to understand what the test case tries to ensure.

- While it is important to verify that Git works correctly, you do not
  want to make the test suite so slow as to be useless. I vividly
  remember how Duy mentioned that he does not have the time to run the
  test suite before contributing. That's how bad things are _already_.

As a rule of thumb, I would like to submit that a test case should fail
when the claimed goal (i.e. the test case's title) is, and a breakage in
that should both be sufficient and necessary for it to fail.

In this instance, the title is:

	12d-setup: Rename (merge) of subdirectory into the root

Now, this does not even tell me clearly what it tries to ensure. But
from the code, I guess that it wants to ensure that the directory rename
detection can detect when a directory's contents were moved into the
top-level directory.

Now, let's apply the rule of the "sufficient". I would expect the
outcome to be that the file that was added to that directory in one
branch is moved together with the directory's contents that the other
branch moved into the top-level directory.

I was missing that test, hence I pointed it out.

But testing the absence of the file `foo.c.t` in its original location
which was moved in one branch, and not touched in the other, i.e. with a
_trivial_ three-way merge resolution, that seems to be extra. It would
not break if the directory rename detection is broken, so it is an extra
test that does not do what the test case's title claims to test.

Now for the "necessary" part. It is totally unclear to me how all those
other things you test for would be failing _only_ if the directory
rename detection failed. I would actually expect that there are _many_
ways that this test case could fail in ways that have not the faintest
thing to do with directory rename detection.

As somebody who, as I stated above, investigates dozens of test case
failures per week (although, granted, most of them are relatively
trivial to resolve, it is invariably test cases like the one I am
commenting on that require the most time to get resolved), I have to
admit that there are few more frustrating things in debugging than
having to chase issues that have _nothing_ to do with the stated goal of
the test case.

> I understand my checks above look overzealous, but merge-recursive has
> been a bit of a pain to understand and deal with at times, and I've
> been bitten one too many times with tests (of merge-recursive and
> elsewhere in git.git) that passed but still did the wrong thing
> because they only tested one way in which the test problem failed in
> the past rather than specifying what correct behavior is.  And my
> "overzealousness" in the merge-recursive tests really have caught and
> prevented bugs would have been missed otherwise.

If you must have those tests, then please structure them in a way where
they are much more actionable. Ideally, it should be clear from a glance
at the trace of a failed test case (and _only_ from that test case) for
less than five seconds _what_ is being tested, and it should be obvious
what is going wrong.

I cannot say that about the test case in this mail.

> Oh, and I think there's another place in the code that needs to be
> tweaked to make sure we handle renaming subdirectories into the root
> directory that I missed (and just wasn't tested by this testcase), so
> I'll check into it and if so fix the code and add another testcase,
> and include the fixups I agreed to above and send out a v2.  Probably
> won't get to it until the middle of next week, though.
>
>
> Thanks again for the review and food for thought.  The CI/PR builds
> sound particularly interesting; I'd really like to make those better
> for you if it can be done in a way that doesn't break command line
> builds with clear documentation of setup and expectations while
> supporting test skipping and post-facto investigation (even without
> --immediate or re-running), etc.  Hmm...

My hope was that CI/PR builds could help improve the code quality in
`pu`, and to a large part it did.

The time I need to spend on understanding test cases' code (t0021 is a
particularly nasty one, not your fault, of course) before I can even get
to fire up a debugger, this amount of time is insane, though.

Ciao,
Dscho

Re: [PATCH 2/2] merge-recursive: fix merging a subdirectory into the root directory

[Elijah Newren]

Sorry for the long delay before getting back to this; the other stuff
I was working on took longer than expected.

On Mon, Oct 14, 2019 at 3:42 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Sat, 12 Oct 2019, Elijah Newren wrote:
> > On Sat, Oct 12, 2019 at 1:37 PM Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> > >
> > > For the record: I am still a huge anti-fan of splitting `setup` test
> > > cases from the test cases that do actual things, _unless_ it is
> > > _one_, and _only one_, big, honking `setup` test case that is the
> > > very first one in the test script.
[...]
> > The one thing I do agree with you on is test cases need to be
> > optimized for when they report breakages, but that is precisely what
> > led me to splitting setup and testing.
>
> To me, it is so not helpful _not_ to see the output of a `setup` that
> succeeded, and only the output of the actual test that actually failed.
>
> It removes context.
>
> I need to understand the scenario where the breakage happens, and the
> only way I can understand is when I understand the context.
>
> So the context needs to be as close as possible.

I've updated the patch series with a change that I hope helps while
still allowing the setup "steps" to be visibly differentiated from the
testing steps.

> > Way too many tests in the testsuite intersperse several setup and test
> > cases together making it really hard to disentangle, understand what
> > is going on, or even reverse engineer what is relevant.  The absolute
> > worst tests are the ones which just keep making additional changes to
> > some existing repo to provide extra setup, causing all sorts of
> > problems for skipping and resuming and understanding (to say nothing
> > of test prerequisites that aren't always met).
>
> I agree with this sentiment, and have to point out that this is yet
> another fallout of the way our test suite is implemented. If you look
> e.g. at JUnit, there are no "setup test cases". There are specific setup
> steps that you can define, there is even a teardown step you can define,
> and those individual test cases? They can run in parallel, or
> randomized, and they run in their own sandbox, to make sure that they
> don't rely on side effects of unrelated test cases.
>
> We don't do that. We don't enforce the absence of side effects, and
> therefore we have a megaton of them.
>
> But note how this actually speaks _against_ separating the setup from
> the test? Because then you _explicitly_ want those test cases to rely on
> one another. Which flies in the _face_ of trying to disentangling them.

I agree that it is desirable to avoid side effects in the tests, but
I'd like to point out that I'm not at all sure that your conclusion
here is the only logical one to draw here in comparing to JUnit.  As
you point out, JUnit has clearly delineated setup steps for a test (as
well as teardown steps), providing a place to keep them separate.  Our
testsuite lacks that, so how do folks try to get it?  One logical way
would be just inlining the setup steps in the test outside a
test_expect_* block (which has been done in the past), but that has
even worse problems.  Another way, even if suboptimal, is placing
those steps in their own test_expect_* block.  You say just throw the
setup and test together, but that breaks the separation.

I think it's a case of the testsuite not providing the right
abstractions and enough capability, leaving us to argue over which
aspects of a more featureful test harness are most important to
emulate.  You clearly picked one, while I was focusing on another.
Anyway, all that said, I think I have a nice compromise that I'll send
out with V2.

[...]
> > > Makes sense, but the part that I am missing is
> > >
> > >                 test_path_is_file bar.c.t
> > >
> > > I.e. the _most_ important outcome of this test is: the rename was
> > > detected, and the added file was correctly placed into the target
> > > directory of the rename.
> >
> > That's a useful thing to add to the test, I'll add it.  (It's kind of
> > included in the 'git hash-object bar.c.t' half a dozen or so lines
> > above, but this line helps document the expectation a bit better.)
> >
> > I'd be very reticent to include only this test_path_is_file check, as
> > it makes no guarantee that it has the right contents or that we didn't
> > also keep around another copy in a/b/bar.c.t, etc.
>
> I agree that it has to strike a balance. There are multiple aspects you
> need to consider:
>
> - It needs to be easy to understand what the test case tries to ensure.
>
> - While it is important to verify that Git works correctly, you do not
>   want to make the test suite so slow as to be useless. I vividly
>   remember how Duy mentioned that he does not have the time to run the
>   test suite before contributing. That's how bad things are _already_.

On this issue I'm having a harder time finding common ground.  Perhaps
there's something clever to be done, but I'm not seeing it.  I can at
least try to explain my perspective...

I understand you probably meant these two items as a non-exhaustive
list of things that need to be balanced, but you did omit what I feel
is the most important thing in the balance:

- The combination of tests should provide adequate confidence of
coverage that someone can refactor the code as necessary.

The tests in t6036, t6042, t6043, and t6046 are more defensive than
normal (defining "correct" behavior more finely-grained than usual),
but that came about precisely because it was so easy to make
refactoring mistakes that broke things in unexpected ways.  There have
been a handful of times already when I have gone in to refactor or fix
a reported bug, thought I had the right solution, and then only one or
maybe two tests failed and it was one of these tests and in particular
the "overzealous" portions of these tests that caught the bug (sorry I
don't still have details, I just remember being surprised and happy
about the extra defensiveness on multiple occasions).  Without this
extra defensiveness, there would have been extra bugs introduced that
I'm sure would have made it into real git releases.  So, there would
need to be a large pile of evidence and problems before I'd be willing
to take these out.

All that said...

I do very much agree with you that the overall testsuite needs to be
relatively speedy too, and that is an important counterbalance in
general.  But testsuite performance is somewhat of a red-herring in
this context: when running all testcases with 'merge' in their name
which I have contributed anything at all to (apparently 17 t[0-9]*.sh
files meet both these criteria), and running them in serial (no -j8 or
anything of the sort), they all combined complete in less than 40% of
the time that t9001-send-email.sh alone takes on my box.  It'd reduce
to barely over a quarter of the time that the send-email test takes if
it weren't for t3425-rebase-topology-merges.sh (which is the slowest
test with 'merge' in its name that I've touched, despite the fact that
it has none of this extra defensiveness you are complaining about and
labelling as unrelated).  Yes, t6043 is pretty long code-wise, and
still has over five dozen tests after ditching the separate "setup"
tests -- but all of those tests still run in 3.6s on my box.  I think
that a few extra calls to ls-files, rev-parse, hash-object and
test_path_is_missing are the wrong place to look if you're worried
about testsuite performance.

Finally, you also brought up "easy to understand" the test case.  You
are absolutely right that the extra calls to ls-files, rev-parse,
hash-object, test_path_is_missing do slow down an external reader and
almost certainly doesn't fit in your "within five seconds" goal you
mentioned below.  That's unfortunate.  But optimizing this at the
expense of preventing actual bugs that would have hit git.git releases
is a bad tradeoff and one I'm not willing to make.  If you want me to
restrict the extra defensiveness to just the merge-recursive tests,
I'm happy to do so.  I do agree that it's unusual, and probably should
be, but I think merge-recursive merits the special handling.


Hope that helps; thanks for all the feedback.

Elijah

[PATCH v2 0/3] Dir rename fixes

[Elijah Newren via GitGitGadget]

This series improves a couple things found after looking into things Dscho
flagged:

 * clarify and slightly restructure code in the get_renamed_dir_portion()
   function
 * extend support of detecting renaming/merging of one directory into
   another to support the root directory as a target directory

First patch best viewed with a --histogram diff (sorry, gitgitgadget does
not yet know how to generate those).

Changes since v1:

 * Incorporated code cleanups suggested by Dscho
 * Fixed to work with an alternate rename-to-root-directory case (end_of_new
   == NULL), with new testcase
 * Added a new patch to the end of the series to stop making setup tests be
   part of a separate test_expect_success block.

Elijah Newren (3):
  merge-recursive: clean up get_renamed_dir_portion()
  merge-recursive: fix merging a subdirectory into the root directory
  t604[236]: do not run setup in separate tests

 merge-recursive.c                      | 104 ++++-
 t/t6042-merge-rename-corner-cases.sh   | 111 +++--
 t/t6043-merge-rename-directories.sh    | 568 ++++++++++++++++---------
 t/t6046-merge-skip-unneeded-updates.sh | 135 +++---
 4 files changed, 582 insertions(+), 336 deletions(-)


base-commit: 08da6496b61341ec45eac36afcc8f94242763468
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-390%2Fnewren%2Fdir-rename-fixes-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-390/newren/dir-rename-fixes-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/390

Range-diff vs v1:

 1:  8ae78679c9 = 1:  8ae78679c9 merge-recursive: clean up get_renamed_dir_portion()
 2:  37aee862e1 ! 2:  a1e80e8fbb merge-recursive: fix merging a subdirectory into the root directory
     @@ -34,9 +34,41 @@
       	strbuf_grow(&new_path, newlen);
       	strbuf_addbuf(&new_path, &entry->new_dir);
      @@
     - 	    *end_of_old == *end_of_new)
     - 		return; /* We haven't modified *old_dir or *new_dir yet. */
     + 	 */
     + 	end_of_old = strrchr(old_path, '/');
     + 	end_of_new = strrchr(new_path, '/');
     +-	if (end_of_old == NULL || end_of_new == NULL)
     +-		return; /* We haven't modified *old_dir or *new_dir yet. */
     ++
     ++	/*
     ++	 * If end_of_old is NULL, old_path wasn't in a directory, so there
     ++	 * could not be a directory rename (our rule elsewhere that a
     ++	 * directory which still exists is not considered to have been
     ++	 * renamed means the root directory can never be renamed -- because
     ++	 * the root directory always exists).
     ++	 */
     ++	if (end_of_old == NULL)
     ++		return; /* Note: *old_dir and *new_dir are still NULL */
     ++
     ++	/*
     ++	 * If new_path contains no directory (end_of_new is NULL), then we
     ++	 * have a rename of old_path's directory to the root directory.
     ++	 */
     ++	if (end_of_new == NULL) {
     ++		*old_dir = xstrndup(old_path, end_of_old - old_path);
     ++		*new_dir = xstrdup("");
     ++		return;
     ++	}
       
     + 	/* Find the first non-matching character traversing backwards */
     + 	while (*--end_of_new == *--end_of_old &&
     +@@
     + 	 */
     + 	if (end_of_old == old_path && end_of_new == new_path &&
     + 	    *end_of_old == *end_of_new)
     +-		return; /* We haven't modified *old_dir or *new_dir yet. */
     ++		return; /* Note: *old_dir and *new_dir are still NULL */
     ++
      +	/*
      +	 * If end_of_new got back to the beginning of its string, and
      +	 * end_of_old got back to the beginning of some subdirectory, then
     @@ -44,21 +76,19 @@
      +	 * needs slightly special handling.
      +	 *
      +	 * Note: There is no need to consider the opposite case, with a
     -+	 * rename/merge of the root directory into some subdirectory.
     -+	 * Our rule elsewhere that a directory which still exists is not
     -+	 * considered to have been renamed means the root directory can
     -+	 * never be renamed (because the root directory always exists).
     ++	 * rename/merge of the root directory into some subdirectory
     ++	 * because as noted above the root directory always exists so it
     ++	 * cannot be considered to be renamed.
      +	 */
      +	if (end_of_new == new_path &&
      +	    end_of_old != old_path && end_of_old[-1] == '/') {
     -+		*old_dir = xstrndup(old_path, end_of_old-1 - old_path);
     -+		*new_dir = xstrndup(new_path, end_of_new - new_path);
     ++		*old_dir = xstrndup(old_path, --end_of_old - old_path);
     ++		*new_dir = xstrdup("");
      +		return;
      +	}
     -+
     + 
       	/*
       	 * We've found the first non-matching character in the directory
     - 	 * paths.  That means the current characters we were looking at
      
       diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
       --- a/t/t6043-merge-rename-directories.sh
     @@ -68,18 +98,18 @@
       '
       
      +# Testcase 12d, Rename/merge of subdirectory into the root
     -+#   Commit O: a/b/{foo.c}
     -+#   Commit A: foo.c
     -+#   Commit B: a/b/{foo.c,bar.c}
     -+#   Expected: a/b/{foo.c,bar.c}
     ++#   Commit O: a/b/subdir/foo
     ++#   Commit A: subdir/foo
     ++#   Commit B: a/b/subdir/foo, a/b/bar
     ++#   Expected: subdir/foo, bar
      +
     -+test_expect_success '12d-setup: Rename (merge) of subdirectory into the root' '
     ++test_expect_success '12d-setup: Rename/merge subdir into the root, variant 1' '
      +	test_create_repo 12d &&
      +	(
      +		cd 12d &&
      +
      +		mkdir -p a/b/subdir &&
     -+		test_commit a/b/subdir/foo.c &&
     ++		test_commit a/b/subdir/foo &&
      +
      +		git branch O &&
      +		git branch A &&
     @@ -87,16 +117,16 @@
      +
      +		git checkout A &&
      +		mkdir subdir &&
     -+		git mv a/b/subdir/foo.c.t subdir/foo.c.t &&
     ++		git mv a/b/subdir/foo.t subdir/foo.t &&
      +		test_tick &&
      +		git commit -m "A" &&
      +
      +		git checkout B &&
     -+		test_commit a/b/bar.c
     ++		test_commit a/b/bar
      +	)
      +'
      +
     -+test_expect_success '12d-check: Rename (merge) of subdirectory into the root' '
     ++test_expect_success '12d-check: Rename/merge subdir into the root, variant 1' '
      +	(
      +		cd 12d &&
      +
     @@ -108,18 +138,76 @@
      +		test_line_count = 2 out &&
      +
      +		git rev-parse >actual \
     -+			HEAD:subdir/foo.c.t   HEAD:bar.c.t &&
     ++			HEAD:subdir/foo.t   HEAD:bar.t &&
     ++		git rev-parse >expect \
     ++			O:a/b/subdir/foo.t  B:a/b/bar.t &&
     ++		test_cmp expect actual &&
     ++
     ++		git hash-object bar.t >actual &&
     ++		git rev-parse B:a/b/bar.t >expect &&
     ++		test_cmp expect actual &&
     ++
     ++		test_must_fail git rev-parse HEAD:a/b/subdir/foo.t &&
     ++		test_must_fail git rev-parse HEAD:a/b/bar.t &&
     ++		test_path_is_missing a/ &&
     ++		test_path_is_file bar.t
     ++	)
     ++'
     ++
     ++# Testcase 12e, Rename/merge of subdirectory into the root
     ++#   Commit O: a/b/foo
     ++#   Commit A: foo
     ++#   Commit B: a/b/foo, a/b/bar
     ++#   Expected: foo, bar
     ++
     ++test_expect_success '12e-setup: Rename/merge subdir into the root, variant 2' '
     ++	test_create_repo 12e &&
     ++	(
     ++		cd 12e &&
     ++
     ++		mkdir -p a/b &&
     ++		test_commit a/b/foo &&
     ++
     ++		git branch O &&
     ++		git branch A &&
     ++		git branch B &&
     ++
     ++		git checkout A &&
     ++		mkdir subdir &&
     ++		git mv a/b/foo.t foo.t &&
     ++		test_tick &&
     ++		git commit -m "A" &&
     ++
     ++		git checkout B &&
     ++		test_commit a/b/bar
     ++	)
     ++'
     ++
     ++test_expect_success '12e-check: Rename/merge subdir into the root, variant 2' '
     ++	(
     ++		cd 12e &&
     ++
     ++		git checkout A^0 &&
     ++
     ++		git -c merge.directoryRenames=true merge -s recursive B^0 &&
     ++
     ++		git ls-files -s >out &&
     ++		test_line_count = 2 out &&
     ++
     ++		git rev-parse >actual \
     ++			HEAD:foo.t   HEAD:bar.t &&
      +		git rev-parse >expect \
     -+			O:a/b/subdir/foo.c.t  B:a/b/bar.c.t &&
     ++			O:a/b/foo.t  B:a/b/bar.t &&
      +		test_cmp expect actual &&
      +
     -+		git hash-object bar.c.t >actual &&
     -+		git rev-parse B:a/b/bar.c.t >expect &&
     ++		git hash-object bar.t >actual &&
     ++		git rev-parse B:a/b/bar.t >expect &&
      +		test_cmp expect actual &&
      +
     -+		test_must_fail git rev-parse HEAD:a/b/subdir/foo.c.t &&
     -+		test_must_fail git rev-parse HEAD:a/b/bar.c.t &&
     -+		test_path_is_missing a/
     ++		test_must_fail git rev-parse HEAD:a/b/foo.t &&
     ++		test_must_fail git rev-parse HEAD:a/b/bar.t &&
     ++		test_path_is_missing a/ &&
     ++		test_path_is_file bar.t
      +	)
      +'
      +
 -:  ---------- > 3:  0dd61a3580 t604[236]: do not run setup in separate tests

-- 
gitgitgadget

[PATCH v2 1/3] merge-recursive: clean up get_renamed_dir_portion()

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

Dscho noted a few things making this function hard to follow.
Restructure it a bit and add comments to make it easier to follow.  The
restructurings include:

  * There was a special case if-check at the end of the function
    checking whether someone just renamed a file within its original
    directory, meaning that there could be no directory rename involved.
    That check was slightly convoluted; it could be done in a more
    straightforward fashion earlier in the function, and can be done
    more cheaply too (no call to strncmp).

  * The conditions for advancing end_of_old and end_of_new before
    calling strchr were both confusing and unnecessary.  If either
    points at a '/', then they need to be advanced in order to find the
    next '/'.  If either doesn't point at a '/', then advancing them one
    char before calling strchr() doesn't hurt.  So, just rip out the
    if conditions and advance both before calling strchr().

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c | 60 ++++++++++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 24 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 22a12cfeba..f80e48f623 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1943,8 +1943,8 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path,
 				    char **old_dir, char **new_dir)
 {
 	char *end_of_old, *end_of_new;
-	int old_len, new_len;
 
+	/* Default return values: NULL, meaning no rename */
 	*old_dir = NULL;
 	*new_dir = NULL;
 
@@ -1955,43 +1955,55 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path,
 	 *    "a/b/c/d" was renamed to "a/b/some/thing/else"
 	 * so, for this example, this function returns "a/b/c/d" in
 	 * *old_dir and "a/b/some/thing/else" in *new_dir.
-	 *
-	 * Also, if the basename of the file changed, we don't care.  We
-	 * want to know which portion of the directory, if any, changed.
+	 */
+
+	/*
+	 * If the basename of the file changed, we don't care.  We want
+	 * to know which portion of the directory, if any, changed.
 	 */
 	end_of_old = strrchr(old_path, '/');
 	end_of_new = strrchr(new_path, '/');
-
 	if (end_of_old == NULL || end_of_new == NULL)
-		return;
+		return; /* We haven't modified *old_dir or *new_dir yet. */
+
+	/* Find the first non-matching character traversing backwards */
 	while (*--end_of_new == *--end_of_old &&
 	       end_of_old != old_path &&
 	       end_of_new != new_path)
 		; /* Do nothing; all in the while loop */
+
 	/*
-	 * We've found the first non-matching character in the directory
-	 * paths.  That means the current directory we were comparing
-	 * represents the rename.  Move end_of_old and end_of_new back
-	 * to the full directory name.
+	 * If both got back to the beginning of their strings, then the
+	 * directory didn't change at all, only the basename did.
 	 */
-	if (*end_of_old == '/')
-		end_of_old++;
-	if (*end_of_old != '/')
-		end_of_new++;
-	end_of_old = strchr(end_of_old, '/');
-	end_of_new = strchr(end_of_new, '/');
+	if (end_of_old == old_path && end_of_new == new_path &&
+	    *end_of_old == *end_of_new)
+		return; /* We haven't modified *old_dir or *new_dir yet. */
 
 	/*
-	 * It may have been the case that old_path and new_path were the same
-	 * directory all along.  Don't claim a rename if they're the same.
+	 * We've found the first non-matching character in the directory
+	 * paths.  That means the current characters we were looking at
+	 * were part of the first non-matching subdir name going back from
+	 * the end of the strings.  Get the whole name by advancing both
+	 * end_of_old and end_of_new to the NEXT '/' character.  That will
+	 * represent the entire directory rename.
+	 *
+	 * The reason for the increment is cases like
+	 *    a/b/star/foo/whatever.c -> a/b/tar/foo/random.c
+	 * After dropping the basename and going back to the first
+	 * non-matching character, we're now comparing:
+	 *    a/b/s          and         a/b/
+	 * and we want to be comparing:
+	 *    a/b/star/      and         a/b/tar/
+	 * but without the pre-increment, the one on the right would stay
+	 * a/b/.
 	 */
-	old_len = end_of_old - old_path;
-	new_len = end_of_new - new_path;
+	end_of_old = strchr(++end_of_old, '/');
+	end_of_new = strchr(++end_of_new, '/');
 
-	if (old_len != new_len || strncmp(old_path, new_path, old_len)) {
-		*old_dir = xstrndup(old_path, old_len);
-		*new_dir = xstrndup(new_path, new_len);
-	}
+	/* Copy the old and new directories into *old_dir and *new_dir. */
+	*old_dir = xstrndup(old_path, end_of_old - old_path);
+	*new_dir = xstrndup(new_path, end_of_new - new_path);
 }
 
 static void remove_hashmap_entries(struct hashmap *dir_renames,
-- 
gitgitgadget

[PATCH v2 2/3] merge-recursive: fix merging a subdirectory into the root directory

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

We allow renaming all entries in e.g. a directory named z/ into a
directory named y/ to be detected as a z/ -> y/ rename, so that if the
other side of history adds any files to the directory z/ in the mean
time, we can provide the hint that they should be moved to y/.

There is no reason to not allow 'y/' to be the root directory, but the
code did not handle that case correctly.  Add a testcase and the
necessary special checks to support this case.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c                   |  52 ++++++++++++-
 t/t6043-merge-rename-directories.sh | 114 ++++++++++++++++++++++++++++
 2 files changed, 163 insertions(+), 3 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index f80e48f623..ec60715368 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1931,6 +1931,16 @@ static char *apply_dir_rename(struct dir_rename_entry *entry,
 		return NULL;
 
 	oldlen = strlen(entry->dir);
+	if (entry->new_dir.len == 0)
+		/*
+		 * If someone renamed/merged a subdirectory into the root
+		 * directory (e.g. 'some/subdir' -> ''), then we want to
+		 * avoid returning
+		 *     '' + '/filename'
+		 * as the rename; we need to make old_path + oldlen advance
+		 * past the '/' character.
+		 */
+		oldlen++;
 	newlen = entry->new_dir.len + (strlen(old_path) - oldlen) + 1;
 	strbuf_grow(&new_path, newlen);
 	strbuf_addbuf(&new_path, &entry->new_dir);
@@ -1963,8 +1973,26 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path,
 	 */
 	end_of_old = strrchr(old_path, '/');
 	end_of_new = strrchr(new_path, '/');
-	if (end_of_old == NULL || end_of_new == NULL)
-		return; /* We haven't modified *old_dir or *new_dir yet. */
+
+	/*
+	 * If end_of_old is NULL, old_path wasn't in a directory, so there
+	 * could not be a directory rename (our rule elsewhere that a
+	 * directory which still exists is not considered to have been
+	 * renamed means the root directory can never be renamed -- because
+	 * the root directory always exists).
+	 */
+	if (end_of_old == NULL)
+		return; /* Note: *old_dir and *new_dir are still NULL */
+
+	/*
+	 * If new_path contains no directory (end_of_new is NULL), then we
+	 * have a rename of old_path's directory to the root directory.
+	 */
+	if (end_of_new == NULL) {
+		*old_dir = xstrndup(old_path, end_of_old - old_path);
+		*new_dir = xstrdup("");
+		return;
+	}
 
 	/* Find the first non-matching character traversing backwards */
 	while (*--end_of_new == *--end_of_old &&
@@ -1978,7 +2006,25 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path,
 	 */
 	if (end_of_old == old_path && end_of_new == new_path &&
 	    *end_of_old == *end_of_new)
-		return; /* We haven't modified *old_dir or *new_dir yet. */
+		return; /* Note: *old_dir and *new_dir are still NULL */
+
+	/*
+	 * If end_of_new got back to the beginning of its string, and
+	 * end_of_old got back to the beginning of some subdirectory, then
+	 * we have a rename/merge of a subdirectory into the root, which
+	 * needs slightly special handling.
+	 *
+	 * Note: There is no need to consider the opposite case, with a
+	 * rename/merge of the root directory into some subdirectory
+	 * because as noted above the root directory always exists so it
+	 * cannot be considered to be renamed.
+	 */
+	if (end_of_new == new_path &&
+	    end_of_old != old_path && end_of_old[-1] == '/') {
+		*old_dir = xstrndup(old_path, --end_of_old - old_path);
+		*new_dir = xstrdup("");
+		return;
+	}
 
 	/*
 	 * We've found the first non-matching character in the directory
diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
index c966147d5d..32cdd1f493 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -4051,6 +4051,120 @@ test_expect_success '12c-check: Moving one directory hierarchy into another w/ c
 	)
 '
 
+# Testcase 12d, Rename/merge of subdirectory into the root
+#   Commit O: a/b/subdir/foo
+#   Commit A: subdir/foo
+#   Commit B: a/b/subdir/foo, a/b/bar
+#   Expected: subdir/foo, bar
+
+test_expect_success '12d-setup: Rename/merge subdir into the root, variant 1' '
+	test_create_repo 12d &&
+	(
+		cd 12d &&
+
+		mkdir -p a/b/subdir &&
+		test_commit a/b/subdir/foo &&
+
+		git branch O &&
+		git branch A &&
+		git branch B &&
+
+		git checkout A &&
+		mkdir subdir &&
+		git mv a/b/subdir/foo.t subdir/foo.t &&
+		test_tick &&
+		git commit -m "A" &&
+
+		git checkout B &&
+		test_commit a/b/bar
+	)
+'
+
+test_expect_success '12d-check: Rename/merge subdir into the root, variant 1' '
+	(
+		cd 12d &&
+
+		git checkout A^0 &&
+
+		git -c merge.directoryRenames=true merge -s recursive B^0 &&
+
+		git ls-files -s >out &&
+		test_line_count = 2 out &&
+
+		git rev-parse >actual \
+			HEAD:subdir/foo.t   HEAD:bar.t &&
+		git rev-parse >expect \
+			O:a/b/subdir/foo.t  B:a/b/bar.t &&
+		test_cmp expect actual &&
+
+		git hash-object bar.t >actual &&
+		git rev-parse B:a/b/bar.t >expect &&
+		test_cmp expect actual &&
+
+		test_must_fail git rev-parse HEAD:a/b/subdir/foo.t &&
+		test_must_fail git rev-parse HEAD:a/b/bar.t &&
+		test_path_is_missing a/ &&
+		test_path_is_file bar.t
+	)
+'
+
+# Testcase 12e, Rename/merge of subdirectory into the root
+#   Commit O: a/b/foo
+#   Commit A: foo
+#   Commit B: a/b/foo, a/b/bar
+#   Expected: foo, bar
+
+test_expect_success '12e-setup: Rename/merge subdir into the root, variant 2' '
+	test_create_repo 12e &&
+	(
+		cd 12e &&
+
+		mkdir -p a/b &&
+		test_commit a/b/foo &&
+
+		git branch O &&
+		git branch A &&
+		git branch B &&
+
+		git checkout A &&
+		mkdir subdir &&
+		git mv a/b/foo.t foo.t &&
+		test_tick &&
+		git commit -m "A" &&
+
+		git checkout B &&
+		test_commit a/b/bar
+	)
+'
+
+test_expect_success '12e-check: Rename/merge subdir into the root, variant 2' '
+	(
+		cd 12e &&
+
+		git checkout A^0 &&
+
+		git -c merge.directoryRenames=true merge -s recursive B^0 &&
+
+		git ls-files -s >out &&
+		test_line_count = 2 out &&
+
+		git rev-parse >actual \
+			HEAD:foo.t   HEAD:bar.t &&
+		git rev-parse >expect \
+			O:a/b/foo.t  B:a/b/bar.t &&
+		test_cmp expect actual &&
+
+		git hash-object bar.t >actual &&
+		git rev-parse B:a/b/bar.t >expect &&
+		test_cmp expect actual &&
+
+		test_must_fail git rev-parse HEAD:a/b/foo.t &&
+		test_must_fail git rev-parse HEAD:a/b/bar.t &&
+		test_path_is_missing a/ &&
+		test_path_is_file bar.t
+	)
+'
+
 ###########################################################################
 # SECTION 13: Checking informational and conflict messages
 #
-- 
gitgitgadget

[PATCH v2 3/3] t604[236]: do not run setup in separate tests

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

Transform the setup "tests" to setup functions, and have the actual
tests call the setup functions.  Advantages:

  * Should make life easier for people working with webby CI/PR builds
    who have to abuse mice (and their own index finger as well) in
    order to switch from viewing one testcase to another.  Sounds
    awful; hopefully this will improve things for them.

  * Improves re-runnability: any failed test in any of these three
    files can now be re-run in isolation, e.g.
       ./t6042* --ver --imm -x --run=21
    whereas before it would require two tests to be specified to the
    --run argument, the other needing to be picked out as the relevant
    setup test from one or two tests before.

  * Importantly, this still keeps the "setup" and "test" sections
    somewhat separate to make it easier for readers to discern what is
    just ancillary setup and what the intent of the test is.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t6042-merge-rename-corner-cases.sh   | 111 +++---
 t/t6043-merge-rename-directories.sh    | 466 ++++++++++++++-----------
 t/t6046-merge-skip-unneeded-updates.sh | 135 ++++---
 3 files changed, 393 insertions(+), 319 deletions(-)

diff --git a/t/t6042-merge-rename-corner-cases.sh b/t/t6042-merge-rename-corner-cases.sh
index c5b57f40c3..b047cf1c1c 100755
--- a/t/t6042-merge-rename-corner-cases.sh
+++ b/t/t6042-merge-rename-corner-cases.sh
@@ -5,7 +5,7 @@ test_description="recursive merge corner cases w/ renames but not criss-crosses"
 
 . ./test-lib.sh
 
-test_expect_success 'setup rename/delete + untracked file' '
+test_setup_rename_delete_untracked () {
 	test_create_repo rename-delete-untracked &&
 	(
 		cd rename-delete-untracked &&
@@ -29,9 +29,10 @@ test_expect_success 'setup rename/delete + untracked file' '
 		git commit -m track-people-instead-of-objects &&
 		echo "Myyy PRECIOUSSS" >ring
 	)
-'
+}
 
 test_expect_success "Does git preserve Gollum's precious artifact?" '
+	test_setup_rename_delete_untracked &&
 	(
 		cd rename-delete-untracked &&
 
@@ -49,7 +50,7 @@ test_expect_success "Does git preserve Gollum's precious artifact?" '
 #
 # We should be able to merge B & C cleanly
 
-test_expect_success 'setup rename/modify/add-source conflict' '
+test_setup_rename_modify_add_source () {
 	test_create_repo rename-modify-add-source &&
 	(
 		cd rename-modify-add-source &&
@@ -70,9 +71,10 @@ test_expect_success 'setup rename/modify/add-source conflict' '
 		git add a &&
 		git commit -m C
 	)
-'
+}
 
 test_expect_failure 'rename/modify/add-source conflict resolvable' '
+	test_setup_rename_modify_add_source &&
 	(
 		cd rename-modify-add-source &&
 
@@ -88,7 +90,7 @@ test_expect_failure 'rename/modify/add-source conflict resolvable' '
 	)
 '
 
-test_expect_success 'setup resolvable conflict missed if rename missed' '
+test_setup_break_detection_1 () {
 	test_create_repo break-detection-1 &&
 	(
 		cd break-detection-1 &&
@@ -110,9 +112,10 @@ test_expect_success 'setup resolvable conflict missed if rename missed' '
 		git add a &&
 		git commit -m C
 	)
-'
+}
 
 test_expect_failure 'conflict caused if rename not detected' '
+	test_setup_break_detection_1 &&
 	(
 		cd break-detection-1 &&
 
@@ -135,7 +138,7 @@ test_expect_failure 'conflict caused if rename not detected' '
 	)
 '
 
-test_expect_success 'setup conflict resolved wrong if rename missed' '
+test_setup_break_detection_2 () {
 	test_create_repo break-detection-2 &&
 	(
 		cd break-detection-2 &&
@@ -160,9 +163,10 @@ test_expect_success 'setup conflict resolved wrong if rename missed' '
 		git add a &&
 		git commit -m E
 	)
-'
+}
 
 test_expect_failure 'missed conflict if rename not detected' '
+	test_setup_break_detection_2 &&
 	(
 		cd break-detection-2 &&
 
@@ -182,7 +186,7 @@ test_expect_failure 'missed conflict if rename not detected' '
 #   Commit B: rename a->b
 #   Commit C: rename a->b, add unrelated a
 
-test_expect_success 'setup undetected rename/add-source causes data loss' '
+test_setup_break_detection_3 () {
 	test_create_repo break-detection-3 &&
 	(
 		cd break-detection-3 &&
@@ -202,9 +206,10 @@ test_expect_success 'setup undetected rename/add-source causes data loss' '
 		git add a &&
 		git commit -m C
 	)
-'
+}
 
 test_expect_failure 'detect rename/add-source and preserve all data' '
+	test_setup_break_detection_3 &&
 	(
 		cd break-detection-3 &&
 
@@ -231,6 +236,7 @@ test_expect_failure 'detect rename/add-source and preserve all data' '
 '
 
 test_expect_failure 'detect rename/add-source and preserve all data, merge other way' '
+	test_setup_break_detection_3 &&
 	(
 		cd break-detection-3 &&
 
@@ -256,10 +262,10 @@ test_expect_failure 'detect rename/add-source and preserve all data, merge other
 	)
 '
 
-test_expect_success 'setup content merge + rename/directory conflict' '
-	test_create_repo rename-directory-1 &&
+test_setup_rename_directory () {
+	test_create_repo rename-directory-$1 &&
 	(
-		cd rename-directory-1 &&
+		cd rename-directory-$1 &&
 
 		printf "1\n2\n3\n4\n5\n6\n" >file &&
 		git add file &&
@@ -290,11 +296,12 @@ test_expect_success 'setup content merge + rename/directory conflict' '
 		test_tick &&
 		git commit -m left
 	)
-'
+}
 
 test_expect_success 'rename/directory conflict + clean content merge' '
+	test_setup_rename_directory 1a &&
 	(
-		cd rename-directory-1 &&
+		cd rename-directory-1a &&
 
 		git checkout left-clean^0 &&
 
@@ -320,8 +327,9 @@ test_expect_success 'rename/directory conflict + clean content merge' '
 '
 
 test_expect_success 'rename/directory conflict + content merge conflict' '
+	test_setup_rename_directory 1b &&
 	(
-		cd rename-directory-1 &&
+		cd rename-directory-1b &&
 
 		git reset --hard &&
 		git clean -fdqx &&
@@ -358,7 +366,7 @@ test_expect_success 'rename/directory conflict + content merge conflict' '
 	)
 '
 
-test_expect_success 'setup content merge + rename/directory conflict w/ disappearing dir' '
+test_setup_rename_directory_2 () {
 	test_create_repo rename-directory-2 &&
 	(
 		cd rename-directory-2 &&
@@ -385,9 +393,10 @@ test_expect_success 'setup content merge + rename/directory conflict w/ disappea
 		test_tick &&
 		git commit -m left
 	)
-'
+}
 
 test_expect_success 'disappearing dir in rename/directory conflict handled' '
+	test_setup_rename_directory_2 &&
 	(
 		cd rename-directory-2 &&
 
@@ -416,10 +425,10 @@ test_expect_success 'disappearing dir in rename/directory conflict handled' '
 #   Commit A: rename a->b, modifying b too
 #   Commit B: modify a, add different b
 
-test_expect_success 'setup rename-with-content-merge vs. add' '
-	test_create_repo rename-with-content-merge-and-add &&
+test_setup_rename_with_content_merge_and_add () {
+	test_create_repo rename-with-content-merge-and-add-$1 &&
 	(
-		cd rename-with-content-merge-and-add &&
+		cd rename-with-content-merge-and-add-$1 &&
 
 		test_seq 1 5 >a &&
 		git add a &&
@@ -438,11 +447,12 @@ test_expect_success 'setup rename-with-content-merge vs. add' '
 		git add a b &&
 		git commit -m B
 	)
-'
+}
 
 test_expect_success 'handle rename-with-content-merge vs. add' '
+	test_setup_rename_with_content_merge_and_add AB &&
 	(
-		cd rename-with-content-merge-and-add &&
+		cd rename-with-content-merge-and-add-AB &&
 
 		git checkout A^0 &&
 
@@ -483,8 +493,9 @@ test_expect_success 'handle rename-with-content-merge vs. add' '
 '
 
 test_expect_success 'handle rename-with-content-merge vs. add, merge other way' '
+	test_setup_rename_with_content_merge_and_add BA &&
 	(
-		cd rename-with-content-merge-and-add &&
+		cd rename-with-content-merge-and-add-BA &&
 
 		git reset --hard &&
 		git clean -fdx &&
@@ -539,7 +550,7 @@ test_expect_success 'handle rename-with-content-merge vs. add, merge other way'
 #   * The working copy should have two files, both of form c~<unique>; does it?
 #   * Nothing else should be present.  Is anything?
 
-test_expect_success 'setup rename/rename (2to1) + modify/modify' '
+test_setup_rename_rename_2to1 () {
 	test_create_repo rename-rename-2to1 &&
 	(
 		cd rename-rename-2to1 &&
@@ -562,9 +573,10 @@ test_expect_success 'setup rename/rename (2to1) + modify/modify' '
 		git add a &&
 		git commit -m C
 	)
-'
+}
 
 test_expect_success 'handle rename/rename (2to1) conflict correctly' '
+	test_setup_rename_rename_2to1 &&
 	(
 		cd rename-rename-2to1 &&
 
@@ -610,7 +622,7 @@ test_expect_success 'handle rename/rename (2to1) conflict correctly' '
 #   Commit A: new file: a
 #   Commit B: rename a->b
 #   Commit C: rename a->c
-test_expect_success 'setup simple rename/rename (1to2) conflict' '
+test_setup_rename_rename_1to2 () {
 	test_create_repo rename-rename-1to2 &&
 	(
 		cd rename-rename-1to2 &&
@@ -631,9 +643,10 @@ test_expect_success 'setup simple rename/rename (1to2) conflict' '
 		test_tick &&
 		git commit -m C
 	)
-'
+}
 
 test_expect_success 'merge has correct working tree contents' '
+	test_setup_rename_rename_1to2 &&
 	(
 		cd rename-rename-1to2 &&
 
@@ -667,7 +680,7 @@ test_expect_success 'merge has correct working tree contents' '
 #
 # Merging of B & C should NOT be clean; there's a rename/rename conflict
 
-test_expect_success 'setup rename/rename(1to2)/add-source conflict' '
+test_setup_rename_rename_1to2_add_source_1 () {
 	test_create_repo rename-rename-1to2-add-source-1 &&
 	(
 		cd rename-rename-1to2-add-source-1 &&
@@ -687,9 +700,10 @@ test_expect_success 'setup rename/rename(1to2)/add-source conflict' '
 		git add a &&
 		git commit -m C
 	)
-'
+}
 
 test_expect_failure 'detect conflict with rename/rename(1to2)/add-source merge' '
+	test_setup_rename_rename_1to2_add_source_1 &&
 	(
 		cd rename-rename-1to2-add-source-1 &&
 
@@ -714,7 +728,7 @@ test_expect_failure 'detect conflict with rename/rename(1to2)/add-source merge'
 	)
 '
 
-test_expect_success 'setup rename/rename(1to2)/add-source resolvable conflict' '
+test_setup_rename_rename_1to2_add_source_2 () {
 	test_create_repo rename-rename-1to2-add-source-2 &&
 	(
 		cd rename-rename-1to2-add-source-2 &&
@@ -737,9 +751,10 @@ test_expect_success 'setup rename/rename(1to2)/add-source resolvable conflict' '
 		test_tick &&
 		git commit -m two
 	)
-'
+}
 
 test_expect_failure 'rename/rename/add-source still tracks new a file' '
+	test_setup_rename_rename_1to2_add_source_2 &&
 	(
 		cd rename-rename-1to2-add-source-2 &&
 
@@ -759,7 +774,7 @@ test_expect_failure 'rename/rename/add-source still tracks new a file' '
 	)
 '
 
-test_expect_success 'setup rename/rename(1to2)/add-dest conflict' '
+test_setup_rename_rename_1to2_add_dest () {
 	test_create_repo rename-rename-1to2-add-dest &&
 	(
 		cd rename-rename-1to2-add-dest &&
@@ -784,9 +799,10 @@ test_expect_success 'setup rename/rename(1to2)/add-dest conflict' '
 		test_tick &&
 		git commit -m two
 	)
-'
+}
 
 test_expect_success 'rename/rename/add-dest merge still knows about conflicting file versions' '
+	test_setup_rename_rename_1to2_add_dest &&
 	(
 		cd rename-rename-1to2-add-dest &&
 
@@ -838,7 +854,7 @@ test_expect_success 'rename/rename/add-dest merge still knows about conflicting
 #   Commit B: rename foo->bar
 #   Expected: CONFLICT (rename/add/delete), two-way merged bar
 
-test_expect_success 'rad-setup: rename/add/delete conflict' '
+test_setup_rad () {
 	test_create_repo rad &&
 	(
 		cd rad &&
@@ -860,9 +876,10 @@ test_expect_success 'rad-setup: rename/add/delete conflict' '
 		git mv foo bar &&
 		git commit -m "rename foo to bar"
 	)
-'
+}
 
 test_expect_failure 'rad-check: rename/add/delete conflict' '
+	test_setup_rad &&
 	(
 		cd rad &&
 
@@ -904,7 +921,7 @@ test_expect_failure 'rad-check: rename/add/delete conflict' '
 #   Commit B: rename bar->baz, rm foo
 #   Expected: CONFLICT (rename/rename/delete/delete), two-way merged baz
 
-test_expect_success 'rrdd-setup: rename/rename(2to1)/delete/delete conflict' '
+test_setup_rrdd () {
 	test_create_repo rrdd &&
 	(
 		cd rrdd &&
@@ -927,9 +944,10 @@ test_expect_success 'rrdd-setup: rename/rename(2to1)/delete/delete conflict' '
 		git rm foo &&
 		git commit -m "Rename bar, remove foo"
 	)
-'
+}
 
 test_expect_failure 'rrdd-check: rename/rename(2to1)/delete/delete conflict' '
+	test_setup_rrdd &&
 	(
 		cd rrdd &&
 
@@ -973,7 +991,7 @@ test_expect_failure 'rrdd-check: rename/rename(2to1)/delete/delete conflict' '
 #   Expected: six CONFLICT(rename/rename) messages, each path in two of the
 #             multi-way merged contents found in two, four, six
 
-test_expect_success 'mod6-setup: chains of rename/rename(1to2) and rename/rename(2to1)' '
+test_setup_mod6 () {
 	test_create_repo mod6 &&
 	(
 		cd mod6 &&
@@ -1009,9 +1027,10 @@ test_expect_success 'mod6-setup: chains of rename/rename(1to2) and rename/rename
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
 test_expect_failure 'mod6-check: chains of rename/rename(1to2) and rename/rename(2to1)' '
+	test_setup_mod6 &&
 	(
 		cd mod6 &&
 
@@ -1108,7 +1127,8 @@ test_conflicts_with_adds_and_renames() {
 	#      files.  Is it present?
 	#   4) There should not be any three~* files in the working
 	#      tree
-	test_expect_success "setup simple $sideL/$sideR conflict" '
+	test_setup_collision_conflict () {
+	#test_expect_success "setup simple $sideL/$sideR conflict" '
 		test_create_repo simple_${sideL}_${sideR} &&
 		(
 			cd simple_${sideL}_${sideR} &&
@@ -1185,9 +1205,11 @@ test_conflicts_with_adds_and_renames() {
 			fi &&
 			test_tick && git commit -m R
 		)
-	'
+	#'
+	}
 
 	test_expect_success "check simple $sideL/$sideR conflict" '
+		test_setup_collision_conflict &&
 		(
 			cd simple_${sideL}_${sideR} &&
 
@@ -1254,7 +1276,7 @@ test_conflicts_with_adds_and_renames add    add
 #
 #   So, we have four different conflicting files that all end up at path
 #   'three'.
-test_expect_success 'setup nested conflicts from rename/rename(2to1)' '
+test_setup_nested_conflicts_from_rename_rename () {
 	test_create_repo nested_conflicts_from_rename_rename &&
 	(
 		cd nested_conflicts_from_rename_rename &&
@@ -1305,9 +1327,10 @@ test_expect_success 'setup nested conflicts from rename/rename(2to1)' '
 		git add one three &&
 		test_tick && git commit -m german
 	)
-'
+}
 
 test_expect_success 'check nested conflicts from rename/rename(2to1)' '
+	test_setup_nested_conflicts_from_rename_rename &&
 	(
 		cd nested_conflicts_from_rename_rename &&
 
diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
index 32cdd1f493..bd2f97ba95 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -38,7 +38,7 @@ test_description="recursive merge with directory renames"
 #   Commit B: z/{b,c,d,e/f}
 #   Expected: y/{b,c,d,e/f}
 
-test_expect_success '1a-setup: Simple directory rename detection' '
+test_setup_1a () {
 	test_create_repo 1a &&
 	(
 		cd 1a &&
@@ -67,9 +67,10 @@ test_expect_success '1a-setup: Simple directory rename detection' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '1a-check: Simple directory rename detection' '
+test_expect_success '1a: Simple directory rename detection' '
+	test_setup_1a &&
 	(
 		cd 1a &&
 
@@ -103,7 +104,7 @@ test_expect_success '1a-check: Simple directory rename detection' '
 #   Commit B: y/{b,c,d}
 #   Expected: y/{b,c,d,e}
 
-test_expect_success '1b-setup: Merge a directory with another' '
+test_setup_1b () {
 	test_create_repo 1b &&
 	(
 		cd 1b &&
@@ -134,9 +135,10 @@ test_expect_success '1b-setup: Merge a directory with another' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '1b-check: Merge a directory with another' '
+test_expect_success '1b: Merge a directory with another' '
+	test_setup_1b &&
 	(
 		cd 1b &&
 
@@ -165,7 +167,7 @@ test_expect_success '1b-check: Merge a directory with another' '
 #   Commit B: z/{b,c,d}
 #   Expected: y/{b,c,d}  (because x/d -> z/d -> y/d)
 
-test_expect_success '1c-setup: Transitive renaming' '
+test_setup_1c () {
 	test_create_repo 1c &&
 	(
 		cd 1c &&
@@ -193,9 +195,10 @@ test_expect_success '1c-setup: Transitive renaming' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '1c-check: Transitive renaming' '
+test_expect_success '1c: Transitive renaming' '
+	test_setup_1c &&
 	(
 		cd 1c &&
 
@@ -227,7 +230,7 @@ test_expect_success '1c-check: Transitive renaming' '
 #   Note: y/m & z/n should definitely move into x.  By the same token, both
 #         y/wham_1 & z/wham_2 should too...giving us a conflict.
 
-test_expect_success '1d-setup: Directory renames cause a rename/rename(2to1) conflict' '
+test_setup_1d () {
 	test_create_repo 1d &&
 	(
 		cd 1d &&
@@ -262,9 +265,10 @@ test_expect_success '1d-setup: Directory renames cause a rename/rename(2to1) con
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '1d-check: Directory renames cause a rename/rename(2to1) conflict' '
+test_expect_success '1d: Directory renames cause a rename/rename(2to1) conflict' '
+	test_setup_1d &&
 	(
 		cd 1d &&
 
@@ -313,7 +317,7 @@ test_expect_success '1d-check: Directory renames cause a rename/rename(2to1) con
 #   Commit B: z/{oldb,oldc,d}
 #   Expected: y/{newb,newc,d}
 
-test_expect_success '1e-setup: Renamed directory, with all files being renamed too' '
+test_setup_1e () {
 	test_create_repo 1e &&
 	(
 		cd 1e &&
@@ -342,9 +346,10 @@ test_expect_success '1e-setup: Renamed directory, with all files being renamed t
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '1e-check: Renamed directory, with all files being renamed too' '
+test_expect_success '1e: Renamed directory, with all files being renamed too' '
+	test_setup_1e &&
 	(
 		cd 1e &&
 
@@ -371,7 +376,7 @@ test_expect_success '1e-check: Renamed directory, with all files being renamed t
 #   Commit B: y/{b,c}, x/{d,e,f}
 #   Expected: y/{b,c}, x/{d,e,f,g}
 
-test_expect_success '1f-setup: Split a directory into two other directories' '
+test_setup_1f () {
 	test_create_repo 1f &&
 	(
 		cd 1f &&
@@ -408,9 +413,10 @@ test_expect_success '1f-setup: Split a directory into two other directories' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '1f-check: Split a directory into two other directories' '
+test_expect_success '1f: Split a directory into two other directories' '
+	test_setup_1f &&
 	(
 		cd 1f &&
 
@@ -459,7 +465,7 @@ test_expect_success '1f-check: Split a directory into two other directories' '
 #   Commit A: y/b, w/c
 #   Commit B: z/{b,c,d}
 #   Expected: y/b, w/c, z/d, with warning about z/ -> (y/ vs. w/) conflict
-test_expect_success '2a-setup: Directory split into two on one side, with equal numbers of paths' '
+test_setup_2a () {
 	test_create_repo 2a &&
 	(
 		cd 2a &&
@@ -489,9 +495,10 @@ test_expect_success '2a-setup: Directory split into two on one side, with equal
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '2a-check: Directory split into two on one side, with equal numbers of paths' '
+test_expect_success '2a: Directory split into two on one side, with equal numbers of paths' '
+	test_setup_2a &&
 	(
 		cd 2a &&
 
@@ -520,7 +527,7 @@ test_expect_success '2a-check: Directory split into two on one side, with equal
 #   Commit A: y/b, w/c
 #   Commit B: z/{b,c}, x/d
 #   Expected: y/b, w/c, x/d; No warning about z/ -> (y/ vs. w/) conflict
-test_expect_success '2b-setup: Directory split into two on one side, with equal numbers of paths' '
+test_setup_2b () {
 	test_create_repo 2b &&
 	(
 		cd 2b &&
@@ -551,9 +558,10 @@ test_expect_success '2b-setup: Directory split into two on one side, with equal
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '2b-check: Directory split into two on one side, with equal numbers of paths' '
+test_expect_success '2b: Directory split into two on one side, with equal numbers of paths' '
+	test_setup_2b &&
 	(
 		cd 2b &&
 
@@ -601,7 +609,7 @@ test_expect_success '2b-check: Directory split into two on one side, with equal
 #   Commit A: z/{b,c,d} (no change)
 #   Commit B: y/{b,c}, x/d
 #   Expected: y/{b,c}, x/d
-test_expect_success '3a-setup: Avoid implicit rename if involved as source on other side' '
+test_setup_3a () {
 	test_create_repo 3a &&
 	(
 		cd 3a &&
@@ -632,9 +640,10 @@ test_expect_success '3a-setup: Avoid implicit rename if involved as source on ot
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '3a-check: Avoid implicit rename if involved as source on other side' '
+test_expect_success '3a: Avoid implicit rename if involved as source on other side' '
+	test_setup_3a &&
 	(
 		cd 3a &&
 
@@ -664,7 +673,7 @@ test_expect_success '3a-check: Avoid implicit rename if involved as source on ot
 #         get it involved in directory rename detection.  If it were, we might
 #         end up with CONFLICT:(z/d -> y/d vs. x/d vs. w/d), i.e. a
 #         rename/rename/rename(1to3) conflict, which is just weird.
-test_expect_success '3b-setup: Avoid implicit rename if involved as source on current side' '
+test_setup_3b () {
 	test_create_repo 3b &&
 	(
 		cd 3b &&
@@ -697,9 +706,10 @@ test_expect_success '3b-setup: Avoid implicit rename if involved as source on cu
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '3b-check: Avoid implicit rename if involved as source on current side' '
+test_expect_success '3b: Avoid implicit rename if involved as source on current side' '
+	test_setup_3b &&
 	(
 		cd 3b &&
 
@@ -786,7 +796,7 @@ test_expect_success '3b-check: Avoid implicit rename if involved as source on cu
 #   Expected: y/{b,c,d}, z/{e,f}
 #   NOTE: Even though most files from z moved to y, we don't want f to follow.
 
-test_expect_success '4a-setup: Directory split, with original directory still present' '
+test_setup_4a () {
 	test_create_repo 4a &&
 	(
 		cd 4a &&
@@ -818,9 +828,10 @@ test_expect_success '4a-setup: Directory split, with original directory still pr
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '4a-check: Directory split, with original directory still present' '
+test_expect_success '4a: Directory split, with original directory still present' '
+	test_setup_4a &&
 	(
 		cd 4a &&
 
@@ -874,7 +885,7 @@ test_expect_success '4a-check: Directory split, with original directory still pr
 #         of history, giving us no way to represent this conflict in the
 #         index.
 
-test_expect_success '5a-setup: Merge directories, other side adds files to original and target' '
+test_setup_5a () {
 	test_create_repo 5a &&
 	(
 		cd 5a &&
@@ -907,9 +918,10 @@ test_expect_success '5a-setup: Merge directories, other side adds files to origi
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '5a-check: Merge directories, other side adds files to original and target' '
+test_expect_success '5a: Merge directories, other side adds files to original and target' '
+	test_setup_5a &&
 	(
 		cd 5a &&
 
@@ -948,7 +960,7 @@ test_expect_success '5a-check: Merge directories, other side adds files to origi
 #         cause us to bail on directory rename detection for that path, falling
 #         back to git behavior without the directory rename detection.
 
-test_expect_success '5b-setup: Rename/delete in order to get add/add/add conflict' '
+test_setup_5b () {
 	test_create_repo 5b &&
 	(
 		cd 5b &&
@@ -981,9 +993,10 @@ test_expect_success '5b-setup: Rename/delete in order to get add/add/add conflic
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '5b-check: Rename/delete in order to get add/add/add conflict' '
+test_expect_success '5b: Rename/delete in order to get add/add/add conflict' '
+	test_setup_5b &&
 	(
 		cd 5b &&
 
@@ -1024,7 +1037,7 @@ test_expect_success '5b-check: Rename/delete in order to get add/add/add conflic
 #             y/d are y/d_2 and y/d_4.  We still do the move from z/e to y/e,
 #             though, because it doesn't have anything in the way.
 
-test_expect_success '5c-setup: Transitive rename would cause rename/rename/rename/add/add/add' '
+test_setup_5c () {
 	test_create_repo 5c &&
 	(
 		cd 5c &&
@@ -1061,9 +1074,10 @@ test_expect_success '5c-setup: Transitive rename would cause rename/rename/renam
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '5c-check: Transitive rename would cause rename/rename/rename/add/add/add' '
+test_expect_success '5c: Transitive rename would cause rename/rename/rename/add/add/add' '
+	test_setup_5c &&
 	(
 		cd 5c &&
 
@@ -1113,7 +1127,7 @@ test_expect_success '5c-check: Transitive rename would cause rename/rename/renam
 #         detection for z/d_2, but that doesn't prevent us from applying the
 #         directory rename detection for z/f -> y/f.
 
-test_expect_success '5d-setup: Directory/file/file conflict due to directory rename' '
+test_setup_5d () {
 	test_create_repo 5d &&
 	(
 		cd 5d &&
@@ -1145,9 +1159,10 @@ test_expect_success '5d-setup: Directory/file/file conflict due to directory ren
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '5d-check: Directory/file/file conflict due to directory rename' '
+test_expect_success '5d: Directory/file/file conflict due to directory rename' '
+	test_setup_5d &&
 	(
 		cd 5d &&
 
@@ -1205,7 +1220,7 @@ test_expect_success '5d-check: Directory/file/file conflict due to directory ren
 #         them under y/ doesn't accidentally catch z/d and make it look like
 #         it is also involved in a rename/delete conflict.
 
-test_expect_success '6a-setup: Tricky rename/delete' '
+test_setup_6a () {
 	test_create_repo 6a &&
 	(
 		cd 6a &&
@@ -1235,9 +1250,10 @@ test_expect_success '6a-setup: Tricky rename/delete' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '6a-check: Tricky rename/delete' '
+test_expect_success '6a: Tricky rename/delete' '
+	test_setup_6a &&
 	(
 		cd 6a &&
 
@@ -1271,7 +1287,7 @@ test_expect_success '6a-check: Tricky rename/delete' '
 #         but B did that rename and still decided to put the file into z/,
 #         so we probably shouldn't apply directory rename detection for it.
 
-test_expect_success '6b-setup: Same rename done on both sides' '
+test_setup_6b () {
 	test_create_repo 6b &&
 	(
 		cd 6b &&
@@ -1300,9 +1316,10 @@ test_expect_success '6b-setup: Same rename done on both sides' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '6b-check: Same rename done on both sides' '
+test_expect_success '6b: Same rename done on both sides' '
+	test_setup_6b &&
 	(
 		cd 6b &&
 
@@ -1334,7 +1351,7 @@ test_expect_success '6b-check: Same rename done on both sides' '
 #   NOTE: Seems obvious, but just checking that the implementation doesn't
 #         "accidentally detect a rename" and give us y/{b,c,d}.
 
-test_expect_success '6c-setup: Rename only done on same side' '
+test_setup_6c () {
 	test_create_repo 6c &&
 	(
 		cd 6c &&
@@ -1362,9 +1379,10 @@ test_expect_success '6c-setup: Rename only done on same side' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '6c-check: Rename only done on same side' '
+test_expect_success '6c: Rename only done on same side' '
+	test_setup_6c &&
 	(
 		cd 6c &&
 
@@ -1396,7 +1414,7 @@ test_expect_success '6c-check: Rename only done on same side' '
 #   NOTE: Again, this seems obvious but just checking that the implementation
 #         doesn't "accidentally detect a rename" and give us y/{b,c,d}.
 
-test_expect_success '6d-setup: We do not always want transitive renaming' '
+test_setup_6d () {
 	test_create_repo 6d &&
 	(
 		cd 6d &&
@@ -1424,9 +1442,10 @@ test_expect_success '6d-setup: We do not always want transitive renaming' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '6d-check: We do not always want transitive renaming' '
+test_expect_success '6d: We do not always want transitive renaming' '
+	test_setup_6d &&
 	(
 		cd 6d &&
 
@@ -1458,7 +1477,7 @@ test_expect_success '6d-check: We do not always want transitive renaming' '
 #         doesn't "accidentally detect a rename" and give us y/{b,c} +
 #         add/add conflict on y/d_1 vs y/d_2.
 
-test_expect_success '6e-setup: Add/add from one side' '
+test_setup_6e () {
 	test_create_repo 6e &&
 	(
 		cd 6e &&
@@ -1487,9 +1506,10 @@ test_expect_success '6e-setup: Add/add from one side' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '6e-check: Add/add from one side' '
+test_expect_success '6e: Add/add from one side' '
+	test_setup_6e &&
 	(
 		cd 6e &&
 
@@ -1552,7 +1572,7 @@ test_expect_success '6e-check: Add/add from one side' '
 #   Expected: y/d, CONFLICT(rename/rename for both z/b and z/c)
 #   NOTE: There's a rename of z/ here, y/ has more renames, so z/d -> y/d.
 
-test_expect_success '7a-setup: rename-dir vs. rename-dir (NOT split evenly) PLUS add-other-file' '
+test_setup_7a () {
 	test_create_repo 7a &&
 	(
 		cd 7a &&
@@ -1583,9 +1603,10 @@ test_expect_success '7a-setup: rename-dir vs. rename-dir (NOT split evenly) PLUS
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '7a-check: rename-dir vs. rename-dir (NOT split evenly) PLUS add-other-file' '
+test_expect_success '7a: rename-dir vs. rename-dir (NOT split evenly) PLUS add-other-file' '
+	test_setup_7a &&
 	(
 		cd 7a &&
 
@@ -1623,7 +1644,7 @@ test_expect_success '7a-check: rename-dir vs. rename-dir (NOT split evenly) PLUS
 #   Commit B: z/{b,c,d_1},        w/d_2
 #   Expected: y/{b,c}, CONFLICT(rename/rename(2to1): x/d_1, w/d_2 -> y_d)
 
-test_expect_success '7b-setup: rename/rename(2to1), but only due to transitive rename' '
+test_setup_7b () {
 	test_create_repo 7b &&
 	(
 		cd 7b &&
@@ -1655,9 +1676,10 @@ test_expect_success '7b-setup: rename/rename(2to1), but only due to transitive r
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '7b-check: rename/rename(2to1), but only due to transitive rename' '
+test_expect_success '7b: rename/rename(2to1), but only due to transitive rename' '
+	test_setup_7b &&
 	(
 		cd 7b &&
 
@@ -1702,7 +1724,7 @@ test_expect_success '7b-check: rename/rename(2to1), but only due to transitive r
 #         neither CONFLICT(x/d -> w/d vs. z/d)
 #         nor CONFLiCT x/d -> w/d vs. y/d vs. z/d)
 
-test_expect_success '7c-setup: rename/rename(1to...2or3); transitive rename may add complexity' '
+test_setup_7c () {
 	test_create_repo 7c &&
 	(
 		cd 7c &&
@@ -1732,9 +1754,10 @@ test_expect_success '7c-setup: rename/rename(1to...2or3); transitive rename may
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '7c-check: rename/rename(1to...2or3); transitive rename may add complexity' '
+test_expect_success '7c: rename/rename(1to...2or3); transitive rename may add complexity' '
+	test_setup_7c &&
 	(
 		cd 7c &&
 
@@ -1766,7 +1789,7 @@ test_expect_success '7c-check: rename/rename(1to...2or3); transitive rename may
 #   Expected: y/{b,c}, CONFLICT(delete x/d vs rename to y/d)
 #   NOTE: z->y so NOT CONFLICT(delete x/d vs rename to z/d)
 
-test_expect_success '7d-setup: transitive rename involved in rename/delete; how is it reported?' '
+test_setup_7d () {
 	test_create_repo 7d &&
 	(
 		cd 7d &&
@@ -1796,9 +1819,10 @@ test_expect_success '7d-setup: transitive rename involved in rename/delete; how
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '7d-check: transitive rename involved in rename/delete; how is it reported?' '
+test_expect_success '7d: transitive rename involved in rename/delete; how is it reported?' '
+	test_setup_7d &&
 	(
 		cd 7d &&
 
@@ -1851,7 +1875,7 @@ test_expect_success '7d-check: transitive rename involved in rename/delete; how
 #         see testcases 9c and 9d for further discussion of this issue and
 #         how it's resolved.
 
-test_expect_success '7e-setup: transitive rename in rename/delete AND dirs in the way' '
+test_setup_7e () {
 	test_create_repo 7e &&
 	(
 		cd 7e &&
@@ -1886,9 +1910,10 @@ test_expect_success '7e-setup: transitive rename in rename/delete AND dirs in th
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '7e-check: transitive rename in rename/delete AND dirs in the way' '
+test_expect_success '7e: transitive rename in rename/delete AND dirs in the way' '
+	test_setup_7e &&
 	(
 		cd 7e &&
 
@@ -1945,7 +1970,7 @@ test_expect_success '7e-check: transitive rename in rename/delete AND dirs in th
 # simple rule from section 5 prevents me from handling this as optimally as
 # we potentially could.
 
-test_expect_success '8a-setup: Dual-directory rename, one into the others way' '
+test_setup_8a () {
 	test_create_repo 8a &&
 	(
 		cd 8a &&
@@ -1977,9 +2002,10 @@ test_expect_success '8a-setup: Dual-directory rename, one into the others way' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '8a-check: Dual-directory rename, one into the others way' '
+test_expect_success '8a: Dual-directory rename, one into the others way' '
+	test_setup_8a &&
 	(
 		cd 8a &&
 
@@ -2023,7 +2049,7 @@ test_expect_success '8a-check: Dual-directory rename, one into the others way' '
 # making us fall back to pre-directory-rename-detection behavior for both
 # e_1 and e_2.
 
-test_expect_success '8b-setup: Dual-directory rename, one into the others way, with conflicting filenames' '
+test_setup_8b () {
 	test_create_repo 8b &&
 	(
 		cd 8b &&
@@ -2055,9 +2081,10 @@ test_expect_success '8b-setup: Dual-directory rename, one into the others way, w
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '8b-check: Dual-directory rename, one into the others way, with conflicting filenames' '
+test_expect_success '8b: Dual-directory rename, one into the others way, with conflicting filenames' '
+	test_setup_8b &&
 	(
 		cd 8b &&
 
@@ -2096,7 +2123,7 @@ test_expect_success '8b-check: Dual-directory rename, one into the others way, w
 #         rename/rename(1to2) conflicts -- see testcase 9h.  See also
 #         notes in 8d.
 
-test_expect_success '8c-setup: modify/delete or rename+modify/delete?' '
+test_setup_8c () {
 	test_create_repo 8c &&
 	(
 		cd 8c &&
@@ -2127,9 +2154,10 @@ test_expect_success '8c-setup: modify/delete or rename+modify/delete?' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '8c-check: modify/delete or rename+modify/delete' '
+test_expect_success '8c: modify/delete or rename+modify/delete' '
+	test_setup_8c &&
 	(
 		cd 8c &&
 
@@ -2175,7 +2203,7 @@ test_expect_success '8c-check: modify/delete or rename+modify/delete' '
 #   during merging are supposed to be about opposite sides doing things
 #   differently.
 
-test_expect_success '8d-setup: rename/delete...or not?' '
+test_setup_8d () {
 	test_create_repo 8d &&
 	(
 		cd 8d &&
@@ -2204,9 +2232,10 @@ test_expect_success '8d-setup: rename/delete...or not?' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '8d-check: rename/delete...or not?' '
+test_expect_success '8d: rename/delete...or not?' '
+	test_setup_8d &&
 	(
 		cd 8d &&
 
@@ -2250,7 +2279,7 @@ test_expect_success '8d-check: rename/delete...or not?' '
 #        about the ramifications of doing that, I didn't know how to rule out
 #        that opening other weird edge and corner cases so I just punted.
 
-test_expect_success '8e-setup: Both sides rename, one side adds to original directory' '
+test_setup_8e () {
 	test_create_repo 8e &&
 	(
 		cd 8e &&
@@ -2279,9 +2308,10 @@ test_expect_success '8e-setup: Both sides rename, one side adds to original dire
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '8e-check: Both sides rename, one side adds to original directory' '
+test_expect_success '8e: Both sides rename, one side adds to original directory' '
+	test_setup_8e &&
 	(
 		cd 8e &&
 
@@ -2333,7 +2363,7 @@ test_expect_success '8e-check: Both sides rename, one side adds to original dire
 #         of which one had the most paths going to it.  A naive implementation
 #         of that could take the new file in commit B at z/i to x/w/i or x/i.
 
-test_expect_success '9a-setup: Inner renamed directory within outer renamed directory' '
+test_setup_9a () {
 	test_create_repo 9a &&
 	(
 		cd 9a &&
@@ -2366,9 +2396,10 @@ test_expect_success '9a-setup: Inner renamed directory within outer renamed dire
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '9a-check: Inner renamed directory within outer renamed directory' '
+test_expect_success '9a: Inner renamed directory within outer renamed directory' '
+	test_setup_9a &&
 	(
 		cd 9a &&
 
@@ -2404,7 +2435,7 @@ test_expect_success '9a-check: Inner renamed directory within outer renamed dire
 #   Commit B: z/{b,c,d_3}
 #   Expected: y/{b,c,d_merged}
 
-test_expect_success '9b-setup: Transitive rename with content merge' '
+test_setup_9b () {
 	test_create_repo 9b &&
 	(
 		cd 9b &&
@@ -2436,9 +2467,10 @@ test_expect_success '9b-setup: Transitive rename with content merge' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '9b-check: Transitive rename with content merge' '
+test_expect_success '9b: Transitive rename with content merge' '
+	test_setup_9b &&
 	(
 		cd 9b &&
 
@@ -2491,7 +2523,7 @@ test_expect_success '9b-check: Transitive rename with content merge' '
 #         away, then ignore that particular rename from the other side of
 #         history for any implicit directory renames.
 
-test_expect_success '9c-setup: Doubly transitive rename?' '
+test_setup_9c () {
 	test_create_repo 9c &&
 	(
 		cd 9c &&
@@ -2526,9 +2558,10 @@ test_expect_success '9c-setup: Doubly transitive rename?' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '9c-check: Doubly transitive rename?' '
+test_expect_success '9c: Doubly transitive rename?' '
+	test_setup_9c &&
 	(
 		cd 9c &&
 
@@ -2579,7 +2612,7 @@ test_expect_success '9c-check: Doubly transitive rename?' '
 #   simple rules that are consistent with what we need for all the other
 #   testcases and simplifies things for the user.
 
-test_expect_success '9d-setup: N-way transitive rename?' '
+test_setup_9d () {
 	test_create_repo 9d &&
 	(
 		cd 9d &&
@@ -2614,9 +2647,10 @@ test_expect_success '9d-setup: N-way transitive rename?' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '9d-check: N-way transitive rename?' '
+test_expect_success '9d: N-way transitive rename?' '
+	test_setup_9d &&
 	(
 		cd 9d &&
 
@@ -2653,7 +2687,7 @@ test_expect_success '9d-check: N-way transitive rename?' '
 #   Expected: combined/{a,b,c,d,e,f,g,h,i,j,k,l}, CONFLICT(Nto1) warnings,
 #             dir1/yo, dir2/yo, dir3/yo, dirN/yo
 
-test_expect_success '9e-setup: N-to-1 whammo' '
+test_setup_9e () {
 	test_create_repo 9e &&
 	(
 		cd 9e &&
@@ -2696,9 +2730,10 @@ test_expect_success '9e-setup: N-to-1 whammo' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success C_LOCALE_OUTPUT '9e-check: N-to-1 whammo' '
+test_expect_success C_LOCALE_OUTPUT '9e: N-to-1 whammo' '
+	test_setup_9e &&
 	(
 		cd 9e &&
 
@@ -2745,7 +2780,7 @@ test_expect_success C_LOCALE_OUTPUT '9e-check: N-to-1 whammo' '
 #   Commit B: goal/{a,b}/$more_files, goal/c
 #   Expected: priority/{a,b}/$more_files, priority/c
 
-test_expect_success '9f-setup: Renamed directory that only contained immediate subdirs' '
+test_setup_9f () {
 	test_create_repo 9f &&
 	(
 		cd 9f &&
@@ -2774,9 +2809,10 @@ test_expect_success '9f-setup: Renamed directory that only contained immediate s
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '9f-check: Renamed directory that only contained immediate subdirs' '
+test_expect_success '9f: Renamed directory that only contained immediate subdirs' '
+	test_setup_9f &&
 	(
 		cd 9f &&
 
@@ -2809,7 +2845,7 @@ test_expect_success '9f-check: Renamed directory that only contained immediate s
 #   Commit B: goal/{a,b}/$more_files, goal/c
 #   Expected: priority/{alpha,bravo}/$more_files, priority/c
 
-test_expect_success '9g-setup: Renamed directory that only contained immediate subdirs, immediate subdirs renamed' '
+test_setup_9g () {
 	test_create_repo 9g &&
 	(
 		cd 9g &&
@@ -2841,9 +2877,9 @@ test_expect_success '9g-setup: Renamed directory that only contained immediate s
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_failure '9g-check: Renamed directory that only contained immediate subdirs, immediate subdirs renamed' '
+test_expect_failure '9g: Renamed directory that only contained immediate subdirs, immediate subdirs renamed' '
 	(
 		cd 9g &&
 
@@ -2877,7 +2913,7 @@ test_expect_failure '9g-check: Renamed directory that only contained immediate s
 #   Expected: y/{b,c}, x/d_2
 #   NOTE: If we applied the z/ -> y/ rename to z/d, then we'd end up with
 #         a rename/rename(1to2) conflict (z/d -> y/d vs. x/d)
-test_expect_success '9h-setup: Avoid dir rename on merely modified path' '
+test_setup_9h () {
 	test_create_repo 9h &&
 	(
 		cd 9h &&
@@ -2910,9 +2946,10 @@ test_expect_success '9h-setup: Avoid dir rename on merely modified path' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '9h-check: Avoid dir rename on merely modified path' '
+test_expect_success '9h: Avoid dir rename on merely modified path' '
+	test_setup_9h &&
 	(
 		cd 9h &&
 
@@ -2957,7 +2994,7 @@ test_expect_success '9h-check: Avoid dir rename on merely modified path' '
 #   Expected: Aborted Merge +
 #       ERROR_MSG(untracked working tree files would be overwritten by merge)
 
-test_expect_success '10a-setup: Overwrite untracked with normal rename/delete' '
+test_setup_10a () {
 	test_create_repo 10a &&
 	(
 		cd 10a &&
@@ -2983,9 +3020,10 @@ test_expect_success '10a-setup: Overwrite untracked with normal rename/delete' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '10a-check: Overwrite untracked with normal rename/delete' '
+test_expect_success '10a: Overwrite untracked with normal rename/delete' '
+	test_setup_10a &&
 	(
 		cd 10a &&
 
@@ -3021,7 +3059,7 @@ test_expect_success '10a-check: Overwrite untracked with normal rename/delete' '
 #             z/c_1 -> z/d_1 rename recorded at stage 3 for y/d +
 #       ERROR_MSG(refusing to lose untracked file at 'y/d')
 
-test_expect_success '10b-setup: Overwrite untracked with dir rename + delete' '
+test_setup_10b () {
 	test_create_repo 10b &&
 	(
 		cd 10b &&
@@ -3050,9 +3088,10 @@ test_expect_success '10b-setup: Overwrite untracked with dir rename + delete' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '10b-check: Overwrite untracked with dir rename + delete' '
+test_expect_success '10b: Overwrite untracked with dir rename + delete' '
+	test_setup_10b &&
 	(
 		cd 10b &&
 
@@ -3098,10 +3137,10 @@ test_expect_success '10b-check: Overwrite untracked with dir rename + delete' '
 #             y/c~B^0 +
 #             ERROR_MSG(Refusing to lose untracked file at y/c)
 
-test_expect_success '10c-setup: Overwrite untracked with dir rename/rename(1to2)' '
-	test_create_repo 10c &&
+test_setup_10c () {
+	test_create_repo 10c_$1 &&
 	(
-		cd 10c &&
+		cd 10c_$1 &&
 
 		mkdir z x &&
 		echo a >z/a &&
@@ -3128,11 +3167,12 @@ test_expect_success '10c-setup: Overwrite untracked with dir rename/rename(1to2)
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '10c-check: Overwrite untracked with dir rename/rename(1to2)' '
+test_expect_success '10c1: Overwrite untracked with dir rename/rename(1to2)' '
+	test_setup_10c 1 &&
 	(
-		cd 10c &&
+		cd 10c_1 &&
 
 		git checkout A^0 &&
 		echo important >y/c &&
@@ -3163,9 +3203,10 @@ test_expect_success '10c-check: Overwrite untracked with dir rename/rename(1to2)
 	)
 '
 
-test_expect_success '10c-check: Overwrite untracked with dir rename/rename(1to2), other direction' '
+test_expect_success '10c2: Overwrite untracked with dir rename/rename(1to2), other direction' '
+	test_setup_10c 2 &&
 	(
-		cd 10c &&
+		cd 10c_2 &&
 
 		git reset --hard &&
 		git clean -fdqx &&
@@ -3208,7 +3249,7 @@ test_expect_success '10c-check: Overwrite untracked with dir rename/rename(1to2)
 #             CONFLICT(rename/rename) z/c_1 vs x/f_2 -> y/wham
 #             ERROR_MSG(Refusing to lose untracked file at y/wham)
 
-test_expect_success '10d-setup: Delete untracked with dir rename/rename(2to1)' '
+test_setup_10d () {
 	test_create_repo 10d &&
 	(
 		cd 10d &&
@@ -3240,9 +3281,10 @@ test_expect_success '10d-setup: Delete untracked with dir rename/rename(2to1)' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '10d-check: Delete untracked with dir rename/rename(2to1)' '
+test_expect_success '10d: Delete untracked with dir rename/rename(2to1)' '
+	test_setup_10d &&
 	(
 		cd 10d &&
 
@@ -3290,7 +3332,7 @@ test_expect_success '10d-check: Delete untracked with dir rename/rename(2to1)' '
 #   Commit B: z/{a,b,c}
 #   Expected: y/{a,b,c} + untracked z/c
 
-test_expect_success '10e-setup: Does git complain about untracked file that is not really in the way?' '
+test_setup_10e () {
 	test_create_repo 10e &&
 	(
 		cd 10e &&
@@ -3317,9 +3359,9 @@ test_expect_success '10e-setup: Does git complain about untracked file that is n
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_failure '10e-check: Does git complain about untracked file that is not really in the way?' '
+test_expect_failure '10e: Does git complain about untracked file that is not really in the way?' '
 	(
 		cd 10e &&
 
@@ -3371,7 +3413,7 @@ test_expect_failure '10e-check: Does git complain about untracked file that is n
 #             z/c~HEAD with contents of B:z/b_v2,
 #             z/c with uncommitted mods on top of A:z/c_v1
 
-test_expect_success '11a-setup: Avoid losing dirty contents with simple rename' '
+test_setup_11a () {
 	test_create_repo 11a &&
 	(
 		cd 11a &&
@@ -3398,9 +3440,10 @@ test_expect_success '11a-setup: Avoid losing dirty contents with simple rename'
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '11a-check: Avoid losing dirty contents with simple rename' '
+test_expect_success '11a: Avoid losing dirty contents with simple rename' '
+	test_setup_11a &&
 	(
 		cd 11a &&
 
@@ -3441,7 +3484,7 @@ test_expect_success '11a-check: Avoid losing dirty contents with simple rename'
 #             ERROR_MSG(Refusing to lose dirty file at z/c)
 
 
-test_expect_success '11b-setup: Avoid losing dirty file involved in directory rename' '
+test_setup_11b () {
 	test_create_repo 11b &&
 	(
 		cd 11b &&
@@ -3470,9 +3513,10 @@ test_expect_success '11b-setup: Avoid losing dirty file involved in directory re
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '11b-check: Avoid losing dirty file involved in directory rename' '
+test_expect_success '11b: Avoid losing dirty file involved in directory rename' '
+	test_setup_11b &&
 	(
 		cd 11b &&
 
@@ -3515,7 +3559,7 @@ test_expect_success '11b-check: Avoid losing dirty file involved in directory re
 #   Expected: Abort_msg("following files would be overwritten by merge") +
 #             y/c left untouched (still has uncommitted mods)
 
-test_expect_success '11c-setup: Avoid losing not-uptodate with rename + D/F conflict' '
+test_setup_11c () {
 	test_create_repo 11c &&
 	(
 		cd 11c &&
@@ -3545,9 +3589,10 @@ test_expect_success '11c-setup: Avoid losing not-uptodate with rename + D/F conf
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '11c-check: Avoid losing not-uptodate with rename + D/F conflict' '
+test_expect_success '11c: Avoid losing not-uptodate with rename + D/F conflict' '
+	test_setup_11c &&
 	(
 		cd 11c &&
 
@@ -3581,7 +3626,7 @@ test_expect_success '11c-check: Avoid losing not-uptodate with rename + D/F conf
 #             Warning_Msg("Refusing to lose dirty file at z/c) +
 #             y/{a,c~HEAD,c/d}, x/b, now-untracked z/c_v1 with uncommitted mods
 
-test_expect_success '11d-setup: Avoid losing not-uptodate with rename + D/F conflict' '
+test_setup_11d () {
 	test_create_repo 11d &&
 	(
 		cd 11d &&
@@ -3612,9 +3657,10 @@ test_expect_success '11d-setup: Avoid losing not-uptodate with rename + D/F conf
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '11d-check: Avoid losing not-uptodate with rename + D/F conflict' '
+test_expect_success '11d: Avoid losing not-uptodate with rename + D/F conflict' '
+	test_setup_11d &&
 	(
 		cd 11d &&
 
@@ -3659,7 +3705,7 @@ test_expect_success '11d-check: Avoid losing not-uptodate with rename + D/F conf
 #             y/c~HEAD has A:y/c_2 contents
 #             y/c has dirty file from before merge
 
-test_expect_success '11e-setup: Avoid deleting not-uptodate with dir rename/rename(1to2)/add' '
+test_setup_11e () {
 	test_create_repo 11e &&
 	(
 		cd 11e &&
@@ -3691,9 +3737,10 @@ test_expect_success '11e-setup: Avoid deleting not-uptodate with dir rename/rena
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '11e-check: Avoid deleting not-uptodate with dir rename/rename(1to2)/add' '
+test_expect_success '11e: Avoid deleting not-uptodate with dir rename/rename(1to2)/add' '
+	test_setup_11e &&
 	(
 		cd 11e &&
 
@@ -3744,7 +3791,7 @@ test_expect_success '11e-check: Avoid deleting not-uptodate with dir rename/rena
 #             CONFLICT(rename/rename) x/c vs x/d -> y/wham
 #             ERROR_MSG(Refusing to lose dirty file at y/wham)
 
-test_expect_success '11f-setup: Avoid deleting not-uptodate with dir rename/rename(2to1)' '
+test_setup_11f () {
 	test_create_repo 11f &&
 	(
 		cd 11f &&
@@ -3773,9 +3820,10 @@ test_expect_success '11f-setup: Avoid deleting not-uptodate with dir rename/rena
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '11f-check: Avoid deleting not-uptodate with dir rename/rename(2to1)' '
+test_expect_success '11f: Avoid deleting not-uptodate with dir rename/rename(2to1)' '
+	test_setup_11f &&
 	(
 		cd 11f &&
 
@@ -3832,7 +3880,7 @@ test_expect_success '11f-check: Avoid deleting not-uptodate with dir rename/rena
 #   Commit B: node1/{leaf1,leaf2,leaf5}, node2/{leaf3,leaf4,leaf6}
 #   Expected: node1/{leaf1,leaf2,leaf5,node2/{leaf3,leaf4,leaf6}}
 
-test_expect_success '12a-setup: Moving one directory hierarchy into another' '
+test_setup_12a () {
 	test_create_repo 12a &&
 	(
 		cd 12a &&
@@ -3862,9 +3910,10 @@ test_expect_success '12a-setup: Moving one directory hierarchy into another' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '12a-check: Moving one directory hierarchy into another' '
+test_expect_success '12a: Moving one directory hierarchy into another' '
+	test_setup_12a &&
 	(
 		cd 12a &&
 
@@ -3910,7 +3959,7 @@ test_expect_success '12a-check: Moving one directory hierarchy into another' '
 #         To which, I can do no more than shrug my shoulders and say that
 #         even simple rules give weird results when given weird inputs.
 
-test_expect_success '12b-setup: Moving two directory hierarchies into each other' '
+test_setup_12b () {
 	test_create_repo 12b &&
 	(
 		cd 12b &&
@@ -3938,9 +3987,10 @@ test_expect_success '12b-setup: Moving two directory hierarchies into each other
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '12b-check: Moving two directory hierarchies into each other' '
+test_expect_success '12b: Moving two directory hierarchies into each other' '
+	test_setup_12b &&
 	(
 		cd 12b &&
 
@@ -3976,7 +4026,7 @@ test_expect_success '12b-check: Moving two directory hierarchies into each other
 #   NOTE: This is *exactly* like 12c, except that every path is modified on
 #         each side of the merge.
 
-test_expect_success '12c-setup: Moving one directory hierarchy into another w/ content merge' '
+test_setup_12c () {
 	test_create_repo 12c &&
 	(
 		cd 12c &&
@@ -4008,9 +4058,10 @@ test_expect_success '12c-setup: Moving one directory hierarchy into another w/ c
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '12c-check: Moving one directory hierarchy into another w/ content merge' '
+test_expect_success '12c: Moving one directory hierarchy into another w/ content merge' '
+	test_setup_12c &&
 	(
 		cd 12c &&
 
@@ -4057,7 +4108,7 @@ test_expect_success '12c-check: Moving one directory hierarchy into another w/ c
 #   Commit B: a/b/subdir/foo, a/b/bar
 #   Expected: subdir/foo, bar
 
-test_expect_success '12d-setup: Rename/merge subdir into the root, variant 1' '
+test_setup_12d () {
 	test_create_repo 12d &&
 	(
 		cd 12d &&
@@ -4078,9 +4129,10 @@ test_expect_success '12d-setup: Rename/merge subdir into the root, variant 1' '
 		git checkout B &&
 		test_commit a/b/bar
 	)
-'
+}
 
-test_expect_success '12d-check: Rename/merge subdir into the root, variant 1' '
+test_expect_success '12d: Rename/merge subdir into the root, variant 1' '
+	test_setup_12d &&
 	(
 		cd 12d &&
 
@@ -4114,7 +4166,7 @@ test_expect_success '12d-check: Rename/merge subdir into the root, variant 1' '
 #   Commit B: a/b/foo, a/b/bar
 #   Expected: foo, bar
 
-test_expect_success '12e-setup: Rename/merge subdir into the root, variant 2' '
+test_setup_12e () {
 	test_create_repo 12e &&
 	(
 		cd 12e &&
@@ -4135,9 +4187,10 @@ test_expect_success '12e-setup: Rename/merge subdir into the root, variant 2' '
 		git checkout B &&
 		test_commit a/b/bar
 	)
-'
+}
 
-test_expect_success '12e-check: Rename/merge subdir into the root, variant 2' '
+test_expect_success '12e: Rename/merge subdir into the root, variant 2' '
+	test_setup_12e &&
 	(
 		cd 12e &&
 
@@ -4182,10 +4235,10 @@ test_expect_success '12e-check: Rename/merge subdir into the root, variant 2' '
 #   Commit B: z/{b,c,d,e/f}
 #   Expected: y/{b,c,d,e/f}, with notices/conflicts for both y/d and y/e/f
 
-test_expect_success '13a-setup: messages for newly added files' '
-	test_create_repo 13a &&
+test_setup_13a () {
+	test_create_repo 13a_$1 &&
 	(
-		cd 13a &&
+		cd 13a_$1 &&
 
 		mkdir z &&
 		echo b >z/b &&
@@ -4211,11 +4264,12 @@ test_expect_success '13a-setup: messages for newly added files' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '13a-check(conflict): messages for newly added files' '
+test_expect_success '13a(conflict): messages for newly added files' '
+	test_setup_13a conflict &&
 	(
-		cd 13a &&
+		cd 13a_conflict &&
 
 		git checkout A^0 &&
 
@@ -4235,9 +4289,10 @@ test_expect_success '13a-check(conflict): messages for newly added files' '
 	)
 '
 
-test_expect_success '13a-check(info): messages for newly added files' '
+test_expect_success '13a(info): messages for newly added files' '
+	test_setup_13a info &&
 	(
-		cd 13a &&
+		cd 13a_info &&
 
 		git reset --hard &&
 		git checkout A^0 &&
@@ -4267,10 +4322,10 @@ test_expect_success '13a-check(info): messages for newly added files' '
 #   Expected: y/{b,c,d_merged}, with two conflict messages for y/d,
 #             one about content, and one about file location
 
-test_expect_success '13b-setup: messages for transitive rename with conflicted content' '
-	test_create_repo 13b &&
+test_setup_13b () {
+	test_create_repo 13b_$1 &&
 	(
-		cd 13b &&
+		cd 13b_$1 &&
 
 		mkdir x &&
 		mkdir z &&
@@ -4299,11 +4354,12 @@ test_expect_success '13b-setup: messages for transitive rename with conflicted c
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '13b-check(conflict): messages for transitive rename with conflicted content' '
+test_expect_success '13b(conflict): messages for transitive rename with conflicted content' '
+	test_setup_13b conflict &&
 	(
-		cd 13b &&
+		cd 13b_conflict &&
 
 		git checkout A^0 &&
 
@@ -4321,9 +4377,10 @@ test_expect_success '13b-check(conflict): messages for transitive rename with co
 	)
 '
 
-test_expect_success '13b-check(info): messages for transitive rename with conflicted content' '
+test_expect_success '13b(info): messages for transitive rename with conflicted content' '
+	test_setup_13b info &&
 	(
-		cd 13b &&
+		cd 13b_info &&
 
 		git reset --hard &&
 		git checkout A^0 &&
@@ -4352,10 +4409,10 @@ test_expect_success '13b-check(info): messages for transitive rename with confli
 #             d and B had full knowledge, but that's a slippery slope as
 #             shown in testcase 13d.
 
-test_expect_success '13c-setup: messages for rename/rename(1to1) via transitive rename' '
-	test_create_repo 13c &&
+test_setup_13c () {
+	test_create_repo 13c_$1 &&
 	(
-		cd 13c &&
+		cd 13c_$1 &&
 
 		mkdir x &&
 		mkdir z &&
@@ -4383,11 +4440,12 @@ test_expect_success '13c-setup: messages for rename/rename(1to1) via transitive
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '13c-check(conflict): messages for rename/rename(1to1) via transitive rename' '
+test_expect_success '13c(conflict): messages for rename/rename(1to1) via transitive rename' '
+	test_setup_13c conflict &&
 	(
-		cd 13c &&
+		cd 13c_conflict &&
 
 		git checkout A^0 &&
 
@@ -4404,9 +4462,10 @@ test_expect_success '13c-check(conflict): messages for rename/rename(1to1) via t
 	)
 '
 
-test_expect_success '13c-check(info): messages for rename/rename(1to1) via transitive rename' '
+test_expect_success '13c(info): messages for rename/rename(1to1) via transitive rename' '
+	test_setup_13c info &&
 	(
-		cd 13c &&
+		cd 13c_info &&
 
 		git reset --hard &&
 		git checkout A^0 &&
@@ -4438,10 +4497,10 @@ test_expect_success '13c-check(info): messages for rename/rename(1to1) via trans
 #               * B renames a/y to c/y, and A renames c/->d/ => a/y -> d/y
 #               No conflict in where a/y ends up, so put it in d/y.
 
-test_expect_success '13d-setup: messages for rename/rename(1to1) via dual transitive rename' '
-	test_create_repo 13d &&
+test_setup_13d () {
+	test_create_repo 13d_$1 &&
 	(
-		cd 13d &&
+		cd 13d_$1 &&
 
 		mkdir a &&
 		mkdir b &&
@@ -4470,11 +4529,12 @@ test_expect_success '13d-setup: messages for rename/rename(1to1) via dual transi
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '13d-check(conflict): messages for rename/rename(1to1) via dual transitive rename' '
+test_expect_success '13d(conflict): messages for rename/rename(1to1) via dual transitive rename' '
+	test_setup_13d conflict &&
 	(
-		cd 13d &&
+		cd 13d_conflict &&
 
 		git checkout A^0 &&
 
@@ -4494,9 +4554,10 @@ test_expect_success '13d-check(conflict): messages for rename/rename(1to1) via d
 	)
 '
 
-test_expect_success '13d-check(info): messages for rename/rename(1to1) via dual transitive rename' '
+test_expect_success '13d(info): messages for rename/rename(1to1) via dual transitive rename' '
+	test_setup_13d info &&
 	(
-		cd 13d &&
+		cd 13d_info &&
 
 		git reset --hard &&
 		git checkout A^0 &&
@@ -4562,7 +4623,7 @@ test_expect_success '13d-check(info): messages for rename/rename(1to1) via dual
 #          in the outer merge for this special kind of setup, but it at
 #          least avoids hitting a BUG().
 #
-test_expect_success '13e-setup: directory rename detection in recursive case' '
+test_setup_13e () {
 	test_create_repo 13e &&
 	(
 		cd 13e &&
@@ -4607,9 +4668,10 @@ test_expect_success '13e-setup: directory rename detection in recursive case' '
 		test_tick &&
 		git commit -m "D"
 	)
-'
+}
 
-test_expect_success '13e-check: directory rename detection in recursive case' '
+test_expect_success '13e: directory rename detection in recursive case' '
+	test_setup_13e &&
 	(
 		cd 13e &&
 
diff --git a/t/t6046-merge-skip-unneeded-updates.sh b/t/t6046-merge-skip-unneeded-updates.sh
index 3a47623ed3..b7e4669832 100755
--- a/t/t6046-merge-skip-unneeded-updates.sh
+++ b/t/t6046-merge-skip-unneeded-updates.sh
@@ -36,10 +36,10 @@ test_description="merge cases"
 #   Commit B: b_3
 #   Expected: b_2
 
-test_expect_success '1a-setup: Modify(A)/Modify(B), change on B subset of A' '
-	test_create_repo 1a &&
+test_setup_1a () {
+	test_create_repo 1a_$1 &&
 	(
-		cd 1a &&
+		cd 1a_$1 &&
 
 		test_write_lines 1 2 3 4 5 6 7 8 9 10 >b &&
 		git add b &&
@@ -62,13 +62,12 @@ test_expect_success '1a-setup: Modify(A)/Modify(B), change on B subset of A' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '1a-check-L: Modify(A)/Modify(B), change on B subset of A' '
-	test_when_finished "git -C 1a reset --hard" &&
-	test_when_finished "git -C 1a clean -fd" &&
+test_expect_success '1a-L: Modify(A)/Modify(B), change on B subset of A' '
+	test_setup_1a L &&
 	(
-		cd 1a &&
+		cd 1a_L &&
 
 		git checkout A^0 &&
 
@@ -96,11 +95,10 @@ test_expect_success '1a-check-L: Modify(A)/Modify(B), change on B subset of A' '
 	)
 '
 
-test_expect_success '1a-check-R: Modify(A)/Modify(B), change on B subset of A' '
-	test_when_finished "git -C 1a reset --hard" &&
-	test_when_finished "git -C 1a clean -fd" &&
+test_expect_success '1a-R: Modify(A)/Modify(B), change on B subset of A' '
+	test_setup_1a R &&
 	(
-		cd 1a &&
+		cd 1a_R &&
 
 		git checkout B^0 &&
 
@@ -133,10 +131,10 @@ test_expect_success '1a-check-R: Modify(A)/Modify(B), change on B subset of A' '
 #   Commit B: c_1
 #   Expected: c_2
 
-test_expect_success '2a-setup: Modify(A)/rename(B)' '
-	test_create_repo 2a &&
+test_setup_2a () {
+	test_create_repo 2a_$1 &&
 	(
-		cd 2a &&
+		cd 2a_$1 &&
 
 		test_seq 1 10 >b &&
 		git add b &&
@@ -158,13 +156,12 @@ test_expect_success '2a-setup: Modify(A)/rename(B)' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '2a-check-L: Modify/rename, merge into modify side' '
-	test_when_finished "git -C 2a reset --hard" &&
-	test_when_finished "git -C 2a clean -fd" &&
+test_expect_success '2a-L: Modify/rename, merge into modify side' '
+	test_setup_2a L &&
 	(
-		cd 2a &&
+		cd 2a_L &&
 
 		git checkout A^0 &&
 
@@ -189,11 +186,10 @@ test_expect_success '2a-check-L: Modify/rename, merge into modify side' '
 	)
 '
 
-test_expect_success '2a-check-R: Modify/rename, merge into rename side' '
-	test_when_finished "git -C 2a reset --hard" &&
-	test_when_finished "git -C 2a clean -fd" &&
+test_expect_success '2a-R: Modify/rename, merge into rename side' '
+	test_setup_2a R &&
 	(
-		cd 2a &&
+		cd 2a_R &&
 
 		git checkout B^0 &&
 
@@ -224,10 +220,10 @@ test_expect_success '2a-check-R: Modify/rename, merge into rename side' '
 #   Commit B: b_3
 #   Expected: c_2
 
-test_expect_success '2b-setup: Rename+Mod(A)/Mod(B), B mods subset of A' '
-	test_create_repo 2b &&
+test_setup_2b () {
+	test_create_repo 2b_$1 &&
 	(
-		cd 2b &&
+		cd 2b_$1 &&
 
 		test_write_lines 1 2 3 4 5 6 7 8 9 10 >b &&
 		git add b &&
@@ -251,13 +247,12 @@ test_expect_success '2b-setup: Rename+Mod(A)/Mod(B), B mods subset of A' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '2b-check-L: Rename+Mod(A)/Mod(B), B mods subset of A' '
-	test_when_finished "git -C 2b reset --hard" &&
-	test_when_finished "git -C 2b clean -fd" &&
+test_expect_success '2b-L: Rename+Mod(A)/Mod(B), B mods subset of A' '
+	test_setup_2b L &&
 	(
-		cd 2b &&
+		cd 2b_L &&
 
 		git checkout A^0 &&
 
@@ -288,11 +283,10 @@ test_expect_success '2b-check-L: Rename+Mod(A)/Mod(B), B mods subset of A' '
 	)
 '
 
-test_expect_success '2b-check-R: Rename+Mod(A)/Mod(B), B mods subset of A' '
-	test_when_finished "git -C 2b reset --hard" &&
-	test_when_finished "git -C 2b clean -fd" &&
+test_expect_success '2b-R: Rename+Mod(A)/Mod(B), B mods subset of A' '
+	test_setup_2b R &&
 	(
-		cd 2b &&
+		cd 2b_R &&
 
 		git checkout B^0 &&
 
@@ -332,7 +326,7 @@ test_expect_success '2b-check-R: Rename+Mod(A)/Mod(B), B mods subset of A' '
 #         skip the update, then we're in trouble.  This test verifies we do
 #         not make that particular mistake.
 
-test_expect_success '2c-setup: Modify b & add c VS rename b->c' '
+test_setup_2c () {
 	test_create_repo 2c &&
 	(
 		cd 2c &&
@@ -358,9 +352,10 @@ test_expect_success '2c-setup: Modify b & add c VS rename b->c' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '2c-check: Modify b & add c VS rename b->c' '
+test_expect_success '2c: Modify b & add c VS rename b->c' '
+	test_setup_2c &&
 	(
 		cd 2c &&
 
@@ -428,10 +423,10 @@ test_expect_success '2c-check: Modify b & add c VS rename b->c' '
 #   Commit B: bq_1, bar/whatever
 #   Expected: bar/{bq_2, whatever}
 
-test_expect_success '3a-setup: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
-	test_create_repo 3a &&
+test_setup_3a () {
+	test_create_repo 3a_$1 &&
 	(
-		cd 3a &&
+		cd 3a_$1 &&
 
 		mkdir foo &&
 		test_seq 1 10 >bq &&
@@ -456,13 +451,12 @@ test_expect_success '3a-setup: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '3a-check-L: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
-	test_when_finished "git -C 3a reset --hard" &&
-	test_when_finished "git -C 3a clean -fd" &&
+test_expect_success '3a-L: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
+	test_setup_3a L &&
 	(
-		cd 3a &&
+		cd 3a_L &&
 
 		git checkout A^0 &&
 
@@ -487,11 +481,10 @@ test_expect_success '3a-check-L: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
 	)
 '
 
-test_expect_success '3a-check-R: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
-	test_when_finished "git -C 3a reset --hard" &&
-	test_when_finished "git -C 3a clean -fd" &&
+test_expect_success '3a-R: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
+	test_setup_3a R &&
 	(
-		cd 3a &&
+		cd 3a_R &&
 
 		git checkout B^0 &&
 
@@ -522,10 +515,10 @@ test_expect_success '3a-check-R: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
 #   Commit B: bq_2, bar/whatever
 #   Expected: bar/{bq_2, whatever}
 
-test_expect_success '3b-setup: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
-	test_create_repo 3b &&
+test_setup_3b () {
+	test_create_repo 3b_$1 &&
 	(
-		cd 3b &&
+		cd 3b_$1 &&
 
 		mkdir foo &&
 		test_seq 1 10 >bq &&
@@ -550,13 +543,12 @@ test_expect_success '3b-setup: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '3b-check-L: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
-	test_when_finished "git -C 3b reset --hard" &&
-	test_when_finished "git -C 3b clean -fd" &&
+test_expect_success '3b-L: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
+	test_setup_3b L &&
 	(
-		cd 3b &&
+		cd 3b_L &&
 
 		git checkout A^0 &&
 
@@ -581,11 +573,10 @@ test_expect_success '3b-check-L: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
 	)
 '
 
-test_expect_success '3b-check-R: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
-	test_when_finished "git -C 3b reset --hard" &&
-	test_when_finished "git -C 3b clean -fd" &&
+test_expect_success '3b-R: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
+	test_setup_3b R &&
 	(
-		cd 3b &&
+		cd 3b_R &&
 
 		git checkout B^0 &&
 
@@ -621,7 +612,7 @@ test_expect_success '3b-check-R: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
 #   Working copy: b_4
 #   Expected: b_2 for merge, b_4 in working copy
 
-test_expect_success '4a-setup: Change on A, change on B subset of A, dirty mods present' '
+test_setup_4a () {
 	test_create_repo 4a &&
 	(
 		cd 4a &&
@@ -647,7 +638,7 @@ test_expect_success '4a-setup: Change on A, change on B subset of A, dirty mods
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
 # NOTE: For as long as we continue using unpack_trees() without index_only
 #   set to true, it will error out on a case like this claiming the the locally
@@ -655,9 +646,8 @@ test_expect_success '4a-setup: Change on A, change on B subset of A, dirty mods
 #   correct requires doing the merge in-memory first, then realizing that no
 #   updates to the file are necessary, and thus that we can just leave the path
 #   alone.
-test_expect_failure '4a-check: Change on A, change on B subset of A, dirty mods present' '
-	test_when_finished "git -C 4a reset --hard" &&
-	test_when_finished "git -C 4a clean -fd" &&
+test_expect_failure '4a: Change on A, change on B subset of A, dirty mods present' '
+	test_setup_4a &&
 	(
 		cd 4a &&
 
@@ -695,7 +685,7 @@ test_expect_failure '4a-check: Change on A, change on B subset of A, dirty mods
 #   Working copy: c_4
 #   Expected: c_2
 
-test_expect_success '4b-setup: Rename+Mod(A)/Mod(B), change on B subset of A, dirty mods present' '
+test_setup_4b () {
 	test_create_repo 4b &&
 	(
 		cd 4b &&
@@ -722,11 +712,10 @@ test_expect_success '4b-setup: Rename+Mod(A)/Mod(B), change on B subset of A, di
 		test_tick &&
 		git commit -m "B"
 	)
-'
+}
 
-test_expect_success '4b-check: Rename+Mod(A)/Mod(B), change on B subset of A, dirty mods present' '
-	test_when_finished "git -C 4b reset --hard" &&
-	test_when_finished "git -C 4b clean -fd" &&
+test_expect_success '4b: Rename+Mod(A)/Mod(B), change on B subset of A, dirty mods present' '
+	test_setup_4b &&
 	(
 		cd 4b &&
 
-- 
gitgitgadget

Re: [PATCH 2/2] merge-recursive: fix merging a subdirectory into the root directory

[Johannes Schindelin]

Hi Elijah,

On Tue, 22 Oct 2019, Elijah Newren wrote:

> [...]
> Yes, t6043 is pretty long code-wise, and still has over five dozen
> tests after ditching the separate "setup" tests -- but all of those
> tests still run in 3.6s on my box. [...]

$ time sh t6043-*.sh --quiet
not ok 74 - 9g-check: Renamed directory that only contained immediate subdirs, immediate subdirs renamed # TODO known breakage
not ok 87 - 10e-check: Does git complain about untracked file that is not really in the way? # TODO known breakage
# still have 2 known breakage(s)
# passed all remaining 117 test(s)
1..119

real    7m22.393s
user    0m52.115s
sys     3m53.212s

And this is not a slow box. So yes, those extra spawned processes? They
accumulate. Spawning processes is what Linux was optimized for. You're
optimizing for Linux.

Ciao,
Dscho

Re: [PATCH 2/2] merge-recursive: fix merging a subdirectory into the root directory

[Elijah Newren]

On Thu, Oct 24, 2019 at 3:23 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Elijah,
>
> On Tue, 22 Oct 2019, Elijah Newren wrote:
>
> > [...]
> > Yes, t6043 is pretty long code-wise, and still has over five dozen
> > tests after ditching the separate "setup" tests -- but all of those
> > tests still run in 3.6s on my box. [...]
>
> $ time sh t6043-*.sh --quiet
> not ok 74 - 9g-check: Renamed directory that only contained immediate subdirs, immediate subdirs renamed # TODO known breakage
> not ok 87 - 10e-check: Does git complain about untracked file that is not really in the way? # TODO known breakage
> # still have 2 known breakage(s)
> # passed all remaining 117 test(s)
> 1..119
>
> real    7m22.393s
> user    0m52.115s
> sys     3m53.212s
>
> And this is not a slow box. So yes, those extra spawned processes? They
> accumulate. Spawning processes is what Linux was optimized for. You're
> optimizing for Linux.
>
> Ciao,
> Dscho

Wow, I knew it'd be slower on other platforms but I certainly didn't
expect a factor of 122; you've made me eat my words about performance
for this case.

Still, I rely pretty heavily on t6036, t6042, t6043, and t6046 for
sanity in the face of refactoring and rewriting -- and as mentioned
before they have caught refactoring bugs in those areas that appear at
first blush as "overzealous", and have done so multiple times.  So,
what's the alternative -- mark the tests as linux only?  Do that but
also add a second copy that is trimmed down so other platforms can run
that one?  Keep a local copy of all these tests?  Jump on the
our-testing-pyramid-is-inverted bandwagon when it materializes and
provides a way to write unit tests instead of just end-to-end tests
(I'm game for that one...)?  Start discussing crazy ideas like a
busybox-like shell, possibly with git extensions (a "git shell" if you
will, though I know the name is already taken), just for running the
git testsuite faster?  Those alternatives all sound either unappealing
or like very large projects that'll take a while to materialize (and
which I certainly won't be spearheading; I've got too many big
backburnered projects already).  This performance is clearly bad, but
gutting the tests isn't tenable either.

Re: [PATCH 2/2] merge-recursive: fix merging a subdirectory into the root directory

[Johannes Schindelin]

Hi Elijah,

On Thu, 24 Oct 2019, Elijah Newren wrote:

> On Thu, Oct 24, 2019 at 3:23 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Tue, 22 Oct 2019, Elijah Newren wrote:
> >
> > > [...]
> > > Yes, t6043 is pretty long code-wise, and still has over five dozen
> > > tests after ditching the separate "setup" tests -- but all of those
> > > tests still run in 3.6s on my box. [...]
> >
> > $ time sh t6043-*.sh --quiet
> > not ok 74 - 9g-check: Renamed directory that only contained immediate subdirs, immediate subdirs renamed # TODO known breakage
> > not ok 87 - 10e-check: Does git complain about untracked file that is not really in the way? # TODO known breakage
> > # still have 2 known breakage(s)
> > # passed all remaining 117 test(s)
> > 1..119
> >
> > real    7m22.393s
> > user    0m52.115s
> > sys     3m53.212s
> >
> > And this is not a slow box. So yes, those extra spawned processes? They
> > accumulate. Spawning processes is what Linux was optimized for. You're
> > optimizing for Linux.
> >
> > Ciao,
> > Dscho
>
> Wow, I knew it'd be slower on other platforms but I certainly didn't
> expect a factor of 122; you've made me eat my words about performance
> for this case.

I am glad that the numbers are more convincing than I am ;-)

> Still, I rely pretty heavily on t6036, t6042, t6043, and t6046 for
> sanity in the face of refactoring and rewriting -- and as mentioned
> before they have caught refactoring bugs in those areas that appear at
> first blush as "overzealous", and have done so multiple times.  So,
> what's the alternative -- mark the tests as linux only?  Do that but
> also add a second copy that is trimmed down so other platforms can run
> that one?  Keep a local copy of all these tests?  Jump on the
> our-testing-pyramid-is-inverted bandwagon when it materializes and
> provides a way to write unit tests instead of just end-to-end tests
> (I'm game for that one...)?  Start discussing crazy ideas like a
> busybox-like shell, possibly with git extensions (a "git shell" if you
> will, though I know the name is already taken), just for running the
> git testsuite faster?  Those alternatives all sound either unappealing
> or like very large projects that'll take a while to materialize (and
> which I certainly won't be spearheading; I've got too many big
> backburnered projects already).  This performance is clearly bad, but
> gutting the tests isn't tenable either.

One idea would be to try to guard those extra careful tests behind the
`EXPENSIVE` prereq.

I _do_ agree with you that it makes sense, in particular with the
recursive merge code, in particular because you are in the middle of
heavy refactoring, to add really, really overzealous tests.

That really helps getting confident in the changes.

I just don't see that we should pay the price (time-wise, and also
electricity-wise) of running those expensive tests even after the
refactoring, or for that matter, even for unrelated patches that are
more than 99.9% certain not to even touch the recursive merge.

Ciao,
Dscho

Re: [PATCH 2/2] merge-recursive: fix merging a subdirectory into the root directory

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Still, I rely pretty heavily on t6036, t6042, t6043, and t6046 for
>> sanity in the face of refactoring and rewriting -- and as mentioned
>> before they have caught refactoring bugs in those areas that appear at
>> first blush as "overzealous", ...
>
> One idea would be to try to guard those extra careful tests behind the
> `EXPENSIVE` prereq.

Yeah, I like that---I think it is perfectly in line with the spirit
of EXPENSIVE, too.

Re: [PATCH 2/2] merge-recursive: fix merging a subdirectory into the root directory

[Elijah Newren]

On Mon, Oct 28, 2019 at 6:20 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> Still, I rely pretty heavily on t6036, t6042, t6043, and t6046 for
> >> sanity in the face of refactoring and rewriting -- and as mentioned
> >> before they have caught refactoring bugs in those areas that appear at
> >> first blush as "overzealous", ...
> >
> > One idea would be to try to guard those extra careful tests behind the
> > `EXPENSIVE` prereq.
>
> Yeah, I like that---I think it is perfectly in line with the spirit
> of EXPENSIVE, too.

Or perhaps EXPENSIVE_ON_WINDOWS, since it's actually pretty cheap on
linux and not that bad on Mac  However, if we're going down that
route, perhaps t9001-send-email.sh could be wrapped in an EXPENSIVE
prerequisite?  That single test file takes an inordinate percentage of
overall runtime.  One one box with a few extra cpus, that test both
starts first and finishes last...and it's not far from that on even
normal boxes.

Re: [PATCH 2/2] merge-recursive: fix merging a subdirectory into the root directory

[Johannes Schindelin]

Hi Elijah,

On Wed, 30 Oct 2019, Elijah Newren wrote:

> On Mon, Oct 28, 2019 at 6:20 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> > >> Still, I rely pretty heavily on t6036, t6042, t6043, and t6046 for
> > >> sanity in the face of refactoring and rewriting -- and as mentioned
> > >> before they have caught refactoring bugs in those areas that appear at
> > >> first blush as "overzealous", ...
> > >
> > > One idea would be to try to guard those extra careful tests behind the
> > > `EXPENSIVE` prereq.
> >
> > Yeah, I like that---I think it is perfectly in line with the spirit
> > of EXPENSIVE, too.
>
> Or perhaps EXPENSIVE_ON_WINDOWS, since it's actually pretty cheap on
> linux and not that bad on Mac

Why the complexity? If you separate out the expensive tests (even if
they are only expensive in terms of run time on Windows), it will make
the regression tests so much more readable to the occasional reader
(making them less expensive in terms of reading time...).

> However, if we're going down that route, perhaps t9001-send-email.sh
> could be wrapped in an EXPENSIVE prerequisite?  That single test file
> takes an inordinate percentage of overall runtime.  One one box with a
> few extra cpus, that test both starts first and finishes last...and
> it's not far from that on even normal boxes.

I would be okay with that.

No, let me try that again. I would be _totally_ okay with that.

Ciao,
Dscho

Re: [PATCH 2/2] merge-recursive: fix merging a subdirectory into the root directory

[Elijah Newren]

Hi Dscho,

On Wed, Oct 30, 2019 at 3:17 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Elijah,
>
> On Wed, 30 Oct 2019, Elijah Newren wrote:
>
> > On Mon, Oct 28, 2019 at 6:20 PM Junio C Hamano <gitster@pobox.com> wrote:
> > >
> > > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > >
> > > >> Still, I rely pretty heavily on t6036, t6042, t6043, and t6046 for
> > > >> sanity in the face of refactoring and rewriting -- and as mentioned
> > > >> before they have caught refactoring bugs in those areas that appear at
> > > >> first blush as "overzealous", ...
> > > >
> > > > One idea would be to try to guard those extra careful tests behind the
> > > > `EXPENSIVE` prereq.
> > >
> > > Yeah, I like that---I think it is perfectly in line with the spirit
> > > of EXPENSIVE, too.
> >
> > Or perhaps EXPENSIVE_ON_WINDOWS, since it's actually pretty cheap on
> > linux and not that bad on Mac
>
> Why the complexity? If you separate out the expensive tests (even if
> they are only expensive in terms of run time on Windows), it will make
> the regression tests so much more readable to the occasional reader
> (making them less expensive in terms of reading time...).

The "extra careful" things you were complaining about with the new
test I was adding to t6043 was true of every single test in that
file...and likely much of t6036, t6042, and perhaps even t6046 (though
those have fewer tests than t6043).  I have no clue where I'd even
begin to draw the line between them.  If it's possible, it sounds
extremely complex.  Just using the EXPENSIVE_ON_WINDOWS prereq that
already exists would be easy and simple.

Or did you mean you wanted me to duplicate every single test and
attempt to trim down the duplicates somehow?  That'd be a rather large
undertaking that sounds rather unappealing on a few fronts, but maybe
that's what you had in mind?

> > However, if we're going down that route, perhaps t9001-send-email.sh
> > could be wrapped in an EXPENSIVE prerequisite?  That single test file
> > takes an inordinate percentage of overall runtime.  One one box with a
> > few extra cpus, that test both starts first and finishes last...and
> > it's not far from that on even normal boxes.
>
> I would be okay with that.
>
> No, let me try that again. I would be _totally_ okay with that.

Ooh, sweet, sounds like I should propose it.

Re: [PATCH 2/2] merge-recursive: fix merging a subdirectory into the root directory

[Johannes Schindelin]

Hi Elijah,

On Wed, 30 Oct 2019, Elijah Newren wrote:

> On Wed, Oct 30, 2019 at 3:17 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Wed, 30 Oct 2019, Elijah Newren wrote:
> >
> > > On Mon, Oct 28, 2019 at 6:20 PM Junio C Hamano <gitster@pobox.com> wrote:
> > > >
> > > > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > > >
> > > > >> Still, I rely pretty heavily on t6036, t6042, t6043, and t6046 for
> > > > >> sanity in the face of refactoring and rewriting -- and as mentioned
> > > > >> before they have caught refactoring bugs in those areas that appear at
> > > > >> first blush as "overzealous", ...
> > > > >
> > > > > One idea would be to try to guard those extra careful tests behind the
> > > > > `EXPENSIVE` prereq.
> > > >
> > > > Yeah, I like that---I think it is perfectly in line with the spirit
> > > > of EXPENSIVE, too.
> > >
> > > Or perhaps EXPENSIVE_ON_WINDOWS, since it's actually pretty cheap on
> > > linux and not that bad on Mac
> >
> > Why the complexity? If you separate out the expensive tests (even if
> > they are only expensive in terms of run time on Windows), it will make
> > the regression tests so much more readable to the occasional reader
> > (making them less expensive in terms of reading time...).
>
> The "extra careful" things you were complaining about with the new
> test I was adding to t6043 was true of every single test in that
> file...and likely much of t6036, t6042, and perhaps even t6046 (though
> those have fewer tests than t6043).  I have no clue where I'd even
> begin to draw the line between them.  If it's possible, it sounds
> extremely complex.  Just using the EXPENSIVE_ON_WINDOWS prereq that
> already exists would be easy and simple.
>
> Or did you mean you wanted me to duplicate every single test and
> attempt to trim down the duplicates somehow?  That'd be a rather large
> undertaking that sounds rather unappealing on a few fronts, but maybe
> that's what you had in mind?

My suggestion was in reply to your question, and not intended for
already-existing tests. That would indeed require a lot of work, and I
am not sure that it would be worth it.

The suggestion was intended for future patches.

Ciao,
Dscho