{"thread":{"id":"37954","subject":"[PATCH] replace: fix replacing object with itself","startedAt":"2014-11-13T14:05:59Z","lastAt":"2014-11-16T18:59:57Z","messageCount":4,"participants":["Manzur Mukhitdinov","Junio C Hamano","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"251818","messageId":"1415887559-16585-1-git-send-email-manzurmm@gmail.com","threadId":"37954","inReplyTo":null,"subject":"[PATCH] replace: fix replacing object with itself","fromName":"Manzur Mukhitdinov","fromEmail":"manzurmm@gmail.com","sentAt":"2014-11-13T14:05:59Z","receivedAt":"2014-11-13T14:05:59Z","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 \"\n\nPrevents user from replacing object with itself(with test for checking\nthis case).\n\nSigned-off-by: Manzur Mukhitdinov \n---\n builtin/replace.c | 8 +++-----\n t/t6050-replace.sh | 11 +++++++++--\n 2 files changed, 12 insertions(+), 7 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..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@@ -440,4 +439,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":"251920","messageId":"xmqqppcp1jvg.fsf@gitster.dls.corp.google.com","threadId":"37954","inReplyTo":"1415887559-16585-1-git-send-email-manzurmm@gmail.com","subject":"Re: [PATCH] replace: fix replacing object with itself","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-14T22:45:55Z","receivedAt":"2014-11-14T22:45:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Manzur Mukhitdinov writes:\n\n> When object is replaced with itself git shows unhelpful messages like(git log):\n> \"fatal: replace depth too high for object \"\n>\n> Prevents user from replacing object with itself(with test for checking\n> this case).\n>\n> Signed-off-by: Manzur Mukhitdinov \n> ---\n\nThe patch is not wrong per-se, but I wonder how useful this \"do not\nreplace itself but all other forms of loops are not checked at all\"\nwould be in practice. If your user did this:\n\n\tgit replace A B ;# pretend as if what is in B is in A\n git replace B C ;# pretend as if what is in C is in B\n git replace C A ;# pretend as if we have loop\n\tgit log C\n\nshe would not be helped with this patch at all, no?\n\nWe have the \"replace depth\" thing, which is a poor-man's substitute\nfor loop detection, primarily because we do not want to incur high\ncost of loop detection at runtime. Shouldn't we be doing at least\nthe same amount of loop-avoidance check, if we really want to avoid\ntriggering the \"replace depth\" check at runtime?\n\n> builtin/replace.c | 8 +++-----\n> t/t6050-replace.sh | 11 +++++++++--\n> 2 files changed, 12 insertions(+), 7 deletions(-)\n>\n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index 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> }\n> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\n> index 4d5a25e..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> @@ -440,4 +439,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"},{"id":"251940","messageId":"CAP8UFD3dZMmGJp1p=ttVUoUkiwh7oYL3gqgsGZ+D8Bd16FiKjQ@mail.gmail.com","threadId":"37954","inReplyTo":"xmqqppcp1jvg.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] replace: fix replacing object with itself","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2014-11-15T11:55:11Z","receivedAt":"2014-11-15T11:55:11Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"[Sorry to resend this. I am really bad at making gmail on my Android\nsmartphone send plain text emails.]\n\nOn Fri, Nov 14, 2014 at 11:45 PM, Junio C Hamano wrote:\n> Manzur Mukhitdinov writes:\n>\n>> When object is replaced with itself git shows unhelpful messages like(git log):\n>> \"fatal: replace depth too high for object \"\n>>\n>> Prevents user from replacing object with itself(with test for checking\n>> this case).\n>>\n>> Signed-off-by: Manzur Mukhitdinov \n>> ---\n>\n> The patch is not wrong per-se, but I wonder how useful this \"do not\n> replace itself but all other forms of loops are not checked at all\"\n> would be in practice. If your user did this:\n>\n> git replace A B ;# pretend as if what is in B is in A\n> git replace B C ;# pretend as if what is in C is in B\n> git replace C A ;# pretend as if we have loop\n> git log C\n>\n> she would not be helped with this patch at all, no?\n\nYeah, but such loops are much less likely mistakes and are more\ndifficult to detect, so I think this patch is at least a good first\nstep.\n\n> We have the \"replace depth\" thing, which is a poor-man's substitute\n> for loop detection, primarily because we do not want to incur high\n> cost of loop detection at runtime. Shouldn't we be doing at least\n> the same amount of loop-avoidance check, if we really want to avoid\n> triggering the \"replace depth\" check at runtime?\n\nWe could check at replace ref creation time, but what if the user\nalready has a ref that replaces A using B, and then a fetch adds a ref\nthat replaces B using A?\n\nMaybe we should accept that we have to rely on the runtime replace\ndepth anyway, and improve its output first.\n\nBest,\nChristian.\n"},{"id":"251967","messageId":"xmqqr3x3ynrm.fsf@gitster.dls.corp.google.com","threadId":"37954","inReplyTo":"CAP8UFD3dZMmGJp1p=ttVUoUkiwh7oYL3gqgsGZ+D8Bd16FiKjQ@mail.gmail.com","subject":"Re: [PATCH] replace: fix replacing object with itself","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-16T18:59:57Z","receivedAt":"2014-11-16T18:59:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n>> The patch is not wrong per-se, but I wonder how useful this \"do not\n>> replace itself but all other forms of loops are not checked at all\"\n>> would be in practice. If your user did this:\n>>\n>> git replace A B ;# pretend as if what is in B is in A\n>> git replace B C ;# pretend as if what is in C is in B\n>> git replace C A ;# pretend as if we have loop\n>> git log C\n>>\n>> she would not be helped with this patch at all, no?\n>\n> Yeah, but such loops are much less likely mistakes and are more\n> difficult to detect, so I think this patch is at least a good first\n> step.\n\nMore difficult to notice by humans, hence deserves more help from\nthe tool.\n\nWhen these two are both mistakes, which one do you think is easier\nto notice (thusly unlikely to happen)?\n\n\tgit replace A A\n git replace C A\n\nOf course, the former is immediately obvious to the human who is\ntyping that it is a typo.\n\nThe latter would be harder to spot as a mistake as the invoker has\nto know that there is an existing \"pretend as if what is in A is in\nC\" aka \"replace A C\" done earlier in the repository.\n\nAnd the cost of detecting such a possible \"too deep replace chain\"\n(do not call that a loop---the runtime barfs if you have too deep a\nreplace chain without any loop) wouldn't be too high. You only need\nto look at existing refs/replace/* refs, their contents, and the two\nobject names that form the proposed new replacement .\n\nEven a kludge (read: I am not suggesting that you solve it this way)\nlike \"first install the replacement as proposed, then enumerate all\nthe replacement refs and their values and try to see if the runtime\ncheck would barf, and if it would, fail the operation and revert the\nchange\" would catch a mistake to cause the repository in trouble.\n"}]}