{"thread":{"id":"15678","subject":"[BUG] merge --no-ff --no-commit && commit","startedAt":"2008-09-25T23:50:29Z","lastAt":"2008-10-06T14:19:03Z","messageCount":22,"participants":["SZEDER Gábor","Miklos Vajna","Shawn O. Pearce","Jakub Narebski"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"91649","messageId":"20080925235029.GA15837@neumann","threadId":"15678","inReplyTo":null,"subject":"[BUG] merge --no-ff --no-commit && commit","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2008-09-25T23:50:29Z","receivedAt":"2008-09-25T23:50:29Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\nafter today's update on master, the merge commit generated by 'git\nmerge --no-ff --no-commit branch && git commit' has only one parent if\nthe merge can be resolved as a fast forward.  The test case below\ndemonstrates this behaviour.\n\nBisecting showed that the regression was introduced by commit 6bb6b034\n(builtin-commit: use commit_tree(), 2008-09-10).\n\n\nRegards,\nGábor\n\n\n -- >8 --\n\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 9516f54..98cfc53 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -511,4 +511,13 @@ test_expect_success 'in-index merge' '\n \n test_debug 'gitk --all'\n \n+test_expect_success 'merge --no-ff --no-commit && commit' '\n+\tgit reset --hard c0 &&\n+\tgit merge --no-ff --no-commit c1 &&\n+\tEDITOR=: git commit &&\n+\tverify_parents $c0 $c1\n+'\n+\n+test_debug 'gitk --all'\n+\n test_done\n"},{"id":"91651","messageId":"1222389359-22191-1-git-send-email-vmiklos@frugalware.org","threadId":"15678","inReplyTo":"20080925235029.GA15837@neumann","subject":"[PATCH] builtin-commit: avoid using reduce_heads()","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-09-26T00:35:59Z","receivedAt":"2008-09-26T00:35:59Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"reduce_heads() can filter \"duplicated\" parents, where \"duplicated\"\nmeans: A is duplicated if both A and B are parent candidates and A is\nreachable from B. Doing so in builtin-merge makes sense, but not in\nbuiltin-commit, because this breaks git merge --no-commit --no-ff.\n\nTest case by SZEDER Gabor <szeder@ira.uka.de>\n---\n\nHere is a patch that fixes the problem for me. In fact I think it would\nbe nice if somehow git-merge could tell git-commit that 'do not reduce\nheads now' and in other cases it could still do so, but:\n\n\t1) This is not something git-commit did in the past, either.\n\n\t2) I have no idea what would be the right interface to do so\n           (maybe a new MERGE_FOO file under .git?)\n\n builtin-commit.c |    1 -\n t/t7600-merge.sh |    9 +++++++++\n 2 files changed, 9 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 55e1087..9daab0a 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -988,7 +988,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\treflog_msg = \"commit\";\n \t\tpptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;\n \t}\n-\tparents = reduce_heads(parents);\n \n \t/* Finally, get the commit message */\n \tstrbuf_init(&sb, 0);\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 9516f54..98cfc53 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -511,4 +511,13 @@ test_expect_success 'in-index merge' '\n \n test_debug 'gitk --all'\n \n+test_expect_success 'merge --no-ff --no-commit && commit' '\n+\tgit reset --hard c0 &&\n+\tgit merge --no-ff --no-commit c1 &&\n+\tEDITOR=: git commit &&\n+\tverify_parents $c0 $c1\n+'\n+\n+test_debug 'gitk --all'\n+\n test_done\n-- \n1.6.0.2\n"},{"id":"91653","messageId":"20080926010312.GE6816@neumann","threadId":"15678","inReplyTo":"1222389359-22191-1-git-send-email-vmiklos@frugalware.org","subject":"Re: [PATCH] builtin-commit: avoid using reduce_heads()","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2008-09-26T01:03:12Z","receivedAt":"2008-09-26T01:03:12Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Szia Miklós,\n\nOn Fri, Sep 26, 2008 at 02:35:59AM +0200, Miklos Vajna wrote:\n> reduce_heads() can filter \"duplicated\" parents, where \"duplicated\"\n> means: A is duplicated if both A and B are parent candidates and A is\n> reachable from B. Doing so in builtin-merge makes sense, but not in\n> builtin-commit, because this breaks git merge --no-commit --no-ff.\n\n> Here is a patch that fixes the problem for me.\n\nThanks for the quick response.  While your patch does fix the bug I\nreported, unfortunately it introduces a new one:  t7502-commit.sh\nfails at me with the following:\n\n\n*   ok 17: a SIGTERM should break locks\n\n* expecting success: \n\n        git rev-parse second master >expect &&\n        test_must_fail git merge second master &&\n        git checkout master g &&\n        EDITOR=: git commit -a &&\n        git cat-file commit HEAD | sed -n -e \"s/^parent //p\" -e\n\"/^$/q\" >actual &&\n        test_cmp expect actual\n\n\nAlready up-to-date with 1ae92d674ba95768a00bace571f5ef295ff1696b\nTrying simple merge with 9af21aa779d9e148680be525ce161baa37e4bdec\nSimple merge did not work, trying automatic merge.\nAuto-merging g\nERROR: Merge conflict in g\nfatal: merge program failed\nAutomatic merge failed; fix conflicts and then commit the result.\nCreated commit 45a4b2b: Merge branches 'second' and 'master' into\nsecond\n--- expect      2008-09-26 00:59:42.000000000 +0000\n+++ actual      2008-09-26 00:59:42.000000000 +0000\n@@ -1,2 +1,3 @@\n 1ae92d674ba95768a00bace571f5ef295ff1696b\n+1ae92d674ba95768a00bace571f5ef295ff1696b\n 9af21aa779d9e148680be525ce161baa37e4bdec\n* FAIL 18: Hand committing of a redundant merge removes dups\n        \n        \n                git rev-parse second master >expect &&\n                test_must_fail git merge second master &&\n                git checkout master g &&\n                EDITOR=: git commit -a &&\n                git cat-file commit HEAD | sed -n -e \"s/^parent //p\"\n-e \"/^$/q\" >actual &&\n                test_cmp expect actual\n        \n        \n\n* failed 1 among 18 test(s)\nmake: *** [t7502-commit.sh] Error 1\n\n\nRegards,\nGábor\n"},{"id":"91659","messageId":"20080926062404.GR23137@genesis.frugalware.org","threadId":"15678","inReplyTo":"20080926010312.GE6816@neumann","subject":"Re: [PATCH] builtin-commit: avoid using reduce_heads()","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-09-26T06:24:04Z","receivedAt":"2008-09-26T06:24:04Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Fri, Sep 26, 2008 at 03:03:12AM +0200, SZEDER Gábor <szeder@ira.uka.de> wrote:\n> Szia Miklós,\n\n:)\n\n> unfortunately it introduces a new one:  t7502-commit.sh\n> fails at me with the following:\n> \n> \n> *   ok 17: a SIGTERM should break locks\n> \n> * expecting success: \n> \n>         git rev-parse second master >expect &&\n>         test_must_fail git merge second master &&\n>         git checkout master g &&\n>         EDITOR=: git commit -a &&\n>         git cat-file commit HEAD | sed -n -e \"s/^parent //p\" -e\n> \"/^$/q\" >actual &&\n>         test_cmp expect actual\n\nUh-oh. I forgot to make test before git send-email. I'll have a look at\nit later today.\n"},{"id":"91695","messageId":"20080926151517.GV23137@genesis.frugalware.org","threadId":"15678","inReplyTo":"20080926010312.GE6816@neumann","subject":"Re: [PATCH] builtin-commit: avoid using reduce_heads()","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-09-26T15:15:17Z","receivedAt":"2008-09-26T15:15:17Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Fri, Sep 26, 2008 at 03:03:12AM +0200, SZEDER Gábor <szeder@ira.uka.de> wrote:\n> * FAIL 18: Hand committing of a redundant merge removes dups\n>         \n>         \n>                 git rev-parse second master >expect &&\n>                 test_must_fail git merge second master &&\n>                 git checkout master g &&\n>                 EDITOR=: git commit -a &&\n>                 git cat-file commit HEAD | sed -n -e \"s/^parent //p\"\n> -e \"/^$/q\" >actual &&\n>                 test_cmp expect actual\n>         \n>         \n> \n> * failed 1 among 18 test(s)\n> make: *** [t7502-commit.sh] Error 1\n\nOK, here is the scenario.\n\nThe basic problemis that if it's git-commit that creates the merge\ncommit and not git-merge, then git-commit does not \"know\" that git-merge\nwas invoked using --no-ff.\n\n--no-ff means that if reduce_heads() removes a parent, that will be a\nproblem, since the resulting commit will no longer be a merge commit.\n\nI think we can't avoid storing this info in a MERGE_MODE file, because\nwe have to add HEAD to the list of parents in case --no-ff is used, but\nwe should not do so in case it's reachable from existing heads and\n--no-ff is not used.\n\nI'll send a patch that does this in a bit.\n"},{"id":"91696","messageId":"1222442432-7430-1-git-send-email-vmiklos@frugalware.org","threadId":"15678","inReplyTo":"20080926151517.GV23137@genesis.frugalware.org","subject":"[PATCH] builtin-commit: avoid always using reduce_heads()","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-09-26T15:20:32Z","receivedAt":"2008-09-26T15:20:32Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"In case git merge --no-ff is used with --no-commit or we have a\nconflict, write info about if fast forwards are allowed or not to\n$GIT_DIR/MERGE_MODE.\n\nBased on this info, don't run reduce_heads() in case fast-forwards are\ndenied, to avoid turning a 'merge --no-ff' to a non-merge commit.\n\nTest case by SZEDER Gabor <szeder@ira.uka.de>\n\nSigned-off-by: Miklos Vajna <vmiklos@frugalware.org>\n---\n\nOn Fri, Sep 26, 2008 at 05:15:17PM +0200, Miklos Vajna <vmiklos@frugalware.org> wrote:\n> I'll send a patch that does this in a bit.\n\nHere it is.\n\n builtin-commit.c |   16 +++++++++++++++-\n builtin-merge.c  |    9 +++++++++\n t/t7600-merge.sh |    9 +++++++++\n 3 files changed, 33 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 55e1087..aac5cb4 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -937,6 +937,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tunsigned char commit_sha1[20];\n \tstruct ref_lock *ref_lock;\n \tstruct commit_list *parents = NULL, **pptr = &parents;\n+\tstruct stat statbuf;\n+\tint allow_fast_forward = 1;\n \n \tgit_config(git_commit_config, NULL);\n \n@@ -988,7 +990,18 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\treflog_msg = \"commit\";\n \t\tpptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;\n \t}\n-\tparents = reduce_heads(parents);\n+\tstrbuf_reset(&sb);\n+\tif (!stat(git_path(\"MERGE_MODE\"), &statbuf)) {\n+\t\tif (strbuf_read_file(&sb, git_path(\"MERGE_MODE\"), 0) < 0)\n+\t\t\tdie(\"could not read MERGE_MODE: %s\", strerror(errno));\n+\t\tprintf(\"debug, contents of buf: '%s'\\n\",sb.buf);\n+\t\tif (!strcmp(sb.buf, \"deny_fast_forward\")) {\n+\t\t\tprintf(\"debug, deny fast forward\\n\");\n+\t\t\tallow_fast_forward = 0;\n+\t\t}\n+\t}\n+\tif (allow_fast_forward)\n+\t\tparents = reduce_heads(parents);\n \n \t/* Finally, get the commit message */\n \tstrbuf_init(&sb, 0);\n@@ -1040,6 +1053,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \tunlink(git_path(\"MERGE_HEAD\"));\n \tunlink(git_path(\"MERGE_MSG\"));\n+\tunlink(git_path(\"MERGE_MODE\"));\n \tunlink(git_path(\"SQUASH_MSG\"));\n \n \tif (commit_index_files())\ndiff --git a/builtin-merge.c b/builtin-merge.c\nindex 5c65a58..973b167 100644\n--- a/builtin-merge.c\n+++ b/builtin-merge.c\n@@ -1210,6 +1210,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\tmerge_msg.len)\n \t\t\tdie(\"Could not write to %s\", git_path(\"MERGE_MSG\"));\n \t\tclose(fd);\n+\t\tfd = open(git_path(\"MERGE_MODE\"), O_WRONLY | O_CREAT, 0666);\n+\t\tif (fd < 0)\n+\t\t\tdie(\"Could open %s for writing\", git_path(\"MERGE_MODE\"));\n+\t\tstrbuf_reset(&buf);\n+\t\tif (!allow_fast_forward)\n+\t\t\tstrbuf_addf(&buf, \"deny_fast_forward\");\n+\t\tif (write_in_full(fd, buf.buf, buf.len) != buf.len)\n+\t\t\tdie(\"Could not write to %s\", git_path(\"MERGE_MODE\"));\n+\t\tclose(fd);\n \t}\n \n \tif (merge_was_ok) {\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 9516f54..98cfc53 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -511,4 +511,13 @@ test_expect_success 'in-index merge' '\n \n test_debug 'gitk --all'\n \n+test_expect_success 'merge --no-ff --no-commit && commit' '\n+\tgit reset --hard c0 &&\n+\tgit merge --no-ff --no-commit c1 &&\n+\tEDITOR=: git commit &&\n+\tverify_parents $c0 $c1\n+'\n+\n+test_debug 'gitk --all'\n+\n test_done\n-- \n1.6.0.2\n"},{"id":"91702","messageId":"20080926155204.GD17584@spearce.org","threadId":"15678","inReplyTo":"1222442432-7430-1-git-send-email-vmiklos@frugalware.org","subject":"Re: [PATCH] builtin-commit: avoid always using reduce_heads()","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-09-26T15:52:04Z","receivedAt":"2008-09-26T15:52:04Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Miklos Vajna <vmiklos@frugalware.org> wrote:\n> In case git merge --no-ff is used with --no-commit or we have a\n> conflict, write info about if fast forwards are allowed or not to\n> $GIT_DIR/MERGE_MODE.\n \n> diff --git a/builtin-commit.c b/builtin-commit.c\n> index 55e1087..aac5cb4 100644\n> --- a/builtin-commit.c\n> +++ b/builtin-commit.c\n> @@ -988,7 +990,18 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>  \t\treflog_msg = \"commit\";\n>  \t\tpptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;\n>  \t}\n> -\tparents = reduce_heads(parents);\n> +\tstrbuf_reset(&sb);\n> +\tif (!stat(git_path(\"MERGE_MODE\"), &statbuf)) {\n> +\t\tif (strbuf_read_file(&sb, git_path(\"MERGE_MODE\"), 0) < 0)\n> +\t\t\tdie(\"could not read MERGE_MODE: %s\", strerror(errno));\n> +\t\tprintf(\"debug, contents of buf: '%s'\\n\",sb.buf);\n> +\t\tif (!strcmp(sb.buf, \"deny_fast_forward\")) {\n> +\t\t\tprintf(\"debug, deny fast forward\\n\");\n\nLeft over debugging.\n\n> diff --git a/builtin-merge.c b/builtin-merge.c\n> index 5c65a58..973b167 100644\n> --- a/builtin-merge.c\n> +++ b/builtin-merge.c\n> @@ -1210,6 +1210,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>  \t\t\tmerge_msg.len)\n>  \t\t\tdie(\"Could not write to %s\", git_path(\"MERGE_MSG\"));\n>  \t\tclose(fd);\n> +\t\tfd = open(git_path(\"MERGE_MODE\"), O_WRONLY | O_CREAT, 0666);\n> +\t\tif (fd < 0)\n> +\t\t\tdie(\"Could open %s for writing\", git_path(\"MERGE_MODE\"));\n> +\t\tstrbuf_reset(&buf);\n> +\t\tif (!allow_fast_forward)\n> +\t\t\tstrbuf_addf(&buf, \"deny_fast_forward\");\n\nI wonder if the option in the file shouldn't be more related to\nthe merge command line option.  Its --no-ff to the user.  Maybe we\nshould call it \"no-ff\" here?  Or \"no-fast-forward\" if we want to go\nwith a longer name that is less likely to be ambiguous in the future?\n\n-- \nShawn.\n"},{"id":"91705","messageId":"m37i8y3mqt.fsf@localhost.localdomain","threadId":"15678","inReplyTo":"20080926151517.GV23137@genesis.frugalware.org","subject":"Re: [PATCH] builtin-commit: avoid using reduce_heads()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-09-26T16:17:39Z","receivedAt":"2008-09-26T16:17:39Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Miklos Vajna <vmiklos@frugalware.org> writes:\n\n> OK, here is the scenario.\n> \n> The basic problemis that if it's git-commit that creates the merge\n> commit and not git-merge, then git-commit does not \"know\" that git-merge\n> was invoked using --no-ff.\n> \n> --no-ff means that if reduce_heads() removes a parent, that will be a\n> problem, since the resulting commit will no longer be a merge commit.\n> \n> I think we can't avoid storing this info in a MERGE_MODE file, because\n> we have to add HEAD to the list of parents in case --no-ff is used, but\n> we should not do so in case it's reachable from existing heads and\n> --no-ff is not used.\n> \n> I'll send a patch that does this in a bit.\n\nThe following is summary of short dicussion about this issue on #git\nsee: http://colabti.org/irclogger/irclogger_log/git?date=2008-09-26,Fri#l1176\n\nThe problem is that sometimes git-commit finishes doing the merge, be\nit use of --no-commit option, or a conflict occurred; depending on\nwhether git-merge was invoked with or without --no-ff (--ff=never)\noption it should recurd reduced or non-reduced heads.\n\nThe problem for example occurs in the following situation:\n\n  .---.---.---.                  <--- a <--- HEAD\n              |\\\n              | \\-1---2          <--- b\n              \\\n               \\--x---y          <--- c\n\n$ git merge b c\n\n                      /------------- b \n                      v\n  .---.---.---.---1---2---M     <--- a <--- HEAD\n              \\          /\n               \\--x---y-/       <--- c\n\n$ git merge --no-ff b c\n\n  .---.---.---.---------M         <--- a <--- HEAD\n              |\\       /|\n              | \\-1---2 |         <--- b\n              \\         /\n               \\--x---y/          <--- c\n\n\n\nWe can select one of the following solutions:\n\n1. As proposed above save git-merge options, for example in MERGE_MODE\n   or MERGE_OPTS file, so git-commit knows what options to use if it\n   was invoked to finish a merge.\n\n2. git-merge saves _reduced_ heads in MERGE_HEAD, and git-commit\n   reduces only HEAD, unless it is in MERGE_HEAD.  This means that\n   git-commit uses the following pseudo-code\n\n     if (resolve_ref(HEAD) == MERGE_HEAD[0]) {\n        /* non fast-forward case */\n        merge HEAD + MERGE_HEAD\n     } else {\n        reduce_HEAD_maybe()\n        merge [HEAD] + MERGE_HEAD\n     }\n\n   This has the advantage that it doesn't change MERGE_HEAD semantic\n   for simple merge which did not began as octopus\n\n3. Remove reduce_heads() from git-commit entirely, and record in\n   MERGE_HEAD (or rather now MERGE_HEADS) _all_ _reduced_ heads.\n   _All_ means that HEAD is included in MERGE_HEAD if it is not\n   reduced, _reduced_ means that only non-dependent heads are in\n   MERGE_HEAD.  This for example means that for simple non-octopus\n   merge case MERGE_HEAD/MERGE_HEADS now contain _all_ parents,\n   and not only other side of merge.\n\n   This solution has the advantage of being clear solution, clarifying\n   semantic of MERGE_HEAD (currently HEAD is used both as target, i.e.\n   where merge is to be recorded, and as one of heads to merge/to\n   consider), and making it possible to separate layers: git-merge\n   is about merging, git-commit doesn't need to know anything about\n   merging.\n\n   The disadvantage is that it changes format (well, semantic) of\n   MERGE_HEAD, possibly breaking users' scripts; perhaps some of\n   git commands like \"git log --merge\" or \"git diff --merge\" should\n   be updated on such change.\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"91714","messageId":"20080926193122.GW23137@genesis.frugalware.org","threadId":"15678","inReplyTo":"m37i8y3mqt.fsf@localhost.localdomain","subject":"Re: [PATCH] builtin-commit: avoid using reduce_heads()","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-09-26T19:31:22Z","receivedAt":"2008-09-26T19:31:22Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Fri, Sep 26, 2008 at 09:17:39AM -0700, Jakub Narebski <jnareb@gmail.com> wrote:\n> 1. As proposed above save git-merge options, for example in MERGE_MODE\n>    or MERGE_OPTS file, so git-commit knows what options to use if it\n>    was invoked to finish a merge.\n\nFirst, thank you for the detailed description, I'm really bad in them.\n:)\n\nACK, that's why I implemented this one.\n\n> 2. git-merge saves _reduced_ heads in MERGE_HEAD, and git-commit\n>    reduces only HEAD, unless it is in MERGE_HEAD.  This means that\n>    git-commit uses the following pseudo-code\n> \n>      if (resolve_ref(HEAD) == MERGE_HEAD[0]) {\n>         /* non fast-forward case */\n>         merge HEAD + MERGE_HEAD\n>      } else {\n>         reduce_HEAD_maybe()\n>         merge [HEAD] + MERGE_HEAD\n>      }\n> \n>    This has the advantage that it doesn't change MERGE_HEAD semantic\n>    for simple merge which did not began as octopus\n\nThis is wrong IMHO. You can write the reduced heads to MERGE_HEAD but\nyou can't know if you can omit the HEAD in case it is reachable already\nfrom one of the heads or not. That depends on if the user used --no-ff\nor not.\n\n> 3. Remove reduce_heads() from git-commit entirely, and record in\n>    MERGE_HEAD (or rather now MERGE_HEADS) _all_ _reduced_ heads.\n>    _All_ means that HEAD is included in MERGE_HEAD if it is not\n>    reduced, _reduced_ means that only non-dependent heads are in\n>    MERGE_HEAD.  This for example means that for simple non-octopus\n>    merge case MERGE_HEAD/MERGE_HEADS now contain _all_ parents,\n>    and not only other side of merge.\n> \n>    This solution has the advantage of being clear solution, clarifying\n>    semantic of MERGE_HEAD (currently HEAD is used both as target, i.e.\n>    where merge is to be recorded, and as one of heads to merge/to\n>    consider), and making it possible to separate layers: git-merge\n>    is about merging, git-commit doesn't need to know anything about\n>    merging.\n> \n>    The disadvantage is that it changes format (well, semantic) of\n>    MERGE_HEAD, possibly breaking users' scripts; perhaps some of\n>    git commands like \"git log --merge\" or \"git diff --merge\" should\n>    be updated on such change.\n\nActually I think this would be ugly. MERGE_HEAD is about you can see\nwhat will be merged once you commit the merge, but once you include HEAD\nthere, you can't easily check that. Maybe it's just me who sometimes\nhave a look at that file myself directly... :-)\n"},{"id":"91715","messageId":"1222457868-9864-1-git-send-email-vmiklos@frugalware.org","threadId":"15678","inReplyTo":"20080926155204.GD17584@spearce.org","subject":"[PATCH] builtin-commit: avoid always using reduce_heads()","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-09-26T19:37:48Z","receivedAt":"2008-09-26T19:37:48Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"In case git merge --no-ff is used with --no-commit or we have a\nconflict, write info about if fast forwards are allowed or not to\n$GIT_DIR/MERGE_MODE.\n\nBased on this info, don't run reduce_heads() in case fast-forwards are\ndenied, to avoid turning a 'merge --no-ff' to a non-merge commit.\n\nTest case by SZEDER Gabor <szeder@ira.uka.de>\n\nSigned-off-by: Miklos Vajna <vmiklos@frugalware.org>\n---\n\nOn Fri, Sep 26, 2008 at 08:52:04AM -0700, \"Shawn O. Pearce\" <spearce@spearce.org> wrote:\n> > +                   printf(\"debug, deny fast forward\\n\");\n>\n> Left over debugging.\n\nUgh. Removed.\n\n> > +           if (!allow_fast_forward)\n> > +                   strbuf_addf(&buf, \"deny_fast_forward\");\n>\n> I wonder if the option in the file shouldn't be more related to\n> the merge command line option.  Its --no-ff to the user.  Maybe we\n> should call it \"no-ff\" here?  Or \"no-fast-forward\" if we want to go\n> with a longer name that is less likely to be ambiguous in the future?\n\nI changed it to \"no-ff\" so that it's consistent with the option naming.\n\n builtin-commit.c |   13 ++++++++++++-\n builtin-merge.c  |    9 +++++++++\n t/t7600-merge.sh |    9 +++++++++\n 3 files changed, 30 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 55e1087..f546cf7 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -937,6 +937,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tunsigned char commit_sha1[20];\n \tstruct ref_lock *ref_lock;\n \tstruct commit_list *parents = NULL, **pptr = &parents;\n+\tstruct stat statbuf;\n+\tint allow_fast_forward = 1;\n \n \tgit_config(git_commit_config, NULL);\n \n@@ -988,7 +990,15 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\treflog_msg = \"commit\";\n \t\tpptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;\n \t}\n-\tparents = reduce_heads(parents);\n+\tstrbuf_reset(&sb);\n+\tif (!stat(git_path(\"MERGE_MODE\"), &statbuf)) {\n+\t\tif (strbuf_read_file(&sb, git_path(\"MERGE_MODE\"), 0) < 0)\n+\t\t\tdie(\"could not read MERGE_MODE: %s\", strerror(errno));\n+\t\tif (!strcmp(sb.buf, \"no-ff\"))\n+\t\t\tallow_fast_forward = 0;\n+\t}\n+\tif (allow_fast_forward)\n+\t\tparents = reduce_heads(parents);\n \n \t/* Finally, get the commit message */\n \tstrbuf_init(&sb, 0);\n@@ -1040,6 +1050,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \tunlink(git_path(\"MERGE_HEAD\"));\n \tunlink(git_path(\"MERGE_MSG\"));\n+\tunlink(git_path(\"MERGE_MODE\"));\n \tunlink(git_path(\"SQUASH_MSG\"));\n \n \tif (commit_index_files())\ndiff --git a/builtin-merge.c b/builtin-merge.c\nindex 5c65a58..20102a0 100644\n--- a/builtin-merge.c\n+++ b/builtin-merge.c\n@@ -1210,6 +1210,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\tmerge_msg.len)\n \t\t\tdie(\"Could not write to %s\", git_path(\"MERGE_MSG\"));\n \t\tclose(fd);\n+\t\tfd = open(git_path(\"MERGE_MODE\"), O_WRONLY | O_CREAT, 0666);\n+\t\tif (fd < 0)\n+\t\t\tdie(\"Could open %s for writing\", git_path(\"MERGE_MODE\"));\n+\t\tstrbuf_reset(&buf);\n+\t\tif (!allow_fast_forward)\n+\t\t\tstrbuf_addf(&buf, \"no-ff\");\n+\t\tif (write_in_full(fd, buf.buf, buf.len) != buf.len)\n+\t\t\tdie(\"Could not write to %s\", git_path(\"MERGE_MODE\"));\n+\t\tclose(fd);\n \t}\n \n \tif (merge_was_ok) {\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 9516f54..98cfc53 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -511,4 +511,13 @@ test_expect_success 'in-index merge' '\n \n test_debug 'gitk --all'\n \n+test_expect_success 'merge --no-ff --no-commit && commit' '\n+\tgit reset --hard c0 &&\n+\tgit merge --no-ff --no-commit c1 &&\n+\tEDITOR=: git commit &&\n+\tverify_parents $c0 $c1\n+'\n+\n+test_debug 'gitk --all'\n+\n test_done\n-- \n1.6.0.2\n"},{"id":"91722","messageId":"200809270151.51785.jnareb@gmail.com","threadId":"15678","inReplyTo":"20080926193122.GW23137@genesis.frugalware.org","subject":"Re: [PATCH] builtin-commit: avoid using reduce_heads()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-09-26T23:51:47Z","receivedAt":"2008-09-26T23:51:47Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 26 Sep 2008, Miklos Vajna wrote:\n> On Fri, Sep 26, 2008 at 09:17:39AM -0700, Jakub Narebski <jnareb@gmail.com> wrote:\n> >\n> > 1. As proposed above save git-merge options, for example in MERGE_MODE\n> >    or MERGE_OPTS file, so git-commit knows what options to use if it\n> >    was invoked to finish a merge.\n> \n> First, thank you for the detailed description, I'm really bad in them.\n> :)\n> \n> ACK, that's why I implemented this one.\n\nThis is I think the fastest and seemingly simplest solution.\n\n> > 2. git-merge saves _reduced_ heads in MERGE_HEAD, and git-commit\n> >    reduces only HEAD, unless it is in MERGE_HEAD.  This means that\n> >    git-commit uses the following pseudo-code\n> > \n> >      if (resolve_ref(HEAD) == MERGE_HEAD[0]) {\n> >         /* non fast-forward case */\n> >         merge HEAD + MERGE_HEAD\n> >      } else {\n> >         reduce_HEAD_maybe()\n> >         merge [HEAD] + MERGE_HEAD\n> >      }\n> > \n> >    This has the advantage that it doesn't change MERGE_HEAD semantic\n> >    for simple merge which did not began as octopus\n> \n> This is wrong IMHO. You can write the reduced heads to MERGE_HEAD but\n> you can't know if you can omit the HEAD in case it is reachable already\n> from one of the heads or not. That depends on if the user used --no-ff\n> or not.\n\nI have tried to explain in above pseudocode git-commit is to use\nadditional hack to decide whether to try to reduce HEAD wrt MERGE_HEAD\nheads (normal case), or not (--no-ff case), namely if resolved HEAD\nis first head in MERGE_HEAD that means --no-ff case.\n\nIt is \"halfway\" solution and bit hacky (and ugly).\n\n> > 3. Remove reduce_heads() from git-commit entirely, and record in\n> >    MERGE_HEAD (or rather now MERGE_HEADS) _all_ _reduced_ heads.\n> >    _All_ means that HEAD is included in MERGE_HEAD if it is not\n> >    reduced, _reduced_ means that only non-dependent heads are in\n> >    MERGE_HEAD.  This for example means that for simple non-octopus\n> >    merge case MERGE_HEAD/MERGE_HEADS now contain _all_ parents,\n> >    and not only other side of merge.\n> > \n> >    This solution has the advantage of being clear solution, clarifying\n> >    semantic of MERGE_HEAD (currently HEAD is used both as target, i.e.\n> >    where merge is to be recorded, and as one of heads to merge/to\n> >    consider), and making it possible to separate layers: git-merge\n> >    is about merging, git-commit doesn't need to know anything about\n> >    merging.\n> > \n> >    The disadvantage is that it changes format (well, semantic) of\n> >    MERGE_HEAD, possibly breaking users' scripts; perhaps some of\n> >    git commands like \"git log --merge\" or \"git diff --merge\" should\n> >    be updated on such change.\n> \n> Actually I think this would be ugly. MERGE_HEAD is about you can see\n> what will be merged once you commit the merge, but once you include HEAD\n> there, you can't easily check that. Maybe it's just me who sometimes\n> have a look at that file myself directly... :-)\n\nThe new semantic would be very simple.  If there is no MERGE_HEAD, it\nis an ordinary no-merge commit, and its only parent would be previous\n(current) version of HEAD.  If there is MERGE_HEAD it contains _all_\nparents in a merge commit, and only those heads which will be parents\n(_reduced_ heads); if HEAD is symref, we modify given branch so it\npoints to newly created merge commit.\n\nCurrently parents of merge commits are 'reduce(HEAD + MERGE_HEAD)'\nin symbolic equation; I propose they would be simply 'MERGE_HEAD'.\nthen we set this branch to new commit\n\n-- \nJakub Narebski\nPoland\n"},{"id":"91723","messageId":"20080927001617.GA6941@neumann","threadId":"15678","inReplyTo":"m37i8y3mqt.fsf@localhost.localdomain","subject":"Re: [PATCH] builtin-commit: avoid using reduce_heads()","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2008-09-27T00:16:17Z","receivedAt":"2008-09-27T00:16:17Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi all,\n\nOn Fri, Sep 26, 2008 at 09:17:39AM -0700, Jakub Narebski wrote:\n> 3. Remove reduce_heads() from git-commit entirely, and record in\n>    MERGE_HEAD (or rather now MERGE_HEADS) _all_ _reduced_ heads.\n>    _All_ means that HEAD is included in MERGE_HEAD if it is not\n>    reduced, _reduced_ means that only non-dependent heads are in\n>    MERGE_HEAD.  This for example means that for simple non-octopus\n>    merge case MERGE_HEAD/MERGE_HEADS now contain _all_ parents,\n>    and not only other side of merge.\n> \n>    This solution has the advantage of being clear solution, clarifying\n>    semantic of MERGE_HEAD (currently HEAD is used both as target, i.e.\n>    where merge is to be recorded, and as one of heads to merge/to\n>    consider), and making it possible to separate layers: git-merge\n>    is about merging, git-commit doesn't need to know anything about\n>    merging.\n\nWell, I'm just following this from the sidelines...  but from design\npoint of view I would prefer #3 because of the clear separation of\nthe merge and commit steps.\n\nBest,\nGábor\n"},{"id":"91846","messageId":"20080929150722.GU23137@genesis.frugalware.org","threadId":"15678","inReplyTo":"200809270151.51785.jnareb@gmail.com","subject":"Re: [PATCH] builtin-commit: avoid using reduce_heads()","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-09-29T15:07:22Z","receivedAt":"2008-09-29T15:07:22Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Sat, Sep 27, 2008 at 01:51:47AM +0200, Jakub Narebski <jnareb@gmail.com> wrote:\n> > Actually I think this would be ugly. MERGE_HEAD is about you can see\n> > what will be merged once you commit the merge, but once you include HEAD\n> > there, you can't easily check that. Maybe it's just me who sometimes\n> > have a look at that file myself directly... :-)\n> \n> The new semantic would be very simple.  If there is no MERGE_HEAD, it\n> is an ordinary no-merge commit, and its only parent would be previous\n> (current) version of HEAD.  If there is MERGE_HEAD it contains _all_\n> parents in a merge commit, and only those heads which will be parents\n> (_reduced_ heads); if HEAD is symref, we modify given branch so it\n> points to newly created merge commit.\n> \n> Currently parents of merge commits are 'reduce(HEAD + MERGE_HEAD)'\n> in symbolic equation; I propose they would be simply 'MERGE_HEAD'.\n> then we set this branch to new commit\n\nYes. Currently - after a merge conflict - you are able to check what\nheads caused were merged, which caused the conflict, but with this\napproach you would not be able to. I think this would be a step back...\n"},{"id":"91868","messageId":"20080929181856.GX23137@genesis.frugalware.org","threadId":"15678","inReplyTo":"20080929150722.GU23137@genesis.frugalware.org","subject":"Re: [PATCH] builtin-commit: avoid using reduce_heads()","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-09-29T18:18:56Z","receivedAt":"2008-09-29T18:18:56Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Mon, Sep 29, 2008 at 05:07:22PM +0200, Miklos Vajna <vmiklos@frugalware.org> wrote:\n> > Currently parents of merge commits are 'reduce(HEAD + MERGE_HEAD)'\n> > in symbolic equation; I propose they would be simply 'MERGE_HEAD'.\n> > then we set this branch to new commit\n> \n> Yes. Currently - after a merge conflict - you are able to check what\n> heads caused were merged, which caused the conflict, but with this\n> approach you would not be able to. I think this would be a step back...\n\nUh, I should read my mail before sending it next time.\n\nI just wanted to say that in case, for example, I merge A^ and A, but I\nget a conflict after octopus tried to merge A^ then it can be a useful\ninfo to see that A^ was a head. Putting reduce(HEAD + MERGE_HEAD) to\nMERGE_HEAD would hide this info, which would make the situation worse,\nIMHO.\n"},{"id":"91871","messageId":"200809292044.24799.jnareb@gmail.com","threadId":"15678","inReplyTo":"20080929181856.GX23137@genesis.frugalware.org","subject":"Re: [PATCH] builtin-commit: avoid using reduce_heads()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-09-29T18:44:24Z","receivedAt":"2008-09-29T18:44:24Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Dnia poniedziałek 29. września 2008 20:18, Miklos Vajna napisał:\n> On Mon, Sep 29, 2008 at 05:07:22PM +0200, Miklos Vajna <vmiklos@frugalware.org> wrote:\n\n> > > Currently parents of merge commits are 'reduce(HEAD + MERGE_HEAD)'\n> > > in symbolic equation; I propose they would be simply 'MERGE_HEAD'.\n> > > then we set this branch to new commit\n> > \n> > Yes. Currently - after a merge conflict - you are able to check what\n> > heads caused were merged, which caused the conflict, but with this\n> > approach you would not be able to. I think this would be a step back...\n> \n> Uh, I should read my mail before sending it next time.\n> \n> I just wanted to say that in case, for example, I merge A^ and A, but I\n> get a conflict after octopus tried to merge A^ then it can be a useful\n> info to see that A^ was a head. Putting reduce(HEAD + MERGE_HEAD) to\n> MERGE_HEAD would hide this info, which would make the situation worse,\n> IMHO.\n\nI don't understand: I thought that merge strategy gets _reduced_ heads.\nMoreover, if head reduction reduces number of heads to two, you would\nuse twohead merge (recursive) instead of octopus, and fast-forward if\nthere is only one head after reduction.\n\nAll that I proposed is to put those reduced heads into MERGE_HEAD.\nI did not proposed to put yet unresolved heads in MERGE_HEAD in case\nof octopus merge conflict. I think either you misunderstood me, or\nI misunderstood you.\n\nTake for example the following case of\n[H@repo]$ git merge a b c\n\n   .---.---.---.---H                   <-- H <-- HEAD\n                    \\\n                     \\.---.---.---a    <-- a\n                           \\\n                            \\-b        <-- b\n                               \\                              \n                                \\--c   <-- c\n\nCurrently after failed merge we have:\nHEAD: \n  refs: refs/heads/H\nMERGE_HEAD\n  sha1(a)\n  sha1(b)\n  sha1(c)\n\nI propose it to be\nHEAD:\n  refs: refs/heads/H\nMERGE_HEAD\n  sha1(a)\n  sha1(c)\n\nAnd merge strategy chosen would be twohead one (recursive).\n\n\nIf the situation was slightly different\n\n   .---.---.---.---.--.---.---.---H    <-- H <-- HEAD\n                    \\\n                     \\.---.---.---a    <-- a\n                           \\\n                            \\-b        <-- b\n                               \\                              \n                                \\--c   <-- c\n\n\nI propose it to be\nHEAD:\n  refs: refs/heads/H\nMERGE_HEAD\n  sha1(H)\n  sha1(a)\n  sha1(c)\n\nAnd merge strategy chosen would be octopus.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"92214","messageId":"20081003023518.GA3291@spearce.org","threadId":"15678","inReplyTo":"1222457868-9864-1-git-send-email-vmiklos@frugalware.org","subject":"Re: [PATCH] builtin-commit: avoid always using reduce_heads()","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-10-03T02:35:18Z","receivedAt":"2008-10-03T02:35:18Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Miklos Vajna <vmiklos@frugalware.org> wrote:\n> diff --git a/builtin-commit.c b/builtin-commit.c\n> index 55e1087..f546cf7 100644\n> --- a/builtin-commit.c\n> +++ b/builtin-commit.c\n> @@ -1040,6 +1050,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>  \n>  \tunlink(git_path(\"MERGE_HEAD\"));\n>  \tunlink(git_path(\"MERGE_MSG\"));\n> +\tunlink(git_path(\"MERGE_MODE\"));\n>  \tunlink(git_path(\"SQUASH_MSG\"));\n>  \n>  \tif (commit_index_files())\n\nHmmph.  Should branch.c and builtin-reset.c clean this new file\nup too?\n\n> diff --git a/builtin-merge.c b/builtin-merge.c\n> index 5c65a58..20102a0 100644\n> --- a/builtin-merge.c\n> +++ b/builtin-merge.c\n> @@ -1210,6 +1210,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>  \t\t\tmerge_msg.len)\n>  \t\t\tdie(\"Could not write to %s\", git_path(\"MERGE_MSG\"));\n>  \t\tclose(fd);\n> +\t\tfd = open(git_path(\"MERGE_MODE\"), O_WRONLY | O_CREAT, 0666);\n> +\t\tif (fd < 0)\n> +\t\t\tdie(\"Could open %s for writing\", git_path(\"MERGE_MODE\"));\n> +\t\tstrbuf_reset(&buf);\n> +\t\tif (!allow_fast_forward)\n> +\t\t\tstrbuf_addf(&buf, \"no-ff\");\n> +\t\tif (write_in_full(fd, buf.buf, buf.len) != buf.len)\n\nShouldn't we open this file with O_TRUNC to avoid this scenario:\n\n  $ git merge --no-ff --no-commit foo\n  $ git reset --hard\n  $ git merge --no-commit foo\n  ... *sigh* MERGE_MODE still has \"no-ff\" in it ...\n\nThis is especially true since some porcelain (e.g. git-gui) just\ndeletes MERGE_HEAD right now and doesn't know about cleaning up\nMERGE_MODE.  We'd want to at least reset it correctly on the next\ninvocation to git-merge.\n\n-- \nShawn.\n"},{"id":"92242","messageId":"1223035487-2576-1-git-send-email-vmiklos@frugalware.org","threadId":"15678","inReplyTo":"20081003023518.GA3291@spearce.org","subject":"[PATCH] builtin-commit: avoid always using reduce_heads()","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-10-03T12:04:47Z","receivedAt":"2008-10-03T12:04:47Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"In case git merge --no-ff is used with --no-commit or we have a\nconflict, write info about if fast forwards are allowed or not to\n$GIT_DIR/MERGE_MODE.\n\nBased on this info, don't run reduce_heads() in case fast-forwards are\ndenied, to avoid turning a 'merge --no-ff' to a non-merge commit.\n\nTest case by SZEDER Gabor <szeder@ira.uka.de>\n\nSigned-off-by: Miklos Vajna <vmiklos@frugalware.org>\n---\n\nOn Thu, Oct 02, 2008 at 07:35:18PM -0700, \"Shawn O. Pearce\" <spearce@spearce.org> wrote:\n> > +   unlink(git_path(\"MERGE_MODE\"));\n> >     unlink(git_path(\"SQUASH_MSG\"));\n> >\n> >     if (commit_index_files())\n>\n> Hmmph.  Should branch.c and builtin-reset.c clean this new file\n> up too?\n\nRight, I added it to branch.c::remove_branch_state() and\nbuiltin-merge.c::drop_save(). I don't think I should touch\nbuiltin-reset.c, it does not delete MERGE_HEAD/MERGE_MSG either.\n\n> > +           fd = open(git_path(\"MERGE_MODE\"), O_WRONLY | O_CREAT,\n> > 0666);\n> > +           if (fd < 0)\n> > +                   die(\"Could open %s for writing\",\n> > git_path(\"MERGE_MODE\"));\n> > +           strbuf_reset(&buf);\n> > +           if (!allow_fast_forward)\n> > +                   strbuf_addf(&buf, \"no-ff\");\n> > +           if (write_in_full(fd, buf.buf, buf.len) != buf.len)\n>\n> Shouldn't we open this file with O_TRUNC to avoid this scenario:\n>\n>   $ git merge --no-ff --no-commit foo\n>   $ git reset --hard\n>   $ git merge --no-commit foo\n>   ... *sigh* MERGE_MODE still has \"no-ff\" in it ...\n>\n> This is especially true since some porcelain (e.g. git-gui) just\n> deletes MERGE_HEAD right now and doesn't know about cleaning up\n> MERGE_MODE.  We'd want to at least reset it correctly on the next\n> invocation to git-merge.\n\nFixed.\n\n branch.c         |    1 +\n builtin-commit.c |   13 ++++++++++++-\n builtin-merge.c  |   10 ++++++++++\n t/t7600-merge.sh |    9 +++++++++\n 4 files changed, 32 insertions(+), 1 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex b1e59f2..205b89d 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -168,5 +168,6 @@ void remove_branch_state(void)\n \tunlink(git_path(\"MERGE_HEAD\"));\n \tunlink(git_path(\"MERGE_RR\"));\n \tunlink(git_path(\"MERGE_MSG\"));\n+\tunlink(git_path(\"MERGE_MODE\"));\n \tunlink(git_path(\"SQUASH_MSG\"));\n }\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 55e1087..f546cf7 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -937,6 +937,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tunsigned char commit_sha1[20];\n \tstruct ref_lock *ref_lock;\n \tstruct commit_list *parents = NULL, **pptr = &parents;\n+\tstruct stat statbuf;\n+\tint allow_fast_forward = 1;\n \n \tgit_config(git_commit_config, NULL);\n \n@@ -988,7 +990,15 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\treflog_msg = \"commit\";\n \t\tpptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;\n \t}\n-\tparents = reduce_heads(parents);\n+\tstrbuf_reset(&sb);\n+\tif (!stat(git_path(\"MERGE_MODE\"), &statbuf)) {\n+\t\tif (strbuf_read_file(&sb, git_path(\"MERGE_MODE\"), 0) < 0)\n+\t\t\tdie(\"could not read MERGE_MODE: %s\", strerror(errno));\n+\t\tif (!strcmp(sb.buf, \"no-ff\"))\n+\t\t\tallow_fast_forward = 0;\n+\t}\n+\tif (allow_fast_forward)\n+\t\tparents = reduce_heads(parents);\n \n \t/* Finally, get the commit message */\n \tstrbuf_init(&sb, 0);\n@@ -1040,6 +1050,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \tunlink(git_path(\"MERGE_HEAD\"));\n \tunlink(git_path(\"MERGE_MSG\"));\n+\tunlink(git_path(\"MERGE_MODE\"));\n \tunlink(git_path(\"SQUASH_MSG\"));\n \n \tif (commit_index_files())\ndiff --git a/builtin-merge.c b/builtin-merge.c\nindex 5c65a58..4c9ed5d 100644\n--- a/builtin-merge.c\n+++ b/builtin-merge.c\n@@ -180,6 +180,7 @@ static void drop_save(void)\n {\n \tunlink(git_path(\"MERGE_HEAD\"));\n \tunlink(git_path(\"MERGE_MSG\"));\n+\tunlink(git_path(\"MERGE_MODE\"));\n }\n \n static void save_state(void)\n@@ -1210,6 +1211,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\tmerge_msg.len)\n \t\t\tdie(\"Could not write to %s\", git_path(\"MERGE_MSG\"));\n \t\tclose(fd);\n+\t\tfd = open(git_path(\"MERGE_MODE\"), O_WRONLY | O_CREAT | O_TRUNC, 0666);\n+\t\tif (fd < 0)\n+\t\t\tdie(\"Could open %s for writing\", git_path(\"MERGE_MODE\"));\n+\t\tstrbuf_reset(&buf);\n+\t\tif (!allow_fast_forward)\n+\t\t\tstrbuf_addf(&buf, \"no-ff\");\n+\t\tif (write_in_full(fd, buf.buf, buf.len) != buf.len)\n+\t\t\tdie(\"Could not write to %s\", git_path(\"MERGE_MODE\"));\n+\t\tclose(fd);\n \t}\n \n \tif (merge_was_ok) {\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 9516f54..98cfc53 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -511,4 +511,13 @@ test_expect_success 'in-index merge' '\n \n test_debug 'gitk --all'\n \n+test_expect_success 'merge --no-ff --no-commit && commit' '\n+\tgit reset --hard c0 &&\n+\tgit merge --no-ff --no-commit c1 &&\n+\tEDITOR=: git commit &&\n+\tverify_parents $c0 $c1\n+'\n+\n+test_debug 'gitk --all'\n+\n test_done\n-- \n1.6.0.2\n"},{"id":"92254","messageId":"20081003145915.GB3291@spearce.org","threadId":"15678","inReplyTo":"1223035487-2576-1-git-send-email-vmiklos@frugalware.org","subject":"Re: [PATCH] builtin-commit: avoid always using reduce_heads()","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-10-03T14:59:15Z","receivedAt":"2008-10-03T14:59:15Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Miklos Vajna <vmiklos@frugalware.org> wrote:\n> In case git merge --no-ff is used with --no-commit or we have a\n> conflict, write info about if fast forwards are allowed or not to\n> $GIT_DIR/MERGE_MODE.\n\n*sigh*\n\n$ git co -b mv/merge-noff master && git am -s <this\n$ make test\n...\n*** t0004-unwritable.sh ***\n* FAIL 1: setup\n...\n\n$ cd t && ./t0004-unwritable.sh -i -v\n...\n./test-lib.sh: line 237:   362 Segmentation fault      git commit -m initial\n* FAIL 1: setup\n\n\nI leave the debugging to you.  ;-)\n \n-- \nShawn.\n"},{"id":"92255","messageId":"20081003150927.GG6839@neumann","threadId":"15678","inReplyTo":"1223035487-2576-1-git-send-email-vmiklos@frugalware.org","subject":"[PATCH] builtin-commit: use reduce_heads() only when appropriate","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2008-10-03T15:09:27Z","receivedAt":"2008-10-03T15:09:27Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Since commit 6bb6b034 (builtin-commit: use commit_tree(), 2008-09-10),\nbuiltin-commit performs a reduce_heads() unconditionally.  However,\nit's not always needed, and in some cases even harmful.\n\nreduce_heads() is not needed for the initial commit or for an\n\"ordinary\" commit, because they don't have any or have only one\nparent, respectively.\n\nreduce_heads() must be avoided when 'git commit' is run after a 'git\nmerge --no-ff --no-commit', otherwise it will turn the\nnon-fast-forward merge into fast-forward.  For the same reason,\nreduce_heads() must be avoided when amending such a merge commit.\n\nTo resolve this issue, 'git merge' will write info about whether\nfast-forward is allowed or not to $GIT_DIR/MERGE_MODE.  Based on this\ninfo, 'git commit' will only perform reduce_heads() when it's\ncommitting a merge and fast-forward is enabled.\n\nAlso add test cases to ensure that non-fast-forward merges are\ncommitted and amended properly.\n\nSigned-off-by: SZEDER Gábor <szeder@ira.uka.de>\n---\n\n  I think this should be squashed onto Miklós' patch.\n\n  First, it fixes the segmentation fault caused by using the strbuf\n  before initializing it.  Second, amending a no-ff merge commit has\n  the same issues with reduce_heads();  see the new test case.  And the\n  commit message is also more detailed ;)\n\n  I'm not sure about putting these two new test into t7600-merge.sh.\n  Although the infrastructure to keep these tests very short (repo with\n  branches already set up, verify_parents) is there available, the first\n  test tests not only merge, but the cooperation of merge and commit,\n  and this second one tests only commit.\n\n  Regards,\n  Gábor\n\n\n builtin-commit.c |   21 +++++++++++----------\n t/t7600-merge.sh |    7 +++++++\n 2 files changed, 18 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex e69b4af..93f360f 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -953,6 +953,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\treturn 1;\n \t}\n \n+\tstrbuf_init(&sb, 0);\n \t/* Determine parents */\n \tif (initial_commit) {\n \t\treflog_msg = \"commit (initial)\";\n@@ -986,22 +987,22 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tfclose(fp);\n \t\tstrbuf_release(&m);\n+\t\tif (!stat(git_path(\"MERGE_MODE\"), &statbuf)) {\n+\t\t\tif (strbuf_read_file(&sb, git_path(\"MERGE_MODE\"), 0) < 0)\n+\t\t\t\tdie(\"could not read MERGE_MODE: %s\",\n+\t\t\t\t\t\tstrerror(errno));\n+\t\t\tif (!strcmp(sb.buf, \"no-ff\"))\n+\t\t\t\tallow_fast_forward = 0;\n+\t\t}\n+\t\tif (allow_fast_forward)\n+\t\t\tparents = reduce_heads(parents);\n \t} else {\n \t\treflog_msg = \"commit\";\n \t\tpptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;\n \t}\n-\tstrbuf_reset(&sb);\n-\tif (!stat(git_path(\"MERGE_MODE\"), &statbuf)) {\n-\t\tif (strbuf_read_file(&sb, git_path(\"MERGE_MODE\"), 0) < 0)\n-\t\t\tdie(\"could not read MERGE_MODE: %s\", strerror(errno));\n-\t\tif (!strcmp(sb.buf, \"no-ff\"))\n-\t\t\tallow_fast_forward = 0;\n-\t}\n-\tif (allow_fast_forward)\n-\t\tparents = reduce_heads(parents);\n \n \t/* Finally, get the commit message */\n-\tstrbuf_init(&sb, 0);\n+\tstrbuf_reset(&sb);\n \tif (strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0) {\n \t\trollback_index_files();\n \t\tdie(\"could not read commit message\");\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 98cfc53..209d7cd 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -520,4 +520,11 @@ test_expect_success 'merge --no-ff --no-commit && commit' '\n \n test_debug 'gitk --all'\n \n+test_expect_success 'amending no-ff merge commit' '\n+\tEDITOR=: git commit --amend &&\n+\tverify_parents $c0 $c1\n+'\n+\n+test_debug 'gitk --all'\n+\n test_done\n-- \n1.6.0.2.430.gfc53\n"},{"id":"92352","messageId":"20081005194340.GT23137@genesis.frugalware.org","threadId":"15678","inReplyTo":"20081003150927.GG6839@neumann","subject":"Re: [PATCH] builtin-commit: use reduce_heads() only when appropriate","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-10-05T19:43:40Z","receivedAt":"2008-10-05T19:43:40Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Fri, Oct 03, 2008 at 05:09:27PM +0200, SZEDER Gábor <szeder@ira.uka.de> wrote:\n>   I think this should be squashed onto Miklós' patch.\n\nI'll resend a squashed patch with your signoff included.\n\n>   I'm not sure about putting these two new test into t7600-merge.sh.\n>   Although the infrastructure to keep these tests very short (repo with\n>   branches already set up, verify_parents) is there available, the first\n>   test tests not only merge, but the cooperation of merge and commit,\n>   and this second one tests only commit.\n\nGiven that originally it was git-merge.sh that did this reduction, I\nthink it's OK.\n"},{"id":"92355","messageId":"1223236300-2170-1-git-send-email-vmiklos@frugalware.org","threadId":"15678","inReplyTo":"20081003145915.GB3291@spearce.org","subject":"[PATCH] builtin-commit: avoid always using reduce_heads()","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-10-05T19:51:40Z","receivedAt":"2008-10-05T19:51:40Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"In case git merge --no-ff is used with --no-commit or we have a\nconflict, write info about if fast forwards are allowed or not to\n$GIT_DIR/MERGE_MODE.\n\nBased on this info, don't run reduce_heads() in case fast-forwards are\ndenied, to avoid turning a 'merge --no-ff' to a non-merge commit.\n\nSigned-off-by: Miklos Vajna <vmiklos@frugalware.org>\nSigned-off-by: SZEDER Gábor <szeder@ira.uka.de>\n---\n\nOn Fri, Oct 03, 2008 at 07:59:15AM -0700, \"Shawn O. Pearce\" <spearce@spearce.org> wrote:\n> ./test-lib.sh: line 237:   362 Segmentation fault      git commit -m\n> initial\n> * FAIL 1: setup\n>\n> I leave the debugging to you.  ;-)\n\nThat's weird, make test passed for me before I sent this patch. It was\nbased on 15dc66a.\n\nNow I squashed in Gabor's patch and rebased it against 52e8370, I hope\nit fixed the issue (make test still passes for me).\n\n branch.c         |    1 +\n builtin-commit.c |   16 ++++++++++++++--\n builtin-merge.c  |   10 ++++++++++\n t/t7600-merge.sh |   16 ++++++++++++++++\n 4 files changed, 41 insertions(+), 2 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex b1e59f2..205b89d 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -168,5 +168,6 @@ void remove_branch_state(void)\n \tunlink(git_path(\"MERGE_HEAD\"));\n \tunlink(git_path(\"MERGE_RR\"));\n \tunlink(git_path(\"MERGE_MSG\"));\n+\tunlink(git_path(\"MERGE_MODE\"));\n \tunlink(git_path(\"SQUASH_MSG\"));\n }\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex b920257..93f360f 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -937,6 +937,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tunsigned char commit_sha1[20];\n \tstruct ref_lock *ref_lock;\n \tstruct commit_list *parents = NULL, **pptr = &parents;\n+\tstruct stat statbuf;\n+\tint allow_fast_forward = 1;\n \n \tgit_config(git_commit_config, NULL);\n \n@@ -951,6 +953,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\treturn 1;\n \t}\n \n+\tstrbuf_init(&sb, 0);\n \t/* Determine parents */\n \tif (initial_commit) {\n \t\treflog_msg = \"commit (initial)\";\n@@ -984,14 +987,22 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tfclose(fp);\n \t\tstrbuf_release(&m);\n+\t\tif (!stat(git_path(\"MERGE_MODE\"), &statbuf)) {\n+\t\t\tif (strbuf_read_file(&sb, git_path(\"MERGE_MODE\"), 0) < 0)\n+\t\t\t\tdie(\"could not read MERGE_MODE: %s\",\n+\t\t\t\t\t\tstrerror(errno));\n+\t\t\tif (!strcmp(sb.buf, \"no-ff\"))\n+\t\t\t\tallow_fast_forward = 0;\n+\t\t}\n+\t\tif (allow_fast_forward)\n+\t\t\tparents = reduce_heads(parents);\n \t} else {\n \t\treflog_msg = \"commit\";\n \t\tpptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;\n \t}\n-\tparents = reduce_heads(parents);\n \n \t/* Finally, get the commit message */\n-\tstrbuf_init(&sb, 0);\n+\tstrbuf_reset(&sb);\n \tif (strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0) {\n \t\trollback_index_files();\n \t\tdie(\"could not read commit message\");\n@@ -1040,6 +1051,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \tunlink(git_path(\"MERGE_HEAD\"));\n \tunlink(git_path(\"MERGE_MSG\"));\n+\tunlink(git_path(\"MERGE_MODE\"));\n \tunlink(git_path(\"SQUASH_MSG\"));\n \n \tif (commit_index_files())\ndiff --git a/builtin-merge.c b/builtin-merge.c\nindex 5c65a58..4c9ed5d 100644\n--- a/builtin-merge.c\n+++ b/builtin-merge.c\n@@ -180,6 +180,7 @@ static void drop_save(void)\n {\n \tunlink(git_path(\"MERGE_HEAD\"));\n \tunlink(git_path(\"MERGE_MSG\"));\n+\tunlink(git_path(\"MERGE_MODE\"));\n }\n \n static void save_state(void)\n@@ -1210,6 +1211,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\tmerge_msg.len)\n \t\t\tdie(\"Could not write to %s\", git_path(\"MERGE_MSG\"));\n \t\tclose(fd);\n+\t\tfd = open(git_path(\"MERGE_MODE\"), O_WRONLY | O_CREAT | O_TRUNC, 0666);\n+\t\tif (fd < 0)\n+\t\t\tdie(\"Could open %s for writing\", git_path(\"MERGE_MODE\"));\n+\t\tstrbuf_reset(&buf);\n+\t\tif (!allow_fast_forward)\n+\t\t\tstrbuf_addf(&buf, \"no-ff\");\n+\t\tif (write_in_full(fd, buf.buf, buf.len) != buf.len)\n+\t\t\tdie(\"Could not write to %s\", git_path(\"MERGE_MODE\"));\n+\t\tclose(fd);\n \t}\n \n \tif (merge_was_ok) {\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 9516f54..209d7cd 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -511,4 +511,20 @@ test_expect_success 'in-index merge' '\n \n test_debug 'gitk --all'\n \n+test_expect_success 'merge --no-ff --no-commit && commit' '\n+\tgit reset --hard c0 &&\n+\tgit merge --no-ff --no-commit c1 &&\n+\tEDITOR=: git commit &&\n+\tverify_parents $c0 $c1\n+'\n+\n+test_debug 'gitk --all'\n+\n+test_expect_success 'amending no-ff merge commit' '\n+\tEDITOR=: git commit --amend &&\n+\tverify_parents $c0 $c1\n+'\n+\n+test_debug 'gitk --all'\n+\n test_done\n-- \n1.6.0.2\n"},{"id":"92409","messageId":"20081006141903.GB7684@spearce.org","threadId":"15678","inReplyTo":"1223236300-2170-1-git-send-email-vmiklos@frugalware.org","subject":"Re: [PATCH] builtin-commit: avoid always using reduce_heads()","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-10-06T14:19:03Z","receivedAt":"2008-10-06T14:19:03Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Miklos Vajna <vmiklos@frugalware.org> wrote:\n> In case git merge --no-ff is used with --no-commit or we have a\n> conflict, write info about if fast forwards are allowed or not to\n> $GIT_DIR/MERGE_MODE.\n \n> On Fri, Oct 03, 2008 at 07:59:15AM -0700, \"Shawn O. Pearce\" <spearce@spearce.org> wrote:\n> > ./test-lib.sh: line 237:   362 Segmentation fault      git commit -m\n> > initial\n> > * FAIL 1: setup\n> >\n> > I leave the debugging to you.  ;-)\n> \n> That's weird, make test passed for me before I sent this patch. It was\n> based on 15dc66a.\n> \n> Now I squashed in Gabor's patch and rebased it against 52e8370, I hope\n> it fixed the issue (make test still passes for me).\n\nYea, Gabor's patch squashed in is what was needed.  This is already\nin next; it was there on Friday.  But thanks anyway.\n\n-- \nShawn.\n"}]}