{"thread":{"id":"36841","subject":"[PATCH v3 2/4] replace: add test for --graft","startedAt":"2014-06-04T19:43:48Z","lastAt":"2014-06-30T10:52:29Z","messageCount":19,"participants":["Christian Couder","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":4},"messages":[{"id":"243367","messageId":"20140604194118.23903.3019.chriscool@tuxfamily.org","threadId":"36841","inReplyTo":null,"subject":"[PATCH v3 0/4] Add --graft option to git replace","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-06-04T19:43:48Z","receivedAt":"2014-06-04T19:43:48Z","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        git replace [-f] --graft <commit> [<parent>...]\n\nThis patch series goes on top of the patch series that\nimplements --edit.\n\nThere is only one change since v2 thanks to Eric:\n\n- improve error messages in convert-grafts-to-replace-refs.sh\n  (patch 4/4)\n\nChristian Couder (4):\n  replace: add --graft option\n  replace: add test for --graft\n  Documentation: replace: add --graft option\n  contrib: add convert-grafts-to-replace-refs.sh\n\n Documentation/git-replace.txt             | 10 ++++\n builtin/replace.c                         | 84 ++++++++++++++++++++++++++++++-\n contrib/convert-grafts-to-replace-refs.sh | 29 +++++++++++\n t/t6050-replace.sh                        | 12 +++++\n 4 files changed, 134 insertions(+), 1 deletion(-)\n create mode 100755 contrib/convert-grafts-to-replace-refs.sh\n\n-- \n2.0.0.rc0.40.gd30ccc4\n"},{"id":"243366","messageId":"20140604194353.23903.89933.chriscool@tuxfamily.org","threadId":"36841","inReplyTo":"20140604194118.23903.3019.chriscool@tuxfamily.org","subject":"[PATCH v3 1/4] replace: add --graft option","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-06-04T19:43:49Z","receivedAt":"2014-06-04T19:43:49Z","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.\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-- \n2.0.0.rc0.40.gd30ccc4\n"},{"id":"243364","messageId":"20140604194353.23903.60343.chriscool@tuxfamily.org","threadId":"36841","inReplyTo":"20140604194118.23903.3019.chriscool@tuxfamily.org","subject":"[PATCH v3 2/4] replace: add test for --graft","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-06-04T19:43:50Z","receivedAt":"2014-06-04T19:43:50Z","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-- \n2.0.0.rc0.40.gd30ccc4\n"},{"id":"243365","messageId":"20140604194353.23903.41340.chriscool@tuxfamily.org","threadId":"36841","inReplyTo":"20140604194118.23903.3019.chriscool@tuxfamily.org","subject":"[PATCH v3 3/4] Documentation: replace: add --graft option","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-06-04T19:43:51Z","receivedAt":"2014-06-04T19:43:51Z","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-- \n2.0.0.rc0.40.gd30ccc4\n"},{"id":"243368","messageId":"20140604194353.23903.58934.chriscool@tuxfamily.org","threadId":"36841","inReplyTo":"20140604194118.23903.3019.chriscool@tuxfamily.org","subject":"[PATCH v3 4/4] contrib: add convert-grafts-to-replace-refs.sh","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-06-04T19:43:52Z","receivedAt":"2014-06-04T19:43:52Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This patch adds into contrib/ an example script to convert\ngrafts from an existing grafts file into replace refs using\nthe new --graft option of \"git replace\".\n\nWhile at it let's mention this new script in the\n\"git replace\" documentation for the --graft option.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n Documentation/git-replace.txt             |  4 +++-\n contrib/convert-grafts-to-replace-refs.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 32 insertions(+), 1 deletion(-)\n create mode 100755 contrib/convert-grafts-to-replace-refs.sh\n\ndiff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt\nindex 491875e..e1be2e6 100644\n--- a/Documentation/git-replace.txt\n+++ b/Documentation/git-replace.txt\n@@ -79,7 +79,9 @@ OPTIONS\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+\tcommit. See contrib/convert-grafts-to-replace-refs.sh for an\n+\texample script based on this option that can convert grafts to\n+\treplace refs.\n \n -l <pattern>::\n --list <pattern>::\ndiff --git a/contrib/convert-grafts-to-replace-refs.sh b/contrib/convert-grafts-to-replace-refs.sh\nnew file mode 100755\nindex 0000000..8472879\n--- /dev/null\n+++ b/contrib/convert-grafts-to-replace-refs.sh\n@@ -0,0 +1,29 @@\n+#!/bin/sh\n+\n+# You should execute this script in the repository where you\n+# want to convert grafts to replace refs.\n+\n+die () {\n+\techo >&2 \"$@\"\n+\texit 1\n+}\n+\n+GRAFTS_FILE=\"${GIT_DIR:-.git}/info/grafts\"\n+\n+test -f \"$GRAFTS_FILE\" || die \"Could not find graft file: '$GRAFTS_FILE'\"\n+\n+grep '^[^# ]' \"$GRAFTS_FILE\" | while read definition\n+do\n+\ttest -n \"$definition\" && {\n+\t\techo \"Converting: $definition\"\n+\t\tgit replace --graft $definition ||\n+\t\t\tdie \"Conversion failed for: $definition\"\n+\t}\n+done\n+\n+mv \"$GRAFTS_FILE\" \"$GRAFTS_FILE.bak\" ||\n+\tdie \"Could not rename '$GRAFTS_FILE' to '$GRAFTS_FILE.bak'\"\n+\n+echo \"Success!\"\n+echo \"All the grafts in '$GRAFTS_FILE' have been converted to replace refs!\"\n+echo \"The grafts file '$GRAFTS_FILE' has been renamed: '$GRAFTS_FILE.bak'\"\n-- \n2.0.0.rc0.40.gd30ccc4\n"},{"id":"243423","messageId":"xmqqfvjjrpq9.fsf@gitster.dls.corp.google.com","threadId":"36841","inReplyTo":"20140604194353.23903.89933.chriscool@tuxfamily.org","subject":"Re: [PATCH v3 1/4] replace: add --graft option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-05T21:49:50Z","receivedAt":"2014-06-05T21:49:50Z","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> +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\nNot a problem with this patch, but the above sequence somehow makes\nme wonder if lookup-commit-or-die is a good API for this sort of\nthing.  Wouldn't it be more natural if it took not \"unsigned char\nold[20]\" but anything that would be understood by get_sha1()?\n\nIt could be that this particular caller is peculiar and all the\nexisting callers are happy, though.  I didn't \"git grep\" to spot\npatterns in existing callers.\n\n> +\tif (read_sha1_commit(old, &buf))\n> +\t\tdie(_(\"Invalid commit: '%s'\"), old_ref);\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\nIt is unclear to me what you are trying to achieve with these two.\nIf the earlier lookup-commit has returned a commit object that has\nalready been parsed, parse_commit_buffer() would not check anything,\nwould it?\n\nA typical sequence would look more like this:\n\n    commit = lookup_commit(...);\n    if (parse_commit(commit))\n\toops there is an error;\n    /* otherwise */\n    use(commit->buffer);\n\nwithout reading a commit object using low-level read-sha1-file\ninterface yourself, no?\n"},{"id":"243424","messageId":"xmqqa99rrpgx.fsf@gitster.dls.corp.google.com","threadId":"36841","inReplyTo":"20140604194353.23903.58934.chriscool@tuxfamily.org","subject":"Re: [PATCH v3 4/4] contrib: add convert-grafts-to-replace-refs.sh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-05T21:55:26Z","receivedAt":"2014-06-05T21:55:26Z","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> diff --git a/contrib/convert-grafts-to-replace-refs.sh b/contrib/convert-grafts-to-replace-refs.sh\n> new file mode 100755\n> index 0000000..8472879\n> --- /dev/null\n> +++ b/contrib/convert-grafts-to-replace-refs.sh\n> @@ -0,0 +1,29 @@\n> +#!/bin/sh\n> +\n> +# You should execute this script in the repository where you\n> +# want to convert grafts to replace refs.\n> +\n> +die () {\n> +\techo >&2 \"$@\"\n> +\texit 1\n> +}\n\nDon't we install git-sh-setup in GIT_EXEC_PATH, in order to allow\nthese third-party scripts to begin with:\n\n        . $(git --exec-path)/git-sh-setup\n\njust like our own scripted Porcelains?\n\n> +GRAFTS_FILE=\"${GIT_DIR:-.git}/info/grafts\"\n> +\n> +test -f \"$GRAFTS_FILE\" || die \"Could not find graft file: '$GRAFTS_FILE'\"\n> +\n> +grep '^[^# ]' \"$GRAFTS_FILE\" | while read definition\n> +do\n\nFormat the above like so:\n\n    grep '^[^# ]' \"$GRAFTS_FILE\" |\n    while read definition\n    do\n\nwhich is easier to see what that \"do\" is doing.\n\n> +\ttest -n \"$definition\" && {\n> +\t\techo \"Converting: $definition\"\n> +\t\tgit replace --graft $definition ||\n> +\t\t\tdie \"Conversion failed for: $definition\"\n> +\t}\n\nHmph, why not if/then/fi?\n"},{"id":"243496","messageId":"CAP8UFD3k98_6Uh+noJgt4xqEooATVMAEf58FFkuy6rHBnP10zw@mail.gmail.com","threadId":"36841","inReplyTo":"xmqqfvjjrpq9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 1/4] replace: add --graft option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2014-06-06T15:29:01Z","receivedAt":"2014-06-06T15:29:01Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Jun 5, 2014 at 11:49 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Christian Couder <chriscool@tuxfamily.org> writes:\n>\n>> +static int create_graft(int argc, const char **argv, int force)\n>> +{\n>> +     unsigned char old[20], new[20];\n>> +     const char *old_ref = argv[0];\n>> +     struct commit *commit;\n>> +     struct strbuf buf = STRBUF_INIT;\n>> +     struct strbuf new_parents = STRBUF_INIT;\n>> +     const char *parent_start, *parent_end;\n>> +     int i;\n>> +\n>> +     if (get_sha1(old_ref, old) < 0)\n>> +             die(_(\"Not a valid object name: '%s'\"), old_ref);\n>> +     commit = lookup_commit_or_die(old, old_ref);\n>\n> Not a problem with this patch, but the above sequence somehow makes\n> me wonder if lookup-commit-or-die is a good API for this sort of\n> thing.  Wouldn't it be more natural if it took not \"unsigned char\n> old[20]\" but anything that would be understood by get_sha1()?\n>\n> It could be that this particular caller is peculiar and all the\n> existing callers are happy, though.  I didn't \"git grep\" to spot\n> patterns in existing callers.\n\nlookup_commit_or_die() looked like a good API to me because I saw that\nit checked a lot of things and die in case of problems, which could\nmake the patch shorter.\n\n>> +     if (read_sha1_commit(old, &buf))\n>> +             die(_(\"Invalid commit: '%s'\"), old_ref);\n>> +     /* make sure the commit is not corrupt */\n>> +     if (parse_commit_buffer(commit, buf.buf, buf.len))\n>> +             die(_(\"Could not parse commit: '%s'\"), old_ref);\n>\n> It is unclear to me what you are trying to achieve with these two.\n> If the earlier lookup-commit has returned a commit object that has\n> already been parsed, parse_commit_buffer() would not check anything,\n> would it?\n\nYeah, you are right. I missed the fact that lookup_commit_or_die()\ncalls parse_object() which itself calls read_sha1_file() and then\nparse_object_buffer() which calls parse_commit_buffer().\n\nHere is a backtrace that shows this:\n\n#0  parse_commit_buffer (item=0x8597b0, buffer=0x851730, size=228) at\ncommit.c:251\n#1  0x00000000004fa215 in parse_object_buffer (sha1=0x7fffffffdbf0\n\"\\t>A\\247\\235J\\213\\376<u\\212\\226\\311^[\\371\\343^\\330\\234\",\ntype=OBJ_COMMIT, size=228,\n    buffer=0x851730, eaten_p=0x7fffffffdacc) at object.c:198\n#2  0x00000000004fa50a in parse_object (sha1=0x7fffffffdbf0\n\"\\t>A\\247\\235J\\213\\376<u\\212\\226\\311^[\\371\\343^\\330\\234\") at\nobject.c:264\n#3  0x00000000004a89ef in lookup_commit_reference_gently\n(sha1=0x7fffffffdbf0\n\"\\t>A\\247\\235J\\213\\376<u\\212\\226\\311^[\\371\\343^\\330\\234\", quiet=0) at\ncommit.c:38\n#4  0x00000000004a8a48 in lookup_commit_reference (sha1=0x7fffffffdbf0\n\"\\t>A\\247\\235J\\213\\376<u\\212\\226\\311^[\\371\\343^\\330\\234\") at\ncommit.c:47\n#5  0x00000000004a8a67 in lookup_commit_or_die (sha1=0x7fffffffdbf0\n\"\\t>A\\247\\235J\\213\\376<u\\212\\226\\311^[\\371\\343^\\330\\234\",\n    ref_name=0x7fffffffe465\n\"093e41a79d4a8bfe3c758a96c95e5bf9e35ed89c\") at commit.c:52\n#6  0x000000000047f89a in create_graft (argc=1, argv=0x7fffffffe130,\nrefdir=0x0, force=0) at builtin/replace.c:353\n#7  0x000000000047ff71 in cmd_replace (argc=1, argv=0x7fffffffe130,\nprefix=0x0) at builtin/replace.c:461\n#8  0x0000000000405441 in run_builtin (p=0x7eee90, argc=3,\nargv=0x7fffffffe130) at git.c:314\n#9  0x000000000040563a in handle_builtin (argc=3, argv=0x7fffffffe130)\nat git.c:487\n#10 0x0000000000405754 in run_argv (argcp=0x7fffffffe01c,\nargv=0x7fffffffe020) at git.c:533\n#11 0x00000000004058f9 in main (argc=3, av=0x7fffffffe128) at git.c:616\n\n> A typical sequence would look more like this:\n>\n>     commit = lookup_commit(...);\n>     if (parse_commit(commit))\n>         oops there is an error;\n>     /* otherwise */\n>     use(commit->buffer);\n>\n> without reading a commit object using low-level read-sha1-file\n> interface yourself, no?\n\nYeah, or I could just rely on the fact that lookup_commit_or_die()\nalready parses the commit, with something like this:\n\n      if (get_sha1(old_ref, old) < 0)\n              die(_(\"Not a valid object name: '%s'\"), old_ref);\n\n      /* parse the commit buffer to make sure the commit is not corrupt */\n      commit = lookup_commit_or_die(old, old_ref);\n\n      /* find existing parents */\n      parent_start = buf.buf;\n      parent_start += 46; /* \"tree \" + \"hex sha1\" + \"\\n\" */\n      parent_end = parent_start;\n...\n\nThanks,\nChristian.\n"},{"id":"243498","messageId":"CAP8UFD2u-DReCv2ZXhAzH_UMW5P--+a=AKS-N88xE6zmdWBLqQ@mail.gmail.com","threadId":"36841","inReplyTo":"CAP8UFD3k98_6Uh+noJgt4xqEooATVMAEf58FFkuy6rHBnP10zw@mail.gmail.com","subject":"Re: [PATCH v3 1/4] replace: add --graft option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2014-06-06T15:44:59Z","receivedAt":"2014-06-06T15:44:59Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Jun 6, 2014 at 5:29 PM, Christian Couder\n<christian.couder@gmail.com> wrote:\n>\n> Yeah, or I could just rely on the fact that lookup_commit_or_die()\n> already parses the commit, with something like this:\n>\n>       if (get_sha1(old_ref, old) < 0)\n>               die(_(\"Not a valid object name: '%s'\"), old_ref);\n>\n>       /* parse the commit buffer to make sure the commit is not corrupt */\n>       commit = lookup_commit_or_die(old, old_ref);\n>\n>       /* find existing parents */\n>       parent_start = buf.buf;\n>       parent_start += 46; /* \"tree \" + \"hex sha1\" + \"\\n\" */\n>       parent_end = parent_start;\n\nThis last part should be:\n\n        /* find existing parents */\n        strbuf_addstr(&buf, commit->buffer);\n        parent_start = buf.buf;\n        parent_start += 46; /* \"tree \" + \"hex sha1\" + \"\\n\" */\n        parent_end = parent_start;\n...\n\nI will send an updated patch series soon.\n"},{"id":"243499","messageId":"CAP8UFD2kSP-ONaANhXhmr5-USC4af9RHEkeTOAhH6Jm-1TNdiA@mail.gmail.com","threadId":"36841","inReplyTo":"xmqqa99rrpgx.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 4/4] contrib: add convert-grafts-to-replace-refs.sh","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2014-06-06T15:47:39Z","receivedAt":"2014-06-06T15:47:39Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Jun 5, 2014 at 11:55 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Christian Couder <chriscool@tuxfamily.org> writes:\n>\n>> diff --git a/contrib/convert-grafts-to-replace-refs.sh b/contrib/convert-grafts-to-replace-refs.sh\n>> new file mode 100755\n>> index 0000000..8472879\n>> --- /dev/null\n>> +++ b/contrib/convert-grafts-to-replace-refs.sh\n>> @@ -0,0 +1,29 @@\n>> +#!/bin/sh\n>> +\n>> +# You should execute this script in the repository where you\n>> +# want to convert grafts to replace refs.\n>> +\n>> +die () {\n>> +     echo >&2 \"$@\"\n>> +     exit 1\n>> +}\n>\n> Don't we install git-sh-setup in GIT_EXEC_PATH, in order to allow\n> these third-party scripts to begin with:\n>\n>         . $(git --exec-path)/git-sh-setup\n>\n> just like our own scripted Porcelains?\n\nOk, I will use that.\n\n>> +GRAFTS_FILE=\"${GIT_DIR:-.git}/info/grafts\"\n>> +\n>> +test -f \"$GRAFTS_FILE\" || die \"Could not find graft file: '$GRAFTS_FILE'\"\n>> +\n>> +grep '^[^# ]' \"$GRAFTS_FILE\" | while read definition\n>> +do\n>\n> Format the above like so:\n>\n>     grep '^[^# ]' \"$GRAFTS_FILE\" |\n>     while read definition\n>     do\n>\n> which is easier to see what that \"do\" is doing.\n\nOk.\n\n>> +     test -n \"$definition\" && {\n>> +             echo \"Converting: $definition\"\n>> +             git replace --graft $definition ||\n>> +                     die \"Conversion failed for: $definition\"\n>> +     }\n>\n> Hmph, why not if/then/fi?\n\nOk, I will use if/then/fi.\n\nThanks,\nChristian.\n"},{"id":"243503","messageId":"xmqqegz2q8ih.fsf@gitster.dls.corp.google.com","threadId":"36841","inReplyTo":"CAP8UFD3k98_6Uh+noJgt4xqEooATVMAEf58FFkuy6rHBnP10zw@mail.gmail.com","subject":"Re: [PATCH v3 1/4] replace: add --graft option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-06T16:59:18Z","receivedAt":"2014-06-06T16:59:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> On Thu, Jun 5, 2014 at 11:49 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Christian Couder <chriscool@tuxfamily.org> writes:\n>>\n>>> +static int create_graft(int argc, const char **argv, int force)\n>>> +{\n>>> +     unsigned char old[20], new[20];\n>>> +     const char *old_ref = argv[0];\n>>> +     struct commit *commit;\n>>> +     struct strbuf buf = STRBUF_INIT;\n>>> +     struct strbuf new_parents = STRBUF_INIT;\n>>> +     const char *parent_start, *parent_end;\n>>> +     int i;\n>>> +\n>>> +     if (get_sha1(old_ref, old) < 0)\n>>> +             die(_(\"Not a valid object name: '%s'\"), old_ref);\n>>> +     commit = lookup_commit_or_die(old, old_ref);\n>>\n>> Not a problem with this patch, but the above sequence somehow makes\n>> me wonder if lookup-commit-or-die is a good API for this sort of\n>> thing.  Wouldn't it be more natural if it took not \"unsigned char\n>> old[20]\" but anything that would be understood by get_sha1()?\n>>\n>> It could be that this particular caller is peculiar and all the\n>> existing callers are happy, though.  I didn't \"git grep\" to spot\n>> patterns in existing callers.\n>\n> lookup_commit_or_die() looked like a good API to me because I saw that\n> it checked a lot of things and die in case of problems, which could\n> make the patch shorter.\n\nPerhaps I was vague.  \"find the commit object and die if that object\nis not properly formed\" is a good thing.  I was referring to the\nfact that you\n\n    - first had to do get-sha1 yourself to turn the extended sha1\n      you got from the user into a binary object name, and die with\n      your own error message if the user input was malformed.\n\n    - and then had to call lookup-commit-or-die to do the checking\n      and let it die.\n\nIt would have been simpler for *this* particular codepath if we had\nanother helper function you can use like so:\n\n\tcommit = lookup_commit_with_extended_sha1_or_die(old_ref);\n\nwhich did the two-call sequence you handrolled above, and I was\nwondering if other existing callers to lookup-commit-or-die wished\nthe same thing.\n"},{"id":"243598","messageId":"CAP8UFD344NFECqtO-uK-2wHA7XEko3XMJWuhW9+KGBdaiqGq+w@mail.gmail.com","threadId":"36841","inReplyTo":"CAP8UFD2u-DReCv2ZXhAzH_UMW5P--+a=AKS-N88xE6zmdWBLqQ@mail.gmail.com","subject":"Re: [PATCH v3 1/4] replace: add --graft option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2014-06-08T06:49:45Z","receivedAt":"2014-06-08T06:49:45Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Jun 6, 2014 at 5:44 PM, Christian Couder\n<christian.couder@gmail.com> wrote:\n>\n>         /* find existing parents */\n>         strbuf_addstr(&buf, commit->buffer);\n\nUnfortunately, it looks like the above will not work if the commit->buffer\ncontains an embedded NUL. I wonder if it is a real problem or not.\n"},{"id":"243604","messageId":"20140608112333.GA9691@sigill.intra.peff.net","threadId":"36841","inReplyTo":"CAP8UFD344NFECqtO-uK-2wHA7XEko3XMJWuhW9+KGBdaiqGq+w@mail.gmail.com","subject":"Re: [PATCH v3 1/4] replace: add --graft option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-08T11:23:33Z","receivedAt":"2014-06-08T11:23:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 08, 2014 at 08:49:45AM +0200, Christian Couder wrote:\n\n> On Fri, Jun 6, 2014 at 5:44 PM, Christian Couder\n> <christian.couder@gmail.com> wrote:\n> >\n> >         /* find existing parents */\n> >         strbuf_addstr(&buf, commit->buffer);\n> \n> Unfortunately, it looks like the above will not work if the commit->buffer\n> contains an embedded NUL. I wonder if it is a real problem or not.\n\nI ran into a similar problem recently[1] and have been pondering\nsolutions to know the size of commit->buffer. What I've been come up\nwith is:\n\n  1. Look up the object size via sha1_object_info. Besides being\n     inefficient (which probably does not matter for you here, but might\n     for using commit->buffer in a traversal), it strikes me as\n     inelegant; is it possible for commit->buffer to ever disagree in\n     size with the results of sha1_object_info, and if so, what happens?\n\n  2. Add an extra member \"len\" to \"struct commit\". This is simple, but\n     bloats \"struct commit\", which may have a performance impact for\n     things like rev-list, where the field will be unused.\n\n  3. Store the length of objects as a size_t, exactly sizeof(size_t)\n     bytes before the object buffer. Provide a macro:\n\n       #define OBJECT_SIZE(buf) (((size_t *)(buf))[-1])\n\n     to access it. Most callers can just use the buffer as-is, but\n     anybody who calls free() would need to be adjusted to use a special\n     \"object_free\".\n\n  4. Keep a static commit_slab that points to the length for each parsed\n     commit. We pay the same memory cost as (2), but as it's not part of\n     the struct, the cache effects are minimized.\n\n-Peff\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/250480\n"},{"id":"243605","messageId":"20140608120439.GA21827@sigill.intra.peff.net","threadId":"36841","inReplyTo":"20140608112333.GA9691@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/4] replace: add --graft option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-08T12:04:39Z","receivedAt":"2014-06-08T12:04:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 08, 2014 at 07:23:33AM -0400, Jeff King wrote:\n\n>   4. Keep a static commit_slab that points to the length for each parsed\n>      commit. We pay the same memory cost as (2), but as it's not part of\n>      the struct, the cache effects are minimized.\n\nI think I favor this solution, which would look something like this:\n\n-- >8 --\nSubject: [PATCH] commit: add slab for commit buffer size\n\nWe store the commit object buffer for later reuse as\ncommit->buffer. However, since we store only a pointer, we\nmust treat the result as a NUL-terminated string. This is\ngenerally OK for pretty-printing, but could be a problem for\nother uses.\n\nAdding a \"len\" member to \"struct commit\" would solve this,\nbut at the cost of bloating the struct even for callers who\ndo not care about the size or buffer (e.g., traversals like\nrev-list or merge-base). Instead, let's use a commit_slab so\nthat the memory is used only when save_commit_buffer is in\neffect (and even then, it should have less cache impact on\nmost uses of \"struct commit\").\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI think it would make sense to actually take this one step further,\nthough, and move commit->buffer into the slab, as well. That has two\nadvantages:\n\n  1. It further decreases the size of \"struct commit\" for callers who do\n     not use save_commit_buffer.\n\n  2. It ensures that no new callers crop up who set \"commit->buffer\" but\n     to not save the size in the slab (you can see in the patch below\n     that I had to modify builtin/blame.c, which (ab)uses\n     commit->buffer).\n\nIt would be more disruptive to existing callers, but I think the end\nresult would be pretty clean. The API would be something like:\n\n  /* attach buffer to commit */\n  set_commit_buffer(struct commit *, void *buf, unsigned long size);\n\n  /* get buffer, either from slab cache or by calling read_sha1_file */\n  void *get_commit_buffer(struct commit *, unsigned long *size);\n\n  /* free() an allocated buffer from above, noop for cached buffer */\n  void unused_commit_buffer(struct commit *, void *buf);\n\n  /* drop saved commit buffer to free memory */\n  void free_commit_buffer(struct commit *);\n\nThe \"get\" function would serve the existing callers in pretty.c, as well\nas the one I'm adding elsewhere in show_signature. And it should work as\na drop-in read_sha1_file/free replacement for you here.\n\n builtin/blame.c |  2 +-\n commit.c        | 13 ++++++++++++-\n commit.h        |  1 +\n object.c        |  2 +-\n 4 files changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex a52a279..1945ea4 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2313,7 +2313,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \t\t    ident, ident, path,\n \t\t    (!contents_from ? path :\n \t\t     (!strcmp(contents_from, \"-\") ? \"standard input\" : contents_from)));\n-\tcommit->buffer = strbuf_detach(&msg, NULL);\n+\tset_commit_buffer(commit, strbuf_detach(&msg, NULL), msg.len);\n \n \tif (!contents_from || strcmp(\"-\", contents_from)) {\n \t\tstruct stat st;\ndiff --git a/commit.c b/commit.c\nindex f479331..71143ed 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -302,6 +302,17 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s\n \treturn 0;\n }\n \n+define_commit_slab(commit_size_slab, unsigned long);\n+static struct commit_size_slab commit_size;\n+\n+void set_commit_buffer(struct commit *item, void *buffer, unsigned long size)\n+{\n+\tif (!commit_size.stride)\n+\t\tinit_commit_size_slab(&commit_size);\n+\t*commit_size_slab_at(&commit_size, item) = size;\n+\titem->buffer = buffer;\n+}\n+\n int parse_commit(struct commit *item)\n {\n \tenum object_type type;\n@@ -324,7 +335,7 @@ int parse_commit(struct commit *item)\n \t}\n \tret = parse_commit_buffer(item, buffer, size);\n \tif (save_commit_buffer && !ret) {\n-\t\titem->buffer = buffer;\n+\t\tset_commit_buffer(item, buffer, size);\n \t\treturn 0;\n \t}\n \tfree(buffer);\ndiff --git a/commit.h b/commit.h\nindex a9f177b..7704ab2 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -48,6 +48,7 @@ struct commit *lookup_commit_reference_by_name(const char *name);\n struct commit *lookup_commit_or_die(const unsigned char *sha1, const char *ref_name);\n \n int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long size);\n+void set_commit_buffer(struct commit *item, void *buffer, unsigned long size);\n int parse_commit(struct commit *item);\n void parse_commit_or_die(struct commit *item);\n \ndiff --git a/object.c b/object.c\nindex 57a0890..c1c6a24 100644\n--- a/object.c\n+++ b/object.c\n@@ -198,7 +198,7 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n \t\t\tif (parse_commit_buffer(commit, buffer, size))\n \t\t\t\treturn NULL;\n \t\t\tif (!commit->buffer) {\n-\t\t\t\tcommit->buffer = buffer;\n+\t\t\t\tset_commit_buffer(commit, buffer, size);\n \t\t\t\t*eaten_p = 1;\n \t\t\t}\n \t\t\tobj = &commit->object;\n-- \n2.0.0.729.g520999f\n"},{"id":"243606","messageId":"20140608120936.GB21827@sigill.intra.peff.net","threadId":"36841","inReplyTo":"20140608120439.GA21827@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/4] replace: add --graft option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-08T12:09:36Z","receivedAt":"2014-06-08T12:09:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 08, 2014 at 08:04:39AM -0400, Jeff King wrote:\n\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index a52a279..1945ea4 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -2313,7 +2313,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n>  \t\t    ident, ident, path,\n>  \t\t    (!contents_from ? path :\n>  \t\t     (!strcmp(contents_from, \"-\") ? \"standard input\" : contents_from)));\n> -\tcommit->buffer = strbuf_detach(&msg, NULL);\n> +\tset_commit_buffer(commit, strbuf_detach(&msg, NULL), msg.len);\n\nSide note: this is wrong, as the fake commit created by blame here does\nnot have its \"index\" field set. This is a bug waiting to happen the\nfirst time somebody uses a slab in builtin/blame.c. It looks like\nmerge-recursive does the same thing in make_virtual_commit.\n\nWe probably want to provide a function to allocate a commit, including\nthe index, and use it consistently.\n\nI'll try to work up a series doing that, and the fuller slab API I\nmentioned in the previous message, but probably not until tomorrow.\n\n-Peff\n"},{"id":"243628","messageId":"xmqq61kaf2zo.fsf@gitster.dls.corp.google.com","threadId":"36841","inReplyTo":"20140608120439.GA21827@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/4] replace: add --graft option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-09T16:43:07Z","receivedAt":"2014-06-09T16:43:07Z","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> I think it would make sense to actually take this one step further,\n> though, and move commit->buffer into the slab, as well. That has two\n> advantages:\n>\n>   1. It further decreases the size of \"struct commit\" for callers who do\n>      not use save_commit_buffer.\n>\n>   2. It ensures that no new callers crop up who set \"commit->buffer\" but\n>      to not save the size in the slab (you can see in the patch below\n>      that I had to modify builtin/blame.c, which (ab)uses\n>      commit->buffer).\n>\n> It would be more disruptive to existing callers, but I think the end\n> result would be pretty clean. The API would be something like:\n>\n>   /* attach buffer to commit */\n>   set_commit_buffer(struct commit *, void *buf, unsigned long size);\n>\n>   /* get buffer, either from slab cache or by calling read_sha1_file */\n>   void *get_commit_buffer(struct commit *, unsigned long *size);\n>\n>   /* free() an allocated buffer from above, noop for cached buffer */\n>   void unused_commit_buffer(struct commit *, void *buf);\n>\n>   /* drop saved commit buffer to free memory */\n>   void free_commit_buffer(struct commit *);\n>\n> The \"get\" function would serve the existing callers in pretty.c, as well\n> as the one I'm adding elsewhere in show_signature. And it should work as\n> a drop-in read_sha1_file/free replacement for you here.\n\nThis solution has the kind of niceness to make everybody (certainly\nincluding me) who has been involved in the code path should say \"why\ndidn't I think of that!\" ;-)\n"},{"id":"245149","messageId":"CAP8UFD25k4X0vVUDDTOj_XDqmShz-aHXFLKD5bruUYYLX6awow@mail.gmail.com","threadId":"36841","inReplyTo":"CAPc5daWBycdmKBZXGhhy4_649p_JFfGf7RQbqa08XA1hL9mFTg@mail.gmail.com","subject":"Re: [PATCH v3 1/4] replace: add --graft option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2014-06-29T06:34:35Z","receivedAt":"2014-06-29T06:34:35Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Jun 8, 2014 at 10:18 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> On Sat, Jun 7, 2014 at 11:49 PM, Christian Couder\n> <christian.couder@gmail.com> wrote:\n>>\n>> On Fri, Jun 6, 2014 at 5:44 PM, Christian Couder\n>> <christian.couder@gmail.com> wrote:\n>> >\n>> >         /* find existing parents */\n>> >         strbuf_addstr(&buf, commit->buffer);\n>>\n>> Unfortunately, it looks like the above will not work if the commit->buffer\n>> contains an embedded NUL. I wonder if it is a real problem or not.\n>\n> Yes, it is a real problem (there was another thread on this regarding the\n> code path that verifies GPG signature on the commit itself), which\n> incidentally reminds us to another thing to think about in your patch as\n> well. I *think* you shoud drop the GPG signature on the commit itself, and\n> you also should drop the merge-tag of a parent you are not going to keep,\n> but should keep the merge-tag of a parent you are keeping.\n\nIn the v5 of the patch series, I now drop the GPG signature on the commit\nitself.\n\nNow, after having read the recent thread about \"git verify-commit\", I understand\nthat you also want me to drop the signature of a tag that was merged, because\nsuch signatures are added to the commit message.\n\nBut I wonder how far should we go in this path. For example merge commits\nhave a title like \"Merge branch 'dev'\" or \"Merge tag 'stuff'\", but\nthis does not make\nsense any more if the replacement commit drops the parent corresponding to\n'dev' or 'stuff'. And I don't think we should change the commit title.\n\nThanks,\nChristian.\n"},{"id":"245170","messageId":"xmqqr426zyw6.fsf@gitster.dls.corp.google.com","threadId":"36841","inReplyTo":"CAP8UFD25k4X0vVUDDTOj_XDqmShz-aHXFLKD5bruUYYLX6awow@mail.gmail.com","subject":"Re: [PATCH v3 1/4] replace: add --graft option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-30T06:37:13Z","receivedAt":"2014-06-30T06:37:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> Now, after having read the recent thread about \"git verify-commit\", I understand\n> that you also want me to drop the signature of a tag that was merged, because\n> such signatures are added to the commit message.\n\nHuh?  I am not sure if I follow.  Perhaps we are talking about\ndifferent things?\n\nWhen you are preparing a replacement for an existing commit that\nmerges a signed tag, there are two cases:\n\n - The replacement commit still merges the same signed tag; or\n\n - The replacement commit does not merge that signed tag (it may\n   become a single-parent commit, or it may stay to be a merge but\n   merges a different commit on the side branch).\n\nIn the former, it would be sensible to keep the \"mergetag\" and\npropagate it to the replacement; it is a signature over the tip of\nthe side branch being merged by the original (and the replacement)\nmerge, and the replacement will not affect the validity of the\nsignature at all.  In the latter, we do want to drop the \"mergetag\"\nfor the parent you are losing in the replacement, because by\ndefinition it will be irrelevant.\n"},{"id":"245178","messageId":"CAP8UFD3w2s8tQ0OVe2eb9AmdsZhDeVJqNBUXewcbBxqs=id+HA@mail.gmail.com","threadId":"36841","inReplyTo":"xmqqr426zyw6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 1/4] replace: add --graft option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2014-06-30T10:52:29Z","receivedAt":"2014-06-30T10:52:29Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Jun 30, 2014 at 8:37 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Christian Couder <christian.couder@gmail.com> writes:\n>\n>> Now, after having read the recent thread about \"git verify-commit\", I understand\n>> that you also want me to drop the signature of a tag that was merged, because\n>> such signatures are added to the commit message.\n>\n> Huh?  I am not sure if I follow.  Perhaps we are talking about\n> different things?\n\nI think we are talking about the same thing but I might not have been clear.\n\n> When you are preparing a replacement for an existing commit that\n> merges a signed tag, there are two cases:\n>\n>  - The replacement commit still merges the same signed tag; or\n>\n>  - The replacement commit does not merge that signed tag (it may\n>    become a single-parent commit, or it may stay to be a merge but\n>    merges a different commit on the side branch).\n>\n> In the former, it would be sensible to keep the \"mergetag\" and\n> propagate it to the replacement; it is a signature over the tip of\n> the side branch being merged by the original (and the replacement)\n> merge, and the replacement will not affect the validity of the\n> signature at all.\n\nOk, this is what is done right now by the patch series.\n\n> In the latter, we do want to drop the \"mergetag\"\n> for the parent you are losing in the replacement, because by\n> definition it will be irrelevant.\n\nYeah, it might be a good idea to drop the \"mergetag\", but note that\nanyway such a commit probably has a title like \"Merge tag '<tag>'\" and\nwe won't automatically change this title and this title will be wrong\n(because we are not merging anymore this tag).\n\nSo anyway in this case, --graft will do something that is not good. So\nit might be better in this case to just error out and say that it\nwould be better to use --edit instead of --graft.\n"}]}