threads / patch / 51771

patchreplace: stop replace lookup when reaching a self-reference

Subject: [PATCH] replace: stop replace lookup when reaching a self-reference

## tl;dr

2 messages between Aug 29, 2019 and Aug 29, 2019. Diffs are folded; open one to read it.

replies: 1people: 2as markdown or json

Mike Hommey· Aug 29, 2019, 11:22 UTC · lore

It is possible to end up in situations where a replace ref points to itself. In that case, it would seem better to stop the lookup rather than try to follow the link infinitely and fail with "replace depth too high".

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 replace-object.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

I'm not entirely sure whether it's actually worth fixing. Arguably, `git replace` should also avoid the footgun in the first place (although in my case, it wasn't due to `git replace` itself).

Show changes to replace-object.c +1 −1

diff --git a/replace-object.c b/replace-object.c
index e295e87943..97e479fe2b 100644
--- a/replace-object.c
+++ b/replace-object.c
@@ -66,7 +66,7 @@ const struct object_id *do_lookup_replace_object(struct repository *r,
 	while (depth-- > 0) {
 		struct replace_object *repl_obj =
 			oidmap_get(r->objects->replace_map, cur);
-		if (!repl_obj)
+		if (!repl_obj || oideq(cur, &repl_obj->replacement))
 			return cur;
 		cur = &repl_obj->replacement;
 	}

-- 
2.23.0

Jeff King· Aug 29, 2019, 13:52 UTC · re: Mike Hommey · lore

Re: [PATCH] replace: stop replace lookup when reaching a self-reference

On Thu, Aug 29, 2019 at 08:22:49PM +0900, Mike Hommey wrote:

> It is possible to end up in situations where a replace ref points to
> itself. In that case, it would seem better to stop the lookup rather
> than try to follow the link infinitely and fail with "replace depth too
> high".

I don't think this is worth doing. It's just a special case (a 2-node cycle in the replace graph) of a more general one (an n-node cycle).

We catch the general case with the depth counter (though of course there are other techniques, which we could debate). Is it worth adding extra code to cover this special one?

> I'm not entirely sure whether it's actually worth fixing. Arguably, `git
> replace` should also avoid the footgun in the first place (although in
> my case, it wasn't due to `git replace` itself).

Yes, if "git replace $OID $OID" does not complain, it probably should.

Perhaps it should even confirm that the replacement can be resolved, and does not point to the original object. That covers the n-node case, as well.

-Peff