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

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

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Oct 12, 2019, 20:37 UTC
Message-ID
<nycvar.QRO.7.76.6.1910122152210.3272@tvgsbejvaqbjf.bet>
In-Reply-To
<37aee862e14b1352eb08485f15ea06bab33679df.1570826543.git.gitgitgadget@gmail.com>
Hi Elijah,
On Fri, 11 Oct 2019, Elijah Newren via GitGitGadget wrote:
Show 12 quoted lines
> 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...

Show 18 quoted lines
> 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.
Show 23 quoted lines
>  	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`?

Show 27 quoted lines
> +		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`.

Show 13 quoted lines
> +
> +		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.

Show 9 quoted lines
> +	(
> +		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?
Show 6 quoted lines
> +
> +		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

Show 9 quoted lines
> +	)
> +'
> +
>  ###########################################################################
>  # SECTION 13: Checking informational and conflict messages
>  #
> --
> gitgitgadget
>
Previous: Elijah Newren via GitGitGadgetNext: Elijah Newren
Message 5 of 21 in “Dir rename fixes”
  1. 0/2 Dir rename fixesElijah Newren via GitGitGadget, Oct 11, 2019
  2. 1/2 merge-recursive: clean up get_renamed_dir_portion()Elijah Newren via GitGitGadget, Oct 11, 2019
  3. Johannes SchindelinOct 12, 2019
  4. 2/2 merge-recursive: fix merging a subdirectory into the root directoryElijah Newren via GitGitGadget, Oct 11, 2019
  5. Johannes SchindelinOct 12, 2019
  6. Elijah NewrenOct 13, 2019
  7. Johannes SchindelinOct 14, 2019
  8. Elijah NewrenOct 22, 2019
  9. Johannes SchindelinOct 24, 2019
  10. Elijah NewrenOct 25, 2019
  11. Johannes SchindelinOct 25, 2019
  12. Junio C HamanoOct 29, 2019
  13. Elijah NewrenOct 30, 2019
  14. Johannes SchindelinOct 30, 2019
  15. Elijah NewrenOct 30, 2019
  16. Johannes SchindelinOct 31, 2019
  17. Johannes SchindelinOct 12, 2019
  18. 0/3 Dir rename fixesElijah Newren via GitGitGadget, Oct 22, 2019
  19. 1/3 merge-recursive: clean up get_renamed_dir_portion()Elijah Newren via GitGitGadget, Oct 22, 2019
  20. 2/3 merge-recursive: fix merging a subdirectory into the root directoryElijah Newren via GitGitGadget, Oct 22, 2019
  21. 3/3 t604[236]: do not run setup in separate testsElijah Newren via GitGitGadget, Oct 22, 2019

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

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