Re: [PATCH] merge: refuse to create too cool a merge by default
Linus Torvalds writes: > ... And I guess it might not be too nasty > to add: it could be done as part of the object checking pass after > downloading the pack. Was that what you were thinking of? Not that fancy, actually. Running an equivalent of git rev-list --max-parents=0 ^HEAD FETCH_HEAD was what I had in mind. This shouldn't be too costly for a normal case (O(N) where N is the number of changes on FETCH_HEAD since it forked from you). It needs to be done even when FETCH_HEAD is a descendant of HEAD, (i.e. when we would have fast forwarded without creating a merge) to be effective, as --no-allow-new-root is about not trusting your subsystem people not doing silly things on purpose, unlike our original patches. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] merge: refuse to create too cool a merge by default
On Wed, Apr 6, 2016 at 11:36 AM, Junio C Hamano wrote: > > I was reviewing the topics to merge to 'master', and a thought > crossed my mind. Both of our series only refuse to create a merge > that does not have any common ancestor, but shouldn't the right > solution refuse to add a new root? So I think that's an independent thing, and I think you're right, it would be a good feature to have. As a maintainer, it would have caught the whole "my submaintainers did something really odd, I need to talk to them" before-the-fact rather than after-the-fact. That said, I'm less worried about my submaintainers doing _intentionally_ annoying things, than people doing silly things by mistake. So if I had a version of git that allowed me to say "don't allow pulls that add a new root commit unless I specify a '--new-root-allowed' flag", then yes, I'd use that. And I guess it might not be too nasty to add: it could be done as part of the object checking pass after downloading the pack. Was that what you were thinking of? But at the same time, I think the existing series you have, which hopefully results in these things not happening by mistake in the future is the more important piece. I'd rather get good pull requests than get errors and have to tell people "you screwed up, now we'll need to re-do this". But if you cook something up that checks that there are no new roots in the pull, I'd certainly appreciate that safety net. Linus -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] merge: refuse to create too cool a merge by default
Linus Torvalds writes: > On Fri, Mar 18, 2016 at 1:21 PM, Junio C Hamano wrote: >> While it makes sense to allow merging unrelated histories of two >> projects that started independently into one, in the way "gitk" was >> merged to "git" itself aka "the coolest merge ever", such a merge is >> still an unusual event. Worse, if somebody creates an independent >> history by starting from a tarball of an established project and >> sends a pull request to the original project, "git merge" however >> happily creates such a merge without any sign of something unusual >> is happening. >> >> Teach "git merge" to refuse to create such a merge by default, >> unless the user passes a new "--allow-unrelated-histories" option to >> tell it that the user is aware that two unrelated projects are >> merged. > > Heh. I had a separate set of patches for you, but hadn't sent them out > because of the other test failures (which you also worked out). I was reviewing the topics to merge to 'master', and a thought crossed my mind. Both of our series only refuse to create a merge that does not have any common ancestor, but shouldn't the right solution refuse to add a new root? That is, somebody may - Create a new root by mistake; - Pull from you to update, with --allow-unrelated-histories option; - Optionally, build more history on top of it; and then finally - Ask you to pull without telling you that there is a new root. Wouldn't the "git pull" you do in response to this pull request have a common ancestor (i.e. the tip of what he pulled from you in the second step) and evade this check? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] merge: refuse to create too cool a merge by default
Linus Torvalds writes: > Mine was slightly different, I just went with a "unrelated" merge option. Yeah, I was debating myself if this should be -Xunrelated specific to recursive or an option that is meant for all strategies. I can go either way, but I think a command-wide option is logically the right way to go, because you do not want two-project merge by default no matter what strategy is used (and some of you may know that I had a long term plan with no lines of code yet to do yet another merge strategy). > I'll attach my two patches anyway, if for no other reason than the > fact that I actually wrote a new test for this. Thanks, but after updating existing scripts, I think those changes already make sure that two-project merges work with the option. What is missing in my version is a new test that ensures the command fails a two-project merge by default. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] merge: refuse to create too cool a merge by default
On Fri, Mar 18, 2016 at 1:21 PM, Junio C Hamano wrote:
> While it makes sense to allow merging unrelated histories of two
> projects that started independently into one, in the way "gitk" was
> merged to "git" itself aka "the coolest merge ever", such a merge is
> still an unusual event. Worse, if somebody creates an independent
> history by starting from a tarball of an established project and
> sends a pull request to the original project, "git merge" however
> happily creates such a merge without any sign of something unusual
> is happening.
>
> Teach "git merge" to refuse to create such a merge by default,
> unless the user passes a new "--allow-unrelated-histories" option to
> tell it that the user is aware that two unrelated projects are
> merged.
Heh. I had a separate set of patches for you, but hadn't sent them out
because of the other test failures (which you also worked out).
Mine was slightly different, I just went with a "unrelated" merge option.
I'll attach my two patches anyway, if for no other reason than the
fact that I actually wrote a new test for this.
Feel free to ignore my patches, they have nothing really different
from yours, just slightly different implementation.
Linus
From 0f3e4a9294eeda6799e3e50e28809133233126db Mon Sep 17 00:00:00 2001
From: Linus Torvalds
Date: Fri, 18 Mar 2016 12:46:06 -0700
Subject: [PATCH 1/2] t3035: test merging of unrelated branches
Right now this succeeds, and the test actually verifies that behavior.
Signed-off-by: Linus Torvalds
---
t/t3035-merge-recursive-across-project.sh | 28
1 file changed, 28 insertions(+)
create mode 100755 t/t3035-merge-recursive-across-project.sh
diff --git a/t/t3035-merge-recursive-across-project.sh b/t/t3035-merge-recursive-across-project.sh
new file mode 100755
index ..b5d614db6b08
--- /dev/null
+++ b/t/t3035-merge-recursive-across-project.sh
@@ -0,0 +1,28 @@
+#!/bin/sh
+
+test_description='merge-recursive with unrelated projects
+
+Test rename detection by examining rename/delete conflicts.
+
+ * A: file A
+
+ * B: file B
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup repository' \
+ 'echo Hello >A &&
+ git add A &&
+ git commit -m "Branch A" A &&
+ git branch A &&
+ git mv A B &&
+ echo Hi >B &&
+ git commit -m "Branch B" --amend B &&
+ git branch B'
+
+test_expect_success 'merge unrelated branches' \
+ 'git checkout -b merged A &&
+ git merge B'
+
+test_done
--
2.8.0.rc3.9.g44915db
From cd6b2388c73f37b3dd6180d9a42993fd219ebb31 Mon Sep 17 00:00:00 2001
From: Linus Torvalds
Date: Fri, 18 Mar 2016 12:57:30 -0700
Subject: [PATCH 2/2] merge: fail merging of unrelated branches
Add test for this, and add the "unrelated" merge option to allow it to succeed.
Signed-off-by: Linus Torvalds
---
merge-recursive.c | 6 ++
merge-recursive.h | 1 +
t/t3035-merge-recursive-across-project.sh | 7 +--
3 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index b880ae50e7ee..92779db0bbe6 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1927,6 +1927,9 @@ int merge_recursive(struct merge_options *o,
/* if there is no common ancestor, use an empty tree */
struct tree *tree;
+ if (!o->allow_unrelated)
+ die(_("will not merge unrelated branches"));
+
tree = lookup_tree(EMPTY_TREE_SHA1_BIN);
merged_common_ancestors = make_virtual_commit(tree, "ancestor");
}
@@ -2039,6 +2042,7 @@ void init_merge_options(struct merge_options *o)
memset(o, 0, sizeof(struct merge_options));
o->verbosity = 2;
o->buffer_output = 1;
+ o->allow_unrelated = 0;
o->diff_rename_limit = -1;
o->merge_rename_limit = -1;
o->renormalize = 0;
@@ -2092,6 +2096,8 @@ int parse_merge_opt(struct merge_options *o, const char *s)
o->renormalize = 1;
else if (!strcmp(s, "no-renormalize"))
o->renormalize = 0;
+ else if (!strcmp(s, "unrelated"))
+ o->allow_unrelated = 1;
else if (!strcmp(s, "no-renames"))
o->detect_rename = 0;
else if (!strcmp(s, "find-renames")) {
diff --git a/merge-recursive.h b/merge-recursive.h
index 52f0201f68a3..19eb52eeb732 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -15,6 +15,7 @@ struct merge_options {
const char *subtree_shift;
unsigned buffer_output : 1;
unsigned renormalize : 1;
+ unsigned allow_unrelated : 1;
long xdl_opts;
int verbosity;
int detect_rename;
diff --git a/t/t3035-merge-recursive-across-project.sh b/t/t3035-merge-recursive-across-project.sh
index b5d614db6b08..87902f1c8f66 100755
--- a/t/t3035-merge-recursive-across-project.sh
+++ b/t/t3035-merge-recursive-across-project.sh
@@ -21,8 +21,11 @@ test_expect_success 'setup repository' \
git commit -m "Branch B" --amend B &&
git branch B'
-test_expect_success 'merge unrelated branches' \
+test_expect_success 'fail merging of unrelated branches' \
'git checkout -b merged A &&
- git merge B'
+ test_must_fail git merge
Re: [PATCH] merge: refuse to create too cool a merge by default
On Fri, Mar 18, 2016 at 4:21 PM, Junio C Hamano wrote: > While it makes sense to allow merging unrelated histories of two > projects that started independently into one, in the way "gitk" was > merged to "git" itself aka "the coolest merge ever", such a merge is > still an unusual event. Worse, if somebody creates an independent > history by starting from a tarball of an established project and > sends a pull request to the original project, "git merge" however > happily creates such a merge without any sign of something unusual > is happening. > > Teach "git merge" to refuse to create such a merge by default, > unless the user passes a new "--allow-unrelated-histories" option to > tell it that the user is aware that two unrelated projects are > merged. > [...] > Signed-off-by: Junio C Hamano > --- > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh > @@ -279,9 +280,10 @@ test_expect_success 'clone shallow depth count' ' > test_expect_success 'clone shallow object count' ' > ( > cd shallow && > + git prune && > git count-objects -v > ) > count.shallow && > - grep "^count: 55" count.shallow > + grep "^count: 54" count.shallow > ' Um, wasn't this change the subject of a separate patch[1]? [1]: http://article.gmane.org/gmane.comp.version-control.git/289246 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] merge: refuse to create too cool a merge by default
On 18/03/16 20:21, Junio C Hamano wrote: > While it makes sense to allow merging unrelated histories of two > projects that started independently into one, in the way "gitk" was > merged to "git" itself aka "the coolest merge ever", such a merge is > still an unusual event. Worse, if somebody creates an independent > history by starting from a tarball of an established project and > sends a pull request to the original project, "git merge" however > happily creates such a merge without any sign of something unusual > is happening. > > Teach "git merge" to refuse to create such a merge by default, > unless the user passes a new "--allow-unrelated-histories" option to > tell it that the user is aware that two unrelated projects are > merged. > > Because such a "two project merge" is a rare event, a configuration > option to always allow such a merge is not added. > > We could add the same option to "git pull" and have it passed > through to underlying "git merge". I do not have a fundamental > opposition against such a feature, but this commit does not do so > and instead leaves it as low-hanging fruit for others, because such > a "two project merge" would be done after fetching the other project > into some location in the working tree of an existing project and > making sure how well they fit together, it is sufficient to allow a > local merge without such an option pass-through from "git pull" to > "git merge". Many tests that are updated by this patch does the > pass-through manually by turning: > > git pull something > > into its equivalent: > > git fetch something && > git merge --allow-unrelated-histories FETCH_HEAD > > If somebody is inclined to add such an option, updated tests in this > change need to be adjusted back to: > > git pull --allow-unrelated-histories something > > Signed-off-by: Junio C Hamano > --- > > builtin/merge.c | 12 +--- > t/t3412-rebase-root.sh | 2 +- > t/t5500-fetch-pack.sh | 6 -- > t/t6009-rev-list-parent.sh | 4 +++- > t/t6010-merge-base.sh | 6 -- > t/t6012-rev-list-simplify.sh| 2 +- > t/t6026-merge-attr.sh | 3 ++- > t/t6029-merge-subtree.sh| 2 +- > t/t6101-rev-parse-parents.sh| 2 +- > t/t9400-git-cvsserver-server.sh | 3 ++- > 10 files changed, 28 insertions(+), 14 deletions(-) > [snip] > diff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh > index 39b3238..e0c5f44 100755 > --- a/t/t6010-merge-base.sh > +++ b/t/t6010-merge-base.sh > @@ -215,11 +215,13 @@ test_expect_success 'criss-cross merge-base for > octopus-step' ' > git reset --hard E && > test_commit CC2 && > test_tick && > - git merge -s ours CC1 && > + # E is a root commit unrelated to MMR root on which CC1 is based > + git merge -s ours --allow-unrelated-histories CC1 && > test_commit CC-o && > test_commit CCB && > git reset --hard CC1 && > - git merge -s ours CC2 && > + # E is a root commit unrelated to MMR root on which CC1 is based > + git merge -s ours --allow-unrelated-histories CC2 && I was only skimming this patch, but the above caught my eye - I assume that the comment should reference CC2 not CC1. yes? ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] merge: refuse to create too cool a merge by default
On Fri, 2016-03-18 at 13:21 -0700, Junio C Hamano wrote: > Many tests that are updated by this patch does the > pass-through manually by turning: Nit: Should be many tests ... do Also, I didn't notice a test that shows that "cool" merges without allow-unrelated-histories are forbidden. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] merge: refuse to create too cool a merge by default
David Turner writes: > Also, I didn't notice a test that shows that "cool" merges without > allow-unrelated-histories are forbidden. Yeah, because I didn't write one in the version that was sent out, which has been corrected in the one that will be on 'pu'. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
