{"thread":{"id":"26971","subject":"[PATCH] Allow multiple merges to invalid HEAD","startedAt":"2011-04-03T06:46:56Z","lastAt":"2011-04-05T06:01:53Z","messageCount":6,"participants":["Timothy Chen","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"164981","messageId":"1301813216-19507-1-git-send-email-tnachen@gmail.com","threadId":"26971","inReplyTo":null,"subject":"[PATCH] Allow multiple merges to invalid HEAD","fromName":"Timothy Chen","fromEmail":"tnachen@gmail.com","sentAt":"2011-04-03T06:46:56Z","receivedAt":"2011-04-03T06:46:56Z","isPatch":true,"sender":{"key":"tnachen@gmail.com","avatar":null},"body":"Currently git merge only allows one branch when current HEAD\nis not yet pointing to a valid commit.\n\nThis patch will allow multiple branches to be passed in,\nand first updates current HEAD to the first branch's head then subsequently\nmerge the rest of the branches.\n---\n builtin/merge.c |   57 +++++++++++++++++++++++++++++-------------------------\n 1 files changed, 31 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex d54e7dd..290e0d4 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1090,9 +1090,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t * to forbid \"git merge\" into a branch yet to be born.\n \t\t * We do the same for \"git pull\".\n \t\t */\n-\t\tif (argc != 1)\n-\t\t\tdie(_(\"Can merge only exactly one commit into \"\n-\t\t\t\t\"empty head\"));\n \t\tif (squash)\n \t\t\tdie(_(\"Squash commit into empty head not supported yet\"));\n \t\tif (!allow_fast_forward)\n@@ -1101,36 +1098,44 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tremote_head = peel_to_type(argv[0], 0, NULL, OBJ_COMMIT);\n \t\tif (!remote_head)\n \t\t\tdie(_(\"%s - not something we can merge\"), argv[0]);\n-\t\tread_empty(remote_head->sha1, 0);\n \t\tupdate_ref(\"initial pull\", \"HEAD\", remote_head->sha1, NULL, 0,\n \t\t\t\tDIE_ON_ERR);\n-\t\treturn 0;\n-\t} else {\n-\t\tstruct strbuf merge_names = STRBUF_INIT;\n \n-\t\t/* We are invoked directly as the first-class UI. */\n+\t\tif (argc < 2)\n+\t\t\treturn 0;\n+\n+\t\thashcpy(head, remote_head->sha1);\n+\t\tread_empty(remote_head->sha1, 0);\n+\t\thead_arg = argv[0];\n+\t\targc--;\n+\t\targv++;\n+\t}\n+\n+\tstruct strbuf merge_names = STRBUF_INIT;\n+\n+\t/* We are invoked directly as the first-class UI. */\n+\tif(!head_invalid)\n \t\thead_arg = \"HEAD\";\n \n-\t\t/*\n-\t\t * All the rest are the commits being merged;\n-\t\t * prepare the standard merge summary message to\n-\t\t * be appended to the given message.  If remote\n-\t\t * is invalid we will die later in the common\n-\t\t * codepath so we discard the error in this\n-\t\t * loop.\n-\t\t */\n-\t\tfor (i = 0; i < argc; i++)\n-\t\t\tmerge_name(argv[i], &merge_names);\n-\n-\t\tif (!have_message || shortlog_len) {\n-\t\t\tfmt_merge_msg(&merge_names, &merge_msg, !have_message,\n-\t\t\t\t      shortlog_len);\n-\t\t\tif (merge_msg.len)\n-\t\t\t\tstrbuf_setlen(&merge_msg, merge_msg.len - 1);\n-\t\t}\n+\t/*\n+\t * All the rest are the commits being merged;\n+\t * prepare the standard merge summary message to\n+\t * be appended to the given message.  If remote\n+\t * is invalid we will die later in the common\n+\t * codepath so we discard the error in this\n+\t * loop.\n+\t */\n+\tfor (i = 0; i < argc; i++)\n+\t\tmerge_name(argv[i], &merge_names);\n+\n+\tif (!have_message || shortlog_len) {\n+\t\tfmt_merge_msg(&merge_names, &merge_msg, !have_message,\n+\t\t\t\t  shortlog_len);\n+\t\tif (merge_msg.len)\n+\t\t\tstrbuf_setlen(&merge_msg, merge_msg.len - 1);\n \t}\n \n-\tif (head_invalid || !argc)\n+\tif (!argc)\n \t\tusage_with_options(builtin_merge_usage,\n \t\t\tbuiltin_merge_options);\n \n-- \n1.7.3.4\n"},{"id":"164982","messageId":"7vlizrg6ef.fsf@alter.siamese.dyndns.org","threadId":"26971","inReplyTo":"1301813216-19507-1-git-send-email-tnachen@gmail.com","subject":"Re: [PATCH] Allow multiple merges to invalid HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-03T07:07:04Z","receivedAt":"2011-04-03T07:07:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Timothy Chen <tnachen@gmail.com> writes:\n\n> Subject: Re: [PATCH] Allow multiple merges to invalid HEAD\n\nI thought somebody already suggested to reword \"invalid HEAD\" to something\nmore sensible, like \"unborn branch\", but I may be misremembering things...\n\n> Currently git merge only allows one branch when current HEAD\n> is not yet pointing to a valid commit.\n>\n> This patch will allow multiple branches to be passed in,\n> and first updates current HEAD to the first branch's head then subsequently\n> merge the rest of the branches.\n> ---\n\nMissing sign-off.\n\nWhat's the point of being able to do this in the first place?\n\nThe _only_ reason \"git pull $there $branch\" into an unborn branch exists\nis because it feels like\n\n\t$ git clone $there $directory\n\ncould be broken down as this command sequence:\n\n\t$ mkdir $directory\n        $ cd $directory\n        $ git init\n        $ git remote add origin $there\n\t$ git fetch\n        $ git pull $there ;# or \"git merge origin/master\"\n\nAnd all the above steps actually do work, except for the last step and\nthat is why we added the \"pull into emptiness\" hack.\n\nEven though it does a very different thing at the mechanical level, having\nthe hack helps these two sequences (a single \"clone\" vs the \"init/remote\nadd/fetch/merge\") that are conceptually similar to behave the same.\n\nI however don't think of a halfway sane rationale similar to why we have\n\"merge one into emptiness\" to justify this patch.  It very much feels like\n\"because I can make things more complex\", not \"because this is an often\ndesired missing feature that is a major pain point\".\n"},{"id":"164984","messageId":"7vei5jg3jb.fsf@alter.siamese.dyndns.org","threadId":"26971","inReplyTo":"1301813216-19507-1-git-send-email-tnachen@gmail.com","subject":"Re: [PATCH] Allow multiple merges to invalid HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-03T08:08:56Z","receivedAt":"2011-04-03T08:08:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Timothy Chen <tnachen@gmail.com> writes:\n\n> This patch will allow multiple branches to be passed in,\n> and first updates current HEAD to the first branch's head then subsequently\n> merge the rest of the branches.\n\nI've questioned the motivation of the patch already, but let's comment on\nthe mechanics as well while I am waiting for some builds to finish ;-)\n\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index d54e7dd..290e0d4 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -1090,9 +1090,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>  \t\t * to forbid \"git merge\" into a branch yet to be born.\n>  \t\t * We do the same for \"git pull\".\n>  \t\t */\n>  \t\tif (squash)\n>  \t\t\tdie(_(\"Squash commit into empty head not supported yet\"));\n>  \t\tif (!allow_fast_forward)\n> @@ -1101,36 +1098,44 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>  \t\tremote_head = peel_to_type(argv[0], 0, NULL, OBJ_COMMIT);\n>  \t\tif (!remote_head)\n>  \t\t\tdie(_(\"%s - not something we can merge\"), argv[0]);\n>  \t\tupdate_ref(\"initial pull\", \"HEAD\", remote_head->sha1, NULL, 0,\n>  \t\t\t\tDIE_ON_ERR);\n\nYou are going to perform a series of operations that is a lot more complex\nthan what we traditionally have done at this point.  I do not think it is\nsafe at all to update the ref this early before even knowing if the rest\nof the command succeeds.\n\n> +\t\tif (argc < 2)\n> +\t\t\treturn 0;\n> +\n> +\t\thashcpy(head, remote_head->sha1);\n> +\t\tread_empty(remote_head->sha1, 0);\n> +\t\thead_arg = argv[0];\n> +\t\targc--;\n> +\t\targv++;\n> +\t}\n> +\n> +\tstruct strbuf merge_names = STRBUF_INIT;\n\nDecl-after-statement.\n\n> +\t/* We are invoked directly as the first-class UI. */\n> +\tif(!head_invalid)\n\nSP after syntactic keyword and the open paren associated with it.\n\n>  \t\thead_arg = \"HEAD\";\n>  \n> +\t/*\n> +\t * All the rest are the commits being merged;\n> +\t * prepare the standard merge summary message to\n> +\t * be appended to the given message.  If remote\n> +\t * is invalid we will die later in the common\n> +\t * codepath so we discard the error in this\n> +\t * loop.\n> +\t */\n> +\tfor (i = 0; i < argc; i++)\n> +\t\tmerge_name(argv[i], &merge_names);\n> +\n> +\tif (!have_message || shortlog_len) {\n> +\t\tfmt_merge_msg(&merge_names, &merge_msg, !have_message,\n> +\t\t\t\t  shortlog_len);\n> +\t\tif (merge_msg.len)\n> +\t\t\tstrbuf_setlen(&merge_msg, merge_msg.len - 1);\n>  \t}\n>  \n> +\tif (!argc)\n>  \t\tusage_with_options(builtin_merge_usage,\n>  \t\t\tbuiltin_merge_options);\n"},{"id":"165091","messageId":"20110403215246.GA6332@elie","threadId":"26971","inReplyTo":"7vlizrg6ef.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Allow multiple merges to invalid HEAD","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-04-03T21:52:46Z","receivedAt":"2011-04-03T21:52:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(+cc: Martin, who also requested something like this long ago[1] iirc)\nHi,\n\nThe following is only about motivation; I'm ignoring the mechanics for\nnow.\n\nJunio C Hamano wrote:\n\n> What's the point of being able to do this in the first place?\n>\n> The _only_ reason \"git pull $there $branch\" into an unborn branch exists\n> is because it feels like\n>\n> \t$ git clone $there $directory\n>\n> could be broken down as this command sequence:\n>\n> \t$ mkdir $directory\n>         $ cd $directory\n>         $ git init\n>         $ git remote add origin $there\n> \t$ git fetch\n>         $ git pull $there ;# or \"git merge origin/master\"\n\nI'm to blame for the suggestion, I'm afraid.  \n\nI haven't wanted to be able to \"git init && git remote add origin\n$there && git fetch && git merge origin/topic-1 origin/topic-2\" for a\nlong time, but there was a time (around 3 years ago, I think) I wanted\nit a lot.\n\nIn fact I can't remember the details of why.  The facts I remember:\n\n - /etc was kept in a git repository managed by etckeeper;\n - history was not precious --- it was just a way of understanding the\n   final result;\n - the user wasn't sure how to manage topic branches yet.  The usual\n   workflow involved\n\n    (1) cherry-picking each automatic commit to an appropriate topic\n        branch and writing a sensible description\n    (2) bundling the topic branches together with an octopus merge\n    (3) diff-ing against the automatically committed latest state to\n        see if anything was missing\n    (4) force a push.\n\nSo I suspect my use case was \"start with a bundle of these branches\".\nI could have (and did) used \"git checkout -b\" to grab the first one\nand merged the rest, but somehow a merge into nothingness seemed more\nintuitive.  Remember, this is before the days of checkout --orphan\nand rebase -i --everything.\n\nThat said.\n\nI doubt the above is a very compelling use case, to justify the\nmaintenance cost and easily explain in documentation.  I would find\nsomething like the following more compelling:\n\n\tCurrently git merge has a special case to \"pull into\n\temptiness\", to support the following sequence of commands:\n\n\t\tgit init repo\n\t\tcd repo\n\t\tgit remote add origin $url\n\t\tgit fetch origin\n\t\tgit merge origin/master\n\n\tBut it is very much a special case.  It does not support\n\toctopus merges, --squash, --stat, alternate strategies,\n\tor any of the rest of merge machinery.\n\n\tIt would be more intuitive if the above features _did_ work\n\twhere they make sense, and if this essentially worked as\n\tthough performing an ordinary merge with merge base being an\n\tempty tree that is ancestor of everything.\n\n\tHere is a first step --- support for octopus merges.  It\n\talso makes it easier to support the others, by [...].\n\n> It very much feels like\n> \"because I can make things more complex\", not \"because this is an often\n> desired missing feature that is a major pain point\".\n\nTherefore a good goal would be to make this make the code more simple\nat the same time as making the semantics so.\n\n(But also: Tim, please don't feel attached to this topic if you're not\ninterested in it; there are many more out there.)\n\nThanks for some useful feedback.\nJonathan\n\n[1] http://bugs.debian.org/432558\n"},{"id":"165083","messageId":"20110403222234.GB6537@elie","threadId":"26971","inReplyTo":"1301813216-19507-1-git-send-email-tnachen@gmail.com","subject":"Re: [PATCH] Allow multiple merges to invalid HEAD","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-04-03T22:22:56Z","receivedAt":"2011-04-03T22:22:56Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nTimothy Chen wrote:\n\n>  builtin/merge.c |   57 +++++++++++++++++++++++++++++-------------------------\n>  1 files changed, 31 insertions(+), 26 deletions(-)\n\nNow for mechanics.\n\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n[...]\n> @@ -1101,36 +1098,44 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>  \t\tremote_head = peel_to_type(argv[0], 0, NULL, OBJ_COMMIT);\n>  \t\tif (!remote_head)\n>  \t\t\tdie(_(\"%s - not something we can merge\"), argv[0]);\n> -\t\tread_empty(remote_head->sha1, 0);\n>  \t\tupdate_ref(\"initial pull\", \"HEAD\", remote_head->sha1, NULL, 0,\n>  \t\t\t\tDIE_ON_ERR);\n> -\t\treturn 0;\n> +\n> +\t\tif (argc < 2)\n> +\t\t\treturn 0;\n\nWhen argc == 1, this means read_empty never gets called.  Is that\nintended?\n\nIt breaks 7607.13.  Running \"make test\" is a good way to find some\nbreakages.\n\n> +\n> +\t\thashcpy(head, remote_head->sha1);\n> +\t\tread_empty(remote_head->sha1, 0);\n> +\t\thead_arg = argv[0];\n> +\t\targc--;\n> +\t\targv++;\n\nAs always when pretending something, I think a comment would be\nhelpful.  Something to the effect of:\n\n\t/*\n\t * We were called as \"git merge <branch1> <branch2> <branch3>...\".\n\t *\n\t * Now HEAD has advanced to <branch1>, and we can pretend\n\t * we were called as \"git merge <branch2> <branch3>...\".\n\t */\n\nThough I think I prefer the more explicit comment I suggested last\ntime[1].\n\n> +\t}\n> +\n> +\tstruct strbuf merge_names = STRBUF_INIT;\n> +\n> -\t} else {\n> -\t\tstruct strbuf merge_names = STRBUF_INIT;\n> -\n> -\t\t/* We are invoked directly as the first-class UI. */\n> +\t/* We are invoked directly as the first-class UI. */\n\nWon't this break\n\n\tgit merge \"message here\" $(git rev-parse HEAD) foo bar\n\n?  Previously this code was in an \"else\" block so it wasn't reached in\nthe is_old_style_invocation case.\n\n> -\tif (head_invalid || !argc)\n> +\tif (!argc)\n>  \t\tusage_with_options(builtin_merge_usage,\n>  \t\t\tbuiltin_merge_options);\n\nWhat happens with\n\n\tgit merge \"message here\" HEAD foo bar\n\nfrom an unborn branch?\n\nHope that helps.\nJonathan\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/170456\n"},{"id":"165166","messageId":"ABB52AD0-EAF4-43CA-951D-9ED8A5FB1929@gmail.com","threadId":"26971","inReplyTo":"20110403222234.GB6537@elie","subject":"Re: [PATCH] Allow multiple merges to invalid HEAD","fromName":"Timothy Chen","fromEmail":"tnachen@gmail.com","sentAt":"2011-04-05T06:01:53Z","receivedAt":"2011-04-05T06:01:53Z","isPatch":true,"sender":{"key":"tnachen@gmail.com","avatar":null},"body":"Thanks for all the review.\n\nYes I agree it does brings more complexity with a small gain in feature.\n\nSo please disregard this patch (also please forgive my missing signed-off as it's my first patch), as I'm gonna move on to work on some fixes on submodules as I originally intended.\n\nThanks!\n\nTim\n\nOn Apr 3, 2011, at 3:22 PM, Jonathan Nieder wrote:\n\n> Hi,\n> \n> Timothy Chen wrote:\n> \n>> builtin/merge.c |   57 +++++++++++++++++++++++++++++-------------------------\n>> 1 files changed, 31 insertions(+), 26 deletions(-)\n> \n> Now for mechanics.\n> \n>> --- a/builtin/merge.c\n>> +++ b/builtin/merge.c\n> [...]\n>> @@ -1101,36 +1098,44 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>> \t\tremote_head = peel_to_type(argv[0], 0, NULL, OBJ_COMMIT);\n>> \t\tif (!remote_head)\n>> \t\t\tdie(_(\"%s - not something we can merge\"), argv[0]);\n>> -\t\tread_empty(remote_head->sha1, 0);\n>> \t\tupdate_ref(\"initial pull\", \"HEAD\", remote_head->sha1, NULL, 0,\n>> \t\t\t\tDIE_ON_ERR);\n>> -\t\treturn 0;\n>> +\n>> +\t\tif (argc < 2)\n>> +\t\t\treturn 0;\n> \n> When argc == 1, this means read_empty never gets called.  Is that\n> intended?\n> \n> It breaks 7607.13.  Running \"make test\" is a good way to find some\n> breakages.\n> \n>> +\n>> +\t\thashcpy(head, remote_head->sha1);\n>> +\t\tread_empty(remote_head->sha1, 0);\n>> +\t\thead_arg = argv[0];\n>> +\t\targc--;\n>> +\t\targv++;\n> \n> As always when pretending something, I think a comment would be\n> helpful.  Something to the effect of:\n> \n> \t/*\n> \t * We were called as \"git merge <branch1> <branch2> <branch3>...\".\n> \t *\n> \t * Now HEAD has advanced to <branch1>, and we can pretend\n> \t * we were called as \"git merge <branch2> <branch3>...\".\n> \t */\n> \n> Though I think I prefer the more explicit comment I suggested last\n> time[1].\n> \n>> +\t}\n>> +\n>> +\tstruct strbuf merge_names = STRBUF_INIT;\n>> +\n>> -\t} else {\n>> -\t\tstruct strbuf merge_names = STRBUF_INIT;\n>> -\n>> -\t\t/* We are invoked directly as the first-class UI. */\n>> +\t/* We are invoked directly as the first-class UI. */\n> \n> Won't this break\n> \n> \tgit merge \"message here\" $(git rev-parse HEAD) foo bar\n> \n> ?  Previously this code was in an \"else\" block so it wasn't reached in\n> the is_old_style_invocation case.\n> \n>> -\tif (head_invalid || !argc)\n>> +\tif (!argc)\n>> \t\tusage_with_options(builtin_merge_usage,\n>> \t\t\tbuiltin_merge_options);\n> \n> What happens with\n> \n> \tgit merge \"message here\" HEAD foo bar\n> \n> from an unborn branch?\n> \n> Hope that helps.\n> Jonathan\n> \n> [1] http://thread.gmane.org/gmane.comp.version-control.git/170456\n"}]}