{"thread":{"id":"51771","subject":"[PATCH] replace: stop replace lookup when reaching a self-reference","startedAt":"2019-08-29T11:22:57Z","lastAt":"2019-08-29T13:52:16Z","messageCount":2,"participants":["Mike Hommey","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"381501","messageId":"20190829112249.32262-1-mh@glandium.org","threadId":"51771","inReplyTo":null,"subject":"[PATCH] replace: stop replace lookup when reaching a self-reference","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2019-08-29T11:22:49Z","receivedAt":"2019-08-29T11:22:57Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"It is possible to end up in situations where a replace ref points to\nitself. In that case, it would seem better to stop the lookup rather\nthan try to follow the link infinitely and fail with \"replace depth too\nhigh\".\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n replace-object.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\nI'm not entirely sure whether it's actually worth fixing. Arguably, `git\nreplace` should also avoid the footgun in the first place (although in\nmy case, it wasn't due to `git replace` itself).\n\ndiff --git a/replace-object.c b/replace-object.c\nindex e295e87943..97e479fe2b 100644\n--- a/replace-object.c\n+++ b/replace-object.c\n@@ -66,7 +66,7 @@ const struct object_id *do_lookup_replace_object(struct repository *r,\n \twhile (depth-- > 0) {\n \t\tstruct replace_object *repl_obj =\n \t\t\toidmap_get(r->objects->replace_map, cur);\n-\t\tif (!repl_obj)\n+\t\tif (!repl_obj || oideq(cur, &repl_obj->replacement))\n \t\t\treturn cur;\n \t\tcur = &repl_obj->replacement;\n \t}\n-- \n2.23.0\n\n"},{"id":"381507","messageId":"20190829135213.GB1797@sigill.intra.peff.net","threadId":"51771","inReplyTo":"20190829112249.32262-1-mh@glandium.org","subject":"Re: [PATCH] replace: stop replace lookup when reaching a self-reference","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-29T13:52:14Z","receivedAt":"2019-08-29T13:52:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 29, 2019 at 08:22:49PM +0900, Mike Hommey wrote:\n\n> It is possible to end up in situations where a replace ref points to\n> itself. In that case, it would seem better to stop the lookup rather\n> than try to follow the link infinitely and fail with \"replace depth too\n> high\".\n\nI don't think this is worth doing. It's just a special case (a 2-node\ncycle in the replace graph) of a more general one (an n-node cycle).\n\nWe catch the general case with the depth counter (though of course there\nare other techniques, which we could debate). Is it worth adding extra\ncode to cover this special one?\n\n> I'm not entirely sure whether it's actually worth fixing. Arguably, `git\n> replace` should also avoid the footgun in the first place (although in\n> my case, it wasn't due to `git replace` itself).\n\nYes, if \"git replace $OID $OID\" does not complain, it probably should.\n\nPerhaps it should even confirm that the replacement can be resolved, and\ndoes not point to the original object. That covers the n-node case, as\nwell.\n\n-Peff\n"}]}