[PATCH] replace: fix replacing object with itself

Manzur Mukhitdinov· Nov 10, 2014, 23:20 UTC · lore

When object is replaced with itself git shows unhelpful messages like(git log):
    "fatal: replace depth too high for object <SHA1>"

Prevents user from replacing object with itself(with test for checking
this case).
---
 builtin/replace.c  | 8 +++-----
 t/t6050-replace.sh | 8 ++++++++
 2 files changed, 11 insertions(+), 5 deletions(-)

Show changes to 2 files +11 −5

builtin/replace.c, t/t6050-replace.sh

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..98ac9dd 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -440,4 +440,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

Jeff King· Nov 11, 2014, 03:03 UTC · re: Manzur Mukhitdinov · lore

Re: [PATCH] replace: fix replacing object with itself

On Tue, Nov 11, 2014 at 12:20:56AM +0100, Manzur Mukhitdinov wrote:

Show 5 quoted lines

> When object is replaced with itself git shows unhelpful messages like(git log):
>     "fatal: replace depth too high for object <SHA1>"
> 
> Prevents user from replacing object with itself(with test for checking
> this case).

Thanks, this looks good to me.

Show 7 quoted lines

> +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
> +'

I think some of these overlap with earlier tests (I know that the "--edit" case is checked already), but I think it is nice to keep the related checks together here.

Should we maybe squash this in to drop the now redundant test (AFAICT, that is the only one that overlaps)?

Show changes to t/t6050-replace.sh +1 −2

diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 98ac9dd..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" &&

patchreplace: fix replacing object with itself