{"thread":{"id":"26938","subject":"Re: [PATCH] Merging multiple branches in invalid HEAD state","startedAt":"2011-03-31T06:11:10Z","lastAt":"2011-03-31T06:11:10Z","messageCount":1,"participants":["Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"164759","messageId":"20110331061110.GA16353@elie","threadId":"26938","inReplyTo":"AANLkTik3ip7Ukv56oyEsYX7isTFBGcK3mN367evnvW6A@mail.gmail.com","subject":"Re: [PATCH] Merging multiple branches in invalid HEAD state","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-31T06:11:10Z","receivedAt":"2011-03-31T06:11:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(+cc: git@vger)\nHi Tim,\n\nTimothy Chen wrote:\n\n> I believe my patch never get delievered to the mailing list.\n>\n> Can you review my patch and maybe forward it for me?\n\nYep, it seems this never hit the mailing list.  What mail client do\nyou use?  Documentation/SubmittingPatches has some advice under\n\"Gmail\".\n\nQuick review follows.\n\n> Hi all, I'm interested in Gsoc and this is my first patch to Git.\n>\n> Please let me know if there is any format/things to change.\n\nBy the way, I'd encourage you to submit rough proposals for projects\nyou're interested in at google-melange.com asap and to send a copy to\nthis list (cc-ing potential mentors) as soon as you're ready to\ndiscuss and refine them.\n\n> ---------------------------------------------------------------------------------------\n> \n> From a72e72f58c073eccd987159f72d969d6e748aff0 Mon Sep 17 00:00:00 2001\n> From: Timothy Chen <tnachen@gmail.com>\n> Date: Mon, 28 Mar 2011 23:58:32 -0700\n> Subject: [PATCH] Allow multiple merges to invalid head\n\nOkay, let's get the message format out of the way first. :)  These\nlines should not be part of the message body but are meant for your\nmailer --- in other words, the usual patch format is\n\n\tFrom: Patch Author <a.u.thor@example.com>\n\tDate: Mon, 28 Mar 2011 23:58:32 -0700\n\tSubject: [PATCH] subsystem: quick summary of impact of change\n\n\tExplanation of the behavior the patch will change, why it\n\tneeds changing, and how the patch changes it.\n\n\tSign-off.\n\t---\n\tHello,\n\n\tAdditional comments that are not suitable for the changelog.\n\n\tDiffstat and actual patch.\n\nwhere the From, Date, and Subject lines above represent lines from the\nrfc822 email header.  So the actual body of the message begins with\n\"Explanation of...\" [2].\n\nNot so important now, but doing that makes the messages a little\neasier to read, and more importantly, allows the patch can be\nautomatically applied with \"git am\" without editing once it is ready.\n\n> Subject: [PATCH] Allow multiple merges to invalid head\n>\n> Currently git merge will only accept one branch when the current branch's\n> HEAD is invalid (or pointing to null), which it will when it is a newly\n> created repo.\n\nAt first, this sounded to me like it was about when HEAD is corrupt or\nhas been explicitly set to the null sha1.  Maybe a phrase like \"HEAD\ndoes not point to a valid commit yet\" or \"HEAD points to a branch yet\nto be born\" would be clearer?\n\n[...]\n> This patch will allow multiple branches to be passed in, and first updates\n> current HEAD to the first branch's head then subsequently merge the rest of\n> the branches.\n\nThanks; sounds intuitive and useful.\n\n> ---\n>  builtin/merge.c |   60 +++++++++++++++++++++++++++++++-----------------------\n>  1 files changed, 34 insertions(+), 26 deletions(-)\n> \n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index aa3453c..6956d5e 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -1052,9 +1052,7 @@ int cmd_merge(int argc, const char **argv, const char\n> *prefix)\n\nIt looks like your mailer corrupts the patch a bit (by wrapping lines\nand compressing whitespace).  SubmittingPatches can help here; until\nthe perfect setup is ready, attaching patches might be a good\nworkaround to get patches through unscathed.\n\nI've just made up the whitespace below to get by, so it might be wrong.\n\n>  \t * to forbid \"git merge\" into a branch yet to be born.\n>  \t * We do the same for \"git pull\".\n>  \t */\n> -\tif (argc != 1)\n> -\t\tdie(\"Can merge only exactly one commit into \"\n> -\t\t\t\"empty head\");\n> +\n\nDropping the restriction.  Yippee!\n\n>  \tif (squash)\n>  \t\tdie(\"Squash commit into empty head not supported yet\");\n>  \tif (!allow_fast_forward)\n> @@ -1065,34 +1063,44 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\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> +\t\tmemcpy(head, remote_head->sha1, 20);\n>  \t\tread_empty(remote_head->sha1, 0);\n\nAlso can be written as\n\n\t\thashcpy(head, remote_head->sha1);\n\nAt this point, we have advanced the current branch to the first commit\nnamed on the command line, the \"branch\" var has the name of the\nbranch, and \"head\" contains the commit name.\n\n> -\t\treturn 0;\n> +\t\tif (argc < 2) {\n> +\t\t\treturn 0;\n> +\t\t} else {\n> +\t\t\thead_arg = argv[0];\n> +\t\t\targc--;\n> +\t\t\targv++;\n> +       \t}\n> +\t}\n\nThe \"else\" is not needed, since the easy case returned already.  So:\n\n\t\tif (argc < 2)\n\t\t\treturn 0;\n\n\t\t/*\n\t\t * \"git merge foo bar baz\" from an unborn branch amounts to\n\t\t *\n\t\t * 1. git reset --keep foo\n\t\t * 2. git merge bar baz\n\t\t *\n\t\t * We just did step 1; now on to step 2...\n\t\t */\n\t\thead_arg = argv[0];\n\t\thashcpy(head, remote_head->sha1);\n\t\targc--;\n\t\targv++;\n\t}\n\nWhat happens if argc == 0?  That case was checked higher up in the\nfunction; phew.\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\thead_arg = \"HEAD\";\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\nMakes sense.  (I guess if I ran the world, head_arg would be\ninitialized to begin with to NULL, and this would say\n\n\tif (!head_arg)\n\t\thead_arg = \"HEAD\";\n\nso the reader of this part would not have to worry about what\nhead_invalid means and why the first-class UI is guarding against\nit.  But that is really a tiny nit.)\n\n> -\n> -\t\t/*\n> -\t\t * All the rest are the commits being merged;\n> +\t/*\n> +\t * All the rest are the commits being merged;\n[etc]\n\nThese are just whitespace spaces; makes sense.\n\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 was the head_invalid case originally guarding against?  Is it\nprotected against now?\n\nIt would also be nice to add some tests so we don't lose this\nfunctionality later; but aside from that and the small points\nnoted above, the patch looks good to me.  Thanks for writing it.\n\nHope that helps,\nJonathan\n\n[1] http://bugs.debian.org/432558\n[2] There is also an alternative \"am --scissors\" format the might be\nuseful for sending a patch and maintaining the flow of a previous\nconversation:\n\n\tSomeone wrote:\n\n\t> blah blah blah\n\n\tYes, that makes sense.  How about this patch?\n\t-- 8< --\n\tSubject: subsystem: quick summary of the impact of the change\n\n\tExplanation ...\n\n\tSign-off.\n\t---\n\tDiffstat and actual patch.\n\nIn both cases, you can add a \"From:\" and \"Date:\" line before the patch\nexplanation to override the values from the email header, in case you\nare forwarding a patch from someone else.  \"git am --help\" gives more\ndetails.\n"}]}