[PATCH] replace: fix replacing object with itself

[PATCH] replace: fix replacing object with itself

[Manzur Mukhitdinov]

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).

Signed-off-by: Manzur Mukhitdinov <manzurmm@gmail.com>
---
 builtin/replace.c  | 3 +++
 t/t6050-replace.sh | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/builtin/replace.c b/builtin/replace.c
index 294b61b..b7e05ad 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -186,6 +186,9 @@ static int replace_object(const char *object_ref, const char *replace_ref, int f
 	if (get_sha1(replace_ref, repl))
 		die("Failed to resolve '%s' as a valid ref.", replace_ref);
 
+	if (!hashcmp(object, repl))
+		return error("new object is the same as the old one: '%s'", sha1_to_hex(object));
+
 	return replace_object_sha1(object_ref, object, replace_ref, repl, force);
 }
 
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 4d5a25e..f2b0307 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -440,4 +440,10 @@ 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_done
-- 
1.9.1

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

[Jeff King]

On Sun, Nov 09, 2014 at 01:05:31AM +0100, Manzur Mukhitdinov wrote:

> 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).

I thought we already did this in the last round of git-replace patches,
but it looks like we only did it for the newly added --edit and --graft
cases, not "git replace X X". I think this is probably a good step. I've
also considered that this should be another way of deleting the
replacement, but I think we decided that was too magical.

> diff --git a/builtin/replace.c b/builtin/replace.c
> index 294b61b..b7e05ad 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -186,6 +186,9 @@ static int replace_object(const char *object_ref, const char *replace_ref, int f
>  	if (get_sha1(replace_ref, repl))
>  		die("Failed to resolve '%s' as a valid ref.", replace_ref);
>  
> +	if (!hashcmp(object, repl))
> +		return error("new object is the same as the old one: '%s'", sha1_to_hex(object));
> +
>  	return replace_object_sha1(object_ref, object, replace_ref, repl, force);

I think all of the callers of replace_object_sha1 do this same check
now. Can we just move the check into that function instead of adding
another instance of it?

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think all of the callers of replace_object_sha1 do this same check
> now. Can we just move the check into that function instead of adding
> another instance of it?

Good thinking.  Thanks.

[PATCH] replace: fix replacing object with itself

[Manzur Mukhitdinov]

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(-)

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

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

[Jeff King]

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

> 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.

> +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)?

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" &&

[PATCH] replace: fix replacing object with itself

[Manzur Mukhitdinov]

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).

Signed-off-by: Manzur Mukhitdinov <manzurmm@gmail.com>
---
 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

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

[Junio C Hamano]

Manzur Mukhitdinov <manzurmm@gmail.com> writes:

> 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).
>
> Signed-off-by: Manzur Mukhitdinov <manzurmm@gmail.com>
> ---

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

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

[Christian Couder]

[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 <gitster@pobox.com> wrote:
> Manzur Mukhitdinov <manzurmm@gmail.com> writes:
>
>> 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).
>>
>> Signed-off-by: Manzur Mukhitdinov <manzurmm@gmail.com>
>> ---
>
> 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.

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

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> 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 <old,new>.

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.