{"thread":{"id":"37068","subject":"[PATCH v6 00/10] Add --graft option to git replace","startedAt":"2014-07-07T06:35:29Z","lastAt":"2014-07-11T18:29:02Z","messageCount":26,"participants":["Christian Couder","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":6,"patchTotal":10},"messages":[{"id":"245454","messageId":"20140707063342.3708.83493.chriscool@tuxfamily.org","threadId":"37068","inReplyTo":null,"subject":"[PATCH v6 00/10] Add --graft option to git replace","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-07-07T06:35:29Z","receivedAt":"2014-07-07T06:35:29Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Here is a small 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 v5, thanks to Junio, are:\n\n- new patch 1/10 to clean up redirection style in t6050\n\n- new patches 8/10, 9/10 and 10/10 to check mergetags\n\n- add functions to test parents in patch 3/10 and 7/10\n\n- improve testing signed commits in patch 7/10\n\n- improve warning when removing commit signature in\n  patch 6/10\n\nChristian Couder (10):\n  replace: cleanup redirection style in tests\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: remove signature when using --graft\n  replace: add test for --graft with signed commit\n  commit: add for_each_mergetag()\n  replace: check mergetags when using --graft\n  replace: add test for --graft with a mergetag\n\n Documentation/git-replace.txt             |  10 +++\n builtin/replace.c                         | 126 +++++++++++++++++++++++++++-\n commit.c                                  |  47 +++++++++++\n commit.h                                  |   7 ++\n contrib/convert-grafts-to-replace-refs.sh |  28 +++++++\n log-tree.c                                |  15 +---\n t/t6050-replace.sh                        | 135 ++++++++++++++++++++++++------\n 7 files changed, 332 insertions(+), 36 deletions(-)\n create mode 100755 contrib/convert-grafts-to-replace-refs.sh\n\n-- \n2.0.0.421.g786a89d.dirty\n"},{"id":"245458","messageId":"20140707063540.3708.19216.chriscool@tuxfamily.org","threadId":"37068","inReplyTo":"20140707063342.3708.83493.chriscool@tuxfamily.org","subject":"[PATCH v6 01/10] replace: cleanup redirection style in tests","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-07-07T06:35:30Z","receivedAt":"2014-07-07T06:35:30Z","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 | 48 ++++++++++++++++++++++++------------------------\n 1 file changed, 24 insertions(+), 24 deletions(-)\n\ndiff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\nindex 68b3cb2..fb07ad2 100755\n--- a/t/t6050-replace.sh\n+++ b/t/t6050-replace.sh\n@@ -27,36 +27,36 @@ HASH6=\n HASH7=\n \n test_expect_success 'set up buggy branch' '\n-     echo \"line 1\" >> hello &&\n-     echo \"line 2\" >> hello &&\n-     echo \"line 3\" >> hello &&\n-     echo \"line 4\" >> hello &&\n+     echo \"line 1\" >>hello &&\n+     echo \"line 2\" >>hello &&\n+     echo \"line 3\" >>hello &&\n+     echo \"line 4\" >>hello &&\n      add_and_commit_file hello \"4 lines\" &&\n      HASH1=$(git rev-parse --verify HEAD) &&\n-     echo \"line BUG\" >> hello &&\n-     echo \"line 6\" >> hello &&\n-     echo \"line 7\" >> hello &&\n-     echo \"line 8\" >> hello &&\n+     echo \"line BUG\" >>hello &&\n+     echo \"line 6\" >>hello &&\n+     echo \"line 7\" >>hello &&\n+     echo \"line 8\" >>hello &&\n      add_and_commit_file hello \"4 more lines with a BUG\" &&\n      HASH2=$(git rev-parse --verify HEAD) &&\n-     echo \"line 9\" >> hello &&\n-     echo \"line 10\" >> hello &&\n+     echo \"line 9\" >>hello &&\n+     echo \"line 10\" >>hello &&\n      add_and_commit_file hello \"2 more lines\" &&\n      HASH3=$(git rev-parse --verify HEAD) &&\n-     echo \"line 11\" >> hello &&\n+     echo \"line 11\" >>hello &&\n      add_and_commit_file hello \"1 more line\" &&\n      HASH4=$(git rev-parse --verify HEAD) &&\n-     sed -e \"s/BUG/5/\" hello > hello.new &&\n+     sed -e \"s/BUG/5/\" hello >hello.new &&\n      mv hello.new hello &&\n      add_and_commit_file hello \"BUG fixed\" &&\n      HASH5=$(git rev-parse --verify HEAD) &&\n-     echo \"line 12\" >> hello &&\n-     echo \"line 13\" >> hello &&\n+     echo \"line 12\" >>hello &&\n+     echo \"line 13\" >>hello &&\n      add_and_commit_file hello \"2 more lines\" &&\n      HASH6=$(git rev-parse --verify HEAD) &&\n-     echo \"line 14\" >> hello &&\n-     echo \"line 15\" >> hello &&\n-     echo \"line 16\" >> hello &&\n+     echo \"line 14\" >>hello &&\n+     echo \"line 15\" >>hello &&\n+     echo \"line 16\" >>hello &&\n      add_and_commit_file hello \"again 3 more lines\" &&\n      HASH7=$(git rev-parse --verify HEAD)\n '\n@@ -95,7 +95,7 @@ test_expect_success 'tag replaced commit' '\n '\n \n test_expect_success '\"git fsck\" works' '\n-     git fsck master > fsck_master.out &&\n+     git fsck master >fsck_master.out &&\n      grep \"dangling commit $R\" fsck_master.out &&\n      grep \"dangling tag $(cat .git/refs/tags/mytag)\" fsck_master.out &&\n      test -z \"$(git fsck)\"\n@@ -217,14 +217,14 @@ test_expect_success 'fetch branch with replacement' '\n      (\n \t  cd clone_dir &&\n \t  git fetch origin refs/heads/tofetch:refs/heads/parallel3 &&\n-\t  git log --pretty=oneline parallel3 > output.txt &&\n+\t  git log --pretty=oneline parallel3 >output.txt &&\n \t  ! grep $PARA3 output.txt &&\n-\t  git show $PARA3 > para3.txt &&\n+\t  git show $PARA3 >para3.txt &&\n \t  grep \"A U Thor\" para3.txt &&\n \t  git fetch origin \"refs/replace/*:refs/replace/*\" &&\n-\t  git log --pretty=oneline parallel3 > output.txt &&\n+\t  git log --pretty=oneline parallel3 >output.txt &&\n \t  grep $PARA3 output.txt &&\n-\t  git show $PARA3 > para3.txt &&\n+\t  git show $PARA3 >para3.txt &&\n \t  grep \"O Thor\" para3.txt\n      )\n '\n@@ -302,7 +302,7 @@ test_expect_success 'test --format medium' '\n \t\techo \"$PARA3 -> $S\" &&\n \t\techo \"$MYTAG -> $HASH1\"\n \t} | sort >expected &&\n-\tgit replace -l --format medium | sort > actual &&\n+\tgit replace -l --format medium | sort >actual &&\n \ttest_cmp expected actual\n '\n \n@@ -314,7 +314,7 @@ test_expect_success 'test --format long' '\n \t\techo \"$PARA3 (commit) -> $S (commit)\" &&\n \t\techo \"$MYTAG (tag) -> $HASH1 (commit)\"\n \t} | sort >expected &&\n-\tgit replace --format=long | sort > actual &&\n+\tgit replace --format=long | sort >actual &&\n \ttest_cmp expected actual\n '\n \n-- \n2.0.0.421.g786a89d.dirty\n"},{"id":"245455","messageId":"20140707063540.3708.51047.chriscool@tuxfamily.org","threadId":"37068","inReplyTo":"20140707063342.3708.83493.chriscool@tuxfamily.org","subject":"[PATCH v6 02/10] replace: add --graft option","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-07-07T06:35:31Z","receivedAt":"2014-07-07T06:35:31Z","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 | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 73 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 1bb491d..ad47237 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,66 @@ 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 void replace_parents(struct strbuf *buf, int argc, const char **argv)\n+{\n+\tstruct strbuf new_parents = STRBUF_INIT;\n+\tconst char *parent_start, *parent_end;\n+\tint i;\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+\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(&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 +364,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 +388,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 +414,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":"245461","messageId":"20140707063540.3708.9611.chriscool@tuxfamily.org","threadId":"37068","inReplyTo":"20140707063342.3708.83493.chriscool@tuxfamily.org","subject":"[PATCH v6 03/10] replace: add test for --graft","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-07-07T06:35:32Z","receivedAt":"2014-07-07T06:35:32Z","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 | 40 ++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 40 insertions(+)\n\ndiff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\nindex fb07ad2..d80a89e 100755\n--- a/t/t6050-replace.sh\n+++ b/t/t6050-replace.sh\n@@ -18,6 +18,33 @@ add_and_commit_file()\n     git commit --quiet -m \"$_file: $_msg\"\n }\n \n+commit_buffer_contains_parents()\n+{\n+    git cat-file commit \"$1\" >payload &&\n+    sed -n -e '/^$/q' -e '/^parent /p' <payload >actual &&\n+    shift &&\n+    : >expected &&\n+    for _parent\n+    do\n+\techo \"parent $_parent\" >>expected || return 1\n+    done &&\n+    test_cmp actual expected\n+}\n+\n+commit_has_parents()\n+{\n+    _parent_number=1\n+    _commit=\"$1\"\n+    shift &&\n+    for _parent\n+    do\n+\t_found=$(git rev-parse --verify $_commit^$_parent_number) || return 1\n+\ttest \"$_found\" = \"$_parent\" || return 1\n+\t_parent_number=$(( $_parent_number + 1 ))\n+    done &&\n+    test_must_fail git rev-parse --verify $_commit^$_parent_number\n+}\n+\n HASH1=\n HASH2=\n HASH3=\n@@ -351,4 +378,17 @@ 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+\tcommit_buffer_contains_parents $HASH5 &&\n+\tcommit_has_parents $HASH5 &&\n+\ttest_must_fail git replace --graft $HASH5 $HASH4 $HASH3 &&\n+\tgit replace --force -g $HASH5 $HASH4 $HASH3 &&\n+\tcommit_buffer_contains_parents $HASH5 $HASH4 $HASH3 &&\n+\tcommit_has_parents $HASH5 $HASH4 $HASH3 &&\n+\tgit replace -d $HASH5\n+'\n+\n test_done\n-- \n2.0.0.421.g786a89d.dirty\n"},{"id":"245463","messageId":"20140707063540.3708.57293.chriscool@tuxfamily.org","threadId":"37068","inReplyTo":"20140707063342.3708.83493.chriscool@tuxfamily.org","subject":"[PATCH v6 04/10] Documentation: replace: add --graft option","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-07-07T06:35:33Z","receivedAt":"2014-07-07T06:35:33Z","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":"245460","messageId":"20140707063540.3708.70204.chriscool@tuxfamily.org","threadId":"37068","inReplyTo":"20140707063342.3708.83493.chriscool@tuxfamily.org","subject":"[PATCH v6 05/10] contrib: add convert-grafts-to-replace-refs.sh","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-07-07T06:35:34Z","receivedAt":"2014-07-07T06:35:34Z","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":"245456","messageId":"20140707063540.3708.20173.chriscool@tuxfamily.org","threadId":"37068","inReplyTo":"20140707063342.3708.83493.chriscool@tuxfamily.org","subject":"[PATCH v6 06/10] replace: remove signature when using --graft","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-07-07T06:35:35Z","receivedAt":"2014-07-07T06:35:35Z","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..cc29ef2 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.\"), old_ref);\n+\t\twarning(_(\"the signature will be removed in the replacement commit!\"));\n+\t}\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":"245459","messageId":"20140707063540.3708.33700.chriscool@tuxfamily.org","threadId":"37068","inReplyTo":"20140707063342.3708.83493.chriscool@tuxfamily.org","subject":"[PATCH v6 07/10] replace: add test for --graft with signed commit","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-07-07T06:35:36Z","receivedAt":"2014-07-07T06:35:36Z","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 | 25 +++++++++++++++++++++++++\n 1 file changed, 25 insertions(+)\n\ndiff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\nindex d80a89e..15fd541 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@@ -391,4 +392,28 @@ 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+\tcommit_buffer_contains_parents $HASH8 &&\n+\tcommit_has_parents $HASH8 &&\n+\ttest_must_fail git verify-commit $HASH8 &&\n+\tsed -n -e \"/^tree /p\" -e \"/^author /p\" -e \"/^committer /p\" orig >expected &&\n+\techo >>expected &&\n+\tsed -e \"/^$/q\" repl >actual &&\n+\ttest_cmp expected actual &&\n+\tgit replace -d $HASH8\n+'\n+\n test_done\n-- \n2.0.0.421.g786a89d.dirty\n"},{"id":"245457","messageId":"20140707063540.3708.39506.chriscool@tuxfamily.org","threadId":"37068","inReplyTo":"20140707063342.3708.83493.chriscool@tuxfamily.org","subject":"[PATCH v6 08/10] commit: add for_each_mergetag()","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-07-07T06:35:37Z","receivedAt":"2014-07-07T06:35:37Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"In the same way as there is for_each_ref() to\niterate on refs, it might be useful to have\nfor_each_mergetag() to iterate on the mergetags\nof a given commit.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n commit.c   | 13 +++++++++++++\n commit.h   |  5 +++++\n log-tree.c | 15 ++++-----------\n 3 files changed, 22 insertions(+), 11 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 54e157d..0f83ff7 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1348,6 +1348,19 @@ struct commit_extra_header *read_commit_extra_headers(struct commit *commit,\n \treturn extra;\n }\n \n+void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data)\n+{\n+\tstruct commit_extra_header *extra, *to_free;\n+\n+\tto_free = read_commit_extra_headers(commit, NULL);\n+\tfor (extra = to_free; extra; extra = extra->next) {\n+\t\tif (strcmp(extra->key, \"mergetag\"))\n+\t\t\tcontinue; /* not a merge tag */\n+\t\tfn(commit, extra, data);\n+\t}\n+\tfree_commit_extra_headers(to_free);\n+}\n+\n static inline int standard_header_field(const char *field, size_t len)\n {\n \treturn ((len == 4 && !memcmp(field, \"tree \", 5)) ||\ndiff --git a/commit.h b/commit.h\nindex 4234dae..c81ba85 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -312,6 +312,11 @@ extern struct commit_extra_header *read_commit_extra_headers(struct commit *, co\n \n extern void free_commit_extra_headers(struct commit_extra_header *extra);\n \n+typedef void (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra,\n+\t\t\t\t void *cb_data);\n+\n+extern void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data);\n+\n struct merge_remote_desc {\n \tstruct object *obj; /* the named object, could be a tag */\n \tconst char *name;\ndiff --git a/log-tree.c b/log-tree.c\nindex 10e6844..706ed4c 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -413,10 +413,11 @@ static int is_common_merge(const struct commit *commit)\n \t\t&& !commit->parents->next->next);\n }\n \n-static void show_one_mergetag(struct rev_info *opt,\n+static void show_one_mergetag(struct commit *commit,\n \t\t\t      struct commit_extra_header *extra,\n-\t\t\t      struct commit *commit)\n+\t\t\t      void *data)\n {\n+\tstruct rev_info *opt = (struct rev_info *)data;\n \tunsigned char sha1[20];\n \tstruct tag *tag;\n \tstruct strbuf verify_message;\n@@ -463,15 +464,7 @@ static void show_one_mergetag(struct rev_info *opt,\n \n static void show_mergetag(struct rev_info *opt, struct commit *commit)\n {\n-\tstruct commit_extra_header *extra, *to_free;\n-\n-\tto_free = read_commit_extra_headers(commit, NULL);\n-\tfor (extra = to_free; extra; extra = extra->next) {\n-\t\tif (strcmp(extra->key, \"mergetag\"))\n-\t\t\tcontinue; /* not a merge tag */\n-\t\tshow_one_mergetag(opt, extra, commit);\n-\t}\n-\tfree_commit_extra_headers(to_free);\n+\tfor_each_mergetag(show_one_mergetag, commit, opt);\n }\n \n void show_log(struct rev_info *opt)\n-- \n2.0.0.421.g786a89d.dirty\n"},{"id":"245464","messageId":"20140707063540.3708.29773.chriscool@tuxfamily.org","threadId":"37068","inReplyTo":"20140707063342.3708.83493.chriscool@tuxfamily.org","subject":"[PATCH v6 09/10] replace: check mergetags when using --graft","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-07-07T06:35:38Z","receivedAt":"2014-07-07T06:35:38Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"When using --graft, with a mergetag in the original\ncommit, we should check that the commit pointed to by\nthe mergetag is still a parent of then new commit we\ncreate, otherwise the mergetag could be misleading.\n\nIf the commit pointed to by the mergetag is no more\na parent of the new commit, we could remove the\nmergetag, but in this case there is a good chance\nthat the title or other elements of the commit might\nalso be misleading. So let's just error out and\nsuggest to use --edit instead on the commit.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n builtin/replace.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 47 insertions(+)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex cc29ef2..2290529 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -13,6 +13,7 @@\n #include \"refs.h\"\n #include \"parse-options.h\"\n #include \"run-command.h\"\n+#include \"tag.h\"\n \n static const char * const git_replace_usage[] = {\n \tN_(\"git replace [-f] <object> <replacement>\"),\n@@ -325,6 +326,50 @@ static void replace_parents(struct strbuf *buf, int argc, const char **argv)\n \tstrbuf_release(&new_parents);\n }\n \n+struct check_mergetag_data {\n+\tint argc;\n+\tconst char **argv;\n+};\n+\n+static void check_one_mergetag(struct commit *commit,\n+\t\t\t       struct commit_extra_header *extra,\n+\t\t\t       void *data)\n+{\n+\tstruct check_mergetag_data *mergetag_data = (struct check_mergetag_data *)data;\n+\tconst char *ref = mergetag_data->argv[0];\n+\tunsigned char tag_sha1[20];\n+\tstruct tag *tag;\n+\tint i;\n+\n+\thash_sha1_file(extra->value, extra->len, typename(OBJ_TAG), tag_sha1);\n+\ttag = lookup_tag(tag_sha1);\n+\tif (!tag)\n+\t\tdie(_(\"bad mergetag in commit '%s'\"), ref);\n+\tif (parse_tag_buffer(tag, extra->value, extra->len))\n+\t\tdie(_(\"malformed mergetag in commit '%s'\"), ref);\n+\n+\t/* iterate over new parents */\n+\tfor (i = 1; i < mergetag_data->argc; i++) {\n+\t\tunsigned char sha1[20];\n+\t\tif (get_sha1(mergetag_data->argv[i], sha1) < 0)\n+\t\t\tdie(_(\"Not a valid object name: '%s'\"), mergetag_data->argv[i]);\n+\t\tif (!hashcmp(tag->tagged->sha1, sha1))\n+\t\t\treturn; /* found */\n+\t}\n+\n+\tdie(_(\"original commit '%s' contains mergetag '%s' that is discarded; \"\n+\t      \"use --edit instead of --graft\"), ref, sha1_to_hex(tag_sha1));\n+}\n+\n+static void check_mergetags(struct commit *commit, int argc, const char **argv)\n+{\n+\tstruct check_mergetag_data mergetag_data;\n+\n+\tmergetag_data.argc = argc;\n+\tmergetag_data.argv = argv;\n+\tfor_each_mergetag(check_one_mergetag, commit, &mergetag_data);\n+}\n+\n static int create_graft(int argc, const char **argv, int force)\n {\n \tunsigned char old[20], new[20];\n@@ -349,6 +394,8 @@ static int create_graft(int argc, const char **argv, int force)\n \t\twarning(_(\"the signature will be removed in the replacement commit!\"));\n \t}\n \n+\tcheck_mergetags(commit, 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-- \n2.0.0.421.g786a89d.dirty\n"},{"id":"245462","messageId":"20140707063540.3708.57409.chriscool@tuxfamily.org","threadId":"37068","inReplyTo":"20140707063342.3708.83493.chriscool@tuxfamily.org","subject":"[PATCH v6 10/10] replace: add test for --graft with a mergetag","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-07-07T06:35:39Z","receivedAt":"2014-07-07T06:35:39Z","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 15fd541..3bb8d06 100755\n--- a/t/t6050-replace.sh\n+++ b/t/t6050-replace.sh\n@@ -416,4 +416,26 @@ test_expect_success GPG '--graft with a signed commit' '\n \tgit replace -d $HASH8\n '\n \n+test_expect_success GPG 'set up a merge commit with a mergetag' '\n+\tgit reset --hard HEAD &&\n+\tgit checkout -b test_branch HEAD~2 &&\n+\techo \"line 1 from test branch\" >>hello &&\n+\techo \"line 2 from test branch\" >>hello &&\n+\tgit add hello &&\n+\ttest_tick &&\n+\tgit commit -m \"hello: 2 more lines from a test branch\" &&\n+\tHASH9=$(git rev-parse --verify HEAD) &&\n+\tgit tag -s -m \"tag for testing with a mergetag\" test_tag HEAD &&\n+\tgit checkout master &&\n+\tgit merge -s ours test_tag &&\n+\tHASH10=$(git rev-parse --verify HEAD) &&\n+\tgit cat-file commit $HASH10 | grep \"^mergetag object\"\n+'\n+\n+test_expect_success GPG '--graft on a commit with a mergetag' '\n+\ttest_must_fail git replace --graft $HASH10 $HASH8^1 &&\n+\tgit replace --graft $HASH10 $HASH8^1 $HASH9 &&\n+\tgit replace -d $HASH10\n+'\n+\n test_done\n-- \n2.0.0.421.g786a89d.dirty\n"},{"id":"245498","messageId":"xmqqmwckbyri.fsf@gitster.dls.corp.google.com","threadId":"37068","inReplyTo":"20140707063540.3708.9611.chriscool@tuxfamily.org","subject":"Re: [PATCH v6 03/10] replace: add test for --graft","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-07T22:16:33Z","receivedAt":"2014-07-07T22:16:33Z","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 | 40 ++++++++++++++++++++++++++++++++++++++++\n>  1 file changed, 40 insertions(+)\n>\n> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\n> index fb07ad2..d80a89e 100755\n> --- a/t/t6050-replace.sh\n> +++ b/t/t6050-replace.sh\n> @@ -18,6 +18,33 @@ add_and_commit_file()\n>      git commit --quiet -m \"$_file: $_msg\"\n>  }\n>  \n> +commit_buffer_contains_parents()\n\nSP before (), perhaps?\n\n> +{\n> +    git cat-file commit \"$1\" >payload &&\n> +    sed -n -e '/^$/q' -e '/^parent /p' <payload >actual &&\n> +    shift &&\n> +    : >expected &&\n> +    for _parent\n> +    do\n> +\techo \"parent $_parent\" >>expected || return 1\n> +    done &&\n\nYou can redirect the entire loop to simplify the above 5 lines,\nwhich would read better, no?\n\n\tfor _parent\n        do\n\t\techo \"parent $_parent\"\n\tdone >expect\n\n> +    test_cmp actual expected\n\nAs \"test_cmp\" normally runs \"diff\", it is better to compare expect\nwith actual, not the other way around; running the test verbosely,\ni.e. \"t6050-replace.sh -v\", shows how the actual output differs from\nwhat is expected when diagnosing breakage and would be more useful\nthat way.\n\nIf your goal is to test both the object contents with this code\n*and* the overall system behaviour with the \"$child^$parent\" below,\nperhaps they should be in the same \"commit_has_parents\" function,\nwithout forcing the caller to choose between the two or call both,\nno?\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> +\tcommit_buffer_contains_parents $HASH5 &&\n> +\tcommit_has_parents $HASH5 &&\n> +\ttest_must_fail git replace --graft $HASH5 $HASH4 $HASH3 &&\n> +\tgit replace --force -g $HASH5 $HASH4 $HASH3 &&\n> +\tcommit_buffer_contains_parents $HASH5 $HASH4 $HASH3 &&\n> +\tcommit_has_parents $HASH5 $HASH4 $HASH3 &&\n> +\tgit replace -d $HASH5\n> +'\n> +\n>  test_done\n"},{"id":"245500","messageId":"xmqqion8by82.fsf@gitster.dls.corp.google.com","threadId":"37068","inReplyTo":"20140707063540.3708.29773.chriscool@tuxfamily.org","subject":"Re: [PATCH v6 09/10] replace: check mergetags when using --graft","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-07T22:28:13Z","receivedAt":"2014-07-07T22:28:13Z","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> When using --graft, with a mergetag in the original\n> commit, we should check that the commit pointed to by\n> the mergetag is still a parent of then new commit we\n> create, otherwise the mergetag could be misleading.\n>\n> If the commit pointed to by the mergetag is no more\n> a parent of the new commit, we could remove the\n> mergetag, but in this case there is a good chance\n> that the title or other elements of the commit might\n> also be misleading. So let's just error out and\n> suggest to use --edit instead on the commit.\n\nI do not quite understand the reasoning.  If you have a merge you\nearlier made with a signed tag, and then want to redo the merge with\nan updated tip of that branch (perhaps the side branch was earlier\nbased on maint but now was rebased on master), it will perfectly be\nnormal to expect that the title or other elements of the resulting\nmerge to stay the same.  Why is it a good idea to error it out?\n\nIf the argument were '\"replace --graft\" that changes the parents is\nlikely to affect merge summary message, so error out and suggest to\nuse --edit instead', regardless of the 'mergetag', I'd understand\nit, but that would essentially mean that 'replace --graft' should\nnever be used, so...\n"},{"id":"245501","messageId":"xmqqegxwby3l.fsf@gitster.dls.corp.google.com","threadId":"37068","inReplyTo":"20140707063540.3708.39506.chriscool@tuxfamily.org","subject":"Re: [PATCH v6 08/10] commit: add for_each_mergetag()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-07T22:30:54Z","receivedAt":"2014-07-07T22:30:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n> In the same way as there is for_each_ref() to\n> iterate on refs, it might be useful to have\n> for_each_mergetag() to iterate on the mergetags\n> of a given commit.\n>\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n\nHeh, \"might be useful\" is an understatement ;-) We won't apply a\nchange that \"might be useful\" very lightly, but this refactoring\nalready uses the helper in existing code, showing that it *is*\nuseful, no?\n\nLet's have this early in the series, or perhaps even independent of\nthe \"replace\" series.\n\nThanks.\n\n>  commit.c   | 13 +++++++++++++\n>  commit.h   |  5 +++++\n>  log-tree.c | 15 ++++-----------\n>  3 files changed, 22 insertions(+), 11 deletions(-)\n>\n> diff --git a/commit.c b/commit.c\n> index 54e157d..0f83ff7 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -1348,6 +1348,19 @@ struct commit_extra_header *read_commit_extra_headers(struct commit *commit,\n>  \treturn extra;\n>  }\n>  \n> +void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data)\n> +{\n> +\tstruct commit_extra_header *extra, *to_free;\n> +\n> +\tto_free = read_commit_extra_headers(commit, NULL);\n> +\tfor (extra = to_free; extra; extra = extra->next) {\n> +\t\tif (strcmp(extra->key, \"mergetag\"))\n> +\t\t\tcontinue; /* not a merge tag */\n> +\t\tfn(commit, extra, data);\n> +\t}\n> +\tfree_commit_extra_headers(to_free);\n> +}\n> +\n>  static inline int standard_header_field(const char *field, size_t len)\n>  {\n>  \treturn ((len == 4 && !memcmp(field, \"tree \", 5)) ||\n> diff --git a/commit.h b/commit.h\n> index 4234dae..c81ba85 100644\n> --- a/commit.h\n> +++ b/commit.h\n> @@ -312,6 +312,11 @@ extern struct commit_extra_header *read_commit_extra_headers(struct commit *, co\n>  \n>  extern void free_commit_extra_headers(struct commit_extra_header *extra);\n>  \n> +typedef void (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra,\n> +\t\t\t\t void *cb_data);\n> +\n> +extern void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data);\n> +\n>  struct merge_remote_desc {\n>  \tstruct object *obj; /* the named object, could be a tag */\n>  \tconst char *name;\n> diff --git a/log-tree.c b/log-tree.c\n> index 10e6844..706ed4c 100644\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -413,10 +413,11 @@ static int is_common_merge(const struct commit *commit)\n>  \t\t&& !commit->parents->next->next);\n>  }\n>  \n> -static void show_one_mergetag(struct rev_info *opt,\n> +static void show_one_mergetag(struct commit *commit,\n>  \t\t\t      struct commit_extra_header *extra,\n> -\t\t\t      struct commit *commit)\n> +\t\t\t      void *data)\n>  {\n> +\tstruct rev_info *opt = (struct rev_info *)data;\n>  \tunsigned char sha1[20];\n>  \tstruct tag *tag;\n>  \tstruct strbuf verify_message;\n> @@ -463,15 +464,7 @@ static void show_one_mergetag(struct rev_info *opt,\n>  \n>  static void show_mergetag(struct rev_info *opt, struct commit *commit)\n>  {\n> -\tstruct commit_extra_header *extra, *to_free;\n> -\n> -\tto_free = read_commit_extra_headers(commit, NULL);\n> -\tfor (extra = to_free; extra; extra = extra->next) {\n> -\t\tif (strcmp(extra->key, \"mergetag\"))\n> -\t\t\tcontinue; /* not a merge tag */\n> -\t\tshow_one_mergetag(opt, extra, commit);\n> -\t}\n> -\tfree_commit_extra_headers(to_free);\n> +\tfor_each_mergetag(show_one_mergetag, commit, opt);\n>  }\n>  \n>  void show_log(struct rev_info *opt)\n"},{"id":"245508","messageId":"20140708.073501.2265697032770915844.chriscool@tuxfamily.org","threadId":"37068","inReplyTo":"xmqqion8by82.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 09/10] replace: check mergetags when using --graft","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-07-08T05:35:01Z","receivedAt":"2014-07-08T05:35:01Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Junio C Hamano <gitster@pobox.com>\n>\n> Christian Couder <chriscool@tuxfamily.org> writes:\n> \n>> When using --graft, with a mergetag in the original\n>> commit, we should check that the commit pointed to by\n>> the mergetag is still a parent of then new commit we\n>> create, otherwise the mergetag could be misleading.\n>>\n>> If the commit pointed to by the mergetag is no more\n>> a parent of the new commit, we could remove the\n>> mergetag, but in this case there is a good chance\n>> that the title or other elements of the commit might\n>> also be misleading. So let's just error out and\n>> suggest to use --edit instead on the commit.\n> \n> I do not quite understand the reasoning.  If you have a merge you\n> earlier made with a signed tag, and then want to redo the merge with\n> an updated tip of that branch (perhaps the side branch was earlier\n> based on maint but now was rebased on master), it will perfectly be\n> normal to expect that the title or other elements of the resulting\n> merge to stay the same.  \n\nYeah, but then you might also want to have a mergetag for the updated\ntip of the branch and --graft will not put it in the new commit, so it\nwould be better to use --edit in this case.\n\n> Why is it a good idea to error it out?\n\nBecause sometimes, in complex cases, it is misleading to do as if you\ncan do the right thing, when there is a good chance you cannot.\n\n> If the argument were '\"replace --graft\" that changes the parents is\n> likely to affect merge summary message, so error out and suggest to\n> use --edit instead', regardless of the 'mergetag', I'd understand\n> it, but that would essentially mean that 'replace --graft' should\n> never be used, so...\n\nI think that when \"replace --graft\" is used on a regular commit there\nis much better chance that the resulting replacement commit will be as\nthe user expect it to be.\n\nThanks,\nChristian.\n"},{"id":"245509","messageId":"20140708.073651.595198574881163961.chriscool@tuxfamily.org","threadId":"37068","inReplyTo":"xmqqmwckbyri.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 03/10] replace: add test for --graft","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-07-08T05:36:51Z","receivedAt":"2014-07-08T05:36:51Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Junio C Hamano <gitster@pobox.com>\n\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 | 40 ++++++++++++++++++++++++++++++++++++++++\n>>  1 file changed, 40 insertions(+)\n>>\n>> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\n>> index fb07ad2..d80a89e 100755\n>> --- a/t/t6050-replace.sh\n>> +++ b/t/t6050-replace.sh\n>> @@ -18,6 +18,33 @@ add_and_commit_file()\n>>      git commit --quiet -m \"$_file: $_msg\"\n>>  }\n>>  \n>> +commit_buffer_contains_parents()\n> \n> SP before (), perhaps?\n\nOk.\n\n>> +{\n>> +    git cat-file commit \"$1\" >payload &&\n>> +    sed -n -e '/^$/q' -e '/^parent /p' <payload >actual &&\n>> +    shift &&\n>> +    : >expected &&\n>> +    for _parent\n>> +    do\n>> +\techo \"parent $_parent\" >>expected || return 1\n>> +    done &&\n> \n> You can redirect the entire loop to simplify the above 5 lines,\n> which would read better, no?\n> \n> \tfor _parent\n>         do\n> \t\techo \"parent $_parent\"\n> \tdone >expect\n\nOk, thanks.\n \n>> +    test_cmp actual expected\n> \n> As \"test_cmp\" normally runs \"diff\", it is better to compare expect\n> with actual, not the other way around; running the test verbosely,\n> i.e. \"t6050-replace.sh -v\", shows how the actual output differs from\n> what is expected when diagnosing breakage and would be more useful\n> that way.\n\nOk.\n\n> If your goal is to test both the object contents with this code\n> *and* the overall system behaviour with the \"$child^$parent\" below,\n> perhaps they should be in the same \"commit_has_parents\" function,\n> without forcing the caller to choose between the two or call both,\n> no?\n\nYeah, will do.\n\nThanks,\nChristian.\n"},{"id":"245510","messageId":"20140708.075316.1145736046390054920.chriscool@tuxfamily.org","threadId":"37068","inReplyTo":"xmqqegxwby3l.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 08/10] commit: add for_each_mergetag()","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-07-08T05:53:16Z","receivedAt":"2014-07-08T05:53:16Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Junio C Hamano <gitster@pobox.com>\n\n> Christian Couder <chriscool@tuxfamily.org> writes:\n> \n>> In the same way as there is for_each_ref() to\n>> iterate on refs, it might be useful to have\n>> for_each_mergetag() to iterate on the mergetags\n>> of a given commit.\n>>\n>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n>> ---\n> \n> Heh, \"might be useful\" is an understatement ;-) We won't apply a\n> change that \"might be useful\" very lightly, but this refactoring\n> already uses the helper in existing code, showing that it *is*\n> useful, no?\n> \n> Let's have this early in the series, or perhaps even independent of\n> the \"replace\" series.\n\nOk, thanks,\nChristian.\n"},{"id":"245632","messageId":"xmqqsima7f3r.fsf@gitster.dls.corp.google.com","threadId":"37068","inReplyTo":"20140707063540.3708.51047.chriscool@tuxfamily.org","subject":"Re: [PATCH v6 02/10] replace: add --graft option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-09T14:59:20Z","receivedAt":"2014-07-09T14:59:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n> The usage string for this option is:\n>\n> git replace [-f] --graft <commit> [<parent>...]\n>\n> First we create a new commit that is the same as <commit>\n> except that its parents are [<parents>...]\n>\n> Then we create a replace ref that replace <commit> with\n> the commit we just created.\n>\n> With this new option, it should be straightforward to\n> convert grafts to replace refs.\n\nSensible.\n\n>\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  builtin/replace.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n>  1 file changed, 73 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index 1bb491d..ad47237 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,66 @@ 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 void replace_parents(struct strbuf *buf, int argc, const char **argv)\n> +{\n> +\tstruct strbuf new_parents = STRBUF_INIT;\n> +\tconst char *parent_start, *parent_end;\n> +\tint i;\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\nIt looks somewhat strange that both replace_parents() and\ncreate_graft() take familiar-looking <argc, argv> pair, but one\nignores argv[0] and uses the remainder and the other uses argv[0].\nShouldn't this function consume argv[] starting from [0] for\nconsistency?  You'd obviously need to update the caller to adjust\nthe arguments it gives to this function.\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> +...\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(&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\nIs this really an error?  It may be a warning-worthy situation for a\nuser or a script to end up doing a no-op graft, e.g.\n\n\tgit replace --graft HEAD HEAD^\n\nbut I wonder if it is more convenient to signal an error (like this\npatch does) or just ignore the request and return without adding the\nreplace ref.\n\nOther than these two points, looks good to me.\n\nThanks.\n"},{"id":"245684","messageId":"CAP8UFD2X7j2TGEQfX3h8CfiZypJ5tVPqaZ2bNE0k1-jbeJj=Zw@mail.gmail.com","threadId":"37068","inReplyTo":"xmqqsima7f3r.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 02/10] replace: add --graft option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2014-07-10T09:30:40Z","receivedAt":"2014-07-10T09:30:40Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Jul 9, 2014 at 4:59 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Christian Couder <chriscool@tuxfamily.org> writes:\n>\n>> +static void replace_parents(struct strbuf *buf, int argc, const char **argv)\n>> +{\n>> +     struct strbuf new_parents = STRBUF_INIT;\n>> +     const char *parent_start, *parent_end;\n>> +     int i;\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>> +     while (starts_with(parent_end, \"parent \"))\n>> +             parent_end += 48; /* \"parent \" + \"hex sha1\" + \"\\n\" */\n>> +\n>> +     /* prepare new parents */\n>> +     for (i = 1; i < argc; i++) {\n>\n> It looks somewhat strange that both replace_parents() and\n> create_graft() take familiar-looking <argc, argv> pair, but one\n> ignores argv[0] and uses the remainder and the other uses argv[0].\n> Shouldn't this function consume argv[] starting from [0] for\n> consistency?  You'd obviously need to update the caller to adjust\n> the arguments it gives to this function.\n\nOk, will do.\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>> +...\n>> +\n>> +     replace_parents(&buf, argc, argv);\n>> +\n>> +     if (write_sha1_file(buf.buf, buf.len, commit_type, new))\n>> +             die(_(\"could not write replacement commit for: '%s'\"), old_ref);\n>> +\n>> +     strbuf_release(&buf);\n>> +\n>> +     if (!hashcmp(old, new))\n>> +             return error(\"new commit is the same as the old one: '%s'\", sha1_to_hex(old));\n>\n> Is this really an error?  It may be a warning-worthy situation for a\n> user or a script to end up doing a no-op graft, e.g.\n>\n>         git replace --graft HEAD HEAD^\n>\n> but I wonder if it is more convenient to signal an error (like this\n> patch does) or just ignore the request and return without adding the\n> replace ref.\n\nAs the user might expect that a new replace ref was created on success\n(0 exit code), and as we should at least warn if we would create a\ncommit that is the same as an existing one, I think it is just simpler\nto error out in this case.\n\nThough maybe we could use a special exit code (for example 2) in this\ncase, so that the user might more easily ignore this error in a\nscript.\n\n> Other than these two points, looks good to me.\n\nThanks,\nChristian.\n"},{"id":"245694","messageId":"xmqqr41t88dz.fsf@gitster.dls.corp.google.com","threadId":"37068","inReplyTo":"CAP8UFD2X7j2TGEQfX3h8CfiZypJ5tVPqaZ2bNE0k1-jbeJj=Zw@mail.gmail.com","subject":"Re: [PATCH v6 02/10] replace: add --graft option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-10T16:51:20Z","receivedAt":"2014-07-10T16:51:20Z","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>> Is this really an error?  It may be a warning-worthy situation for a\n>> user or a script to end up doing a no-op graft, e.g.\n>>\n>>         git replace --graft HEAD HEAD^\n>>\n>> but I wonder if it is more convenient to signal an error (like this\n>> patch does) or just ignore the request and return without adding the\n>> replace ref.\n>\n> As the user might expect that a new replace ref was created on success\n> (0 exit code), and as we should at least warn if we would create a\n> commit that is the same as an existing one,...\n\nWhy is it an event that needs a warning?  I do not buy that \"as we\nshould at least\" at all.\n\nIf you say \"make sure A's parent is B\" and then you asked the same\nthing again when there already is a replacement in place, that\nshould be a no-op.  \"Making sure A's parent is B\" would be an\nidempotent operation, no?  Why not just make sure A's parent is\nalready B and report \"Your wish has been granted\" to the user?\n\nWhy would it be simpler for the user to get an error, inspect the\nsituation and realize that his wish has been granted after all?\n"},{"id":"245699","messageId":"xmqqegxt86ba.fsf@gitster.dls.corp.google.com","threadId":"37068","inReplyTo":"xmqqr41t88dz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 02/10] replace: add --graft option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-10T17:36:09Z","receivedAt":"2014-07-10T17:36:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Christian Couder <christian.couder@gmail.com> writes:\n>\n>>> Is this really an error?  It may be a warning-worthy situation for a\n>>> user or a script to end up doing a no-op graft, e.g.\n>>>\n>>>         git replace --graft HEAD HEAD^\n>>>\n>>> but I wonder if it is more convenient to signal an error (like this\n>>> patch does) or just ignore the request and return without adding the\n>>> replace ref.\n>>\n>> As the user might expect that a new replace ref was created on success\n>> (0 exit code), and as we should at least warn if we would create a\n>> commit that is the same as an existing one,...\n>\n> Why is it an event that needs a warning?  I do not buy that \"as we\n> should at least\" at all.\n\nEhh, it came a bit differently from what I meant.  Perhaps s/do not\nbuy/do not understand/ is closer to what I think---that is, it is\nnot like I with a strong conviction think you are wrong.  It is more\nlike I do not understand why you think it needs a warning, meaing\nyou would need to explain it better.\n\n> If you say \"make sure A's parent is B\" and then you asked the same\n> thing again when there already is a replacement in place, that\n> should be a no-op.  \"Making sure A's parent is B\" would be an\n> idempotent operation, no?  Why not just make sure A's parent is\n> already B and report \"Your wish has been granted\" to the user?\n>\n> Why would it be simpler for the user to get an error, inspect the\n> situation and realize that his wish has been granted after all?\n"},{"id":"245767","messageId":"CAP8UFD0_m5aFVcBQr3d9pXR=9rLjAVPGrj=UsBYFcnTQFwNKGA@mail.gmail.com","threadId":"37068","inReplyTo":"xmqqegxt86ba.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 02/10] replace: add --graft option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2014-07-11T08:59:32Z","receivedAt":"2014-07-11T08:59:32Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>>>\n>>> As the user might expect that a new replace ref was created on success\n>>> (0 exit code), and as we should at least warn if we would create a\n>>> commit that is the same as an existing one,...\n>>\n>> Why is it an event that needs a warning?  I do not buy that \"as we\n>> should at least\" at all.\n\nHere you ask why this event needs a warning...\n\n> Ehh, it came a bit differently from what I meant.  Perhaps s/do not\n> buy/do not understand/ is closer to what I think---that is, it is\n> not like I with a strong conviction think you are wrong.  It is more\n> like I do not understand why you think it needs a warning, meaing\n> you would need to explain it better.\n>\n>> If you say \"make sure A's parent is B\" and then you asked the same\n>> thing again when there already is a replacement in place, that\n>> should be a no-op.\n\n(When there is already a replacement in place we error out in\nreplace_object_sha1() unless --force is used. What we are talking\nabout here is what happens if the replacement commit is the same as\nthe original commit.)\n\n>> \"Making sure A's parent is B\" would be an\n>> idempotent operation, no?  Why not just make sure A's parent is\n>> already B and report \"Your wish has been granted\" to the user?\n\n... and here you say we should report \"your wish has been granted\"...\n\n>> Why would it be simpler for the user to get an error, inspect the\n>> situation and realize that his wish has been granted after all?\n\n... but for me reporting to the user \"your wish has been granted\" and\nwarning (or errorring out) saying \"the new commit would be the same as\nthe old one\" are nearly the same thing.\n\nSo I wonder what exactly you are not happy with.\n\nIs it the fact that I use the error() function, because it would\nprefix the message with \"fatal:\" and that would be too scary?\n\nIs it because with error() the exit code would not be 0?\n\nIs it because the message \"new commit is the same as the old one:\n'%s'\" is too scary or unnecessarily technical by itself?\n\nIs it ok if I just print the message on stderr without \"warning:\" or\n\"fatal:\" in front of it?\n\nBy the way, when I say \"As ... and ..., I think it is just simpler to\nerror out in this case.\", I mean that it is simpler from the\ndeveloper's point of view, not necessarily for the user.\n\nOf course I am ok with improving things for the user even if it\nrequires more code/work, but it is difficult to properly do that if I\ndon't see how I could do it.\n\nThanks,\nChristian.\n"},{"id":"245780","messageId":"xmqqmwcg561j.fsf@gitster.dls.corp.google.com","threadId":"37068","inReplyTo":"CAP8UFD0_m5aFVcBQr3d9pXR=9rLjAVPGrj=UsBYFcnTQFwNKGA@mail.gmail.com","subject":"Re: [PATCH v6 02/10] replace: add --graft option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-11T14:22:32Z","receivedAt":"2014-07-11T14:22:32Z","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, Jul 10, 2014 at 7:36 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n>>> \"Making sure A's parent is B\" would be an\n>>> idempotent operation, no?  Why not just make sure A's parent is\n>>> already B and report \"Your wish has been granted\" to the user?\n>\n> ... and here you say we should report \"your wish has been granted\"...\n\nNormal way for \"git replace\" to report that is to exit with status 0\nand without any noise, I would think.\n\n>>> Why would it be simpler for the user to get an error, inspect the\n>>> situation and realize that his wish has been granted after all?\n>\n> ... but for me reporting to the user \"your wish has been granted\" and\n> warning (or errorring out) saying \"the new commit would be the same as\n> the old one\" are nearly the same thing.\n>\n> So I wonder what exactly you are not happy with.\n\nSee above.\n"},{"id":"245790","messageId":"CAP8UFD2db=ZMFSvzW6be3gBv7DP93C7Z5yfOEhs9QLNOkX0fGQ@mail.gmail.com","threadId":"37068","inReplyTo":"xmqqmwcg561j.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 02/10] replace: add --graft option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2014-07-11T16:24:50Z","receivedAt":"2014-07-11T16:24:50Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Jul 11, 2014 at 4:22 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Christian Couder <christian.couder@gmail.com> writes:\n>\n>> On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>>>> \"Making sure A's parent is B\" would be an\n>>>> idempotent operation, no?  Why not just make sure A's parent is\n>>>> already B and report \"Your wish has been granted\" to the user?\n>>\n>> ... and here you say we should report \"your wish has been granted\"...\n>\n> Normal way for \"git replace\" to report that is to exit with status 0\n> and without any noise, I would think.\n\nIn a similar case \"git replace --edit\" we error out instead of just\nexiting (with status 0), see:\n\nf22166b5fee7dc (replace: make sure --edit results in a different object)\n"},{"id":"245814","messageId":"xmqq4myn4us8.fsf@gitster.dls.corp.google.com","threadId":"37068","inReplyTo":"CAP8UFD2db=ZMFSvzW6be3gBv7DP93C7Z5yfOEhs9QLNOkX0fGQ@mail.gmail.com","subject":"Re: [PATCH v6 02/10] replace: add --graft option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-11T18:25:43Z","receivedAt":"2014-07-11T18:25:43Z","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 Fri, Jul 11, 2014 at 4:22 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Christian Couder <christian.couder@gmail.com> writes:\n>>\n>>> On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>>\n>>>>> \"Making sure A's parent is B\" would be an\n>>>>> idempotent operation, no?  Why not just make sure A's parent is\n>>>>> already B and report \"Your wish has been granted\" to the user?\n>>>\n>>> ... and here you say we should report \"your wish has been granted\"...\n>>\n>> Normal way for \"git replace\" to report that is to exit with status 0\n>> and without any noise, I would think.\n>\n> In a similar case \"git replace --edit\" we error out instead of just\n> exiting (with status 0), see:\n>\n> f22166b5fee7dc (replace: make sure --edit results in a different object)\n\nI do not care *too* deeply, but if you ask me, that may be a mistake\nwe may want to fix before the next release.\n"},{"id":"245815","messageId":"20140711182902.GH7856@sigill.intra.peff.net","threadId":"37068","inReplyTo":"xmqq4myn4us8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 02/10] replace: add --graft option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-11T18:29:02Z","receivedAt":"2014-07-11T18:29:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 11, 2014 at 11:25:43AM -0700, Junio C Hamano wrote:\n\n> Christian Couder <christian.couder@gmail.com> writes:\n> \n> > On Fri, Jul 11, 2014 at 4:22 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> >> Christian Couder <christian.couder@gmail.com> writes:\n> >>\n> >>> On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> >>>\n> >>>>> \"Making sure A's parent is B\" would be an\n> >>>>> idempotent operation, no?  Why not just make sure A's parent is\n> >>>>> already B and report \"Your wish has been granted\" to the user?\n> >>>\n> >>> ... and here you say we should report \"your wish has been granted\"...\n> >>\n> >> Normal way for \"git replace\" to report that is to exit with status 0\n> >> and without any noise, I would think.\n> >\n> > In a similar case \"git replace --edit\" we error out instead of just\n> > exiting (with status 0), see:\n> >\n> > f22166b5fee7dc (replace: make sure --edit results in a different object)\n> \n> I do not care *too* deeply, but if you ask me, that may be a mistake\n> we may want to fix before the next release.\n\nYeah, I also do not care too deeply, but I mentioned in the earlier\nreview that I would expect it to just remove the replacement if it ends\nup generating the same object.\n\n-Peff\n"}]}