Re: [PATCH v2 2/4] merge with untracked file that are the same without failure

[Matthieu Moy <Matthieu.Moy@univ-lyon1.fr>]

On 5/27/22 21:55, Jonathan Bressat wrote:
> Keep the old behavior as default.
> 
> Add the option --overwrite-same-content, when this option is used merge
> will overwrite untracked file that have the same content.
> 
> It make the merge nicer to the user, usefull for a simple utilisation,

make_s_

usefull -> useful

utilisation -> use

> for exemple if you copy and paste files from another project and then

ex_a_mple.

> you decide to pull this project, git will not proceed even if you didn't
> modify those files.

I'd avoid saying "you" in a commit message. "the user" seems clearer to me.

Also, don't use the future to talk about the behavior before the patch, 
it's really confusing. Actually, the commit message talks about the 
previous behavior, but doesn't really document the new one.

> +--overwrite-same-content::
> +       Silently overwrite untracked files that have the same content
> +       and name than files in the merged commit from the merge result.

I don't understand what "in the merged commit from the merge result" means.

Perhaps "overwrite" is not the best name. We actually re-use the file 
without touching it.

> --- /dev/null
> +++ b/t/t7615-merge-untracked.sh

Why a new file? These are minor variants of the ones you just added in 
the previous commit, and would deserve being written next to them.

> +test_expect_success 'fastforward overwrite untracked file that has the same content' '
> +test_expect_success 'fastforward fail when untracked file has different content' '
> +test_expect_success 'normal merge overwrite untracked file that has the same content' '
> +test_expect_success 'normal merge fail when untracked file has different content' '
> +test_expect_success 'merge fail when tracked file modification is unstaged' '

We're making a lot of tests, very similar to each other and very similar 
to other existing ones. I think we've reached the point where we need to 
refactor a bit and write one generic function that covers

- index state : same / different
- worktree state : same / different
- --overwrite-untracked : present / absent
- kind of merge : fast-forward / real merge

and then call this function with the appropriate set of parameters. 
Either the function can be called within tests (each test becoming a 
one-liner), or perhaps the function can call test_expect_success and 
then we can write stg like

for index in same different
do
	for worktree in same different
	do
	...
		run_test_merge $index $worktree ....
	done
done

> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -2257,6 +2257,10 @@ static int check_ok_to_remove(const char *name, int len, int dtype,
>   	if (result) {
>   		if (result->ce_flags & CE_REMOVE)
>   			return 0;
> +	} else if (ce && !ie_modified(o->src_index, ce, st, 0)) {
> +		if(o->overwrite_same_content) {
> +			return 0;
> +		}

This looks good, but honestly I'm a bit lost between o->src_index, 
o->dst_index and o.result, so the review of someone more familiar with 
this part of the codebase would be welcome.

> + * is not tracked, unless it is ignored or it has the same content
> + * than the merged file with the option --overwrite_same_content.

"same content _as_".

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