{"thread":{"id":"36687","subject":"[RFC/PATCH] replace: add --graft option","startedAt":"2014-05-18T18:29:38Z","lastAt":"2014-05-23T06:39:54Z","messageCount":8,"participants":["Christian Couder","Michael Haggerty","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"242085","messageId":"20140518182939.5260.91202.chriscool@tuxfamily.org","threadId":"36687","inReplyTo":null,"subject":"[RFC/PATCH] replace: add --graft option","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-05-18T18:29:38Z","receivedAt":"2014-05-18T18:29:38Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"The usage string for this option is:\n\ngit replace [-f] --graft <commit> [<parent>...]\n\nFirst we create a new commit that is the same as <commit>\nexcept that its parents are [<parents>...]\n\nThen we create a replace ref that replace <commit> with\nthe commit we just created.\n\nWith this new option, it should be straightforward to\nconvert grafts to replace refs, with something like:\n\ncat .git/info/grafts | while read line\ndo git replace --graft $line; done\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n builtin/replace.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 78 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 1bb491d..4b3705d 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -17,6 +17,7 @@\n static const char * const git_replace_usage[] = {\n \tN_(\"git replace [-f] <object> <replacement>\"),\n \tN_(\"git replace [-f] --edit <object>\"),\n+\tN_(\"git replace [-f] --graft <commit> [<parent>...]\"),\n \tN_(\"git replace -d <object>...\"),\n \tN_(\"git replace [--format=<format>] [-l [<pattern>]]\"),\n \tNULL\n@@ -294,6 +295,71 @@ static int edit_and_replace(const char *object_ref, int force)\n \treturn replace_object_sha1(object_ref, old, \"replacement\", new, force);\n }\n \n+static int read_sha1_commit(const unsigned char *sha1, struct strbuf *dst)\n+{\n+\tvoid *buf;\n+\tenum object_type type;\n+\tunsigned long size;\n+\n+\tbuf = read_sha1_file(sha1, &type, &size);\n+\tif (!buf)\n+\t\treturn error(_(\"cannot read object %s\"), sha1_to_hex(sha1));\n+\tif (type != OBJ_COMMIT) {\n+\t\tfree(buf);\n+\t\treturn error(_(\"object %s is not a commit\"), sha1_to_hex(sha1));\n+\t}\n+\tstrbuf_attach(dst, buf, size, size + 1);\n+\treturn 0;\n+}\n+\n+static int create_graft(int argc, const char **argv, int force)\n+{\n+\tunsigned char old[20], new[20];\n+\tconst char *old_ref = argv[0];\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf new_parents = STRBUF_INIT;\n+\tconst char *parent_start, *parent_end;\n+\tint i;\n+\n+\tif (get_sha1(old_ref, old) < 0)\n+\t\tdie(_(\"Not a valid object name: '%s'\"), old_ref);\n+\tlookup_commit_or_die(old, old_ref);\n+\tif (read_sha1_commit(old, &buf))\n+\t\tdie(_(\"Invalid commit: '%s'\"), old_ref);\n+\n+\t/* find existing parents */\n+\tparent_start = buf.buf;\n+\tparent_start += 46; /* \"tree \" + \"hex sha1\" + \"\\n\" */\n+\tparent_end = parent_start;\n+\n+\twhile (starts_with(parent_end, \"parent \"))\n+\t\tparent_end += 48; /* \"parent \" + \"hex sha1\" + \"\\n\" */\n+\n+\t/* prepare new parents */\n+\tfor (i = 1; i < argc; i++) {\n+\t\tunsigned char sha1[20];\n+\t\tif (get_sha1(argv[i], sha1) < 0)\n+\t\t\tdie(_(\"Not a valid object name: '%s'\"), argv[i]);\n+\t\tlookup_commit_or_die(sha1, argv[i]);\n+\t\tstrbuf_addf(&new_parents, \"parent %s\\n\", sha1_to_hex(sha1));\n+\t}\n+\n+\t/* replace existing parents with new ones */\n+\tstrbuf_splice(&buf, parent_start - buf.buf, parent_end - parent_start,\n+\t\t      new_parents.buf, new_parents.len);\n+\n+\tif (write_sha1_file(buf.buf, buf.len, commit_type, new))\n+\t\tdie(_(\"could not write replacement commit for: '%s'\"), old_ref);\n+\n+\tstrbuf_release(&new_parents);\n+\tstrbuf_release(&buf);\n+\n+\tif (!hashcmp(old, new))\n+\t\treturn error(\"new commit is the same as the old one: '%s'\", sha1_to_hex(old));\n+\n+\treturn replace_object_sha1(old_ref, old, \"replacement\", new, force);\n+}\n+\n int cmd_replace(int argc, const char **argv, const char *prefix)\n {\n \tint force = 0;\n@@ -303,12 +369,14 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n \t\tMODE_LIST,\n \t\tMODE_DELETE,\n \t\tMODE_EDIT,\n+\t\tMODE_GRAFT,\n \t\tMODE_REPLACE\n \t} cmdmode = MODE_UNSPECIFIED;\n \tstruct option options[] = {\n \t\tOPT_CMDMODE('l', \"list\", &cmdmode, N_(\"list replace refs\"), MODE_LIST),\n \t\tOPT_CMDMODE('d', \"delete\", &cmdmode, N_(\"delete replace refs\"), MODE_DELETE),\n \t\tOPT_CMDMODE('e', \"edit\", &cmdmode, N_(\"edit existing object\"), MODE_EDIT),\n+\t\tOPT_CMDMODE('g', \"graft\", &cmdmode, N_(\"change a commit's parents\"), MODE_GRAFT),\n \t\tOPT_BOOL('f', \"force\", &force, N_(\"replace the ref if it exists\")),\n \t\tOPT_STRING(0, \"format\", &format, N_(\"format\"), N_(\"use this format\")),\n \t\tOPT_END()\n@@ -325,7 +393,10 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n \t\tusage_msg_opt(\"--format cannot be used when not listing\",\n \t\t\t      git_replace_usage, options);\n \n-\tif (force && cmdmode != MODE_REPLACE && cmdmode != MODE_EDIT)\n+\tif (force &&\n+\t    cmdmode != MODE_REPLACE &&\n+\t    cmdmode != MODE_EDIT &&\n+\t    cmdmode != MODE_GRAFT)\n \t\tusage_msg_opt(\"-f only makes sense when writing a replacement\",\n \t\t\t      git_replace_usage, options);\n \n@@ -348,6 +419,12 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n \t\t\t\t      git_replace_usage, options);\n \t\treturn edit_and_replace(argv[0], force);\n \n+\tcase MODE_GRAFT:\n+\t\tif (argc < 1)\n+\t\t\tusage_msg_opt(\"-g needs at least one argument\",\n+\t\t\t\t      git_replace_usage, options);\n+\t\treturn create_graft(argc, argv, force);\n+\n \tcase MODE_LIST:\n \t\tif (argc > 1)\n \t\t\tusage_msg_opt(\"only one pattern can be given with -l\",\n-- \n1.9.rc0.17.g651113e\n"},{"id":"242150","messageId":"5379D1EF.8040203@alum.mit.edu","threadId":"36687","inReplyTo":"20140518182939.5260.91202.chriscool@tuxfamily.org","subject":"Re: [RFC/PATCH] replace: add --graft option","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-05-19T09:42:07Z","receivedAt":"2014-05-19T09:42:07Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/18/2014 08:29 PM, Christian Couder wrote:\n> The usage string for this option is:\n> \n> git replace [-f] --graft <commit> [<parent>...]\n> \n> First we create a new commit that is the same as <commit>\n> except that its parents are [<parents>...]\n> \n> Then we create a replace ref that replace <commit> with\n> the commit we just created.\n> \n> With this new option, it should be straightforward to\n> convert grafts to replace refs, with something like:\n> \n> cat .git/info/grafts | while read line\n> do git replace --graft $line; done\n\nI love the functionality; I think it's a great step towards making\ngrafts obsolete.\n\nI haven't worked with Git's object reading/writing code much, but it\nsurprised me that you are editing the commit object basically as a\nstring, using hard-coded length constants and stuff.  It seems\nerror-prone, and we already have a commit parser.\n\nWould it be possible to program this at a higher layer of abstraction\nbased on the commit object produced by the existing commit parser?\nE.g., edit the object it produces, and write the result?  Or create a\nnew commit object out of the parsed commit object and write that?\n\nIt's great that you're working on this!\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"242156","messageId":"20140519111940.GB17492@sigill.intra.peff.net","threadId":"36687","inReplyTo":"5379D1EF.8040203@alum.mit.edu","subject":"Re: [RFC/PATCH] replace: add --graft option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-19T11:19:40Z","receivedAt":"2014-05-19T11:19:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 19, 2014 at 11:42:07AM +0200, Michael Haggerty wrote:\n\n> On 05/18/2014 08:29 PM, Christian Couder wrote:\n> > The usage string for this option is:\n> > \n> > git replace [-f] --graft <commit> [<parent>...]\n> > \n> > First we create a new commit that is the same as <commit>\n> > except that its parents are [<parents>...]\n> > \n> > Then we create a replace ref that replace <commit> with\n> > the commit we just created.\n> > \n> > With this new option, it should be straightforward to\n> > convert grafts to replace refs, with something like:\n> > \n> > cat .git/info/grafts | while read line\n> > do git replace --graft $line; done\n> \n> I love the functionality; I think it's a great step towards making\n> grafts obsolete.\n\nMe too.\n\n> I haven't worked with Git's object reading/writing code much, but it\n> surprised me that you are editing the commit object basically as a\n> string, using hard-coded length constants and stuff.  It seems\n> error-prone, and we already have a commit parser.\n\nI think we have at least two commit parsers already. :)\n\nThe one in commit.c:parse_commit_buffer tries hard to be fast and only\nget out enough information for a traversal (so parents, commit\ntimestamp, and tree pointer).\n\nThe ones in pretty.c that back format_commit_message\n(parse_commit_header, parse_commit_message) are a bit more flexible, as\nthey record the offsets and lengths of certain key lines. But \"parents\"\nis not one of those lines (we rely on the already-parsed copy in the\n\"struct commit\" for parents and tree information).\n\nIt might make sense to just teach parse_commit_header to be a little\nmore thorough; it has to read past those lines anyway to find the author\nand committer lines, so it would not be much more expensive to note\nthem.  And then of course the code needs to be pulled out of the\npretty-printer and made generally accessible.\n\nWhile I do like the thought of having a decent reusable commit parser,\nthis particular case is really about trying to tweak one header, without\ntouching anything else. IOW, it is basically:\n\n  sed '/^parent /d; /^tree /a\\\n  parent ...\\\n  parent ...\n  '\n\nThat's more or less what Christian's function is doing, though it\nassumes things like that the parents come immediately after the tree,\nand that they are all in a group. Those are all true for objects created\nby git, but I think we can be a little flexible.\n\nIt turns out I wrote a proof-of-concept of this in March when we had the\noriginal discussion, but forgot to ever send it to the list. My parsing\nfunction looked like:\n\n\t/*\n\t * Rewrite the commit object found in \"buf\", removing any existing\n\t * parents and adding lines for any parents in the NULL-terminated\n\t * array \"parents\" (whose strings should be 40-char hex sha1s).\n\t *\n\t * The output is written to the strbuf \"out\".\n\t */\n\tstatic void rewrite_parents(struct strbuf *out, const char *buf, const char **parents)\n\t{\n\t\tint added = 0;\n\t\n\t\twhile (buf && *buf) {\n\t\t\tconst char *eol = strchrnul(buf, '\\n');\n\t\t\tconst char *next = *eol ? eol + 1 : eol;\n\t\n\t\t\tif (eol == buf + 1)\n\t\t\t\tbreak;\n\t\n\t\t\tif (!starts_with(buf, \"parent \"))\n\t\t\t\tstrbuf_add(out, buf, next - buf);\n\t\n\t\t\t/*\n\t\t\t * We always put our parent lines right after the tree line,\n\t\t\t * which matches how git creates commit objects.\n\t\t\t */\n\t\t\tif (starts_with(buf, \"tree \")) {\n\t\t\t\tconst char **p;\n\t\n\t\t\t\tif (added++)\n\t\t\t\t\tdie(\"commit has two tree lines?\");\n\t\t\t\tfor (p = parents; *p; p++)\n\t\t\t\t\tstrbuf_addf(out, \"parent %s\\n\", *p);\n\t\t\t}\n\t\n\t\t\tbuf = next;\n\t\t}\n\t\n\t\tif (!added)\n\t\t\tdie(\"commit has no tree line?\");\n\t\n\t\tstrbuf_addstr(out, buf);\n\t}\n\nRather than a splice, I did it as I copied into the strbuf, and I\ntreated each line independently (dropping parent lines, no matter where,\nand then finding the tree line as the \"proper\" place to add new parent\nlines).\n\n-Peff\n"},{"id":"242157","messageId":"20140519112151.GC17492@sigill.intra.peff.net","threadId":"36687","inReplyTo":"20140518182939.5260.91202.chriscool@tuxfamily.org","subject":"Re: [RFC/PATCH] replace: add --graft option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-19T11:21:51Z","receivedAt":"2014-05-19T11:21:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 18, 2014 at 08:29:38PM +0200, Christian Couder wrote:\n\n> +static int create_graft(int argc, const char **argv, int force)\n> +{\n> +\tunsigned char old[20], new[20];\n> +\tconst char *old_ref = argv[0];\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tstruct strbuf new_parents = STRBUF_INIT;\n> +\tconst char *parent_start, *parent_end;\n> +\tint i;\n> +\n> +\tif (get_sha1(old_ref, old) < 0)\n> +\t\tdie(_(\"Not a valid object name: '%s'\"), old_ref);\n> +\tlookup_commit_or_die(old, old_ref);\n> +\tif (read_sha1_commit(old, &buf))\n> +\t\tdie(_(\"Invalid commit: '%s'\"), old_ref);\n\nDo we want to peel to commits here? That is, should:\n\n  git replace --graft v1.5.0 v1.4.0\n\nwork? On the one hand, I see it as friendly. On the other, it may be a\nbit surprising when working with something as potentially dangerous a\nreplace refs. If we don't do it automatically, the user can still say\n\"v1.5.0^{commit}\" to be explicit. I dunno; maybe I am being overly\nparanoid.\n\n> +\t/* prepare new parents */\n> +\tfor (i = 1; i < argc; i++) {\n> +\t\tunsigned char sha1[20];\n> +\t\tif (get_sha1(argv[i], sha1) < 0)\n> +\t\t\tdie(_(\"Not a valid object name: '%s'\"), argv[i]);\n> +\t\tlookup_commit_or_die(sha1, argv[i]);\n> +\t\tstrbuf_addf(&new_parents, \"parent %s\\n\", sha1_to_hex(sha1));\n> +\t}\n\nEither way, I think _this_ peeling is a sane thing to do.\n\n-Peff\n"},{"id":"242187","messageId":"xmqqvbt11y15.fsf@gitster.dls.corp.google.com","threadId":"36687","inReplyTo":"20140519111940.GB17492@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] replace: add --graft option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-19T17:25:10Z","receivedAt":"2014-05-19T17:25:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> It might make sense to just teach parse_commit_header to be a little\n> more thorough; it has to read past those lines anyway to find the author\n> and committer lines, so it would not be much more expensive to note\n> them.  And then of course the code needs to be pulled out of the\n> pretty-printer and made generally accessible.\n\nI notice that you said \"might\" above.\n\n> That's more or less what Christian's function is doing, though it\n> assumes things like that the parents come immediately after the tree,\n> and that they are all in a group. Those are all true for objects created\n> by git, but I think we can be a little flexible.\n\nThe headers up to committer are cast in stone in their ordering, and\nI do not immediately see how loosening it would be beneficial.\n\nUnless you are trying to give users a new way to record exactly the\nsame commit in twenty-four (or more) ways with their own object\nnames, that is ;-)\n"},{"id":"242188","messageId":"20140519173505.GA28673@sigill.intra.peff.net","threadId":"36687","inReplyTo":"xmqqvbt11y15.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH] replace: add --graft option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-19T17:35:05Z","receivedAt":"2014-05-19T17:35:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 19, 2014 at 10:25:10AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > It might make sense to just teach parse_commit_header to be a little\n> > more thorough; it has to read past those lines anyway to find the author\n> > and committer lines, so it would not be much more expensive to note\n> > them.  And then of course the code needs to be pulled out of the\n> > pretty-printer and made generally accessible.\n> \n> I notice that you said \"might\" above.\n\nYeah. I think having a reusable parser is definitely a good thing. I'm\nnot sure if it's worth the massive amount of refactoring that would be\nrequired for this particular use case.\n\n> > That's more or less what Christian's function is doing, though it\n> > assumes things like that the parents come immediately after the tree,\n> > and that they are all in a group. Those are all true for objects created\n> > by git, but I think we can be a little flexible.\n> \n> The headers up to committer are cast in stone in their ordering, and\n> I do not immediately see how loosening it would be beneficial.\n> \n> Unless you are trying to give users a new way to record exactly the\n> same commit in twenty-four (or more) ways with their own object\n> names, that is ;-)\n\nSorry, I didn't mean to imply that people can do what they want with\nthat ordering. Implementations that reorder the headers are stupid and\nwrong, and should be fixed.\n\nBUT. I really don't like making these assumptions or doing ad-hoc\nparsing all through the code. We _do_ see quirky, wrong objects, and\nwant to handle them sanely and consistently. That's hard to do when\nthere are parsers sprinkled throughout the code which handle each case\nslightly differently. I don't recall seeing this with header ordering,\nbut I know that broken ident lines have caused us headaches in the past,\nand I'm happy that we've (mostly) settled on the code in\nsplit_ident_line to handle this.\n\nThings like:\n\n  +       /* find existing parents */\n  +       parent_start = buf.buf;\n  +       parent_start += 46; /* \"tree \" + \"hex sha1\" + \"\\n\" */\n  +       parent_end = parent_start;\n\nscare me. Is buf actually 46 bytes long? If not, we read past the end of\nthe buffer. What if it contains something besides \"tree <sha1>\\n\" at the\nbeginning? We don't even notice!\n\nThe version I posted elsewhere in the thread at least operates on a\nline-by-line basis, and tries to verify its assumptions (and barfs if\nnot). It's still doing its own parsing, but at least it's keeping its\nassumptions on the object format to a minimum (\"drop old parent lines\",\n\"add new ones after tree, and barf if there's not exactly one tree\nline\").\n\n-Peff\n"},{"id":"242191","messageId":"xmqqoayt1usx.fsf@gitster.dls.corp.google.com","threadId":"36687","inReplyTo":"20140519173505.GA28673@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] replace: add --graft option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-19T18:34:54Z","receivedAt":"2014-05-19T18:34:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, May 19, 2014 at 10:25:10AM -0700, Junio C Hamano wrote:\n>\n>> The headers up to committer are cast in stone in their ordering, and\n>> I do not immediately see how loosening it would be beneficial.\n>> \n>> Unless you are trying to give users a new way to record exactly the\n>> same commit in twenty-four (or more) ways with their own object\n>> names, that is ;-)\n>\n> Sorry, I didn't mean to imply that people can do what they want with\n> that ordering. Implementations that reorder the headers are stupid and\n> wrong, and should be fixed.\n\nYeah, that was the only thing I meant to say, and what you said in\nthe rest of message makes sense to me.  I very much like the\napproach to parse line-by-line, noticing products from stupid and\nwrong implementations and warning or erroring out against them,\nwhile allowing an option to be lenient to help users who want to fix\ntheir repositories contaminated with such objects.\n"},{"id":"242557","messageId":"20140523.083954.56876133762506067.chriscool@tuxfamily.org","threadId":"36687","inReplyTo":"5379D1EF.8040203@alum.mit.edu","subject":"Re: [RFC/PATCH] replace: add --graft option","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-05-23T06:39:54Z","receivedAt":"2014-05-23T06:39:54Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Michael Haggerty <mhagger@alum.mit.edu>\n\n> On 05/18/2014 08:29 PM, Christian Couder wrote:\n>> The usage string for this option is:\n>> \n>> git replace [-f] --graft <commit> [<parent>...]\n>> \n>> First we create a new commit that is the same as <commit>\n>> except that its parents are [<parents>...]\n>> \n>> Then we create a replace ref that replace <commit> with\n>> the commit we just created.\n>> \n>> With this new option, it should be straightforward to\n>> convert grafts to replace refs, with something like:\n>> \n>> cat .git/info/grafts | while read line\n>> do git replace --graft $line; done\n> \n> I love the functionality; I think it's a great step towards making\n> grafts obsolete.\n> \n> I haven't worked with Git's object reading/writing code much, but it\n> surprised me that you are editing the commit object basically as a\n> string, using hard-coded length constants and stuff.  It seems\n> error-prone, and we already have a commit parser.\n> \n> Would it be possible to program this at a higher layer of abstraction\n> based on the commit object produced by the existing commit parser?\n> E.g., edit the object it produces, and write the result?  Or create a\n> new commit object out of the parsed commit object and write that?\n\nI tried to program this at a higher layer of abstraction first, but it\nwas not easy to properly write the new commit.\n\n> It's great that you're working on this!\n\nThanks,\nChristian.\n"}]}