{"thread":{"id":"37018","subject":"[PATCH v5 2/7] replace: add test for --graft","startedAt":"2014-06-28T18:11:09Z","lastAt":"2014-07-03T18:45:37Z","messageCount":17,"participants":["Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":5,"patchTotal":7},"messages":[{"id":"245128","messageId":"20140628171731.5687.30308.chriscool@tuxfamily.org","threadId":"37018","inReplyTo":null,"subject":"[PATCH v5 0/7] Add --graft option to git replace","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-06-28T18:11:09Z","receivedAt":"2014-06-28T18:11:09Z","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\nThe changes since v4, thanks to Junio and Peff, are:\n\n- The series has been rebased on top of Peff's series\n  to store the commit buffer length\n  (jk/commit-buffer-length). This changes patch 1/7\n  and fixes the problem that we lost everything after\n  a NUL byte in the commit buffer. \n\n- Patches 5/7, 6/7 and 7/7 have been added to lose\n  a gpg signature in the original commit buffer.\n  \nNotes:\n\n- Maybe patches could be reordered or squashed, but\n  I prefered not to do it in this round so that\n  changes compared to previous version are easier to\n  spot.\n\n- Junio suggested to drop the merge-tag of a parent\n  we are not going to keep, but to keep the merge-tag\n  of a parent we are keeping. This has not yet been\n  done. It is unfortunate that merge-tags don't use\n  a format like the trailer format, because we could\n  have factorised droping a merge-tag in the\n  \"git interpret-trailers\" command.\n\nChristian Couder (7):\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  replace: refactor replacing parents\n  replace: remove signature when using --graft\n  replace: add test for --graft with signed commit\n\n Documentation/git-replace.txt             | 10 ++++\n builtin/replace.c                         | 79 ++++++++++++++++++++++++++++++-\n commit.c                                  | 34 +++++++++++++\n commit.h                                  |  2 +\n contrib/convert-grafts-to-replace-refs.sh | 28 +++++++++++\n t/t6050-replace.sh                        | 34 +++++++++++++\n 6 files changed, 186 insertions(+), 1 deletion(-)\n create mode 100755 contrib/convert-grafts-to-replace-refs.sh\n\n-- \n2.0.0.421.g786a89d.dirty\n"},{"id":"245129","messageId":"20140628181117.5687.51444.chriscool@tuxfamily.org","threadId":"37018","inReplyTo":"20140628171731.5687.30308.chriscool@tuxfamily.org","subject":"[PATCH v5 1/7] replace: add --graft option","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-06-28T18:11:10Z","receivedAt":"2014-06-28T18:11:10Z","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>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/replace.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 65 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 1bb491d..3515979 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,58 @@ 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 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+\tconst char *buffer;\n+\tunsigned long size;\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+\n+\t/* find existing parents */\n+\tbuffer = get_commit_buffer(commit, &size);\n+\tstrbuf_add(&buf, buffer, size);\n+\tunuse_commit_buffer(commit, buffer);\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 +356,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 +380,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 +406,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.421.g786a89d.dirty\n"},{"id":"245127","messageId":"20140628181117.5687.23923.chriscool@tuxfamily.org","threadId":"37018","inReplyTo":"20140628171731.5687.30308.chriscool@tuxfamily.org","subject":"[PATCH v5 2/7] replace: add test for --graft","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-06-28T18:11:11Z","receivedAt":"2014-06-28T18:11:11Z","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>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\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.421.g786a89d.dirty\n"},{"id":"245132","messageId":"20140628181117.5687.92350.chriscool@tuxfamily.org","threadId":"37018","inReplyTo":"20140628171731.5687.30308.chriscool@tuxfamily.org","subject":"[PATCH v5 3/7] Documentation: replace: add --graft option","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-06-28T18:11:12Z","receivedAt":"2014-06-28T18:11:12Z","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>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\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.421.g786a89d.dirty\n"},{"id":"245133","messageId":"20140628181117.5687.16837.chriscool@tuxfamily.org","threadId":"37018","inReplyTo":"20140628171731.5687.30308.chriscool@tuxfamily.org","subject":"[PATCH v5 4/7] contrib: add convert-grafts-to-replace-refs.sh","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-06-28T18:11:13Z","receivedAt":"2014-06-28T18:11:13Z","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>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/git-replace.txt             |  4 +++-\n contrib/convert-grafts-to-replace-refs.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 31 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..0cbc917\n--- /dev/null\n+++ b/contrib/convert-grafts-to-replace-refs.sh\n@@ -0,0 +1,28 @@\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+GRAFTS_FILE=\"${GIT_DIR:-.git}/info/grafts\"\n+\n+. $(git --exec-path)/git-sh-setup\n+\n+test -f \"$GRAFTS_FILE\" || die \"Could not find graft file: '$GRAFTS_FILE'\"\n+\n+grep '^[^# ]' \"$GRAFTS_FILE\" |\n+while read definition\n+do\n+\tif test -n \"$definition\"\n+\tthen\n+\t\techo \"Converting: $definition\"\n+\t\tgit replace --graft $definition ||\n+\t\t\tdie \"Conversion failed for: $definition\"\n+\tfi\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.421.g786a89d.dirty\n"},{"id":"245131","messageId":"20140628181117.5687.43446.chriscool@tuxfamily.org","threadId":"37018","inReplyTo":"20140628171731.5687.30308.chriscool@tuxfamily.org","subject":"[PATCH v5 5/7] replace: refactor replacing parents","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-06-28T18:11:14Z","receivedAt":"2014-06-28T18:11:14Z","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 builtin/replace.c | 42 +++++++++++++++++++++++++-----------------\n 1 file changed, 25 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 3515979..ad47237 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -295,27 +295,14 @@ 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 create_graft(int argc, const char **argv, int force)\n+static void replace_parents(struct strbuf *buf, int argc, const char **argv)\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-\tconst char *buffer;\n-\tunsigned long size;\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-\n \t/* find existing parents */\n-\tbuffer = get_commit_buffer(commit, &size);\n-\tstrbuf_add(&buf, buffer, size);\n-\tunuse_commit_buffer(commit, buffer);\n-\tparent_start = buf.buf;\n+\tparent_start = buf->buf;\n \tparent_start += 46; /* \"tree \" + \"hex sha1\" + \"\\n\" */\n \tparent_end = parent_start;\n \n@@ -332,13 +319,34 @@ static int create_graft(int argc, const char **argv, int force)\n \t}\n \n \t/* replace existing parents with new ones */\n-\tstrbuf_splice(&buf, parent_start - buf.buf, parent_end - parent_start,\n+\tstrbuf_splice(buf, parent_start - buf->buf, parent_end - parent_start,\n \t\t      new_parents.buf, new_parents.len);\n \n+\tstrbuf_release(&new_parents);\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+\tconst char *buffer;\n+\tunsigned long size;\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+\n+\tbuffer = get_commit_buffer(commit, &size);\n+\tstrbuf_add(&buf, buffer, size);\n+\tunuse_commit_buffer(commit, buffer);\n+\n+\treplace_parents(&buf, argc, argv);\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-- \n2.0.0.421.g786a89d.dirty\n"},{"id":"245130","messageId":"20140628181117.5687.38249.chriscool@tuxfamily.org","threadId":"37018","inReplyTo":"20140628171731.5687.30308.chriscool@tuxfamily.org","subject":"[PATCH v5 6/7] replace: remove signature when using --graft","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-06-28T18:11:15Z","receivedAt":"2014-06-28T18:11:15Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"It could be misleading to keep a signature in a\nreplacement commit, so let's remove it.\n\nNote that there should probably be a way to sign\nthe replacement commit created when using --graft,\nbut this can be dealt with in another commit or\npatch series.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n builtin/replace.c |  5 +++++\n commit.c          | 34 ++++++++++++++++++++++++++++++++++\n commit.h          |  2 ++\n 3 files changed, 41 insertions(+)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex ad47237..000db65 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -344,6 +344,11 @@ static int create_graft(int argc, const char **argv, int force)\n \n \treplace_parents(&buf, argc, argv);\n \n+\tif (remove_signature(&buf))\n+\t\twarning(_(\"the original commit '%s' has a gpg signature.\\n\"\n+\t\t\t  \"It will be removed in the replacement commit!\"),\n+\t\t\told_ref);\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 \ndiff --git a/commit.c b/commit.c\nindex fb7897c..54e157d 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1177,6 +1177,40 @@ int parse_signed_commit(const struct commit *commit,\n \treturn saw_signature;\n }\n \n+int remove_signature(struct strbuf *buf)\n+{\n+\tconst char *line = buf->buf;\n+\tconst char *tail = buf->buf + buf->len;\n+\tint in_signature = 0;\n+\tconst char *sig_start = NULL;\n+\tconst char *sig_end = NULL;\n+\n+\twhile (line < tail) {\n+\t\tconst char *next = memchr(line, '\\n', tail - line);\n+\t\tnext = next ? next + 1 : tail;\n+\n+\t\tif (in_signature && line[0] == ' ')\n+\t\t\tsig_end = next;\n+\t\telse if (starts_with(line, gpg_sig_header) &&\n+\t\t\t line[gpg_sig_header_len] == ' ') {\n+\t\t\tsig_start = line;\n+\t\t\tsig_end = next;\n+\t\t\tin_signature = 1;\n+\t\t} else {\n+\t\t\tif (*line == '\\n')\n+\t\t\t\t/* dump the whole remainder of the buffer */\n+\t\t\t\tnext = tail;\n+\t\t\tin_signature = 0;\n+\t\t}\n+\t\tline = next;\n+\t}\n+\n+\tif (sig_start)\n+\t\tstrbuf_remove(buf, sig_start - buf->buf, sig_end - sig_start);\n+\n+\treturn sig_start != NULL;\n+}\n+\n static void handle_signed_tag(struct commit *parent, struct commit_extra_header ***tail)\n {\n \tstruct merge_remote_desc *desc;\ndiff --git a/commit.h b/commit.h\nindex 2e1492a..4234dae 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -327,6 +327,8 @@ struct commit *get_merge_parent(const char *name);\n \n extern int parse_signed_commit(const struct commit *commit,\n \t\t\t       struct strbuf *message, struct strbuf *signature);\n+extern int remove_signature(struct strbuf *buf);\n+\n extern void print_commit_list(struct commit_list *list,\n \t\t\t      const char *format_cur,\n \t\t\t      const char *format_last);\n-- \n2.0.0.421.g786a89d.dirty\n"},{"id":"245134","messageId":"20140628181117.5687.83337.chriscool@tuxfamily.org","threadId":"37018","inReplyTo":"20140628171731.5687.30308.chriscool@tuxfamily.org","subject":"[PATCH v5 7/7] replace: add test for --graft with signed commit","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-06-28T18:11:16Z","receivedAt":"2014-06-28T18:11:16Z","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 | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\nindex ca45a84..80b85e3 100755\n--- a/t/t6050-replace.sh\n+++ b/t/t6050-replace.sh\n@@ -7,6 +7,7 @@ test_description='Tests replace refs functionality'\n exec </dev/null\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-gpg.sh\"\n \n add_and_commit_file()\n {\n@@ -363,4 +364,25 @@ test_expect_success '--graft with and without already replaced object' '\n \tgit replace -d $HASH5\n '\n \n+test_expect_success GPG 'set up a signed commit' '\n+\techo \"line 17\" >> hello &&\n+\techo \"line 18\" >> hello &&\n+\tgit add hello &&\n+\ttest_tick &&\n+\tgit commit --quiet -S -m \"hello: 2 more lines in a signed commit\" &&\n+\tHASH8=$(git rev-parse --verify HEAD) &&\n+\tgit verify-commit $HASH8\n+'\n+\n+test_expect_success GPG '--graft with a signed commit' '\n+\tgit cat-file commit $HASH8 >orig &&\n+\tgit replace --graft $HASH8 &&\n+\tgit cat-file commit $HASH8 >repl &&\n+\ttest_must_fail grep gpgsig repl &&\n+\tdiff -u orig repl | grep \"^-parent $HASH7\" &&\n+\tdiff -u orig repl | grep \"^-gpgsig -----BEGIN PGP SIGNATURE-----\" &&\n+\ttest_must_fail git verify-commit $HASH8 &&\n+\tgit replace -d $HASH8\n+'\n+\n test_done\n-- \n2.0.0.421.g786a89d.dirty\n"},{"id":"245352","messageId":"xmqqy4wbfpux.fsf@gitster.dls.corp.google.com","threadId":"37018","inReplyTo":"20140628181117.5687.23923.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 2/7] replace: add test for --graft","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-02T20:49:26Z","receivedAt":"2014-07-02T20:49: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> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  t/t6050-replace.sh | 12 ++++++++++++\n>  1 file changed, 12 insertions(+)\n>\n> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\n> index 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\nPlease do not run non-git command under test_must_fail.\n\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\nFor all these \"git cat-file -p $commit | grep ...\", I would think\nyou should add \"commit_has_parents\" helper function to allow a bit\nmore careful testing.  As written, the test will mistake a commit\nwith string \"parent $HASHx\" anywhere, not in the header part, and\nthe test does not ensure that $HASH{3,4} is the {first,second}\nparent.\n\n\tcommit_has_parents $HASH5 $HASH4 $HASH3\n\nwould then may be implemented like:\n\n\tcommit_has_parents () {\n\t\tgit cat-file commit \"$1\" >payload &&\n                sed -n -e '/^$/q' -e 's/^parent //p' <payload >actual &&\n\t\tshift &&\n                printf 'parent %s\\n' \"$@\" >expect &&\n\t\ttest_cmp expect actual\n\t}\n"},{"id":"245353","messageId":"xmqqtx6zfp3m.fsf@gitster.dls.corp.google.com","threadId":"37018","inReplyTo":"20140628181117.5687.43446.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 5/7] replace: refactor replacing parents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-02T21:05:49Z","receivedAt":"2014-07-02T21:05:49Z","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> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n>  builtin/replace.c | 42 +++++++++++++++++++++++++-----------------\n>  1 file changed, 25 insertions(+), 17 deletions(-)\n>\n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index 3515979..ad47237 100644\n> --- a/builtin/replace.c\n> +++ b/builtin/replace.c\n> @@ -295,27 +295,14 @@ 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 create_graft(int argc, const char **argv, int force)\n> +static void replace_parents(struct strbuf *buf, int argc, const char **argv)\n\nIt is somewhat strange to see that a new function introduced earlier\nin the series is rewritten with a \"refactoring\".  Shouldn't the new\nfunction have been done right from the beginning instead?\n\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> -\tconst char *buffer;\n> -\tunsigned long size;\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> -\n>  \t/* find existing parents */\n> -\tbuffer = get_commit_buffer(commit, &size);\n> -\tstrbuf_add(&buf, buffer, size);\n> -\tunuse_commit_buffer(commit, buffer);\n> -\tparent_start = buf.buf;\n> +\tparent_start = buf->buf;\n>  \tparent_start += 46; /* \"tree \" + \"hex sha1\" + \"\\n\" */\n>  \tparent_end = parent_start;\n>  \n> @@ -332,13 +319,34 @@ static int create_graft(int argc, const char **argv, int force)\n>  \t}\n>  \n>  \t/* replace existing parents with new ones */\n> -\tstrbuf_splice(&buf, parent_start - buf.buf, parent_end - parent_start,\n> +\tstrbuf_splice(buf, parent_start - buf->buf, parent_end - parent_start,\n>  \t\t      new_parents.buf, new_parents.len);\n>  \n> +\tstrbuf_release(&new_parents);\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> +\tconst char *buffer;\n> +\tunsigned long size;\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> +\n> +\tbuffer = get_commit_buffer(commit, &size);\n> +\tstrbuf_add(&buf, buffer, size);\n> +\tunuse_commit_buffer(commit, buffer);\n> +\n> +\treplace_parents(&buf, argc, argv);\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"},{"id":"245354","messageId":"xmqqpphnfogx.fsf@gitster.dls.corp.google.com","threadId":"37018","inReplyTo":"20140628181117.5687.38249.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 6/7] replace: remove signature when using --graft","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-02T21:19:26Z","receivedAt":"2014-07-02T21:19: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> It could be misleading to keep a signature in a\n> replacement commit, so let's remove it.\n>\n> Note that there should probably be a way to sign\n> the replacement commit created when using --graft,\n> but this can be dealt with in another commit or\n> patch series.\n\nBoth paragraphs read very sensibly.\n\n>\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n>  builtin/replace.c |  5 +++++\n>  commit.c          | 34 ++++++++++++++++++++++++++++++++++\n>  commit.h          |  2 ++\n>  3 files changed, 41 insertions(+)\n>\n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index ad47237..000db65 100644\n> --- a/builtin/replace.c\n> +++ b/builtin/replace.c\n> @@ -344,6 +344,11 @@ static int create_graft(int argc, const char **argv, int force)\n>  \n>  \treplace_parents(&buf, argc, argv);\n>  \n> +\tif (remove_signature(&buf))\n> +\t\twarning(_(\"the original commit '%s' has a gpg signature.\\n\"\n> +\t\t\t  \"It will be removed in the replacement commit!\"),\n\nHmmm...  does the second line of this message start with the usual\n\"warning:\" prefix?\n\n> diff --git a/commit.c b/commit.c\n> index fb7897c..54e157d 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -1177,6 +1177,40 @@ int parse_signed_commit(const struct commit *commit,\n>  \treturn saw_signature;\n>  }\n>  \n> +int remove_signature(struct strbuf *buf)\n> +{\n> +\tconst char *line = buf->buf;\n> +\tconst char *tail = buf->buf + buf->len;\n> +\tint in_signature = 0;\n> +\tconst char *sig_start = NULL;\n> +\tconst char *sig_end = NULL;\n> +\n> +\twhile (line < tail) {\n> +\t\tconst char *next = memchr(line, '\\n', tail - line);\n> +\t\tnext = next ? next + 1 : tail;\n\nThis almost makes me wonder if we want something similar to\nstrchrnul() we use for NUL-terminated strings, and I suspect that\nyou would find more instances by running \"git grep -A2 memchr\".\n\nI don't know what such a helper function should be named, though.\nCertainly not \"memchrnul()\".\n\n> +\n> +\t\tif (in_signature && line[0] == ' ')\n> +\t\t\tsig_end = next;\n> +\t\telse if (starts_with(line, gpg_sig_header) &&\n> +\t\t\t line[gpg_sig_header_len] == ' ') {\n> +\t\t\tsig_start = line;\n> +\t\t\tsig_end = next;\n> +\t\t\tin_signature = 1;\n> +\t\t} else {\n> +\t\t\tif (*line == '\\n')\n> +\t\t\t\t/* dump the whole remainder of the buffer */\n> +\t\t\t\tnext = tail;\n> +\t\t\tin_signature = 0;\n> +\t\t}\n> +\t\tline = next;\n> +\t}\n> +\n> +\tif (sig_start)\n> +\t\tstrbuf_remove(buf, sig_start - buf->buf, sig_end - sig_start);\n\nIf there are two instances of gpg_sig, this will remove only the\nlast one, but there is no chance both signatures of such a commit\ncan validate OK, and we won't be losing something in between anyway,\nso it should be fine.\n\n> +\treturn sig_start != NULL;\n> +}\n> +\n>  static void handle_signed_tag(struct commit *parent, struct commit_extra_header ***tail)\n>  {\n>  \tstruct merge_remote_desc *desc;\n> diff --git a/commit.h b/commit.h\n> index 2e1492a..4234dae 100644\n> --- a/commit.h\n> +++ b/commit.h\n> @@ -327,6 +327,8 @@ struct commit *get_merge_parent(const char *name);\n>  \n>  extern int parse_signed_commit(const struct commit *commit,\n>  \t\t\t       struct strbuf *message, struct strbuf *signature);\n> +extern int remove_signature(struct strbuf *buf);\n> +\n>  extern void print_commit_list(struct commit_list *list,\n>  \t\t\t      const char *format_cur,\n>  \t\t\t      const char *format_last);\n"},{"id":"245355","messageId":"xmqqlhsbfocm.fsf@gitster.dls.corp.google.com","threadId":"37018","inReplyTo":"20140628181117.5687.83337.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 7/7] replace: add test for --graft with signed commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-02T21:22:01Z","receivedAt":"2014-07-02T21:22:01Z","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> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n>  t/t6050-replace.sh | 22 ++++++++++++++++++++++\n>  1 file changed, 22 insertions(+)\n>\n> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\n> index ca45a84..80b85e3 100755\n> --- a/t/t6050-replace.sh\n> +++ b/t/t6050-replace.sh\n> @@ -7,6 +7,7 @@ test_description='Tests replace refs functionality'\n>  exec </dev/null\n>  \n>  . ./test-lib.sh\n> +. \"$TEST_DIRECTORY/lib-gpg.sh\"\n>  \n>  add_and_commit_file()\n>  {\n> @@ -363,4 +364,25 @@ test_expect_success '--graft with and without already replaced object' '\n>  \tgit replace -d $HASH5\n>  '\n>  \n> +test_expect_success GPG 'set up a signed commit' '\n> +\techo \"line 17\" >> hello &&\n> +\techo \"line 18\" >> hello &&\n\nStyle?\n\n> +\tgit add hello &&\n> +\ttest_tick &&\n> +\tgit commit --quiet -S -m \"hello: 2 more lines in a signed commit\" &&\n> +\tHASH8=$(git rev-parse --verify HEAD) &&\n> +\tgit verify-commit $HASH8\n> +'\n> +\n> +test_expect_success GPG '--graft with a signed commit' '\n> +\tgit cat-file commit $HASH8 >orig &&\n> +\tgit replace --graft $HASH8 &&\n> +\tgit cat-file commit $HASH8 >repl &&\n> +\ttest_must_fail grep gpgsig repl &&\n> +\tdiff -u orig repl | grep \"^-parent $HASH7\" &&\n> +\tdiff -u orig repl | grep \"^-gpgsig -----BEGIN PGP SIGNATURE-----\" &&\n\nAlmost the same comment as the one for 2/7 applies here.\n\n> +\ttest_must_fail git verify-commit $HASH8 &&\n> +\tgit replace -d $HASH8\n> +'\n> +\n>  test_done\n"},{"id":"245371","messageId":"CAP8UFD2L8wV=7dyW6ChA2Y1UddQnLMZ=b4eUvGYGQY65ndLgHA@mail.gmail.com","threadId":"37018","inReplyTo":"xmqqy4wbfpux.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 2/7] replace: add test for --graft","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2014-07-03T13:39:09Z","receivedAt":"2014-07-03T13:39:09Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Jul 2, 2014 at 10:49 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Christian Couder <chriscool@tuxfamily.org> writes:\n>\n>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n>> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n>> ---\n>>  t/t6050-replace.sh | 12 ++++++++++++\n>>  1 file changed, 12 insertions(+)\n>>\n>> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\n>> index 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>>       test -z \"$(git replace)\"\n>>  '\n>>\n>> +test_expect_success '--graft with and without already replaced object' '\n>> +     test $(git log --oneline | wc -l) = 7 &&\n>> +     git replace --graft $HASH5 &&\n>> +     test $(git log --oneline | wc -l) = 3 &&\n>> +     git cat-file -p $HASH5 | test_must_fail grep parent &&\n>\n> Please do not run non-git command under test_must_fail.\n\nOk, I think I will just use the following then:\n\ntest_must_fail git rev-parse $HASH5^1\n\n>> +     test_must_fail git replace --graft $HASH5 $HASH4 $HASH3 &&\n>> +     git replace --force -g $HASH5 $HASH4 $HASH3 &&\n>> +     git cat-file -p $HASH5 | grep \"parent $HASH4\" &&\n>> +     git cat-file -p $HASH5 | grep \"parent $HASH3\" &&\n>> +     git replace -d $HASH5\n>> +'\n>> +\n>>  test_done\n>\n> For all these \"git cat-file -p $commit | grep ...\", I would think\n> you should add \"commit_has_parents\" helper function to allow a bit\n> more careful testing.  As written, the test will mistake a commit\n> with string \"parent $HASHx\" anywhere, not in the header part, and\n> the test does not ensure that $HASH{3,4} is the {first,second}\n> parent.\n>\n>         commit_has_parents $HASH5 $HASH4 $HASH3\n>\n> would then may be implemented like:\n>\n>         commit_has_parents () {\n>                 git cat-file commit \"$1\" >payload &&\n>                 sed -n -e '/^$/q' -e 's/^parent //p' <payload >actual &&\n>                 shift &&\n>                 printf 'parent %s\\n' \"$@\" >expect &&\n>                 test_cmp expect actual\n>         }\n\nI think I'll rather use something like:\n\ntest $(git rev-parse $HASH5^1) = \"$HASH4\" &&\ntest $(git rev-parse $HASH5^2) = \"$HASH3\" &&\n...\n\nThanks,\nChristian.\n"},{"id":"245372","messageId":"CAP8UFD3wSxYVuwSLApQikEbXgsvU2H=6PQ-tmvRjdBinDNmGVA@mail.gmail.com","threadId":"37018","inReplyTo":"xmqqtx6zfp3m.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 5/7] replace: refactor replacing parents","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2014-07-03T13:42:34Z","receivedAt":"2014-07-03T13:42:34Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Jul 2, 2014 at 11:05 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Christian Couder <chriscool@tuxfamily.org> writes:\n>\n>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n>> ---\n>>  builtin/replace.c | 42 +++++++++++++++++++++++++-----------------\n>>  1 file changed, 25 insertions(+), 17 deletions(-)\n>>\n>> diff --git a/builtin/replace.c b/builtin/replace.c\n>> index 3515979..ad47237 100644\n>> --- a/builtin/replace.c\n>> +++ b/builtin/replace.c\n>> @@ -295,27 +295,14 @@ static int edit_and_replace(const char *object_ref, int force)\n>>       return replace_object_sha1(object_ref, old, \"replacement\", new, force);\n>>  }\n>>\n>> -static int create_graft(int argc, const char **argv, int force)\n>> +static void replace_parents(struct strbuf *buf, int argc, const char **argv)\n>\n> It is somewhat strange to see that a new function introduced earlier\n> in the series is rewritten with a \"refactoring\".  Shouldn't the new\n> function have been done right from the beginning instead?\n\nYeah, I will do it right from the beginning in the next patch series.\n"},{"id":"245374","messageId":"CAP8UFD1DHQf9PuSMVCcsgqJ2xAOxPBEk3S3+iOZRmAY=XmDqaw@mail.gmail.com","threadId":"37018","inReplyTo":"xmqqpphnfogx.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 6/7] replace: remove signature when using --graft","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2014-07-03T14:09:42Z","receivedAt":"2014-07-03T14:09:42Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Jul 2, 2014 at 11:19 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Christian Couder <chriscool@tuxfamily.org> writes:\n>\n>> It could be misleading to keep a signature in a\n>> replacement commit, so let's remove it.\n>>\n>> Note that there should probably be a way to sign\n>> the replacement commit created when using --graft,\n>> but this can be dealt with in another commit or\n>> patch series.\n>\n> Both paragraphs read very sensibly.\n\nThanks.\n\n>> --- a/builtin/replace.c\n>> +++ b/builtin/replace.c\n>> @@ -344,6 +344,11 @@ static int create_graft(int argc, const char **argv, int force)\n>>\n>>       replace_parents(&buf, argc, argv);\n>>\n>> +     if (remove_signature(&buf))\n>> +             warning(_(\"the original commit '%s' has a gpg signature.\\n\"\n>> +                       \"It will be removed in the replacement commit!\"),\n>\n> Hmmm...  does the second line of this message start with the usual\n> \"warning:\" prefix?\n\nOk, I will use following:\n\nif (remove_signature(&buf)) {\n        warning(_(\"the original commit '%s' has a gpg signature.\"), old_ref);\n        warning(_(\"the signature will be removed in the replacement commit!\"));\n}\n\n>> diff --git a/commit.c b/commit.c\n>> index fb7897c..54e157d 100644\n>> --- a/commit.c\n>> +++ b/commit.c\n>> @@ -1177,6 +1177,40 @@ int parse_signed_commit(const struct commit *commit,\n>>       return saw_signature;\n>>  }\n>>\n>> +int remove_signature(struct strbuf *buf)\n>> +{\n>> +     const char *line = buf->buf;\n>> +     const char *tail = buf->buf + buf->len;\n>> +     int in_signature = 0;\n>> +     const char *sig_start = NULL;\n>> +     const char *sig_end = NULL;\n>> +\n>> +     while (line < tail) {\n>> +             const char *next = memchr(line, '\\n', tail - line);\n>> +             next = next ? next + 1 : tail;\n>\n> This almost makes me wonder if we want something similar to\n> strchrnul() we use for NUL-terminated strings, and I suspect that\n> you would find more instances by running \"git grep -A2 memchr\".\n>\n> I don't know what such a helper function should be named, though.\n> Certainly not \"memchrnul()\".\n\nI can add this to a GSoC microproject page for next year.\n\n>> +             if (in_signature && line[0] == ' ')\n>> +                     sig_end = next;\n>> +             else if (starts_with(line, gpg_sig_header) &&\n>> +                      line[gpg_sig_header_len] == ' ') {\n>> +                     sig_start = line;\n>> +                     sig_end = next;\n>> +                     in_signature = 1;\n>> +             } else {\n>> +                     if (*line == '\\n')\n>> +                             /* dump the whole remainder of the buffer */\n>> +                             next = tail;\n>> +                     in_signature = 0;\n>> +             }\n>> +             line = next;\n>> +     }\n>> +\n>> +     if (sig_start)\n>> +             strbuf_remove(buf, sig_start - buf->buf, sig_end - sig_start);\n>\n> If there are two instances of gpg_sig, this will remove only the\n> last one, but there is no chance both signatures of such a commit\n> can validate OK, and we won't be losing something in between anyway,\n> so it should be fine.\n\nOk.\n\nThanks,\nChristian.\n"},{"id":"245375","messageId":"CAP8UFD2ddGr=cHSv06chD402k1dYrcQ=mxxWP+EHs48zE5twuA@mail.gmail.com","threadId":"37018","inReplyTo":"xmqqlhsbfocm.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 7/7] replace: add test for --graft with signed commit","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2014-07-03T14:17:20Z","receivedAt":"2014-07-03T14:17:20Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Jul 2, 2014 at 11:22 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Christian Couder <chriscool@tuxfamily.org> writes:\n>\n>>\n>> +test_expect_success GPG 'set up a signed commit' '\n>> +     echo \"line 17\" >> hello &&\n>> +     echo \"line 18\" >> hello &&\n>\n> Style?\n\nYeah, I will change it to:\n\n     echo \"line 17\" >>hello &&\n     echo \"line 18\" >>hello &&\n\n>> +     git add hello &&\n>> +     test_tick &&\n>> +     git commit --quiet -S -m \"hello: 2 more lines in a signed commit\" &&\n>> +     HASH8=$(git rev-parse --verify HEAD) &&\n>> +     git verify-commit $HASH8\n>> +'\n>> +\n>> +test_expect_success GPG '--graft with a signed commit' '\n>> +     git cat-file commit $HASH8 >orig &&\n>> +     git replace --graft $HASH8 &&\n>> +     git cat-file commit $HASH8 >repl &&\n>> +     test_must_fail grep gpgsig repl &&\n>> +     diff -u orig repl | grep \"^-parent $HASH7\" &&\n>> +     diff -u orig repl | grep \"^-gpgsig -----BEGIN PGP SIGNATURE-----\" &&\n>\n> Almost the same comment as the one for 2/7 applies here.\n\nOk, I will find a way to make it better.\n\nThanks,\nChristian.\n"},{"id":"245385","messageId":"xmqq8uoaffhq.fsf@gitster.dls.corp.google.com","threadId":"37018","inReplyTo":"CAP8UFD2L8wV=7dyW6ChA2Y1UddQnLMZ=b4eUvGYGQY65ndLgHA@mail.gmail.com","subject":"Re: [PATCH v5 2/7] replace: add test for --graft","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-03T18:45:37Z","receivedAt":"2014-07-03T18:45:37Z","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 Wed, Jul 2, 2014 at 10:49 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Christian Couder <chriscool@tuxfamily.org> writes:\n>>\n>>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n>>> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n>>> ---\n>>>  t/t6050-replace.sh | 12 ++++++++++++\n>>>  1 file changed, 12 insertions(+)\n>>>\n>>> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\n>>> index 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>>>       test -z \"$(git replace)\"\n>>>  '\n>>>\n>>> +test_expect_success '--graft with and without already replaced object' '\n>>> +     test $(git log --oneline | wc -l) = 7 &&\n>>> +     git replace --graft $HASH5 &&\n>>> +     test $(git log --oneline | wc -l) = 3 &&\n>>> +     git cat-file -p $HASH5 | test_must_fail grep parent &&\n>>\n>> Please do not run non-git command under test_must_fail.\n>\n> Ok, I think I will just use the following then:\n>\n> test_must_fail git rev-parse $HASH5^1\n> ...\n\nSee below.\n\n>>> +     test_must_fail git replace --graft $HASH5 $HASH4 $HASH3 &&\n>>> +     git replace --force -g $HASH5 $HASH4 $HASH3 &&\n>>> +     git cat-file -p $HASH5 | grep \"parent $HASH4\" &&\n>>> +     git cat-file -p $HASH5 | grep \"parent $HASH3\" &&\n>>> +     git replace -d $HASH5\n>>> +'\n>>> +\n>>>  test_done\n>>\n>> For all these \"git cat-file -p $commit | grep ...\", I would think\n>> you should add \"commit_has_parents\" helper function to allow a bit\n>> more careful testing.  As written, the test will mistake a commit\n>> with string \"parent $HASHx\" anywhere, not in the header part, and\n>> the test does not ensure that $HASH{3,4} is the {first,second}\n>> parent.\n>>\n>>         commit_has_parents $HASH5 $HASH4 $HASH3\n>>\n>> would then may be implemented like:\n>>\n>>         commit_has_parents () {\n>>                 git cat-file commit \"$1\" >payload &&\n>>                 sed -n -e '/^$/q' -e 's/^parent //p' <payload >actual &&\n>>                 shift &&\n>>                 printf 'parent %s\\n' \"$@\" >expect &&\n>>                 test_cmp expect actual\n>>         }\n>\n> I think I'll rather use something like:\n>\n> test $(git rev-parse $HASH5^1) = \"$HASH4\" &&\n> test $(git rev-parse $HASH5^2) = \"$HASH3\" &&\n> ...\n\nEven in that case, I'd suggest using the same \"commit_has_parents\"\nabstraction, which you will also be using to check the \"replaced to\nbe a new root\" case in the earlier part of this test.\n\nIn case you do not get what I mean by \"in that case\", you are saying\nthat you will now be testing a different thing.  You can test what\nyour new code produces at the bit level by directly obtaining the\nresulting object via \"cat-file\" and that lets you not to depend on\nthe rest of the system (i.e. the part that allows you to pretend an\nexisting object you have a corresponding replace ref for has contents\nof a totally different object) working correctly.  Or you can choose\nto test the system as a whole (i.e. not just the \"git replace\" produced\na new object with contents you planned to fabricate, but also by\nhaving a replace ref, you can let the rest of the system use th\ncontents of that object when asked for the replaced object).\n\nThe implementation suggested in my previous message was in line with\nthe former, because your use of \"cat-file\" seemed to indicate that\nyou wanted to avoid depending on the rest of the system to test this\nnew feature in your new tests.  You seem to be saying that you now\nwant to take the other approach of testing both at the same time.\n\nI am fine with either approach, but I want to make sure that you are\naware of the distinction.  The last thing I want to see is to flip\nthe approach you take to test not because \"testing as a whole is\nbetter than testing one thing without getting interfered by\npotential breakage in other parts\" but because \"testing as a whole\nis easier.\"\n\nThe implementation of commit_has_parents that tests the system as a\nwhole may look like\n\n        commit=$1 parent_number=1\n        shift\n        for parent\n        do\n                test $(git rev-parse --verify \"$commit^$parent_number\") = \"$parent\" ||\n\t\treturn 1\n\t\tparent_number=$(( $parent_number + 1 ))\n\tdone\n        test_must_fail git rev-parse $commit^$parent_number\n\nand you would still use it like this:\n\n  commit_has_parents $HASH5 ;# must have no parent\n  commit_has_parents $HASH5 $HASH4 $HASH3 ;# must have these parents\n\nThanks.\n"}]}