From: Matthieu Moy <matthieu.moy@univ-lyon1.fr>
Date: Sat, 04 Jun 2022 09:44:56 GMT
Subject: Re: [PATCH v2 1/4] t6436: tests how merge behave when there is untracked file with the same content
Message-ID: <2c6ee145-5116-25db-9fbf-bcffe67a9360@univ-lyon1.fr>
In-Reply-To: <dfea1d98c15047428b1a11adbc002eef@SAMBXP02.univ-lyon1.fr>

On 5/27/22 21:55, Jonathan Bressat wrote:
> add test to show explicitly that merge doesn't overwrite untracked files
> or unstaged even when they have the same content than files int the
> merged commit

Nit: capital at the beginning of the sentence, period at the end.

"untracked files or unstaged" -> "untracked or unstaged files"

> +test_expect_success 'create branch A' '
> +	git reset --hard c0 &&
> +	git checkout -b A
> +'
> +
> +test_expect_success 'fastforward will not overwrite untracked file with the same content' '

Git usually spells fast-forward with a hyphen, not fastforward.

> +	test_when_finished "git branch -D B && git reset --hard c0 && git clean --force" &&
> +	git checkout -b B &&
> +	test_commit --no-tag "tracked" file "content" &&
> +	git checkout A &&
> +	echo content >file &&
> +	test_must_fail git merge B

Other tests in the same file test a bit more: the file mustn't be 
touched. It's a very important thing with Git: 99% of the times, when an 
operation fails, it fails before starting any change on-disk, as opposed 
to "I started messing up with your repo, I can't go further, go fix the 
mess yourself" ;-).

The way it's done is by creating a file with the content, using "cp" 
instead of "echo >" and "test_cmp" to check the content.

Other tests also check the absence of .git/MERGE_HEAD, which seems to be 
a sensible thing to do.

> +test_expect_success 'will not overwrite untracked file with the same content' '
> +	test_when_finished "git branch -D B && git reset --hard c0 && git clean --force" &&
> +	git checkout -b B &&
> +	test_commit --no-tag "tracked" file "content" fileB "content" &&
> +	git checkout A &&
> +	test_commit --no-tag "exA" fileA "content" &&
> +	echo content >file &&
> +	test_must_fail git merge B
> +'
> +
> +test_expect_success 'will not overwrite unstaged file with the same content' '
> +	test_when_finished "git branch -D B && git reset --hard c0 && git clean --force" &&
> +	test_commit --no-tag "unstaged" file "other" &&
> +	git checkout -b B &&
> +	test_commit --no-tag "staged" file "content" &&
> +	git checkout A &&
> +	echo content >file &&
> +	test_must_fail git merge B
> +'

As discussed IRL, I think two more cases should be tested:

- index matches commit being merged, but the worktree file doesn't
- worktree file doesn't match content, but index does

in both cases, I'd expect the old and the new behavior to abort the 
merge. Perhaps there are use-cases where one would expect a successful 
merge silently, but for rare corner-cases, it's safe to ask the user to 
fix the situation manually and too much magic can only confuse the user.

-- 
Matthieu Moy
https://matthieu-moy.fr/