{"thread":{"id":"36735","subject":"[PATCH v1 0/3] Add --graft option to git replace","startedAt":"2014-05-22T21:33:03Z","lastAt":"2014-06-01T16:06:26Z","messageCount":15,"participants":["Christian Couder","Junio C Hamano","Jeff King","Eric Sunshine","Jakub Narębski"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"242552","messageId":"20140522211836.27162.80311.chriscool@tuxfamily.org","threadId":"36735","inReplyTo":null,"subject":"[PATCH v1 0/3] Add --graft option to git replace","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-05-22T21:33:03Z","receivedAt":"2014-05-22T21:33:03Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Here is a small patch series to implement:\n\n\tgit replace [-f] --graft <commit> [<parent>...]\n\nThe changes since the RFC/PATCH are the following:\n\n- in patch 1/3, parse_commit_buffer() is now used to\n  make sure <commit> is not corrupt\n- patch 2/3 add some tests\n- patch 3/3 add some documentation\n\nAbout the documentation, maybe we should add that --graft\ncan now be used instead of grafts in .git/info/grafts,\nand maybe we could add an example that shows how it can\nbe done.\n\nChristian Couder (3):\n  replace: add --graft option\n  replace: add test for --graft\n  Documentation: replace: add --graft option\n\n Documentation/git-replace.txt |  8 +++++\n builtin/replace.c             | 84 ++++++++++++++++++++++++++++++++++++++++++-\n t/t6050-replace.sh            | 12 +++++++\n 3 files changed, 103 insertions(+), 1 deletion(-)\n\n-- \n1.9.rc0.17.g651113e\n"},{"id":"242553","messageId":"20140522213307.27162.14455.chriscool@tuxfamily.org","threadId":"36735","inReplyTo":"20140522211836.27162.80311.chriscool@tuxfamily.org","subject":"[PATCH v1 1/3] replace: add --graft option","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-05-22T21:33:04Z","receivedAt":"2014-05-22T21:33:04Z","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 | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 83 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 1bb491d..9d1e82f 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,76 @@ 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 commit *commit;\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+\tcommit = lookup_commit_or_die(old, old_ref);\n+\tif (read_sha1_commit(old, &buf))\n+\t\tdie(_(\"Invalid commit: '%s'\"), old_ref);\n+\n+\t/* make sure the commit is not corrupt */\n+\tif (parse_commit_buffer(commit, buf.buf, buf.len))\n+\t\tdie(_(\"Could not parse 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 +374,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 +398,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 +424,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":"242554","messageId":"20140522213307.27162.82339.chriscool@tuxfamily.org","threadId":"36735","inReplyTo":"20140522211836.27162.80311.chriscool@tuxfamily.org","subject":"[PATCH v1 2/3] replace: add test for --graft","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-05-22T21:33:05Z","receivedAt":"2014-05-22T21:33:05Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n t/t6050-replace.sh | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\nindex 68b3cb2..ca45a84 100755\n--- a/t/t6050-replace.sh\n+++ b/t/t6050-replace.sh\n@@ -351,4 +351,16 @@ test_expect_success 'replace ref cleanup' '\n \ttest -z \"$(git replace)\"\n '\n \n+test_expect_success '--graft with and without already replaced object' '\n+\ttest $(git log --oneline | wc -l) = 7 &&\n+\tgit replace --graft $HASH5 &&\n+\ttest $(git log --oneline | wc -l) = 3 &&\n+\tgit cat-file -p $HASH5 | test_must_fail grep parent &&\n+\ttest_must_fail git replace --graft $HASH5 $HASH4 $HASH3 &&\n+\tgit replace --force -g $HASH5 $HASH4 $HASH3 &&\n+\tgit cat-file -p $HASH5 | grep \"parent $HASH4\" &&\n+\tgit cat-file -p $HASH5 | grep \"parent $HASH3\" &&\n+\tgit replace -d $HASH5\n+'\n+\n test_done\n-- \n1.9.rc0.17.g651113e\n"},{"id":"242555","messageId":"20140522213307.27162.3251.chriscool@tuxfamily.org","threadId":"36735","inReplyTo":"20140522211836.27162.80311.chriscool@tuxfamily.org","subject":"[PATCH v1 3/3] Documentation: replace: add --graft option","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-05-22T21:33:06Z","receivedAt":"2014-05-22T21:33:06Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n Documentation/git-replace.txt | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt\nindex 61461b9..491875e 100644\n--- a/Documentation/git-replace.txt\n+++ b/Documentation/git-replace.txt\n@@ -10,6 +10,7 @@ SYNOPSIS\n [verse]\n 'git replace' [-f] <object> <replacement>\n 'git replace' [-f] --edit <object>\n+'git replace' [-f] --graft <commit> [<parent>...]\n 'git replace' -d <object>...\n 'git replace' [--format=<format>] [-l [<pattern>]]\n \n@@ -73,6 +74,13 @@ OPTIONS\n \tnewly created object. See linkgit:git-var[1] for details about\n \thow the editor will be chosen.\n \n+--graft <commit> [<parent>...]::\n+\tCreate a graft commit. A new commit is created with the same\n+\tcontent as <commit> except that its parents will be\n+\t[<parent>...] instead of <commit>'s parents. A replacement ref\n+\tis then created to replace <commit> with the newly created\n+\tcommit.\n+\n -l <pattern>::\n --list <pattern>::\n \tList replace refs for objects that match the given pattern (or\n-- \n1.9.rc0.17.g651113e\n"},{"id":"242579","messageId":"xmqq7g5cl9d2.fsf@gitster.dls.corp.google.com","threadId":"36735","inReplyTo":"20140522211836.27162.80311.chriscool@tuxfamily.org","subject":"Re: [PATCH v1 0/3] Add --graft option to git replace","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-23T16:59:05Z","receivedAt":"2014-05-23T16:59:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n> Here is a small patch series to implement:\n>\n> \tgit replace [-f] --graft <commit> [<parent>...]\n>\n> The changes since the RFC/PATCH are the following:\n>\n> - in patch 1/3, parse_commit_buffer() is now used to\n>   make sure <commit> is not corrupt\n> - patch 2/3 add some tests\n> - patch 3/3 add some documentation\n>\n> About the documentation, maybe we should add that --graft\n> can now be used instead of grafts in .git/info/grafts,\n> and maybe we could add an example that shows how it can\n> be done.\n\nOr a procedure that reads .git/info/grafts, converts it to a set of\nreplacements and drops .git/info/grafts.  A sample script could be\nthrown in to contrib/ somewhere as \"convert-graft-to-replace.sh\".\n\n> Christian Couder (3):\n>   replace: add --graft option\n>   replace: add test for --graft\n>   Documentation: replace: add --graft option\n>\n>  Documentation/git-replace.txt |  8 +++++\n>  builtin/replace.c             | 84 ++++++++++++++++++++++++++++++++++++++++++-\n>  t/t6050-replace.sh            | 12 +++++++\n>  3 files changed, 103 insertions(+), 1 deletion(-)\n"},{"id":"293753","messageId":"537F8021.8080304@gmail.com","threadId":"36735","inReplyTo":"20140522213307.27162.3251.chriscool@tuxfamily.org","subject":"Re: [PATCH v1 3/3] Documentation: replace: add --graft option","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2014-05-23T17:06:41Z","receivedAt":"2014-05-23T17:06:41Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 2014-05-22 23:33, Christian Couder pisze:\n\n> +--graft <commit> [<parent>...]::\n> +\tCreate a graft commit. A new commit is created with the same\n> +\tcontent as <commit> except that its parents will be\n> +\t[<parent>...] instead of <commit>'s parents. A replacement ref\n> +\tis then created to replace <commit> with the newly created\n> +\tcommit.\n> +\n>   -l <pattern>::\n>   --list <pattern>::\n>   \tList replace refs for objects that match the given pattern (or\n\nHere I think you can add the graft replacing example:\n\n   cat .git/info/grafts | while read line\n   do git replace --graft $line; done\n\n-- \nJakub Narębski\n"},{"id":"242589","messageId":"xmqqioowjqqk.fsf@gitster.dls.corp.google.com","threadId":"36735","inReplyTo":"537F8021.8080304@gmail.com","subject":"Re: [PATCH v1 3/3] Documentation: replace: add --graft option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-23T18:26:43Z","receivedAt":"2014-05-23T18:26:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narębski <jnareb@gmail.com> writes:\n\n> W dniu 2014-05-22 23:33, Christian Couder pisze:\n>\n>> +--graft <commit> [<parent>...]::\n>> +\tCreate a graft commit. A new commit is created with the same\n>> +\tcontent as <commit> except that its parents will be\n>> +\t[<parent>...] instead of <commit>'s parents. A replacement ref\n>> +\tis then created to replace <commit> with the newly created\n>> +\tcommit.\n>> +\n>>   -l <pattern>::\n>>   --list <pattern>::\n>>   \tList replace refs for objects that match the given pattern (or\n>\n> Here I think you can add the graft replacing example:\n>\n>   cat .git/info/grafts | while read line\n>   do git replace --graft $line; done\n\nDo not cat a single file into a pipeline.\n\n    while read definition\n    do\n    \tgit replace --graft $definition\n    done <\"${GIT_DIR:-.git}/info/grafts\"\n\nor something.  You might also have to be careful to use \"read -r\"\nand/or avoid feeding a comment line (if info/grafts supports it) to\nthe command inside do ... done, but I didn't check what the graft\nreading code does myself ;-)\n"},{"id":"242598","messageId":"xmqqsio0i9c5.fsf@gitster.dls.corp.google.com","threadId":"36735","inReplyTo":"20140522213307.27162.14455.chriscool@tuxfamily.org","subject":"Re: [PATCH v1 1/3] replace: add --graft option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-23T19:27:54Z","receivedAt":"2014-05-23T19:27:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\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\nShucks.  An older version of this is already in 'next'.  I'll\npostpone queuing the rerolled one until post-release when we will\nrewind 'next'.\n\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\nDon't cat a single file into a pipeline.\n"},{"id":"242600","messageId":"20140523195153.GB19088@sigill.intra.peff.net","threadId":"36735","inReplyTo":"20140522213307.27162.14455.chriscool@tuxfamily.org","subject":"Re: [PATCH v1 1/3] replace: add --graft option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-23T19:51:53Z","receivedAt":"2014-05-23T19:51:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 22, 2014 at 11:33:04PM +0200, Christian Couder wrote:\n\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 think this script at the end should end up in the documentation or a\ncontrib script (probably the former, as it is short enough that somebody\nmight just cut-and-paste).\n\nThe graft file ignores comments and blank lines, so maybe \"grep '^[^#]'\"\nwould be in order.\n\nAnd maybe it's just me, but I think spacing it like:\n\n  grep '^[^#]' .git/info/grafts |\n  while read line; do\n\tgit replace --graft $line\n  done\n\nis more readable (I think some would even argue for putting the \"do\" on\na separate line).\n\n> +\t/* make sure the commit is not corrupt */\n> +\tif (parse_commit_buffer(commit, buf.buf, buf.len))\n> +\t\tdie(_(\"Could not parse commit: '%s'\"), old_ref);\n\nI guess the checks here are sufficient to make...\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...this number-based parsing safe, though it would miss removing a stray\nparent line elsewhere in the commit. It still feels rather magical to\nme, as we are depending on unspoken format guarantees defined inside\nparse_commit_buffer. I'd prefer something like the line-based parser I\nshowed in the other thread, but I suppose it may just be a matter of\npreference.\n\n-Peff\n"},{"id":"242603","messageId":"xmqqlhtsi7l7.fsf@gitster.dls.corp.google.com","threadId":"36735","inReplyTo":"20140523195153.GB19088@sigill.intra.peff.net","subject":"Re: [PATCH v1 1/3] replace: add --graft option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-23T20:05:40Z","receivedAt":"2014-05-23T20:05:40Z","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 Thu, May 22, 2014 at 11:33:04PM +0200, Christian Couder wrote:\n>\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 think this script at the end should end up in the documentation or a\n> contrib script (probably the former, as it is short enough that somebody\n> might just cut-and-paste).\n>\n> The graft file ignores comments and blank lines, so maybe \"grep '^[^#]'\"\n> would be in order.\n>\n> And maybe it's just me, but I think spacing it like:\n>\n>   grep '^[^#]' .git/info/grafts |\n>   while read line; do\n> \tgit replace --graft $line\n>   done\n>\n> is more readable (I think some would even argue for putting the \"do\" on\n> a separate line).\n\nYes, I would ;-)\n\nI just read read_graft_line(); it allows an empty line (both\nlength-0 before the terminating LF or CRLF, and a line with\nisspace() only) and ignore them, so \"grep '^[^#]'\" is not\nsufficient.\n\n>> +\t/* make sure the commit is not corrupt */\n>> +\tif (parse_commit_buffer(commit, buf.buf, buf.len))\n>> +\t\tdie(_(\"Could not parse commit: '%s'\"), old_ref);\n>\n> I guess the checks here are sufficient to make...\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> ...this number-based parsing safe, though it would miss removing a stray\n> parent line elsewhere in the commit. It still feels rather magical to\n> me, as we are depending on unspoken format guarantees defined inside\n> parse_commit_buffer.\n\nDo you mean \"we have a carved-in-stone rule that all 'parent ' lines\nmust come immediately after a single 'tree ' line, and that is\nimplemented in parse_commit_buffer().  The above code follows the\nexact same rule.  It would be nice to have some mechanism to tell\nsomebody who wants to update the former that s/he must update this\nnew code at the same time\"?\n\nI think a commit object that violates the rule is simply broken, and\nit is OK to add a mode to tell parse-commit-buffer to tolerate such\na broken object, if only so that filter-branch or some other tools\ncan be used to correct a history that contains it.\n\nPerhaps a more future-proof way to write Christian's code may be:\n\n    - find \"tree \";\n\n    - splice the new parents in immediately after that \"tree \" line;\n\n    - starting from the end of these new parents, scan up to the end\n      of the header line-by-line, and splice out any line that\n      begins with \"parent \".\n\nwhich may not be too bad.\n"},{"id":"242608","messageId":"20140523202853.GH19088@sigill.intra.peff.net","threadId":"36735","inReplyTo":"xmqqlhtsi7l7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v1 1/3] replace: add --graft option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-23T20:28:53Z","receivedAt":"2014-05-23T20:28:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 23, 2014 at 01:05:40PM -0700, Junio C Hamano wrote:\n\n> I just read read_graft_line(); it allows an empty line (both\n> length-0 before the terminating LF or CRLF, and a line with\n> isspace() only) and ignore them, so \"grep '^[^#]'\" is not\n> sufficient.\n\nThanks, I missed the space trimming. I think:\n\n  grep '^[^# \\t]'\n\nwould be enough, though I am not sure that \"\\t\" is portable over a raw\ntab.\n\n> > ...this number-based parsing safe, though it would miss removing a stray\n> > parent line elsewhere in the commit. It still feels rather magical to\n> > me, as we are depending on unspoken format guarantees defined inside\n> > parse_commit_buffer.\n> \n> Do you mean \"we have a carved-in-stone rule that all 'parent ' lines\n> must come immediately after a single 'tree ' line, and that is\n> implemented in parse_commit_buffer().  The above code follows the\n> exact same rule.  It would be nice to have some mechanism to tell\n> somebody who wants to update the former that s/he must update this\n> new code at the same time\"?\n\nYes, exactly.\n\n> Perhaps a more future-proof way to write Christian's code may be:\n> \n>     - find \"tree \";\n> \n>     - splice the new parents in immediately after that \"tree \" line;\n> \n>     - starting from the end of these new parents, scan up to the end\n>       of the header line-by-line, and splice out any line that\n>       begins with \"parent \".\n> \n> which may not be too bad.\n\nSounds familiar:\n\n  http://article.gmane.org/gmane.comp.version-control.git/249575\n\n-Peff\n"},{"id":"242613","messageId":"xmqqd2f4i41l.fsf@gitster.dls.corp.google.com","threadId":"36735","inReplyTo":"20140523202853.GH19088@sigill.intra.peff.net","subject":"Re: [PATCH v1 1/3] replace: add --graft option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-23T21:22:14Z","receivedAt":"2014-05-23T21:22:14Z","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 Fri, May 23, 2014 at 01:05:40PM -0700, Junio C Hamano wrote:\n>\n>> I just read read_graft_line(); it allows an empty line (both\n>> length-0 before the terminating LF or CRLF, and a line with\n>> isspace() only) and ignore them, so \"grep '^[^#]'\" is not\n>> sufficient.\n>\n> Thanks, I missed the space trimming. I think:\n>\n>   grep '^[^# \\t]'\n>\n> would be enough, though I am not sure that \"\\t\" is portable over a raw\n> tab.\n\nPerhaps.  That would filter out an empty line as well, which would\nbe good.\n\n>> Perhaps a more future-proof way to write Christian's code may be:\n>> ...\n>> which may not be too bad.\n>\n> Sounds familiar:\n>\n>   http://article.gmane.org/gmane.comp.version-control.git/249575\n\nYup, instead of having to memmove() repeatedly, copying into it\nwhile skipping 'parent ' lines may be better.\n"},{"id":"242625","messageId":"CAPig+cRG_LsV3q1qRP7C6LpFA7hcBCDmX8Xm=6AdxtRmY3oYWQ@mail.gmail.com","threadId":"36735","inReplyTo":"xmqqd2f4i41l.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v1 1/3] replace: add --graft option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-05-23T22:59:14Z","receivedAt":"2014-05-23T22:59:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 23, 2014 at 5:22 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jeff King <peff@peff.net> writes:\n>\n>> On Fri, May 23, 2014 at 01:05:40PM -0700, Junio C Hamano wrote:\n>>\n>>> I just read read_graft_line(); it allows an empty line (both\n>>> length-0 before the terminating LF or CRLF, and a line with\n>>> isspace() only) and ignore them, so \"grep '^[^#]'\" is not\n>>> sufficient.\n>>\n>> Thanks, I missed the space trimming. I think:\n>>\n>>   grep '^[^# \\t]'\n>>\n>> would be enough, though I am not sure that \"\\t\" is portable over a raw\n>> tab.\n\n'grep' does not recognize \"\\t\" on BSD or derivatives, such as Mac OS\nX. (Nor does 'sed'.)\n"},{"id":"242766","messageId":"20140527.210520.2165451218485597326.chriscool@tuxfamily.org","threadId":"36735","inReplyTo":"xmqq7g5cl9d2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v1 0/3] Add --graft option to git replace","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-05-27T19:05:20Z","receivedAt":"2014-05-27T19:05:20Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Junio C Hamano <gitster@pobox.com>\nSubject: Re: [PATCH v1 0/3] Add --graft option to git replace\nDate: Fri, 23 May 2014 09:59:05 -0700\n\n> Christian Couder <chriscool@tuxfamily.org> writes:\n> \n>> Here is a small patch series to implement:\n>>\n>> \tgit replace [-f] --graft <commit> [<parent>...]\n>>\n>> The changes since the RFC/PATCH are the following:\n>>\n>> - in patch 1/3, parse_commit_buffer() is now used to\n>>   make sure <commit> is not corrupt\n>> - patch 2/3 add some tests\n>> - patch 3/3 add some documentation\n>>\n>> About the documentation, maybe we should add that --graft\n>> can now be used instead of grafts in .git/info/grafts,\n>> and maybe we could add an example that shows how it can\n>> be done.\n> \n> Or a procedure that reads .git/info/grafts, converts it to a set of\n> replacements and drops .git/info/grafts.  A sample script could be\n> thrown in to contrib/ somewhere as \"convert-graft-to-replace.sh\".\n\nOk, I just sent a patch that adds such a sample script.\n\nThanks,\nChristian.\n"},{"id":"243098","messageId":"20140601.180626.1114637687965252195.chriscool@tuxfamily.org","threadId":"36735","inReplyTo":"20140523195153.GB19088@sigill.intra.peff.net","subject":"Re: [PATCH v1 1/3] replace: add --graft option","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-06-01T16:06:26Z","receivedAt":"2014-06-01T16:06:26Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Jeff King <peff@peff.net>\n\n> On Thu, May 22, 2014 at 11:33:04PM +0200, Christian Couder wrote:\n> \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 think this script at the end should end up in the documentation or a\n> contrib script (probably the former, as it is short enough that somebody\n> might just cut-and-paste).\n> \n> The graft file ignores comments and blank lines, so maybe \"grep '^[^#]'\"\n> would be in order.\n> \n> And maybe it's just me, but I think spacing it like:\n> \n>   grep '^[^#]' .git/info/grafts |\n>   while read line; do\n> \tgit replace --graft $line\n>   done\n> \n> is more readable (I think some would even argue for putting the \"do\" on\n> a separate line).\n\nThanks, I used something like that in the contrib script.\n \n>> +\t/* make sure the commit is not corrupt */\n>> +\tif (parse_commit_buffer(commit, buf.buf, buf.len))\n>> +\t\tdie(_(\"Could not parse commit: '%s'\"), old_ref);\n> \n> I guess the checks here are sufficient to make...\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> ...this number-based parsing safe, though it would miss removing a stray\n> parent line elsewhere in the commit.\n\nYeah, but I don't think that it is a problem.\n\nThose parent lines are not standard in the first place, because they\nare not parsed by parse_commit_buffer(). And I don't think this option\nshould mess with non standard stuff.\n\n> It still feels rather magical to\n> me, as we are depending on unspoken format guarantees defined inside\n> parse_commit_buffer. \n\nMy opinion is that we are depending on the standard way to parse\nheaders, and that's good. I think it would be \"black magic\" to mess\nwith non standard stuff.\n\n> I'd prefer something like the line-based parser I\n> showed in the other thread, but I suppose it may just be a matter of\n> preference.\n\nYeah probably.\n\nThanks,\nChristian.\n"}]}