# [PATCH] replace: fix replacing object with itself 4 messages from 2014-11-13 to 2014-11-16. Participants: Manzur Mukhitdinov, Junio C Hamano, Christian Couder. Thread: https://gitlist.dev/t/37954 ## Manzur Mukhitdinov, 2014-11-13 14:05 Subject: [PATCH] replace: fix replacing object with itself Message-ID: <1415887559-16585-1-git-send-email-manzurmm@gmail.com> URL: https://gitlist.dev/e/1415887559-16585-1-git-send-email-manzurmm%40gmail.com ``` When object is replaced with itself git shows unhelpful messages like(git log): "fatal: replace depth too high for object " Prevents user from replacing object with itself(with test for checking this case). Signed-off-by: Manzur Mukhitdinov --- builtin/replace.c | 8 +++----- t/t6050-replace.sh | 11 +++++++++-- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/builtin/replace.c b/builtin/replace.c index 294b61b..628377a 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -157,6 +157,9 @@ static int replace_object_sha1(const char *object_ref, char ref[PATH_MAX]; struct ref_lock *lock; + if (!hashcmp(object, repl)) + return error("new object is the same as the old one: '%s'", sha1_to_hex(object)); + obj_type = sha1_object_info(object, NULL); repl_type = sha1_object_info(repl, NULL); if (!force && obj_type != repl_type) @@ -295,9 +298,6 @@ static int edit_and_replace(const char *object_ref, int force, int raw) free(tmpfile); - if (!hashcmp(old, new)) - return error("new object is the same as the old one: '%s'", sha1_to_hex(old)); - return replace_object_sha1(object_ref, old, "replacement", new, force); } @@ -406,8 +406,6 @@ static int create_graft(int argc, const char **argv, int force) strbuf_release(&buf); - if (!hashcmp(old, new)) - return error("new commit is the same as the old one: '%s'", sha1_to_hex(old)); return replace_object_sha1(old_ref, old, "replacement", new, force); } diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh index 4d5a25e..5f96374 100755 --- a/t/t6050-replace.sh +++ b/t/t6050-replace.sh @@ -369,9 +369,8 @@ test_expect_success '--edit with and without already replaced object' ' git cat-file commit "$PARA3" | grep "A fake Thor" ' -test_expect_success '--edit and change nothing or command failed' ' +test_expect_success '--edit with failed editor' ' git replace -d "$PARA3" && - test_must_fail env GIT_EDITOR=true git replace --edit "$PARA3" && test_must_fail env GIT_EDITOR="./fakeeditor;false" git replace --edit "$PARA3" && GIT_EDITOR=./fakeeditor git replace --edit "$PARA3" && git replace -l | grep "$PARA3" && @@ -440,4 +439,12 @@ test_expect_success GPG '--graft on a commit with a mergetag' ' git replace -d $HASH10 ' +test_expect_success 'replacing object with itself must fail' ' + test_must_fail git replace $HASH1 $HASH1 && + HASH8=$(git rev-parse --verify HEAD) && + test_must_fail git replace HEAD $HASH8 && + test_must_fail git replace --graft HEAD HEAD^ && + test_must_fail env GIT_EDITOR=true git replace --edit HEAD +' + test_done -- 1.9.1 ``` ## Junio C Hamano, 2014-11-14 22:45 Subject: Re: [PATCH] replace: fix replacing object with itself Message-ID: URL: https://gitlist.dev/e/xmqqppcp1jvg.fsf%40gitster.dls.corp.google.com In-Reply-To: <1415887559-16585-1-git-send-email-manzurmm@gmail.com> ``` Manzur Mukhitdinov writes: > When object is replaced with itself git shows unhelpful messages like(git log): > "fatal: replace depth too high for object " > > Prevents user from replacing object with itself(with test for checking > this case). > > Signed-off-by: Manzur Mukhitdinov > --- The patch is not wrong per-se, but I wonder how useful this "do not replace itself but all other forms of loops are not checked at all" would be in practice. If your user did this: git replace A B ;# pretend as if what is in B is in A git replace B C ;# pretend as if what is in C is in B git replace C A ;# pretend as if we have loop git log C she would not be helped with this patch at all, no? We have the "replace depth" thing, which is a poor-man's substitute for loop detection, primarily because we do not want to incur high cost of loop detection at runtime. Shouldn't we be doing at least the same amount of loop-avoidance check, if we really want to avoid triggering the "replace depth" check at runtime? > builtin/replace.c | 8 +++----- > t/t6050-replace.sh | 11 +++++++++-- > 2 files changed, 12 insertions(+), 7 deletions(-) > > diff --git a/builtin/replace.c b/builtin/replace.c > index 294b61b..628377a 100644 > --- a/builtin/replace.c > +++ b/builtin/replace.c > @@ -157,6 +157,9 @@ static int replace_object_sha1(const char *object_ref, > char ref[PATH_MAX]; > struct ref_lock *lock; > > + if (!hashcmp(object, repl)) > + return error("new object is the same as the old one: '%s'", sha1_to_hex(object)); > + > obj_type = sha1_object_info(object, NULL); > repl_type = sha1_object_info(repl, NULL); > if (!force && obj_type != repl_type) > @@ -295,9 +298,6 @@ static int edit_and_replace(const char *object_ref, int force, int raw) > > free(tmpfile); > > - if (!hashcmp(old, new)) > - return error("new object is the same as the old one: '%s'", sha1_to_hex(old)); > - > return replace_object_sha1(object_ref, old, "replacement", new, force); > } > > @@ -406,8 +406,6 @@ static int create_graft(int argc, const char **argv, int force) > > strbuf_release(&buf); > > - if (!hashcmp(old, new)) > - return error("new commit is the same as the old one: '%s'", sha1_to_hex(old)); > > return replace_object_sha1(old_ref, old, "replacement", new, force); > } > diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh > index 4d5a25e..5f96374 100755 > --- a/t/t6050-replace.sh > +++ b/t/t6050-replace.sh > @@ -369,9 +369,8 @@ test_expect_success '--edit with and without already replaced object' ' > git cat-file commit "$PARA3" | grep "A fake Thor" > ' > > -test_expect_success '--edit and change nothing or command failed' ' > +test_expect_success '--edit with failed editor' ' > git replace -d "$PARA3" && > - test_must_fail env GIT_EDITOR=true git replace --edit "$PARA3" && > test_must_fail env GIT_EDITOR="./fakeeditor;false" git replace --edit "$PARA3" && > GIT_EDITOR=./fakeeditor git replace --edit "$PARA3" && > git replace -l | grep "$PARA3" && > @@ -440,4 +439,12 @@ test_expect_success GPG '--graft on a commit with a mergetag' ' > git replace -d $HASH10 > ' > > +test_expect_success 'replacing object with itself must fail' ' > + test_must_fail git replace $HASH1 $HASH1 && > + HASH8=$(git rev-parse --verify HEAD) && > + test_must_fail git replace HEAD $HASH8 && > + test_must_fail git replace --graft HEAD HEAD^ && > + test_must_fail env GIT_EDITOR=true git replace --edit HEAD > +' > + > test_done ``` ## Christian Couder, 2014-11-15 11:55 Subject: Re: [PATCH] replace: fix replacing object with itself Message-ID: URL: https://gitlist.dev/e/CAP8UFD3dZMmGJp1p%3DttVUoUkiwh7oYL3gqgsGZ%2BD8Bd16FiKjQ%40mail.gmail.com In-Reply-To: ``` [Sorry to resend this. I am really bad at making gmail on my Android smartphone send plain text emails.] On Fri, Nov 14, 2014 at 11:45 PM, Junio C Hamano wrote: > Manzur Mukhitdinov writes: > >> When object is replaced with itself git shows unhelpful messages like(git log): >> "fatal: replace depth too high for object " >> >> Prevents user from replacing object with itself(with test for checking >> this case). >> >> Signed-off-by: Manzur Mukhitdinov >> --- > > The patch is not wrong per-se, but I wonder how useful this "do not > replace itself but all other forms of loops are not checked at all" > would be in practice. If your user did this: > > git replace A B ;# pretend as if what is in B is in A > git replace B C ;# pretend as if what is in C is in B > git replace C A ;# pretend as if we have loop > git log C > > she would not be helped with this patch at all, no? Yeah, but such loops are much less likely mistakes and are more difficult to detect, so I think this patch is at least a good first step. > We have the "replace depth" thing, which is a poor-man's substitute > for loop detection, primarily because we do not want to incur high > cost of loop detection at runtime. Shouldn't we be doing at least > the same amount of loop-avoidance check, if we really want to avoid > triggering the "replace depth" check at runtime? We could check at replace ref creation time, but what if the user already has a ref that replaces A using B, and then a fetch adds a ref that replaces B using A? Maybe we should accept that we have to rely on the runtime replace depth anyway, and improve its output first. Best, Christian. ``` ## Junio C Hamano, 2014-11-16 18:59 Subject: Re: [PATCH] replace: fix replacing object with itself Message-ID: URL: https://gitlist.dev/e/xmqqr3x3ynrm.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Christian Couder writes: >> The patch is not wrong per-se, but I wonder how useful this "do not >> replace itself but all other forms of loops are not checked at all" >> would be in practice. If your user did this: >> >> git replace A B ;# pretend as if what is in B is in A >> git replace B C ;# pretend as if what is in C is in B >> git replace C A ;# pretend as if we have loop >> git log C >> >> she would not be helped with this patch at all, no? > > Yeah, but such loops are much less likely mistakes and are more > difficult to detect, so I think this patch is at least a good first > step. More difficult to notice by humans, hence deserves more help from the tool. When these two are both mistakes, which one do you think is easier to notice (thusly unlikely to happen)? git replace A A git replace C A Of course, the former is immediately obvious to the human who is typing that it is a typo. The latter would be harder to spot as a mistake as the invoker has to know that there is an existing "pretend as if what is in A is in C" aka "replace A C" done earlier in the repository. And the cost of detecting such a possible "too deep replace chain" (do not call that a loop---the runtime barfs if you have too deep a replace chain without any loop) wouldn't be too high. You only need to look at existing refs/replace/* refs, their contents, and the two object names that form the proposed new replacement . Even a kludge (read: I am not suggesting that you solve it this way) like "first install the replacement as proposed, then enumerate all the replacement refs and their values and try to see if the runtime check would barf, and if it would, fail the operation and revert the change" would catch a mistake to cause the repository in trouble. ```