| Message ID | 20220412191556.21135-2-Jonathan.bressat@etu.univ-lyon1.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Be nicer to the user on tracked/untracked merge conflicts | expand |
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 commit change this > behavior and make git overwrite files if their contents are the same. > This new behaviour is more pleasant for a user and will never be a > frustrating moment. > > Add a if statement that check if the file has the same content as the > merged file thanks to the function ie_modified() (read-cache.c). > ie_modified () checks the status of both files, if they are different, > it verifies their contents. > > Add new tests that need to pass to confirm that the new feature works. > > Co-authored-by: COGONI Guillaume <cogoni.guillaume@gmail.com> > --- > t/t7615-merge-untracked.sh | 79 ++++++++++++++++++++++++++++++++++++++ > unpack-trees.c | 4 ++ > 2 files changed, 83 insertions(+) > create mode 100755 t/t7615-merge-untracked.sh > > diff --git a/t/t7615-merge-untracked.sh b/t/t7615-merge-untracked.sh > new file mode 100755 > index 0000000000..71a34041d2 > --- /dev/null > +++ b/t/t7615-merge-untracked.sh > @@ -0,0 +1,79 @@ > +#!/bin/sh > + > +test_description='test when merge with untracked file' > + > +. ./test-lib.sh > + > + Too much whitespace. > +test_expect_success 'overwrite the file when fastforward and the same content' ' > + echo content >README.md && The coding style in this project is TAB-indent, not 4 spaces > + test_commit "init" README.md && > + git branch A && > + git checkout -b B && > + echo content >file && > + git add file && > + git commit -m "tracked" && Can't these and a lot of the test also just use test_commit, you can do this sort of thing with its multi-param invocation, if you're trying to specifically avoid tags there's an option for that. > + git switch A && > + echo content >file && > + git merge B > +' > + > +test_expect_success 'merge fail with fastforward and different content' ' > + rm * && > + rm -r .git && Can we just set this up in a "git init repo" or whatever instead? > + git init && > + echo content >README.md && > + test_commit "init" README.md && > + git branch A && > + git checkout -b B && > + echo content >file && > + git add file && > + git commit -m "tracked" && > + git switch A && > + echo dif >file && > + test_must_fail git merge B And thendo this in a sub-shell? > +' > + > +test_expect_success 'normal merge with untracked with the same content' ' > + rm * && > + rm -r .git && Please use test_when_finished in the tests themselves for teardown, rather than having the "next test" do the cleanup after the last one. > + git init && > + echo content >README.md && > + test_commit "init" README.md && > + git branch A && > + git checkout -b B && > + echo content >fileB && > + echo content >file && > + git add fileB && > + git add file && > + git commit -m "tracked" && > + git switch A && > + echo content >fileA && > + git add fileA && > + git commit -m "exA" && > + echo content >file && > + git merge B -m "merge" > +' > + > +test_expect_success 'normal merge fail when untracked with different content' ' > + rm * && > + rm -r .git && > + git init && > + echo content >README.md && > + test_commit "init" README.md && > + git branch A && > + git checkout -b B && > + echo content >fileB && > + echo content >file && > + git add fileB && > + git add file && > + git commit -m "tracked" && > + git switch A && > + echo content >fileA && > + git add fileA && > + git commit -m "exA" && > + echo dif >file && > + test_must_fail git merge B -m "merge" > +' > + > +test_done > \ No newline at end of file Git's telling you something here... :) > diff --git a/unpack-trees.c b/unpack-trees.c > index 360844bda3..834aca0da9 100644 > --- a/unpack-trees.c > +++ b/unpack-trees.c > @@ -2259,6 +2259,10 @@ static int check_ok_to_remove(const char *name, int len, int dtype, > return 0; > } > > + if (!ie_modified(&o->result, ce, st, 0)) > + return 0; > + > + Too much whitespace. > return add_rejected_path(o, error_type, name); > }
Jonathan <git.jonathan.bressat@gmail.com> writes: > diff --git a/unpack-trees.c b/unpack-trees.c > index 360844bda3..834aca0da9 100644 > --- a/unpack-trees.c > +++ b/unpack-trees.c > @@ -2259,6 +2259,10 @@ static int check_ok_to_remove(const char *name, int len, int dtype, > return 0; > } > > + if (!ie_modified(&o->result, ce, st, 0)) > + return 0; > + > + > return add_rejected_path(o, error_type, name); > } It probably is better to step back a bit and take a wider look at the original to assess this change. The only two callers of this function appears in this function. /* * We do not want to remove or overwrite a working tree file that * is not tracked, unless it is ignored. */ static int verify_absent_1(const struct cache_entry *ce, enum unpack_trees_error_types error_type, struct unpack_trees_options *o) { ... Notice what the comment in front says? Does this patch change the behaviour from what the comment tells us it does? We should adjust the comment to the new world order if it does. The existing code before the pre-context of the hunk reads like this: /* * The previous round may already have decided to * delete this path, which is in a subdirectory that * is being replaced with a blob. */ result = index_file_exists(&o->result, name, len, 0); if (result) { if (result->ce_flags & CE_REMOVE) return 0; } We've called index_file_exists(), and the new code added here does not take the outcome into account. If we truly care the case "we have _UNTRACKED_ path and it happens to be identical to what we are going to resolve to anyway", shouldn't we be making sure that the <name,len> refers to an untracked path by checking if result is NULL here? If so, if (result) { ... - } + } else if (!ie_modified(...)) { + return 0; + } return add_rejected_path(...); is what you want, perhaps? Or if we have cases where we have a tracked path and ask this function if it is OK to remove, should the same reasoning you invented to deal with untracked paths equally apply? If that is the case, then the code may be OK but the proposed log message to explain and justify the change needs to be updated to explain what happens in such a case (or how such a case will not happen). Thanks.
diff --git a/t/t7615-merge-untracked.sh b/t/t7615-merge-untracked.sh new file mode 100755 index 0000000000..71a34041d2 --- /dev/null +++ b/t/t7615-merge-untracked.sh @@ -0,0 +1,79 @@ +#!/bin/sh + +test_description='test when merge with untracked file' + +. ./test-lib.sh + + +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 && + git commit -m "tracked" && + git switch A && + echo content >file && + git merge B +' + +test_expect_success 'merge fail with fastforward and different content' ' + rm * && + rm -r .git && + git init && + echo content >README.md && + test_commit "init" README.md && + git branch A && + git checkout -b B && + echo content >file && + git add file && + git commit -m "tracked" && + git switch A && + echo dif >file && + test_must_fail git merge B +' + +test_expect_success 'normal merge with untracked with the same content' ' + rm * && + rm -r .git && + git init && + echo content >README.md && + test_commit "init" README.md && + git branch A && + git checkout -b B && + echo content >fileB && + echo content >file && + git add fileB && + git add file && + git commit -m "tracked" && + git switch A && + echo content >fileA && + git add fileA && + git commit -m "exA" && + echo content >file && + git merge B -m "merge" +' + +test_expect_success 'normal merge fail when untracked with different content' ' + rm * && + rm -r .git && + git init && + echo content >README.md && + test_commit "init" README.md && + git branch A && + git checkout -b B && + echo content >fileB && + echo content >file && + git add fileB && + git add file && + git commit -m "tracked" && + git switch A && + echo content >fileA && + git add fileA && + git commit -m "exA" && + echo dif >file && + test_must_fail git merge B -m "merge" +' + +test_done \ No newline at end of file diff --git a/unpack-trees.c b/unpack-trees.c index 360844bda3..834aca0da9 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -2259,6 +2259,10 @@ static int check_ok_to_remove(const char *name, int len, int dtype, return 0; } + if (!ie_modified(&o->result, ce, st, 0)) + return 0; + + return add_rejected_path(o, error_type, name); }