{"thread":{"id":"37928","subject":"[PATCH] replace: fix replacing object with itself","startedAt":"2014-11-10T23:20:56Z","lastAt":"2014-11-11T03:03:41Z","messageCount":2,"participants":["Manzur Mukhitdinov","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"251696","messageId":"1415661656-3657-1-git-send-email-manzurmm@gmail.com","threadId":"37928","inReplyTo":null,"subject":"[PATCH] replace: fix replacing object with itself","fromName":"Manzur Mukhitdinov","fromEmail":"manzurmm@gmail.com","sentAt":"2014-11-10T23:20:56Z","receivedAt":"2014-11-10T23:20:56Z","isPatch":true,"sender":{"key":"manzurmm@gmail.com","avatar":"https://gravatar.com/avatar/5e48a8390e02919dedd50464608fa1cdae9ffa475efaa453d91cb618f67f018d?d=mp&s=160"},"body":"When object is replaced with itself git shows unhelpful messages like(git log):\n    \"fatal: replace depth too high for object <SHA1>\"\n\nPrevents user from replacing object with itself(with test for checking\nthis case).\n---\n builtin/replace.c  | 8 +++-----\n t/t6050-replace.sh | 8 ++++++++\n 2 files changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 294b61b..628377a 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -157,6 +157,9 @@ static int replace_object_sha1(const char *object_ref,\n \tchar ref[PATH_MAX];\n \tstruct ref_lock *lock;\n \n+\tif (!hashcmp(object, repl))\n+\t\treturn error(\"new object is the same as the old one: '%s'\", sha1_to_hex(object));\n+\n \tobj_type = sha1_object_info(object, NULL);\n \trepl_type = sha1_object_info(repl, NULL);\n \tif (!force && obj_type != repl_type)\n@@ -295,9 +298,6 @@ static int edit_and_replace(const char *object_ref, int force, int raw)\n \n \tfree(tmpfile);\n \n-\tif (!hashcmp(old, new))\n-\t\treturn error(\"new object is the same as the old one: '%s'\", sha1_to_hex(old));\n-\n \treturn replace_object_sha1(object_ref, old, \"replacement\", new, force);\n }\n \n@@ -406,8 +406,6 @@ static int create_graft(int argc, const char **argv, int force)\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 }\ndiff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\nindex 4d5a25e..98ac9dd 100755\n--- a/t/t6050-replace.sh\n+++ b/t/t6050-replace.sh\n@@ -440,4 +440,12 @@ test_expect_success GPG '--graft on a commit with a mergetag' '\n \tgit replace -d $HASH10\n '\n \n+test_expect_success 'replacing object with itself must fail' '\n+    test_must_fail git replace $HASH1 $HASH1 &&\n+    HASH8=$(git rev-parse --verify HEAD) &&\n+    test_must_fail git replace HEAD $HASH8 &&\n+    test_must_fail git replace --graft HEAD HEAD^ &&\n+    test_must_fail env GIT_EDITOR=true git replace --edit HEAD\n+'\n+\n test_done\n-- \n1.9.1\n"},{"id":"251710","messageId":"20141111030341.GC21328@peff.net","threadId":"37928","inReplyTo":"1415661656-3657-1-git-send-email-manzurmm@gmail.com","subject":"Re: [PATCH] replace: fix replacing object with itself","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-11T03:03:41Z","receivedAt":"2014-11-11T03:03:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 11, 2014 at 12:20:56AM +0100, Manzur Mukhitdinov wrote:\n\n> When object is replaced with itself git shows unhelpful messages like(git log):\n>     \"fatal: replace depth too high for object <SHA1>\"\n> \n> Prevents user from replacing object with itself(with test for checking\n> this case).\n\nThanks, this looks good to me.\n\n> +test_expect_success 'replacing object with itself must fail' '\n> +    test_must_fail git replace $HASH1 $HASH1 &&\n> +    HASH8=$(git rev-parse --verify HEAD) &&\n> +    test_must_fail git replace HEAD $HASH8 &&\n> +    test_must_fail git replace --graft HEAD HEAD^ &&\n> +    test_must_fail env GIT_EDITOR=true git replace --edit HEAD\n> +'\n\nI think some of these overlap with earlier tests (I know that the\n\"--edit\" case is checked already), but I think it is nice to keep the\nrelated checks together here.\n\nShould we maybe squash this in to drop the now redundant test (AFAICT,\nthat is the only one that overlaps)?\n\ndiff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\nindex 98ac9dd..5f96374 100755\n--- a/t/t6050-replace.sh\n+++ b/t/t6050-replace.sh\n@@ -369,9 +369,8 @@ test_expect_success '--edit with and without already replaced object' '\n \tgit cat-file commit \"$PARA3\" | grep \"A fake Thor\"\n '\n \n-test_expect_success '--edit and change nothing or command failed' '\n+test_expect_success '--edit with failed editor' '\n \tgit replace -d \"$PARA3\" &&\n-\ttest_must_fail env GIT_EDITOR=true git replace --edit \"$PARA3\" &&\n \ttest_must_fail env GIT_EDITOR=\"./fakeeditor;false\" git replace --edit \"$PARA3\" &&\n \tGIT_EDITOR=./fakeeditor git replace --edit \"$PARA3\" &&\n \tgit replace -l | grep \"$PARA3\" &&\n"}]}