[0/1] Be nicer to the user on tracked/untracked merge conflicts

Message

Jonathan Bressat April 12, 2022, 7:15 p.m. UTC

When doing a merge while there is untracked files with the same name
as merged files, git refuses to proceed. This patch make git overwrite
files if their content are the same.

We added a statement to check_ok_to_remove() (unpack-trees.c) 
with ie_modified() (read-cache.c) to test if the untracked file 
has the same content as the merged one. It seems to work well 
with all three o->result, o->dst_index and o->src_index,
We are not sure of what is the usage of those three, did we used it
properly?

Our tests need some improvement, for example using test_commit,
and testing more possibilities, it's not a real patch, just 
to comfirm if we are on the right track.

The next idea is when it's a fastforward, attempt to merge the
untracked file and the upstream version (like if the file had
just been committed, but without introducing an extra commit).

you can see this idea here: 
https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#Be_nicer_to_the_user_on_tracked.2Funtracked_merge_conflicts

Questions:
The old behaviour was here for technical reasons?
The new behavior that we introduce here become the default one?
If the old behavior was important for some people or for some reasons,
we can set a global variable to switch between the old and the new one.
And if we define a global variable, should we print a warning to let 
users know that there is a new behavior when a merge is called and that
he can switch between the old and new one.
For some reason, test_commit make the merge not working like if it's the
old behaviour of merge, I dont understand why ?

Jonathan (1):
  Merge with untracked file that are the same without failure and test

 t/t7615-merge-untracked.sh | 79 ++++++++++++++++++++++++++++++++++++++
 unpack-trees.c             |  4 ++
 2 files changed, 83 insertions(+)
 create mode 100755 t/t7615-merge-untracked.sh

Comments

Ævar Arnfjörð Bjarmason April 12, 2022, 7:24 p.m. UTC | #1

On Tue, Apr 12 2022, Jonathan wrote:

> When doing a merge while there is untracked files with the same name
> as merged files, git refuses to proceed. This patch make git overwrite
> files if their content are the same.
>
> We added a statement to check_ok_to_remove() (unpack-trees.c) 
> with ie_modified() (read-cache.c) to test if the untracked file 
> has the same content as the merged one. It seems to work well 
> with all three o->result, o->dst_index and o->src_index,
> We are not sure of what is the usage of those three, did we used it
> properly?
>
> Our tests need some improvement, for example using test_commit,
> and testing more possibilities, it's not a real patch, just 
> to comfirm if we are on the right track.
>
> The next idea is when it's a fastforward, attempt to merge the
> untracked file and the upstream version (like if the file had
> just been committed, but without introducing an extra commit).
>
> you can see this idea here: 
> https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#Be_nicer_to_the_user_on_tracked.2Funtracked_merge_conflicts

I left some comments on the patch itself, but structurally it wolud be
really nice to make this and similar changes:

 1. Test for current behavior
 2. Change behavior and relevant (new) tests

Rather than the current one-step, that would also communicate that wiki
link (and better) via code.

> Questions:
> The old behaviour was here for technical reasons?
> The new behavior that we introduce here become the default one?
> If the old behavior was important for some people or for some reasons,
> we can set a global variable to switch between the old and the new one.
> And if we define a global variable, should we print a warning to let 
> users know that there is a new behavior when a merge is called and that
> he can switch between the old and new one.

I don't know if we need a config etc., but FWIW my first reaction to
this is that it's a bit iffy/fragile, i.e. before this we'd basically
error out and say "fix your index/working tree".

But now just because the newly merged content happens to be identical
we'll silently merge it over that "staged" content?

Anyway, I can also see how that would be useful for some people.

I've personally been annoyed by a subset of this behavior in the past, I
can't remember if it's with merge or rebase that we'll refuse to do
anything because we have a locally modified/staged (can't remember) file
"X", even though "X" won't be touched at all if the merge/rebase
happens.

But I haven't wanted git to have quite this level of DWYM behavior in
this area, just my 0.02.

> For some reason, test_commit make the merge not working like if it's the
> old behaviour of merge, I dont understand why ?

Ah, I left some comments on "why not test_commit"...

Do you have an example of such a non-working case? I'm not sure why it
wouldn't work.

Jonathan Bressat April 14, 2022, 8:57 a.m. UTC | #2

On Tue, Apr 12 2022, Ævar Arnfjörð Bjarmason <[e-mail]> wrote:
> Do you have an example of such a non-working case? I'm not sure why it
> wouldn't work.

For exemple this test fail:

+test_expect_success 'overwrite the file when fastforward and the same
content' '
+ echo content >README.md &&
+ test_commit "init" README.md &&
+ git branch A &&
+ git checkout -b B &&
+ echo content >file &&
+ test_commit "tracked" file &&
+ git checkout A &&
+ echo content >file &&
+ git merge B
+'

but this one works:

+test_expect_success 'overwrite the file when fastforward and the same
content' '
+ echo content >README.md &&
+ test_commit "init" README.md &&
+ git branch A &&
+ git checkout -b B &&
+ echo content >file &&
+ git add file &&
+ test_commit "tracked" &&
+ git checkout A &&
+ echo content >file &&
+ git merge B
+'

will send you a new version of our patch soon.
Thanks to your reviews and help.

Jonathan BRESSAT and
Guillaume COGONI