* [WIP]: make merge nicer to the user @ 2022-03-27 15:41 Guillaume Cogoni 2022-04-12 19:15 ` [PATCH 0/1] Be nicer to the user on tracked/untracked merge conflicts Jonathan 0 siblings, 1 reply; 27+ messages in thread From: Guillaume Cogoni @ 2022-03-27 15:41 UTC (permalink / raw) To: git; +Cc: Matthieu.Moy, git.jonathan.bressat, guillaume.cogoni Hi, We were working on a patch to make merge nicer to the user on tracked/untracked merge conflicts. You can see that idea on this page: https://git.wiki.kernel.org/index.php/SmallProjectsIdeas When merging a commit which has tracked files with the same name as local untracked files, Git refuses to proceed. We want to change this behaviour. The idea is to check if the untracked and the tracked file has the same content, so we can overwrite it. Examples of use cases where it can be interesting: The scenarios are the following: A team member is modifying the templates for a website we are working on. They are adding some images to the images directory (but forgets to add them under source control). They are sending the images by mail, later, to me. I'm adding the images under the source control and pushing them to GitHub together with other changes They cannot pull updates from GitHub because Git doesn't want to overwrite their files. Source : https://stackoverflow.com/questions/1125968/how-do-i-force-git-pull-to-overwrite-local-files When using rsync to get files from a distant directory, but then those files are pushed on a repo from the distant directory, you don't want to reset the change when you just need to pull the repo because files are the same. The following parts is our test file: diff --git a/t/t7615-merge-conflict.sh b/t/t7615-merge-conflict.sh new file mode 100644 index 0000000000..4d89fe99ed --- /dev/null +++ b/t/t7615-merge-conflict.sh @@ -0,0 +1,47 @@ +#!/bin/sh +# +# Copyright (c) 2022 Cogoni Guillaume and Bressat Jonathan +# +test_description='merge conflitct' +. ./test-lib.sh + +test_expect_success '[FAST_FORWARD] merge conflict when untracked file and tracked file have the same name and content' ' + echo content >readme.md && + test_commit "README" readme.md && + git branch B && + git checkout -b A && + echo content >file && + test_commit "tracked_file" file && + git switch B && + echo content >file && + test_merge merge A +' + +test_expect_success '[MERGE] merge conflict when untracked file and tracked file have the same name and content' ' + echo content >readme.md && + test_commit "README" readme.md && + git branch A && + git checkout -b B && + echo content1 >file1 && + test_commit "B_tracked_file" file1 && + git checkout A && + echo content2 >file2 && + test_commit "A_tracked_file" file2 && + git switch B && + echo content2 >file2 && + test_merge merge A +' + +test_expect_thatfailure 'merge conflict when untracked file and tracked file have not the same content but the same name' ' + echo content >readme.md && + test_commit "README" readme.md && + git branch B && + git checkout -b A && + echo content1 >file && + test_commit "tracked_file" file && + git switch B && + echo content2 >file && + test_merge merge A +' + +test_done Those tests must have assert in the end but it's just to explain our idea. Our research lead us to these functions: verify_absent_1() from /unpack-trees.c seems to be called for all files and it check if a file from the merged branch exists in the current branch in regard of the name and the path (In our test above, if a file from the branch A exist in the branch B.). Then call check_ok_to_remove() from /unpack-trees.c when an untracked file with the same name than a tracked file on the merged branch is spotted. static int verify_absent_1(const struct cache_entry *ce, enum unpack_trees_error_types error_type, enum absent_checking_type absent_type, struct unpack_trees_options *o); static int check_ok_to_remove(const char *name, int len, int dtype, const struct cache_entry *ce, struct stat *st, enum unpack_trees_error_types error_type, enum absent_checking_type absent_type, struct unpack_trees_options *o); We think that a good way to solve this problem is to check the hash of the tracked and untracked file in check_ok_to_remove and then if they are similar we can overwrite it (return 0). The hash is in ce->object_id. In fact, it's more efficient than decompress the file and check the content. Do you think, we are going in the good way, and is it a good idea ? thanks for your help and review. Guillaume Cogoni and Jonathan Bressat ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 0/1] Be nicer to the user on tracked/untracked merge conflicts 2022-03-27 15:41 [WIP]: make merge nicer to the user Guillaume Cogoni @ 2022-04-12 19:15 ` Jonathan 2022-04-12 19:15 ` [PATCH 1/1] Merge with untracked file that are the same without failure and test Jonathan 2022-04-12 19:24 ` [PATCH 0/1] Be nicer to the user on tracked/untracked merge conflicts Ævar Arnfjörð Bjarmason 0 siblings, 2 replies; 27+ messages in thread From: Jonathan @ 2022-04-12 19:15 UTC (permalink / raw) To: cogoni.guillaume Cc: Matthieu.Moy, git.jonathan.bressat, git, guillaume.cogoni, Jonathan 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 -- 2.35.1.7.gc8609858e0.dirty ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 1/1] Merge with untracked file that are the same without failure and test 2022-04-12 19:15 ` [PATCH 0/1] Be nicer to the user on tracked/untracked merge conflicts Jonathan @ 2022-04-12 19:15 ` Jonathan 2022-04-12 19:21 ` Ævar Arnfjörð Bjarmason 2022-04-13 18:18 ` Junio C Hamano 2022-04-12 19:24 ` [PATCH 0/1] Be nicer to the user on tracked/untracked merge conflicts Ævar Arnfjörð Bjarmason 1 sibling, 2 replies; 27+ messages in thread From: Jonathan @ 2022-04-12 19:15 UTC (permalink / raw) To: cogoni.guillaume Cc: Matthieu.Moy, git.jonathan.bressat, git, guillaume.cogoni, Jonathan 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 + + +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); } -- 2.35.1.7.gc8609858e0.dirty ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 1/1] Merge with untracked file that are the same without failure and test 2022-04-12 19:15 ` [PATCH 1/1] Merge with untracked file that are the same without failure and test Jonathan @ 2022-04-12 19:21 ` Ævar Arnfjörð Bjarmason 2022-04-13 18:18 ` Junio C Hamano 1 sibling, 0 replies; 27+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-04-12 19:21 UTC (permalink / raw) To: Jonathan; +Cc: cogoni.guillaume, Matthieu.Moy, git, guillaume.cogoni, Jonathan 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); > } ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/1] Merge with untracked file that are the same without failure and test 2022-04-12 19:15 ` [PATCH 1/1] Merge with untracked file that are the same without failure and test Jonathan 2022-04-12 19:21 ` Ævar Arnfjörð Bjarmason @ 2022-04-13 18:18 ` Junio C Hamano 2022-04-25 20:27 ` [PATCH v1 0/2] Be nicer to the user on tracked/untracked merge conflicts Jonathan 1 sibling, 1 reply; 27+ messages in thread From: Junio C Hamano @ 2022-04-13 18:18 UTC (permalink / raw) To: Jonathan; +Cc: cogoni.guillaume, Matthieu.Moy, git, guillaume.cogoni, Jonathan 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v1 0/2] Be nicer to the user on tracked/untracked merge conflicts 2022-04-13 18:18 ` Junio C Hamano @ 2022-04-25 20:27 ` Jonathan 2022-04-25 20:27 ` [PATCH v1 1/2] t7615: test how merge behave when there is untracked file Jonathan ` (3 more replies) 0 siblings, 4 replies; 27+ messages in thread From: Jonathan @ 2022-04-25 20:27 UTC (permalink / raw) To: gitster Cc: Jonathan.bressat, Matthieu.Moy, cogoni.guillaume, git.jonathan.bressat, git, guillaume.cogoni Sorry for being a bit slow to answer. Junio C Hamano <gitster@pobox.com> wrote: > if (result) { > ... > - } > + } else if (!ie_modified(...)) { > + return 0; > + } > return add_rejected_path(...); > > is what you want, perhaps? Yes, but that made us ask if it can be a good idea to extend our patch to overwrite all unstaged file. However if we want to overwrite all unstaged file even tracked one just this may not be enough: if (result) { ... } +if (!ie_modified(...)) { + return 0; +} Because with this merge still fail for unstaged file that has the same content, because unstaged file are not exactly treated the same way. Our patch broke some test in t6436-merge-overwrite.sh so we think that we need to modify those tests to make them follow the patch. Thanks for your reviews Jonathan (2): t7615: test how merge behave when there is untracked file merge with untracked file that are the same without failure t/t7615-merge-untracked.sh | 63 ++++++++++++++++++++++++++++++++++++++ unpack-trees.c | 5 ++- 2 files changed, 67 insertions(+), 1 deletion(-) create mode 100755 t/t7615-merge-untracked.sh Interdiff vs v0 : diff --git a/t/t7615-merge-untracked.sh b/t/t7615-merge-untracked.sh index 71a34041d2..99f8bae4c0 100755 --- a/t/t7615-merge-untracked.sh +++ b/t/t7615-merge-untracked.sh @@ -2,78 +2,62 @@ test_description='test when merge with untracked file' +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + . ./test-lib.sh +test_expect_success 'setup' ' + test_commit "init" README.md "content" && + git checkout -b A +' + +test_expect_success 'fastforward overwrite untracked file that has the same content' ' + test_when_finished "git branch -D B && git reset --hard init && git clean --force" && + git checkout -b B && + test_commit --no-tag "tracked" file "content" && + git checkout A && + echo content >file && + git merge B +' -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 'fastforward fail when untracked file has different content' ' + test_when_finished "git branch -D B && git reset --hard init && git clean --force" && + git checkout -b B && + test_commit --no-tag "tracked" file "content" && + git switch A && + echo other >file && + test_must_fail 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 overwrite untracked file that has the same content' ' + test_when_finished "git branch -D B && git reset --hard init && git clean --force" && + git checkout -b B && + test_commit --no-tag "tracked" file "content" fileB "content" && + git switch A && + test_commit --no-tag "exA" fileA "content" && + echo content >file && + 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 file has different content' ' + test_when_finished "git branch -D B && git reset --hard init && git clean --force" && + git checkout -b B && + test_commit --no-tag "tracked" file "content" fileB "content" && + git switch A && + test_commit --no-tag "exA" fileA "content" && + echo dif >file && + test_must_fail git merge B ' -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_expect_success 'merge fail when tracked file modification is unstaged' ' + test_when_finished "git branch -D B && git reset --hard init && git clean --force" && + test_commit --no-tag "unstaged" file "other" && + git checkout -b B && + test_commit --no-tag "staged" file "content" && + git switch A && + echo content >file && + test_must_fail git merge B ' -test_done \ No newline at end of file +test_done diff --git a/unpack-trees.c b/unpack-trees.c index 834aca0da9..61e06c04be 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -2257,18 +2257,17 @@ static int check_ok_to_remove(const char *name, int len, int dtype, if (result) { if (result->ce_flags & CE_REMOVE) return 0; - } - - if (!ie_modified(&o->result, ce, st, 0)) + } else if (!ie_modified(&o->result, ce, st, 0)) { return 0; - + } return add_rejected_path(o, error_type, name); } /* * We do not want to remove or overwrite a working tree file that - * is not tracked, unless it is ignored. + * is not tracked, unless it is ignored and unless it has the same + * content than the merged file. */ static int verify_absent_1(const struct cache_entry *ce, enum unpack_trees_error_types error_type, -- 2.35.1.7.gc8609858e0.dirty ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v1 1/2] t7615: test how merge behave when there is untracked file 2022-04-25 20:27 ` [PATCH v1 0/2] Be nicer to the user on tracked/untracked merge conflicts Jonathan @ 2022-04-25 20:27 ` Jonathan 2022-04-25 20:27 ` [PATCH v1 2/2] merge with untracked file that are the same without failure Jonathan ` (2 subsequent siblings) 3 siblings, 0 replies; 27+ messages in thread From: Jonathan @ 2022-04-25 20:27 UTC (permalink / raw) To: gitster Cc: Jonathan.bressat, Matthieu.Moy, cogoni.guillaume, git.jonathan.bressat, git, guillaume.cogoni when there is untracked file that has the same name than file in the merged branch git refuse to proceed, even when the file has the same content t6436 test a similar thing but not especially with same content file Signed-off-by: Jonathan <git.jonathan.bressat@gmail.com> Signed-off-by: COGONI Guillaume <cogoni.guillaume@gmail.com> --- t/t7615-merge-untracked.sh | 63 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 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..053e6b80ee --- /dev/null +++ b/t/t7615-merge-untracked.sh @@ -0,0 +1,63 @@ +#!/bin/sh + +test_description='test when merge with untracked file' + +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + +. ./test-lib.sh + +test_expect_success 'setup' ' + test_commit "init" README.md "content" && + git checkout -b A +' + +test_expect_success 'fastforward fail when untracked file has the same content' ' + test_when_finished "git branch -D B && git reset --hard init && 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 +' + +test_expect_success 'fastforward fail when untracked file has different content' ' + test_when_finished "git branch -D B && git reset --hard init && git clean --force" && + git checkout -b B && + test_commit --no-tag "tracked" file "content" && + git switch A && + echo other >file && + test_must_fail git merge B +' + +test_expect_success 'normal merge fail when untracked file has the same content' ' + test_when_finished "git branch -D B && git reset --hard init && git clean --force" && + git checkout -b B && + test_commit --no-tag "tracked" file "content" fileB "content" && + git switch A && + test_commit --no-tag "exA" fileA "content" && + echo content >file && + test_must_fail git merge B +' + +test_expect_success 'normal merge fail when untracked file has different content' ' + test_when_finished "git branch -D B && git reset --hard init && git clean --force" && + git checkout -b B && + test_commit --no-tag "tracked" file "content" fileB "content" && + git switch A && + test_commit --no-tag "exA" fileA "content" && + echo dif >file && + test_must_fail git merge B +' + +test_expect_success 'merge fail when tracked file modification is unstaged' ' + test_when_finished "git branch -D B && git reset --hard init && git clean --force" && + test_commit --no-tag "unstaged" file "other" && + git checkout -b B && + test_commit --no-tag "staged" file "content" && + git switch A && + echo content >file && + test_must_fail git merge B +' + +test_done -- 2.35.1.7.gc8609858e0.dirty ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v1 2/2] merge with untracked file that are the same without failure 2022-04-25 20:27 ` [PATCH v1 0/2] Be nicer to the user on tracked/untracked merge conflicts Jonathan 2022-04-25 20:27 ` [PATCH v1 1/2] t7615: test how merge behave when there is untracked file Jonathan @ 2022-04-25 20:27 ` Jonathan 2022-04-25 21:16 ` [PATCH v1 0/2] Be nicer to the user on tracked/untracked merge conflicts Junio C Hamano [not found] ` <eca66375d8b34154856b7da303bf96d7@SAMBXP02.univ-lyon1.fr> 3 siblings, 0 replies; 27+ messages in thread From: Jonathan @ 2022-04-25 20:27 UTC (permalink / raw) To: gitster Cc: Jonathan.bressat, Matthieu.Moy, cogoni.guillaume, git.jonathan.bressat, git, guillaume.cogoni In unpack-trees.c in the check_ok_to_remove() function: add a new statement, if the file has the same content as the merged file, it can be removed. test this new behaviour in t7615. Signed-off-by: Jonathan <git.jonathan.bressat@gmail.com> Signed-off-by: COGONI Guillaume <cogoni.guillaume@gmail.com> --- t/t7615-merge-untracked.sh | 8 ++++---- unpack-trees.c | 5 ++++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/t/t7615-merge-untracked.sh b/t/t7615-merge-untracked.sh index 053e6b80ee..99f8bae4c0 100755 --- a/t/t7615-merge-untracked.sh +++ b/t/t7615-merge-untracked.sh @@ -12,13 +12,13 @@ test_expect_success 'setup' ' git checkout -b A ' -test_expect_success 'fastforward fail when untracked file has the same content' ' +test_expect_success 'fastforward overwrite untracked file that has the same content' ' test_when_finished "git branch -D B && git reset --hard init && 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 + git merge B ' test_expect_success 'fastforward fail when untracked file has different content' ' @@ -30,14 +30,14 @@ test_expect_success 'fastforward fail when untracked file has different content' test_must_fail git merge B ' -test_expect_success 'normal merge fail when untracked file has the same content' ' +test_expect_success 'normal merge overwrite untracked file that has the same content' ' test_when_finished "git branch -D B && git reset --hard init && git clean --force" && git checkout -b B && test_commit --no-tag "tracked" file "content" fileB "content" && git switch A && test_commit --no-tag "exA" fileA "content" && echo content >file && - test_must_fail git merge B + git merge B ' test_expect_success 'normal merge fail when untracked file has different content' ' diff --git a/unpack-trees.c b/unpack-trees.c index 360844bda3..61e06c04be 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -2257,6 +2257,8 @@ 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 (!ie_modified(&o->result, ce, st, 0)) { + return 0; } return add_rejected_path(o, error_type, name); @@ -2264,7 +2266,8 @@ static int check_ok_to_remove(const char *name, int len, int dtype, /* * We do not want to remove or overwrite a working tree file that - * is not tracked, unless it is ignored. + * is not tracked, unless it is ignored and unless it has the same + * content than the merged file. */ static int verify_absent_1(const struct cache_entry *ce, enum unpack_trees_error_types error_type, -- 2.35.1.7.gc8609858e0.dirty ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v1 0/2] Be nicer to the user on tracked/untracked merge conflicts 2022-04-25 20:27 ` [PATCH v1 0/2] Be nicer to the user on tracked/untracked merge conflicts Jonathan 2022-04-25 20:27 ` [PATCH v1 1/2] t7615: test how merge behave when there is untracked file Jonathan 2022-04-25 20:27 ` [PATCH v1 2/2] merge with untracked file that are the same without failure Jonathan @ 2022-04-25 21:16 ` Junio C Hamano 2022-04-25 22:28 ` Guillaume Cogoni [not found] ` <fdd9f13d14e942f3a1572866761b9580@SAMBXP02.univ-lyon1.fr> [not found] ` <eca66375d8b34154856b7da303bf96d7@SAMBXP02.univ-lyon1.fr> 3 siblings, 2 replies; 27+ messages in thread From: Junio C Hamano @ 2022-04-25 21:16 UTC (permalink / raw) To: Jonathan Cc: Jonathan.bressat, Matthieu.Moy, cogoni.guillaume, git, guillaume.cogoni Jonathan <git.jonathan.bressat@gmail.com> writes: > Because with this merge still fail for unstaged file that has the same > content, because unstaged file are not exactly treated the same way. Correct. If you want to do this correctly, you'd need to make sure that you'd clobber untracked files ONLY when you are not losing any information. And even with that, I think some existing users will be hurt with this change in a huge way. They may have untracked change locally because they are not quite done with it yet, and somebody else throws a pull request at them that has the same change as the local modification. They make a trial merge, look at the result, and discard it because there are also unwanted changes in the branch they pulled into. $ git pull $URL $branch ;# responding to the pull request ... examine the result, finding it unsatisfactory ... $ git reset --hard ORIG_HEAD ... now we are back to where we started; well not really ... Now, without this change, "git pull" used to stop until they stashed away the untracked change safely. But with this change, "git pull" will succeed, and then "reset --hard" will discard it together with other changes that came to us from $URL/$branch. They lost their local, uncommitted change. And "but you can pull the equivalent out of $URL/$branch" is not a good excuse. They may not notice the lossage long after having dealt with this pull request (there are busy people who are handling many pull requests from many people) and they have been relying on "git pull" that never clobbers their local uncommitted changes. And when they noticed the lossage, they may not even remember which one of pull requests happened to have an identical change as their local change to cause this lossage, simply because "git pull" that used to stop just continued without a noise. So, I am not sure if this is really a good idea to begin with. It certainly would make it slightly simpler in a trivial case, but it surely looks like a dangerous behaviour change, especially if it is done unconditionally. > Our patch broke some test in t6436-merge-overwrite.sh so we think that > we need to modify those tests to make them follow the patch. Wait. Isn't it backwards? The existing tests _may_ be casting an undesirable current behaviour in stone, but most of the time it is protecting existing user's expectations. If you have an untracked file, you can rest assured that they won't be clobbered by a merge. So we'd need to think twice and carefully examine if it makes sense to update the expectations. I haven't read the change to the tests, so I cannot tell which case it is. Thanks. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 0/2] Be nicer to the user on tracked/untracked merge conflicts 2022-04-25 21:16 ` [PATCH v1 0/2] Be nicer to the user on tracked/untracked merge conflicts Junio C Hamano @ 2022-04-25 22:28 ` Guillaume Cogoni 2022-04-25 23:10 ` Junio C Hamano [not found] ` <fdd9f13d14e942f3a1572866761b9580@SAMBXP02.univ-lyon1.fr> 1 sibling, 1 reply; 27+ messages in thread From: Guillaume Cogoni @ 2022-04-25 22:28 UTC (permalink / raw) To: Junio C Hamano Cc: Jonathan, Jonathan.bressat, Matthieu Moy, git, guillaume.cogoni Junio C Hamano <gitster@pobox.com> writes: > So, I am not sure if this is really a good idea to begin with. It > certainly would make it slightly simpler in a trivial case, but it > surely looks like a dangerous behaviour change, especially if it is > done unconditionally. Can we create a configuration variable to avoid this problem? We keep the old behavior by default, and make a configuration variable for people who wants to have this new behavior, but if the user set the variable a message informs it about the problem that you mention. Or, we add an option like git pull --doSomething. Maybe, we can think about another behaviour. When the user git pull and this error occurs: error: The following untracked working tree files would be overwritten by merge: file1.txt file2.txt Please move or remove them before you merge. Aborting We don't abort, but we prompt a yes/no for each file, if the user wants to remove it. We just make suggestions, we will think more about it. > Wait. Isn't it backwards? The existing tests _may_ be casting an > undesirable current behaviour in stone, but most of the time it is > protecting existing user's expectations. If you have an untracked > file, you can rest assured that they won't be clobbered by a merge. > So we'd need to think twice and carefully examine if it makes sense > to update the expectations. I haven't read the change to the tests, > so I cannot tell which case it is. Yes, we'll figure out a solution, if there is one. Thanks for your review and your quick response, it give us a lot of information, COGONI Guillaume and BRESSAT Jonathan ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 0/2] Be nicer to the user on tracked/untracked merge conflicts 2022-04-25 22:28 ` Guillaume Cogoni @ 2022-04-25 23:10 ` Junio C Hamano 0 siblings, 0 replies; 27+ messages in thread From: Junio C Hamano @ 2022-04-25 23:10 UTC (permalink / raw) To: Guillaume Cogoni Cc: Jonathan, Jonathan.bressat, Matthieu Moy, git, guillaume.cogoni Guillaume Cogoni <cogoni.guillaume@gmail.com> writes: >> So, I am not sure if this is really a good idea to begin with. It >> certainly would make it slightly simpler in a trivial case, but it >> surely looks like a dangerous behaviour change, especially if it is >> done unconditionally. > > Can we create a configuration variable to avoid this problem? > We keep the old behavior by default, and make a configuration variable > for people who wants to have this new behavior, but if the user set the variable > a message informs it about the problem that you mention. > > Or, we add an option like git pull --doSomething. Probably a command line option ("git merge" would probably want the same one) plus a configuration varaible to give it the default (the latter is optional). > Maybe, we can think about another behaviour. > When the user git pull and this error occurs: > error: The following untracked working tree files would be overwritten by merge: > file1.txt > file2.txt > Please move or remove them before you merge. > Aborting > We don't abort, but we prompt a yes/no for each file, if the user > wants to remove it. I doubt this would fly as-is. Especially if the action that is offered by the prompt is "remove", not "move", as that implies we are not prepared against loss of information. There is no indication whether the untracked file1.txt matches the contents we are pulling in. Most of the time, it is very unlikely that the contents being lost is identical to what the other side has, so answering "yes" to the prompt means "No, I do not care about my garbage, and it is OK that it will forever be lost." I do not think we want to be encouraging people to habitually make such a statement. If we move (instead of removing) them away to somewhere, and give users to easily recover them after running "git pull", it might become more palatable. I wonder if this whole thing is an attempt to work around whatever "stash --untracked" fails to do well (or perhaps there are no such shortcomings, but just the users are not made aware of the command enough). If you have these two untracked files (file1.txt and file2.txt) are "in the way" for a merge to succeed, I have to wonder if "Please move or remove" message that was introduced by 23cbf11b (merge-recursive: porcelain messages for checkout, 2010-08-11) is still giving a good piece of advice to users today. Would "git stash push -u file1.txt file2.txt" be an easier and safer alternative that lets you take these files back later? Back in 2010, when 23cbf11b was current, "git stash" was a shell script and it seems there was no "untracked" option, so from that point of view, "move or remove" may have been the best they could do. Note that I never use "git stash" with "untracked" option, so I do not know if it works well in this context already, or we need more work before it becomes usable in this scenario. But it smells like it is exactly what we might want to use in such a situation to stash away these untracked file1.txt and file2.txt while running the merge, while allowing us to recover them after running the merge or discarding it. I dunno. ^ permalink raw reply [flat|nested] 27+ messages in thread
[parent not found: <fdd9f13d14e942f3a1572866761b9580@SAMBXP02.univ-lyon1.fr>]
* Re: [PATCH v1 0/2] Be nicer to the user on tracked/untracked merge conflicts [not found] ` <fdd9f13d14e942f3a1572866761b9580@SAMBXP02.univ-lyon1.fr> @ 2022-04-26 6:38 ` Matthieu Moy 2022-04-26 16:13 ` Junio C Hamano 0 siblings, 1 reply; 27+ messages in thread From: Matthieu Moy @ 2022-04-26 6:38 UTC (permalink / raw) To: Guillaume Cogoni, Junio C Hamano Cc: Jonathan, BRESSAT JONATHAN p1802864, git@vger.kernel.org, guillaume.cogoni@gmail.com On 4/26/22 00:28, Guillaume Cogoni wrote: > Junio C Hamano <gitster@pobox.com> writes: > >> So, I am not sure if this is really a good idea to begin with. It >> certainly would make it slightly simpler in a trivial case, but it >> surely looks like a dangerous behaviour change, especially if it is >> done unconditionally. > > Can we create a configuration variable to avoid this problem? > We keep the old behavior by default, and make a configuration variable > for people who wants to have this new behavior, but if the user set the variable > a message informs it about the problem that you mention. > > Or, we add an option like git pull --doSomething. > > Maybe, we can think about another behaviour. > When the user git pull and this error occurs: > error: The following untracked working tree files would be overwritten by merge: > file1.txt > file2.txt > Please move or remove them before you merge. > Aborting > We don't abort, but we prompt a yes/no for each file, if the user > wants to remove it. Git very rarely goes interactive like this (only a few special command like git send-email, git clean -i, git add -i/-p prompt the user). Prompting the user in the middle of an operation has several drawbacks: - When the command is launched from a script, the script may work most of the time, and sometimes pause on an interactive prompt which wasn't expected from the author of the script. This can be a bit nasty when the script isn't ran from a place where you can type to the standard input of the command or when its output is redirected. - Asking for each individual file can be tedious when there are many files. Similarly, "rm -i" (plain rm, not "git rm") is a nice safety measure, but not really convenient to me at least. In this particular case, actually, I can't imagine a sane behavior when the user wants a mix of "yes" / "no". If a single untracked file conflicts with what's being merged, the merge aborts, even if you're OK to replace other files. So I can only imagine a single yes/no answer. And then the question can be replaced with a suggestion to re-run with a command-line flag when all the conflicting files are unmodified. -- Matthieu Moy https://matthieu-moy.fr/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 0/2] Be nicer to the user on tracked/untracked merge conflicts 2022-04-26 6:38 ` Matthieu Moy @ 2022-04-26 16:13 ` Junio C Hamano 2022-04-28 10:33 ` Jonathan Bressat 0 siblings, 1 reply; 27+ messages in thread From: Junio C Hamano @ 2022-04-26 16:13 UTC (permalink / raw) To: Matthieu Moy Cc: Guillaume Cogoni, Jonathan, BRESSAT JONATHAN p1802864, git@vger.kernel.org, guillaume.cogoni@gmail.com Matthieu Moy <Matthieu.Moy@univ-lyon1.fr> writes: > Git very rarely goes interactive like this (only a few special command > like git send-email, git clean -i, git add -i/-p prompt the user). > > Prompting the user in the middle of an operation has several drawbacks: > ... > In this particular case, actually, I can't imagine a sane behavior > when the user wants a mix of "yes" / "no". If a single untracked file > conflicts with what's being merged, the merge aborts, even if you're > OK to replace other files. So I can only imagine a single yes/no > answer. And then the question can be replaced with a suggestion to > re-run with a command-line flag when all the conflicting files are > unmodified. Nicely explained. Thanks. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 0/2] Be nicer to the user on tracked/untracked merge conflicts 2022-04-26 16:13 ` Junio C Hamano @ 2022-04-28 10:33 ` Jonathan Bressat 2022-05-27 19:55 ` [PATCH v2 0/4] " Jonathan Bressat 0 siblings, 1 reply; 27+ messages in thread From: Jonathan Bressat @ 2022-04-28 10:33 UTC (permalink / raw) To: Junio C Hamano Cc: Matthieu Moy, Guillaume Cogoni, BRESSAT JONATHAN p1802864, git@vger.kernel.org, guillaume.cogoni@gmail.com Junio C Hamano <gitster@pobox.com> writes: > Probably a command line option ("git merge" would probably want the > same one) plus a configuration varaible to give it the default (the > latter is optional). First, we think that add an option to pull and merge is more suited to our situation, and next, it could be good to add the configuration variable In unpack-trees.c there is a list of files that cause problem with merge. We want to split this list to list files that have the same content, then if all the files have the same content, we can suggest to use the option to overwrite those files. Then we can modify the error message to show the files that have the same content apart. > I wonder if this whole thing is an attempt to work around whatever > "stash --untracked" fails to do well (or perhaps there are no such > shortcomings, but just the users are not made aware of the command > enough). If you have these two untracked files (file1.txt and > file2.txt) are "in the way" for a merge to succeed, I have to wonder > if "Please move or remove" message that was introduced by 23cbf11b > (merge-recursive: porcelain messages for checkout, 2010-08-11) is > still giving a good piece of advice to users today. We got a similar idea, but we finally decide that it was not a very good approach because it's not efficient if we have a lot of files or some big files. And because if there are files that doesn't block the merge, we treat them anyway and they will move from the work tree, it's a bit overkill. > Note that I never use "git stash" with "untracked" option, so I do > not know if it works well in this context already, or we need more > work before it becomes usable in this scenario. But it smells like > it is exactly what we might want to use in such a situation to stash > away these untracked file1.txt and file2.txt while running the > merge, while allowing us to recover them after running the merge or > discarding it. I dunno. Indeed, git stash works well with this kind of problem, however an option would be easier in that specific case. Thanks for you're helpfull review, you always give us a lot of good information and ideas. Cogoni Guillaume and Bressat Jonathan ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 0/4] Be nicer to the user on tracked/untracked merge conflicts 2022-04-28 10:33 ` Jonathan Bressat @ 2022-05-27 19:55 ` Jonathan Bressat 2022-05-27 19:55 ` [PATCH v2 1/4] t6436: tests how merge behave when there is untracked file with the same content Jonathan Bressat ` (7 more replies) 0 siblings, 8 replies; 27+ messages in thread From: Jonathan Bressat @ 2022-05-27 19:55 UTC (permalink / raw) To: git.jonathan.bressat Cc: Matthieu.Moy, cogoni.guillaume, git, gitster, guillaume.cogoni, jonathan.bressat We improve our last patch with adding an option to merge and pull command and a configuration variable. This make the user able to modify the behavior of merge when it meet untracked files that have the same content than files in merged branch. We kept the old behavior as default. thanks Jonathan Bressat and Guillaume Cogoni Jonathan Bressat (4): t6436: tests how merge behave when there is untracked file with the same content merge with untracked file that are the same without failure add configuration variable corresponding to --overwrite-same-content error message now advice to use the new option Documentation/config/merge.txt | 5 ++ Documentation/git-merge.txt | 4 ++ builtin/checkout.c | 1 + builtin/merge.c | 9 +++- builtin/pull.c | 8 +++- cache.h | 3 +- merge-ort.c | 1 + merge-recursive.h | 1 + merge.c | 4 +- sequencer.c | 2 +- t/t6436-merge-overwrite.sh | 34 ++++++++++++++ t/t7615-merge-untracked.sh | 84 ++++++++++++++++++++++++++++++++++ unpack-trees.c | 27 +++++++++-- unpack-trees.h | 2 + 14 files changed, 174 insertions(+), 11 deletions(-) create mode 100755 t/t7615-merge-untracked.sh Interdiff contre v1 : diff --git a/Documentation/config/merge.txt b/Documentation/config/merge.txt index 99e83dd36e..2824dd19c7 100644 --- a/Documentation/config/merge.txt +++ b/Documentation/config/merge.txt @@ -89,6 +89,11 @@ merge.autoStash:: `--autostash` options of linkgit:git-merge[1]. Defaults to false. +merge.overwritesamecontent:: + When set to true, it will modify the behavior of git merge + to overwrite untracked files that have the same name and + content than files in the merged commit. + merge.tool:: Controls which merge tool is used by linkgit:git-mergetool[1]. The list below shows the valid built-in values. diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt index 3125473cc1..ceda0271c2 100644 --- a/Documentation/git-merge.txt +++ b/Documentation/git-merge.txt @@ -100,6 +100,10 @@ will be appended to the specified message. Silently overwrite ignored files from the merge result. This is the default behavior. Use `--no-overwrite-ignore` to abort. +--overwrite-same-content:: + Silently overwrite untracked files that have the same content + and name than files in the merged commit from the merge result. + --abort:: Abort the current conflict resolution process, and try to reconstruct the pre-merge state. If an autostash entry is diff --git a/builtin/checkout.c b/builtin/checkout.c index cc804ba8e1..1b1d1813c7 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -760,6 +760,7 @@ static int merge_working_tree(const struct checkout_opts *opts, &new_branch_info->commit->object.oid : &new_branch_info->oid, NULL); topts.preserve_ignored = !opts->overwrite_ignore; + topts.overwrite_same_content = 0;/* FIXME: opts->overwrite_same_content */ tree = parse_tree_indirect(old_branch_info->commit ? &old_branch_info->commit->object.oid : the_hash_algo->empty_tree); diff --git a/builtin/merge.c b/builtin/merge.c index 74e53cf20a..936cb8480d 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -68,6 +68,7 @@ static int option_edit = -1; static int allow_trivial = 1, have_message, verify_signatures; static int check_trust_level = 1; static int overwrite_ignore = 1; +static int overwrite_same_content; static struct strbuf merge_msg = STRBUF_INIT; static struct strategy **use_strategies; static size_t use_strategies_nr, use_strategies_alloc; @@ -305,6 +306,7 @@ static struct option builtin_merge_options[] = { OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")), OPT_BOOL(0, "signoff", &signoff, N_("add a Signed-off-by trailer")), OPT_BOOL(0, "no-verify", &no_verify, N_("bypass pre-merge-commit and commit-msg hooks")), + OPT_BOOL(0, "overwrite-same-content", &overwrite_same_content, N_("overwrite untracked file with the same content and name")), OPT_END() }; @@ -656,6 +658,8 @@ static int git_merge_config(const char *k, const char *v, void *cb) } else if (!strcmp(k, "merge.autostash")) { autostash = git_config_bool(k, v); return 0; + } else if (!strcmp(k,"merge.overwritesamecontent")) { + overwrite_same_content = git_config_bool(k, v); } status = fmt_merge_msg_config(k, v, cb); @@ -684,6 +688,7 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head, opts.trivial_merges_only = 1; opts.merge = 1; opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */ + opts.overwrite_same_content = overwrite_same_content; trees[nr_trees] = parse_tree_indirect(common); if (!trees[nr_trees++]) return -1; @@ -746,6 +751,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common, o.branch1 = head_arg; o.branch2 = merge_remote_util(remoteheads->item)->name; + o.overwrite_same_content = overwrite_same_content; for (j = common; j; j = j->next) commit_list_insert(j->item, &reversed); @@ -1573,7 +1579,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix) if (checkout_fast_forward(the_repository, &head_commit->object.oid, &commit->object.oid, - overwrite_ignore)) { + overwrite_ignore, + overwrite_same_content)) { apply_autostash(git_path_merge_autostash(the_repository)); ret = 1; goto done; diff --git a/builtin/pull.c b/builtin/pull.c index 100cbf9fb8..46ef68e721 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -92,6 +92,7 @@ static struct strvec opt_strategies = STRVEC_INIT; static struct strvec opt_strategy_opts = STRVEC_INIT; static char *opt_gpg_sign; static int opt_allow_unrelated_histories; +static int opt_overwrite_same_content; /* Options passed to git-fetch */ static char *opt_all; @@ -182,6 +183,7 @@ static struct option pull_options[] = { OPT_SET_INT(0, "allow-unrelated-histories", &opt_allow_unrelated_histories, N_("allow merging unrelated histories"), 1), + OPT_BOOL(0, "overwrite-same-content", &opt_overwrite_same_content, N_("overwrite untracked file with the same content and name")), /* Options passed to git-fetch */ OPT_GROUP(N_("Options related to fetching")), @@ -612,7 +614,7 @@ static int pull_into_void(const struct object_id *merge_head, */ if (checkout_fast_forward(the_repository, the_hash_algo->empty_tree, - merge_head, 0)) + merge_head, 0, opt_overwrite_same_content)) return 1; if (update_ref("initial pull", "HEAD", merge_head, curr_head, 0, UPDATE_REFS_DIE_ON_ERR)) @@ -679,6 +681,8 @@ static int run_merge(void) strvec_pushf(&args, "--cleanup=%s", cleanup_arg); if (opt_ff) strvec_push(&args, opt_ff); + if (opt_overwrite_same_content) + strvec_push(&args, "--overwrite-same-content"); if (opt_verify) strvec_push(&args, opt_verify); if (opt_verify_signatures) @@ -1078,7 +1082,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) "commit %s."), oid_to_hex(&orig_head)); if (checkout_fast_forward(the_repository, &orig_head, - &curr_head, 0)) + &curr_head, 0, opt_overwrite_same_content)) die(_("Cannot fast-forward your working tree.\n" "After making sure that you saved anything precious from\n" "$ git diff %s\n" diff --git a/cache.h b/cache.h index 281f00ab1b..163367ab41 100644 --- a/cache.h +++ b/cache.h @@ -1858,7 +1858,8 @@ int try_merge_command(struct repository *r, int checkout_fast_forward(struct repository *r, const struct object_id *from, const struct object_id *to, - int overwrite_ignore); + int overwrite_ignore, + int overwrite_same_content); int sane_execvp(const char *file, char *const argv[]); diff --git a/merge-ort.c b/merge-ort.c index c319797021..a8d1496b4a 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -4066,6 +4066,7 @@ static int checkout(struct merge_options *opt, unpack_opts.verbose_update = (opt->verbosity > 2); unpack_opts.fn = twoway_merge; unpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore */ + unpack_opts.overwrite_same_content = opt->overwrite_same_content; parse_tree(prev); init_tree_desc(&trees[0], prev->buffer, prev->size); parse_tree(next); diff --git a/merge-recursive.h b/merge-recursive.h index 0795a1d3ec..6f8c5678e0 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -46,6 +46,7 @@ struct merge_options { /* miscellaneous control options */ const char *subtree_shift; unsigned renormalize : 1; + int overwrite_same_content; /* internal fields used by the implementation */ struct merge_options_internal *priv; diff --git a/merge.c b/merge.c index 2382ff66d3..410c92d235 100644 --- a/merge.c +++ b/merge.c @@ -47,7 +47,8 @@ int try_merge_command(struct repository *r, int checkout_fast_forward(struct repository *r, const struct object_id *head, const struct object_id *remote, - int overwrite_ignore) + int overwrite_ignore, + int overwrite_same_content) { struct tree *trees[MAX_UNPACK_TREES]; struct unpack_trees_options opts; @@ -80,6 +81,7 @@ int checkout_fast_forward(struct repository *r, memset(&opts, 0, sizeof(opts)); opts.preserve_ignored = !overwrite_ignore; + opts.overwrite_same_content = overwrite_same_content; opts.head_idx = 1; opts.src_index = r->index; diff --git a/sequencer.c b/sequencer.c index 5213d16e97..d11802c542 100644 --- a/sequencer.c +++ b/sequencer.c @@ -529,7 +529,7 @@ static int fast_forward_to(struct repository *r, struct strbuf err = STRBUF_INIT; repo_read_index(r); - if (checkout_fast_forward(r, from, to, 1)) + if (checkout_fast_forward(r, from, to, 1, 0)) return -1; /* the callee should have complained already */ strbuf_addf(&sb, _("%s: fast-forward"), _(action_name(opts))); diff --git a/t/t6436-merge-overwrite.sh b/t/t6436-merge-overwrite.sh index c0b7bd7c3f..bb323b1ee3 100755 --- a/t/t6436-merge-overwrite.sh +++ b/t/t6436-merge-overwrite.sh @@ -204,4 +204,38 @@ test_expect_success 'will not clobber WT/index when merging into unborn' ' grep bar untracked-file ' +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' ' + 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 +' + +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 +' + test_done diff --git a/t/t7615-merge-untracked.sh b/t/t7615-merge-untracked.sh index 99f8bae4c0..cfefd8f473 100755 --- a/t/t7615-merge-untracked.sh +++ b/t/t7615-merge-untracked.sh @@ -1,6 +1,6 @@ #!/bin/sh -test_description='test when merge with untracked file' +test_description='test when merge with untracked files and the option --overwrite-same-content' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME @@ -18,7 +18,7 @@ test_expect_success 'fastforward overwrite untracked file that has the same cont test_commit --no-tag "tracked" file "content" && git checkout A && echo content >file && - git merge B + git merge --overwrite-same-content B ' test_expect_success 'fastforward fail when untracked file has different content' ' @@ -27,7 +27,7 @@ test_expect_success 'fastforward fail when untracked file has different content' test_commit --no-tag "tracked" file "content" && git switch A && echo other >file && - test_must_fail git merge B + test_must_fail git merge --overwrite-same-content B ' test_expect_success 'normal merge overwrite untracked file that has the same content' ' @@ -37,7 +37,7 @@ test_expect_success 'normal merge overwrite untracked file that has the same con git switch A && test_commit --no-tag "exA" fileA "content" && echo content >file && - git merge B + git merge --overwrite-same-content B ' test_expect_success 'normal merge fail when untracked file has different content' ' @@ -47,7 +47,7 @@ test_expect_success 'normal merge fail when untracked file has different content git switch A && test_commit --no-tag "exA" fileA "content" && echo dif >file && - test_must_fail git merge B + test_must_fail git merge --overwrite-same-content B ' test_expect_success 'merge fail when tracked file modification is unstaged' ' @@ -57,7 +57,28 @@ test_expect_success 'merge fail when tracked file modification is unstaged' ' test_commit --no-tag "staged" file "content" && git switch A && echo content >file && - test_must_fail git merge B + test_must_fail git merge --overwrite-same-content B +' + +test_expect_success 'fastforward overwrite untracked file that has the same content with the configuration variable' ' + test_when_finished "git branch -D B && git reset --hard init && git clean --force" && + test_config merge.overwritesamecontent true && + git checkout -b B && + test_commit --no-tag "tracked" file "content" && + git checkout A && + echo content >file && + git merge B +' + +test_expect_success 'normal merge overwrite untracked file that has the same content with the configuration variable' ' + test_when_finished "git branch -D B && git reset --hard init && git clean --force" && + test_config merge.overwritesamecontent true && + git checkout -b B && + test_commit --no-tag "tracked" file "content" fileB "content" && + git switch A && + test_commit --no-tag "exA" fileA "content" && + echo content >file && + git merge B ' test_done diff --git a/unpack-trees.c b/unpack-trees.c index 61e06c04be..6c660b084b 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -158,17 +158,17 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, if (!strcmp(cmd, "checkout")) msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE) ? _("The following untracked working tree files would be overwritten by checkout:\n%%s" - "Please move or remove them before you switch branches.") + "Please move or remove them before you switch branches.%%s") : _("The following untracked working tree files would be overwritten by checkout:\n%%s"); else if (!strcmp(cmd, "merge")) msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE) ? _("The following untracked working tree files would be overwritten by merge:\n%%s" - "Please move or remove them before you merge.") + "Please move or remove them before you merge.%%s") : _("The following untracked working tree files would be overwritten by merge:\n%%s"); else msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE) ? _("The following untracked working tree files would be overwritten by %s:\n%%s" - "Please move or remove them before you %s.") + "Please move or remove them before you %s.%%s") : _("The following untracked working tree files would be overwritten by %s:\n%%s"); msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = strvec_pushf(&opts->msgs_to_free, msg, cmd, cmd); @@ -251,6 +251,14 @@ static void display_error_msgs(struct unpack_trees_options *o) { int e; unsigned error_displayed = 0; + const char *can_overwrite_msg; + + if (o->can_overwrite) { + can_overwrite_msg = _("\nYou can also rerun the command with --overwrite-same-content to overwrite files with same content."); + } else { + can_overwrite_msg = ""; + } + for (e = 0; e < NB_UNPACK_TREES_ERROR_TYPES; e++) { struct string_list *rejects = &o->unpack_rejects[e]; @@ -261,7 +269,8 @@ static void display_error_msgs(struct unpack_trees_options *o) error_displayed = 1; for (i = 0; i < rejects->nr; i++) strbuf_addf(&path, "\t%s\n", rejects->items[i].string); - error(ERRORMSG(o, e), super_prefixed(path.buf)); + + error(ERRORMSG(o, e), super_prefixed(path.buf), can_overwrite_msg); strbuf_release(&path); } string_list_clear(rejects, 0); @@ -1711,6 +1720,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options struct pattern_list pl; int free_pattern_list = 0; struct dir_struct dir = DIR_INIT; + o->can_overwrite = 1; if (o->reset == UNPACK_RESET_INVALID) BUG("o->reset had a value of 1; should be UNPACK_TREES_*_UNTRACKED"); @@ -2257,8 +2267,12 @@ 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 (!ie_modified(&o->result, ce, st, 0)) { - return 0; + } else if (ce && !ie_modified(o->src_index, ce, st, 0)) { + if(o->overwrite_same_content) { + return 0; + } + } else { + o->can_overwrite = 0; } return add_rejected_path(o, error_type, name); @@ -2266,8 +2280,8 @@ static int check_ok_to_remove(const char *name, int len, int dtype, /* * We do not want to remove or overwrite a working tree file that - * is not tracked, unless it is ignored and unless it has the same - * content than the merged file. + * is not tracked, unless it is ignored or it has the same content + * than the merged file with the option --overwrite_same_content. */ static int verify_absent_1(const struct cache_entry *ce, enum unpack_trees_error_types error_type, diff --git a/unpack-trees.h b/unpack-trees.h index efb9edfbb2..2be74ce5bf 100644 --- a/unpack-trees.h +++ b/unpack-trees.h @@ -71,6 +71,8 @@ struct unpack_trees_options { quiet, exiting_early, show_all_errors, + overwrite_same_content, + can_overwrite, dry_run; enum unpack_trees_reset_type reset; const char *prefix; -- 2.35.1.10.g88248585b1.dirty ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 1/4] t6436: tests how merge behave when there is untracked file with the same content 2022-05-27 19:55 ` [PATCH v2 0/4] " Jonathan Bressat @ 2022-05-27 19:55 ` Jonathan Bressat 2022-05-27 19:55 ` [PATCH v2 2/4] merge with untracked file that are the same without failure Jonathan Bressat ` (6 subsequent siblings) 7 siblings, 0 replies; 27+ messages in thread From: Jonathan Bressat @ 2022-05-27 19:55 UTC (permalink / raw) To: git.jonathan.bressat Cc: Matthieu.Moy, cogoni.guillaume, git, gitster, guillaume.cogoni, jonathan.bressat 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 Signed-off-by: Jonathan Bressat <git.jonathan.bressat@gmail.com> Signed-off-by: COGONI Guillaume <cogoni.guillaume@gmail.com> --- t/t6436-merge-overwrite.sh | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/t/t6436-merge-overwrite.sh b/t/t6436-merge-overwrite.sh index c0b7bd7c3f..bb323b1ee3 100755 --- a/t/t6436-merge-overwrite.sh +++ b/t/t6436-merge-overwrite.sh @@ -204,4 +204,38 @@ test_expect_success 'will not clobber WT/index when merging into unborn' ' grep bar untracked-file ' +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' ' + 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 +' + +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 +' + test_done -- 2.35.1.10.g88248585b1.dirty ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 2/4] merge with untracked file that are the same without failure 2022-05-27 19:55 ` [PATCH v2 0/4] " Jonathan Bressat 2022-05-27 19:55 ` [PATCH v2 1/4] t6436: tests how merge behave when there is untracked file with the same content Jonathan Bressat @ 2022-05-27 19:55 ` Jonathan Bressat 2022-05-27 19:55 ` [PATCH v2 3/4] add configuration variable corresponding to --overwrite-same-content Jonathan Bressat ` (5 subsequent siblings) 7 siblings, 0 replies; 27+ messages in thread From: Jonathan Bressat @ 2022-05-27 19:55 UTC (permalink / raw) To: git.jonathan.bressat Cc: Matthieu.Moy, cogoni.guillaume, git, gitster, guillaume.cogoni, jonathan.bressat 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, for exemple if you copy and paste files from another project and then you decide to pull this project, git will not proceed even if you didn't modify those files. t7615 tests this new behavior. Signed-off-by: Jonathan Bressat <git.jonathan.bressat@gmail.com> Signed-off-by: COGONI Guillaume <cogoni.guillaume@gmail.com> --- Documentation/git-merge.txt | 4 +++ builtin/merge.c | 7 ++++- builtin/pull.c | 8 +++-- cache.h | 3 +- merge-ort.c | 1 + merge-recursive.h | 1 + merge.c | 4 ++- sequencer.c | 2 +- t/t7615-merge-untracked.sh | 63 +++++++++++++++++++++++++++++++++++++ unpack-trees.c | 7 ++++- unpack-trees.h | 1 + 11 files changed, 94 insertions(+), 7 deletions(-) create mode 100755 t/t7615-merge-untracked.sh diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt index 3125473cc1..ceda0271c2 100644 --- a/Documentation/git-merge.txt +++ b/Documentation/git-merge.txt @@ -100,6 +100,10 @@ will be appended to the specified message. Silently overwrite ignored files from the merge result. This is the default behavior. Use `--no-overwrite-ignore` to abort. +--overwrite-same-content:: + Silently overwrite untracked files that have the same content + and name than files in the merged commit from the merge result. + --abort:: Abort the current conflict resolution process, and try to reconstruct the pre-merge state. If an autostash entry is diff --git a/builtin/merge.c b/builtin/merge.c index 74e53cf20a..fffae81068 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -68,6 +68,7 @@ static int option_edit = -1; static int allow_trivial = 1, have_message, verify_signatures; static int check_trust_level = 1; static int overwrite_ignore = 1; +static int overwrite_same_content; static struct strbuf merge_msg = STRBUF_INIT; static struct strategy **use_strategies; static size_t use_strategies_nr, use_strategies_alloc; @@ -305,6 +306,7 @@ static struct option builtin_merge_options[] = { OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")), OPT_BOOL(0, "signoff", &signoff, N_("add a Signed-off-by trailer")), OPT_BOOL(0, "no-verify", &no_verify, N_("bypass pre-merge-commit and commit-msg hooks")), + OPT_BOOL(0, "overwrite-same-content", &overwrite_same_content, N_("overwrite untracked file with the same content and name")), OPT_END() }; @@ -684,6 +686,7 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head, opts.trivial_merges_only = 1; opts.merge = 1; opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */ + opts.overwrite_same_content = overwrite_same_content; trees[nr_trees] = parse_tree_indirect(common); if (!trees[nr_trees++]) return -1; @@ -746,6 +749,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common, o.branch1 = head_arg; o.branch2 = merge_remote_util(remoteheads->item)->name; + o.overwrite_same_content = overwrite_same_content; for (j = common; j; j = j->next) commit_list_insert(j->item, &reversed); @@ -1573,7 +1577,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix) if (checkout_fast_forward(the_repository, &head_commit->object.oid, &commit->object.oid, - overwrite_ignore)) { + overwrite_ignore, + overwrite_same_content)) { apply_autostash(git_path_merge_autostash(the_repository)); ret = 1; goto done; diff --git a/builtin/pull.c b/builtin/pull.c index 100cbf9fb8..46ef68e721 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -92,6 +92,7 @@ static struct strvec opt_strategies = STRVEC_INIT; static struct strvec opt_strategy_opts = STRVEC_INIT; static char *opt_gpg_sign; static int opt_allow_unrelated_histories; +static int opt_overwrite_same_content; /* Options passed to git-fetch */ static char *opt_all; @@ -182,6 +183,7 @@ static struct option pull_options[] = { OPT_SET_INT(0, "allow-unrelated-histories", &opt_allow_unrelated_histories, N_("allow merging unrelated histories"), 1), + OPT_BOOL(0, "overwrite-same-content", &opt_overwrite_same_content, N_("overwrite untracked file with the same content and name")), /* Options passed to git-fetch */ OPT_GROUP(N_("Options related to fetching")), @@ -612,7 +614,7 @@ static int pull_into_void(const struct object_id *merge_head, */ if (checkout_fast_forward(the_repository, the_hash_algo->empty_tree, - merge_head, 0)) + merge_head, 0, opt_overwrite_same_content)) return 1; if (update_ref("initial pull", "HEAD", merge_head, curr_head, 0, UPDATE_REFS_DIE_ON_ERR)) @@ -679,6 +681,8 @@ static int run_merge(void) strvec_pushf(&args, "--cleanup=%s", cleanup_arg); if (opt_ff) strvec_push(&args, opt_ff); + if (opt_overwrite_same_content) + strvec_push(&args, "--overwrite-same-content"); if (opt_verify) strvec_push(&args, opt_verify); if (opt_verify_signatures) @@ -1078,7 +1082,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) "commit %s."), oid_to_hex(&orig_head)); if (checkout_fast_forward(the_repository, &orig_head, - &curr_head, 0)) + &curr_head, 0, opt_overwrite_same_content)) die(_("Cannot fast-forward your working tree.\n" "After making sure that you saved anything precious from\n" "$ git diff %s\n" diff --git a/cache.h b/cache.h index 281f00ab1b..163367ab41 100644 --- a/cache.h +++ b/cache.h @@ -1858,7 +1858,8 @@ int try_merge_command(struct repository *r, int checkout_fast_forward(struct repository *r, const struct object_id *from, const struct object_id *to, - int overwrite_ignore); + int overwrite_ignore, + int overwrite_same_content); int sane_execvp(const char *file, char *const argv[]); diff --git a/merge-ort.c b/merge-ort.c index c319797021..a8d1496b4a 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -4066,6 +4066,7 @@ static int checkout(struct merge_options *opt, unpack_opts.verbose_update = (opt->verbosity > 2); unpack_opts.fn = twoway_merge; unpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore */ + unpack_opts.overwrite_same_content = opt->overwrite_same_content; parse_tree(prev); init_tree_desc(&trees[0], prev->buffer, prev->size); parse_tree(next); diff --git a/merge-recursive.h b/merge-recursive.h index 0795a1d3ec..6f8c5678e0 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -46,6 +46,7 @@ struct merge_options { /* miscellaneous control options */ const char *subtree_shift; unsigned renormalize : 1; + int overwrite_same_content; /* internal fields used by the implementation */ struct merge_options_internal *priv; diff --git a/merge.c b/merge.c index 2382ff66d3..410c92d235 100644 --- a/merge.c +++ b/merge.c @@ -47,7 +47,8 @@ int try_merge_command(struct repository *r, int checkout_fast_forward(struct repository *r, const struct object_id *head, const struct object_id *remote, - int overwrite_ignore) + int overwrite_ignore, + int overwrite_same_content) { struct tree *trees[MAX_UNPACK_TREES]; struct unpack_trees_options opts; @@ -80,6 +81,7 @@ int checkout_fast_forward(struct repository *r, memset(&opts, 0, sizeof(opts)); opts.preserve_ignored = !overwrite_ignore; + opts.overwrite_same_content = overwrite_same_content; opts.head_idx = 1; opts.src_index = r->index; diff --git a/sequencer.c b/sequencer.c index 5213d16e97..d11802c542 100644 --- a/sequencer.c +++ b/sequencer.c @@ -529,7 +529,7 @@ static int fast_forward_to(struct repository *r, struct strbuf err = STRBUF_INIT; repo_read_index(r); - if (checkout_fast_forward(r, from, to, 1)) + if (checkout_fast_forward(r, from, to, 1, 0)) return -1; /* the callee should have complained already */ strbuf_addf(&sb, _("%s: fast-forward"), _(action_name(opts))); diff --git a/t/t7615-merge-untracked.sh b/t/t7615-merge-untracked.sh new file mode 100755 index 0000000000..05a34cf03f --- /dev/null +++ b/t/t7615-merge-untracked.sh @@ -0,0 +1,63 @@ +#!/bin/sh + +test_description='test when merge with untracked files and the option --overwrite-same-content' + +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + +. ./test-lib.sh + +test_expect_success 'setup' ' + test_commit "init" README.md "content" && + git checkout -b A +' + +test_expect_success 'fastforward overwrite untracked file that has the same content' ' + test_when_finished "git branch -D B && git reset --hard init && git clean --force" && + git checkout -b B && + test_commit --no-tag "tracked" file "content" && + git checkout A && + echo content >file && + git merge --overwrite-same-content B +' + +test_expect_success 'fastforward fail when untracked file has different content' ' + test_when_finished "git branch -D B && git reset --hard init && git clean --force" && + git checkout -b B && + test_commit --no-tag "tracked" file "content" && + git switch A && + echo other >file && + test_must_fail git merge --overwrite-same-content B +' + +test_expect_success 'normal merge overwrite untracked file that has the same content' ' + test_when_finished "git branch -D B && git reset --hard init && git clean --force" && + git checkout -b B && + test_commit --no-tag "tracked" file "content" fileB "content" && + git switch A && + test_commit --no-tag "exA" fileA "content" && + echo content >file && + git merge --overwrite-same-content B +' + +test_expect_success 'normal merge fail when untracked file has different content' ' + test_when_finished "git branch -D B && git reset --hard init && git clean --force" && + git checkout -b B && + test_commit --no-tag "tracked" file "content" fileB "content" && + git switch A && + test_commit --no-tag "exA" fileA "content" && + echo dif >file && + test_must_fail git merge --overwrite-same-content B +' + +test_expect_success 'merge fail when tracked file modification is unstaged' ' + test_when_finished "git branch -D B && git reset --hard init && git clean --force" && + test_commit --no-tag "unstaged" file "other" && + git checkout -b B && + test_commit --no-tag "staged" file "content" && + git switch A && + echo content >file && + test_must_fail git merge --overwrite-same-content B +' + +test_done diff --git a/unpack-trees.c b/unpack-trees.c index 360844bda3..1a52be723e 100644 --- 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; + } } return add_rejected_path(o, error_type, name); @@ -2264,7 +2268,8 @@ static int check_ok_to_remove(const char *name, int len, int dtype, /* * We do not want to remove or overwrite a working tree file that - * is not tracked, unless it is ignored. + * is not tracked, unless it is ignored or it has the same content + * than the merged file with the option --overwrite_same_content. */ static int verify_absent_1(const struct cache_entry *ce, enum unpack_trees_error_types error_type, diff --git a/unpack-trees.h b/unpack-trees.h index efb9edfbb2..ebe4be0b35 100644 --- a/unpack-trees.h +++ b/unpack-trees.h @@ -71,6 +71,7 @@ struct unpack_trees_options { quiet, exiting_early, show_all_errors, + overwrite_same_content, dry_run; enum unpack_trees_reset_type reset; const char *prefix; -- 2.35.1.10.g88248585b1.dirty ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 3/4] add configuration variable corresponding to --overwrite-same-content 2022-05-27 19:55 ` [PATCH v2 0/4] " Jonathan Bressat 2022-05-27 19:55 ` [PATCH v2 1/4] t6436: tests how merge behave when there is untracked file with the same content Jonathan Bressat 2022-05-27 19:55 ` [PATCH v2 2/4] merge with untracked file that are the same without failure Jonathan Bressat @ 2022-05-27 19:55 ` Jonathan Bressat 2022-05-27 19:55 ` [PATCH v2 4/4] error message now advice to use the new option Jonathan Bressat ` (4 subsequent siblings) 7 siblings, 0 replies; 27+ messages in thread From: Jonathan Bressat @ 2022-05-27 19:55 UTC (permalink / raw) To: git.jonathan.bressat Cc: Matthieu.Moy, cogoni.guillaume, git, gitster, guillaume.cogoni, jonathan.bressat Configuration variable merge.overwritesamecontent corresponding to the --overwrite-same-content option. This allow merge to overwrite untracked files that have the same content, a configuration variable is interressant because some people may want this activated as default to not have to use the option every time Signed-off-by: Jonathan Bressat <git.jonathan.bressat@gmail.com> Signed-off-by: COGONI Guillaume <cogoni.guillaume@gmail.com> --- Documentation/config/merge.txt | 5 +++++ builtin/merge.c | 2 ++ t/t7615-merge-untracked.sh | 21 +++++++++++++++++++++ 3 files changed, 28 insertions(+) diff --git a/Documentation/config/merge.txt b/Documentation/config/merge.txt index 99e83dd36e..2824dd19c7 100644 --- a/Documentation/config/merge.txt +++ b/Documentation/config/merge.txt @@ -89,6 +89,11 @@ merge.autoStash:: `--autostash` options of linkgit:git-merge[1]. Defaults to false. +merge.overwritesamecontent:: + When set to true, it will modify the behavior of git merge + to overwrite untracked files that have the same name and + content than files in the merged commit. + merge.tool:: Controls which merge tool is used by linkgit:git-mergetool[1]. The list below shows the valid built-in values. diff --git a/builtin/merge.c b/builtin/merge.c index fffae81068..936cb8480d 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -658,6 +658,8 @@ static int git_merge_config(const char *k, const char *v, void *cb) } else if (!strcmp(k, "merge.autostash")) { autostash = git_config_bool(k, v); return 0; + } else if (!strcmp(k,"merge.overwritesamecontent")) { + overwrite_same_content = git_config_bool(k, v); } status = fmt_merge_msg_config(k, v, cb); diff --git a/t/t7615-merge-untracked.sh b/t/t7615-merge-untracked.sh index 05a34cf03f..cfefd8f473 100755 --- a/t/t7615-merge-untracked.sh +++ b/t/t7615-merge-untracked.sh @@ -60,4 +60,25 @@ test_expect_success 'merge fail when tracked file modification is unstaged' ' test_must_fail git merge --overwrite-same-content B ' +test_expect_success 'fastforward overwrite untracked file that has the same content with the configuration variable' ' + test_when_finished "git branch -D B && git reset --hard init && git clean --force" && + test_config merge.overwritesamecontent true && + git checkout -b B && + test_commit --no-tag "tracked" file "content" && + git checkout A && + echo content >file && + git merge B +' + +test_expect_success 'normal merge overwrite untracked file that has the same content with the configuration variable' ' + test_when_finished "git branch -D B && git reset --hard init && git clean --force" && + test_config merge.overwritesamecontent true && + git checkout -b B && + test_commit --no-tag "tracked" file "content" fileB "content" && + git switch A && + test_commit --no-tag "exA" fileA "content" && + echo content >file && + git merge B +' + test_done -- 2.35.1.10.g88248585b1.dirty ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 4/4] error message now advice to use the new option 2022-05-27 19:55 ` [PATCH v2 0/4] " Jonathan Bressat ` (2 preceding siblings ...) 2022-05-27 19:55 ` [PATCH v2 3/4] add configuration variable corresponding to --overwrite-same-content Jonathan Bressat @ 2022-05-27 19:55 ` Jonathan Bressat [not found] ` <dfea1d98c15047428b1a11adbc002eef@SAMBXP02.univ-lyon1.fr> ` (3 subsequent siblings) 7 siblings, 0 replies; 27+ messages in thread From: Jonathan Bressat @ 2022-05-27 19:55 UTC (permalink / raw) To: git.jonathan.bressat Cc: Matthieu.Moy, cogoni.guillaume, git, gitster, guillaume.cogoni, jonathan.bressat When all the untracked files in the working tree have the same content than the files in merged branch then the error message advice to use the --overwrite-same-content option. Signed-off-by: Jonathan Bressat <git.jonathan.bressat@gmail.com> Signed-off-by: COGONI Guillaume <cogoni.guillaume@gmail.com> --- builtin/checkout.c | 1 + unpack-trees.c | 20 ++++++++++++++++---- unpack-trees.h | 1 + 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index cc804ba8e1..1b1d1813c7 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -760,6 +760,7 @@ static int merge_working_tree(const struct checkout_opts *opts, &new_branch_info->commit->object.oid : &new_branch_info->oid, NULL); topts.preserve_ignored = !opts->overwrite_ignore; + topts.overwrite_same_content = 0;/* FIXME: opts->overwrite_same_content */ tree = parse_tree_indirect(old_branch_info->commit ? &old_branch_info->commit->object.oid : the_hash_algo->empty_tree); diff --git a/unpack-trees.c b/unpack-trees.c index 1a52be723e..6c660b084b 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -158,17 +158,17 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, if (!strcmp(cmd, "checkout")) msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE) ? _("The following untracked working tree files would be overwritten by checkout:\n%%s" - "Please move or remove them before you switch branches.") + "Please move or remove them before you switch branches.%%s") : _("The following untracked working tree files would be overwritten by checkout:\n%%s"); else if (!strcmp(cmd, "merge")) msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE) ? _("The following untracked working tree files would be overwritten by merge:\n%%s" - "Please move or remove them before you merge.") + "Please move or remove them before you merge.%%s") : _("The following untracked working tree files would be overwritten by merge:\n%%s"); else msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE) ? _("The following untracked working tree files would be overwritten by %s:\n%%s" - "Please move or remove them before you %s.") + "Please move or remove them before you %s.%%s") : _("The following untracked working tree files would be overwritten by %s:\n%%s"); msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = strvec_pushf(&opts->msgs_to_free, msg, cmd, cmd); @@ -251,6 +251,14 @@ static void display_error_msgs(struct unpack_trees_options *o) { int e; unsigned error_displayed = 0; + const char *can_overwrite_msg; + + if (o->can_overwrite) { + can_overwrite_msg = _("\nYou can also rerun the command with --overwrite-same-content to overwrite files with same content."); + } else { + can_overwrite_msg = ""; + } + for (e = 0; e < NB_UNPACK_TREES_ERROR_TYPES; e++) { struct string_list *rejects = &o->unpack_rejects[e]; @@ -261,7 +269,8 @@ static void display_error_msgs(struct unpack_trees_options *o) error_displayed = 1; for (i = 0; i < rejects->nr; i++) strbuf_addf(&path, "\t%s\n", rejects->items[i].string); - error(ERRORMSG(o, e), super_prefixed(path.buf)); + + error(ERRORMSG(o, e), super_prefixed(path.buf), can_overwrite_msg); strbuf_release(&path); } string_list_clear(rejects, 0); @@ -1711,6 +1720,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options struct pattern_list pl; int free_pattern_list = 0; struct dir_struct dir = DIR_INIT; + o->can_overwrite = 1; if (o->reset == UNPACK_RESET_INVALID) BUG("o->reset had a value of 1; should be UNPACK_TREES_*_UNTRACKED"); @@ -2261,6 +2271,8 @@ static int check_ok_to_remove(const char *name, int len, int dtype, if(o->overwrite_same_content) { return 0; } + } else { + o->can_overwrite = 0; } return add_rejected_path(o, error_type, name); diff --git a/unpack-trees.h b/unpack-trees.h index ebe4be0b35..2be74ce5bf 100644 --- a/unpack-trees.h +++ b/unpack-trees.h @@ -72,6 +72,7 @@ struct unpack_trees_options { exiting_early, show_all_errors, overwrite_same_content, + can_overwrite, dry_run; enum unpack_trees_reset_type reset; const char *prefix; -- 2.35.1.10.g88248585b1.dirty ^ permalink raw reply related [flat|nested] 27+ messages in thread
[parent not found: <dfea1d98c15047428b1a11adbc002eef@SAMBXP02.univ-lyon1.fr>]
* Re: [PATCH v2 1/4] t6436: tests how merge behave when there is untracked file with the same content [not found] ` <dfea1d98c15047428b1a11adbc002eef@SAMBXP02.univ-lyon1.fr> @ 2022-06-04 9:44 ` Matthieu Moy 0 siblings, 0 replies; 27+ messages in thread From: Matthieu Moy @ 2022-06-04 9:44 UTC (permalink / raw) To: Jonathan Bressat Cc: cogoni.guillaume@gmail.com, git@vger.kernel.org, gitster@pobox.com, guillaume.cogoni@gmail.com, BRESSAT JONATHAN p1802864 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/ ^ permalink raw reply [flat|nested] 27+ messages in thread
[parent not found: <be2297bdcd724c3f8abfde2d5d74fb18@SAMBXP02.univ-lyon1.fr>]
* Re: [PATCH v2 2/4] merge with untracked file that are the same without failure [not found] ` <be2297bdcd724c3f8abfde2d5d74fb18@SAMBXP02.univ-lyon1.fr> @ 2022-06-04 9:45 ` Matthieu Moy 0 siblings, 0 replies; 27+ messages in thread From: Matthieu Moy @ 2022-06-04 9:45 UTC (permalink / raw) To: Jonathan Bressat Cc: cogoni.guillaume@gmail.com, git@vger.kernel.org, gitster@pobox.com, guillaume.cogoni@gmail.com, BRESSAT JONATHAN p1802864 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/ ^ permalink raw reply [flat|nested] 27+ messages in thread
[parent not found: <82beb916d9c44a069f30ec4ff261e3be@SAMBXP02.univ-lyon1.fr>]
* Re: [PATCH v2 4/4] error message now advice to use the new option [not found] ` <82beb916d9c44a069f30ec4ff261e3be@SAMBXP02.univ-lyon1.fr> @ 2022-06-04 9:45 ` Matthieu Moy 2022-06-10 12:58 ` Guillaume Cogoni 0 siblings, 1 reply; 27+ messages in thread From: Matthieu Moy @ 2022-06-04 9:45 UTC (permalink / raw) To: Jonathan Bressat Cc: cogoni.guillaume@gmail.com, git@vger.kernel.org, gitster@pobox.com, guillaume.cogoni@gmail.com, BRESSAT JONATHAN p1802864 On 5/27/22 21:55, Jonathan Bressat wrote: > Subject: Re: [PATCH v2 4/4] error message now advice to use the new option Commit messages are usually written with imperative tone. The "now" doesn't add information, the reader can guess that the commit message describes the new behavior. "suggest --overwrite-same-content in error message when appropriate" ? > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -760,6 +760,7 @@ static int merge_working_tree(const struct checkout_opts *opts, > &new_branch_info->commit->object.oid : > &new_branch_info->oid, NULL); > topts.preserve_ignored = !opts->overwrite_ignore; > + topts.overwrite_same_content = 0;/* FIXME: opts->overwrite_same_content */ Why not use opts->overwrite_same_content in the code rather than saying you should in a comment? Actually, doesn't this hunk belong to the previous commit? The rest of the patch looks good to me. -- Matthieu Moy https://matthieu-moy.fr/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 4/4] error message now advice to use the new option 2022-06-04 9:45 ` [PATCH v2 4/4] error message now advice to use the new option Matthieu Moy @ 2022-06-10 12:58 ` Guillaume Cogoni 0 siblings, 0 replies; 27+ messages in thread From: Guillaume Cogoni @ 2022-06-10 12:58 UTC (permalink / raw) To: Matthieu Moy Cc: Jonathan Bressat, git@vger.kernel.org, gitster@pobox.com, guillaume.cogoni@gmail.com, BRESSAT JONATHAN p1802864 Hello, Thanks for your reviews. We will take in consideration what you say for the next version. Sincerely, Guillaume COGONI and Jonathan BRESSAT On Sat, Jun 4, 2022 at 4:57 PM Matthieu Moy <Matthieu.Moy@univ-lyon1.fr> wrote: > > On 5/27/22 21:55, Jonathan Bressat wrote: > > Subject: Re: [PATCH v2 4/4] error message now advice to use the new > option > > Commit messages are usually written with imperative tone. The "now" > doesn't add information, the reader can guess that the commit message > describes the new behavior. > > "suggest --overwrite-same-content in error message when appropriate" ? > > > --- a/builtin/checkout.c > > +++ b/builtin/checkout.c > > @@ -760,6 +760,7 @@ static int merge_working_tree(const struct checkout_opts *opts, > > &new_branch_info->commit->object.oid : > > &new_branch_info->oid, NULL); > > topts.preserve_ignored = !opts->overwrite_ignore; > > + topts.overwrite_same_content = 0;/* FIXME: opts->overwrite_same_content */ > > Why not use opts->overwrite_same_content in the code rather than saying > you should in a comment? > > Actually, doesn't this hunk belong to the previous commit? > > The rest of the patch looks good to me. > > -- > Matthieu Moy > https://matthieu-moy.fr/ ^ permalink raw reply [flat|nested] 27+ messages in thread
[parent not found: <4efbe7d9c95841c691f51954670a1d9f@SAMBXP02.univ-lyon1.fr>]
* Re: [PATCH v2 3/4] add configuration variable corresponding to --overwrite-same-content [not found] ` <4efbe7d9c95841c691f51954670a1d9f@SAMBXP02.univ-lyon1.fr> @ 2022-06-04 9:49 ` Matthieu Moy 0 siblings, 0 replies; 27+ messages in thread From: Matthieu Moy @ 2022-06-04 9:49 UTC (permalink / raw) To: Jonathan Bressat Cc: cogoni.guillaume@gmail.com, git@vger.kernel.org, gitster@pobox.com, guillaume.cogoni@gmail.com, BRESSAT JONATHAN p1802864 On 5/27/22 21:55, Jonathan Bressat wrote: > Configuration variable merge.overwritesamecontent corresponding to We usually write them as camelCase for readability (but options are case-insensitive). > +merge.overwritesamecontent:: > + When set to true, it will modify the behavior of git merge "When set to true, modify", the future is not needed. > + to overwrite untracked files that have the same name and > + content than files in the merged commit. The doc should mention the command-line option too, and say which one takes precedence when both are specified (as it is the case for many other options). -- Matthieu Moy https://matthieu-moy.fr/ ^ permalink raw reply [flat|nested] 27+ messages in thread
[parent not found: <eca66375d8b34154856b7da303bf96d7@SAMBXP02.univ-lyon1.fr>]
* Re: [PATCH v1 1/2] t7615: test how merge behave when there is untracked file [not found] ` <eca66375d8b34154856b7da303bf96d7@SAMBXP02.univ-lyon1.fr> @ 2022-04-26 6:48 ` Matthieu Moy 0 siblings, 0 replies; 27+ messages in thread From: Matthieu Moy @ 2022-04-26 6:48 UTC (permalink / raw) To: Jonathan, gitster@pobox.com Cc: BRESSAT JONATHAN p1802864, cogoni.guillaume@gmail.com, git@vger.kernel.org, guillaume.cogoni@gmail.com On 4/25/22 22:27, Jonathan wrote: > when there is untracked file that has the same name than file in the > merged branch git refuse to proceed, even when the file has the same > content > > t6436 test a similar thing but not especially with same content file Write your commit message like normal english: capitalize start of sentence, and period at the end (we omit the period in the subject line, though). > +test_expect_success 'fastforward fail when untracked file has the same content' ' Here and other test names: third person => s (fail_s_, and overwrite_s_ in the next patch). > + test_when_finished "git branch -D B && git reset --hard init && 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 It would make sense to grep for the correct error message in the output, but maybe that's overkill. -- Matthieu Moy https://matthieu-moy.fr/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 0/1] Be nicer to the user on tracked/untracked merge conflicts 2022-04-12 19:15 ` [PATCH 0/1] Be nicer to the user on tracked/untracked merge conflicts Jonathan 2022-04-12 19:15 ` [PATCH 1/1] Merge with untracked file that are the same without failure and test Jonathan @ 2022-04-12 19:24 ` Ævar Arnfjörð Bjarmason 2022-04-14 8:57 ` Jonathan Bressat 1 sibling, 1 reply; 27+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-04-12 19:24 UTC (permalink / raw) To: Jonathan; +Cc: cogoni.guillaume, Matthieu.Moy, git, guillaume.cogoni, Jonathan 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 0/1] Be nicer to the user on tracked/untracked merge conflicts 2022-04-12 19:24 ` [PATCH 0/1] Be nicer to the user on tracked/untracked merge conflicts Ævar Arnfjörð Bjarmason @ 2022-04-14 8:57 ` Jonathan Bressat 0 siblings, 0 replies; 27+ messages in thread From: Jonathan Bressat @ 2022-04-14 8:57 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Cogoni Guillaume, Matthieu Moy, git, guillaume.cogoni, Jonathan On Tue, Apr 12 2022, Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2022-06-10 12:59 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-27 15:41 [WIP]: make merge nicer to the user Guillaume Cogoni 2022-04-12 19:15 ` [PATCH 0/1] Be nicer to the user on tracked/untracked merge conflicts Jonathan 2022-04-12 19:15 ` [PATCH 1/1] Merge with untracked file that are the same without failure and test Jonathan 2022-04-12 19:21 ` Ævar Arnfjörð Bjarmason 2022-04-13 18:18 ` Junio C Hamano 2022-04-25 20:27 ` [PATCH v1 0/2] Be nicer to the user on tracked/untracked merge conflicts Jonathan 2022-04-25 20:27 ` [PATCH v1 1/2] t7615: test how merge behave when there is untracked file Jonathan 2022-04-25 20:27 ` [PATCH v1 2/2] merge with untracked file that are the same without failure Jonathan 2022-04-25 21:16 ` [PATCH v1 0/2] Be nicer to the user on tracked/untracked merge conflicts Junio C Hamano 2022-04-25 22:28 ` Guillaume Cogoni 2022-04-25 23:10 ` Junio C Hamano [not found] ` <fdd9f13d14e942f3a1572866761b9580@SAMBXP02.univ-lyon1.fr> 2022-04-26 6:38 ` Matthieu Moy 2022-04-26 16:13 ` Junio C Hamano 2022-04-28 10:33 ` Jonathan Bressat 2022-05-27 19:55 ` [PATCH v2 0/4] " Jonathan Bressat 2022-05-27 19:55 ` [PATCH v2 1/4] t6436: tests how merge behave when there is untracked file with the same content Jonathan Bressat 2022-05-27 19:55 ` [PATCH v2 2/4] merge with untracked file that are the same without failure Jonathan Bressat 2022-05-27 19:55 ` [PATCH v2 3/4] add configuration variable corresponding to --overwrite-same-content Jonathan Bressat 2022-05-27 19:55 ` [PATCH v2 4/4] error message now advice to use the new option Jonathan Bressat [not found] ` <dfea1d98c15047428b1a11adbc002eef@SAMBXP02.univ-lyon1.fr> 2022-06-04 9:44 ` [PATCH v2 1/4] t6436: tests how merge behave when there is untracked file with the same content Matthieu Moy [not found] ` <be2297bdcd724c3f8abfde2d5d74fb18@SAMBXP02.univ-lyon1.fr> 2022-06-04 9:45 ` [PATCH v2 2/4] merge with untracked file that are the same without failure Matthieu Moy [not found] ` <82beb916d9c44a069f30ec4ff261e3be@SAMBXP02.univ-lyon1.fr> 2022-06-04 9:45 ` [PATCH v2 4/4] error message now advice to use the new option Matthieu Moy 2022-06-10 12:58 ` Guillaume Cogoni [not found] ` <4efbe7d9c95841c691f51954670a1d9f@SAMBXP02.univ-lyon1.fr> 2022-06-04 9:49 ` [PATCH v2 3/4] add configuration variable corresponding to --overwrite-same-content Matthieu Moy [not found] ` <eca66375d8b34154856b7da303bf96d7@SAMBXP02.univ-lyon1.fr> 2022-04-26 6:48 ` [PATCH v1 1/2] t7615: test how merge behave when there is untracked file Matthieu Moy 2022-04-12 19:24 ` [PATCH 0/1] Be nicer to the user on tracked/untracked merge conflicts Ævar Arnfjörð Bjarmason 2022-04-14 8:57 ` Jonathan Bressat
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).