{"thread":{"id":"62492","subject":"[PATCH] fast-import: avoid making replace refs point to themselves","startedAt":"2024-11-14T18:47:58Z","lastAt":"2024-11-19T00:44:13Z","messageCount":8,"participants":["Elijah Newren via GitGitGadget","Junio C Hamano","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"507294","messageId":"pull.1824.git.1731610074707.gitgitgadget@gmail.com","threadId":"62492","inReplyTo":null,"subject":"[PATCH] fast-import: avoid making replace refs point to themselves","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-14T18:47:54Z","receivedAt":"2024-11-14T18:47:58Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nIf someone replaces a commit with a modified version, then builds on\nthat commit, and then later decides to rewrite history in a format like\n\n    git fast-export --all | CMD_TO_TWEAK_THE_STREAM | git fast-import\n\nand CMD_TO_TWEAK_THE_STREAM undoes the modifications that the\nreplacement did, then at the end you'd get a replace ref that points to\nitself.  For example:\n\n    $ git show-ref | grep replace\n    fb92ebc654641b310e7d0360d0a5a49316fd7264 refs/replace/fb92ebc654641b310e7d0360d0a5a49316fd7264\n\nMost git commands that you try to run in such a repository with a\nself-pointing replace object will result in an error:\n\n    $ git log\n    fatal: replace depth too high for object fb92ebc654641b310e7d0360d0a5a49316fd7264\n\nAvoid such problems by deleting replace refs that will simply end up\npointing to themselves at the end of our writing.  Warn the users when\nwe do so, unless they specify --quiet.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n    fast-import: avoid making replace refs point to themselves\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1824%2Fnewren%2Ffast-import-self-pointing-replace-ref-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1824/newren/fast-import-self-pointing-replace-ref-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1824\n\n builtin/fast-import.c  | 16 +++++++++++++++-\n t/t9300-fast-import.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 76d5c20f141..51c8228cb7b 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -179,6 +179,7 @@ static unsigned long branch_load_count;\n static int failure;\n static FILE *pack_edges;\n static unsigned int show_stats = 1;\n+static unsigned int quiet;\n static int global_argc;\n static const char **global_argv;\n static const char *global_prefix;\n@@ -1602,7 +1603,19 @@ static int update_branch(struct branch *b)\n \tstruct ref_transaction *transaction;\n \tstruct object_id old_oid;\n \tstruct strbuf err = STRBUF_INIT;\n-\n+\tstatic const char *replace_prefix = \"refs/replace/\";\n+\n+\tif (starts_with(b->name, replace_prefix) &&\n+\t    !strcmp(b->name + strlen(replace_prefix),\n+\t\t    oid_to_hex(&b->oid))) {\n+\t\tif (!quiet)\n+\t\t\twarning(\"Dropping %s since it would point to \"\n+\t\t\t\t\"itself (i.e. to %s)\",\n+\t\t\t\tb->name, oid_to_hex(&b->oid));\n+\t\trefs_delete_ref(get_main_ref_store(the_repository),\n+\t\t\t\tNULL, b->name, NULL, 0);\n+\t\treturn 0;\n+\t}\n \tif (is_null_oid(&b->oid)) {\n \t\tif (b->delete)\n \t\t\trefs_delete_ref(get_main_ref_store(the_repository),\n@@ -3388,6 +3401,7 @@ static int parse_one_option(const char *option)\n \t\toption_export_pack_edges(option);\n \t} else if (!strcmp(option, \"quiet\")) {\n \t\tshow_stats = 0;\n+\t\tquiet = 1;\n \t} else if (!strcmp(option, \"stats\")) {\n \t\tshow_stats = 1;\n \t} else if (!strcmp(option, \"allow-unsafe-features\")) {\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 6224f54d4d2..425a261c161 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -3692,6 +3692,34 @@ test_expect_success ICONV 'X: handling encoding' '\n \tgit log -1 --format=%B encoding | grep $(printf \"\\317\\200\")\n '\n \n+test_expect_success 'X: replace ref that becomes useless is removed' '\n+\tgit init -qb main testrepo &&\n+\tcd testrepo &&\n+\t(\n+\t\ttest_commit test &&\n+\n+\t\ttest_commit msg somename content &&\n+\n+\t\tgit mv somename othername &&\n+\t\tNEW_TREE=$(git write-tree) &&\n+\t\tMSG=\"$(git log -1 --format=%B HEAD)\" &&\n+\t\tNEW_COMMIT=$(git commit-tree -p HEAD^1 -m \"$MSG\" $NEW_TREE) &&\n+\t\tgit replace main $NEW_COMMIT &&\n+\n+\t\techo more >>othername &&\n+\t\tgit add othername &&\n+\t\tgit commit -qm more &&\n+\n+\t\tgit fast-export --all >tmp &&\n+\t\tsed -e s/othername/somename/ tmp >tmp2 &&\n+\t\tgit fast-import --force <tmp2 2>msgs &&\n+\n+\t\tgrep \"Dropping.*since it would point to itself\" msgs &&\n+\t\tgit show-ref >refs &&\n+\t\t! grep refs/replace refs\n+\t)\n+'\n+\n ###\n ### series Y (submodules and hash algorithms)\n ###\n\nbase-commit: 8f8d6eee531b3fa1a8ef14f169b0cb5035f7a772\n-- \ngitgitgadget\n"},{"id":"507303","messageId":"xmqqzfm149sv.fsf@gitster.g","threadId":"62492","inReplyTo":"pull.1824.git.1731610074707.gitgitgadget@gmail.com","subject":"Re: [PATCH] fast-import: avoid making replace refs point to themselves","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-15T00:31:28Z","receivedAt":"2024-11-15T00:31:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> Most git commands that you try to run in such a repository with a\n> self-pointing replace object will result in an error:\n>\n>     $ git log\n>     fatal: replace depth too high for object fb92ebc654641b310e7d0360d0a5a49316fd7264\n\nAfter reading up to this point, with \"Most git commands\", I was\nafraid that you were going to say \"... but I found this command that\nfails to stop gracefully, and instead infinitely spin\".\n\nIf that is not a case, then I am happy ;-) but in that case probably\n\"Most\" -> \"All\" is warranted.\n\n> Avoid such problems by deleting replace refs that will simply end up\n> pointing to themselves at the end of our writing.  Warn the users when\n> we do so, unless they specify --quiet.\n\nThis may need a bit of rephrasing.\n\nEven when they specify \"--quiet\", you'd refrain from creating a\nself-referencing replace entry, which makes sense, but it was not\nclear with only the above description.  I had to read the patch text\nto find it out.\n\nIs it reasonable to expect that a self referencing replace entry is\nthe most common thing to happen, or loops with more than one\nelements are equally plausible to happen but the self-referencing\none is singled out by this patch because it is trivial to notice,\nunlike other forms of equally problematic loops?\n\n> diff --git a/builtin/fast-import.c b/builtin/fast-import.c\n> index 76d5c20f141..51c8228cb7b 100644\n> --- a/builtin/fast-import.c\n> +++ b/builtin/fast-import.c\n> @@ -179,6 +179,7 @@ static unsigned long branch_load_count;\n>  static int failure;\n>  static FILE *pack_edges;\n>  static unsigned int show_stats = 1;\n> +static unsigned int quiet;\n>  static int global_argc;\n>  static const char **global_argv;\n>  static const char *global_prefix;\n> @@ -1602,7 +1603,19 @@ static int update_branch(struct branch *b)\n>  \tstruct ref_transaction *transaction;\n>  \tstruct object_id old_oid;\n>  \tstruct strbuf err = STRBUF_INIT;\n> -\n> +\tstatic const char *replace_prefix = \"refs/replace/\";\n> +\n> +\tif (starts_with(b->name, replace_prefix) &&\n\nCurious how the \"diff\" machinery decided to represent the hunk like\nthis, instead of say\n\n>  \tstruct strbuf err = STRBUF_INIT;\n> +\tstatic const char *replace_prefix = \"refs/replace/\";\n> \n> +\tif (starts_with(b->name, replace_prefix) &&\n\nOf course that has nothing to do with this patch or its review.\n\n> +\t    !strcmp(b->name + strlen(replace_prefix),\n> +\t\t    oid_to_hex(&b->oid))) {\n> +\t\tif (!quiet)\n> +\t\t\twarning(\"Dropping %s since it would point to \"\n> +\t\t\t\t\"itself (i.e. to %s)\",\n> +\t\t\t\tb->name, oid_to_hex(&b->oid));\n> +\t\trefs_delete_ref(get_main_ref_store(the_repository),\n> +\t\t\t\tNULL, b->name, NULL, 0);\n> +\t\treturn 0;\n\nI am not sure if a warning is even warranted.  If you decide to\nreplace an object A with the same object A, the result ought to be a\nno-op.  I wonder if it is makes more sense to\n\n (1) do this unconditionally and silently, and\n (2) when we prepare the replace_map, ignore self-referencing ones.\n\ninstead.  If (2) makes sense entirely depends on the answer of an\nearlier question (i.e. \"is there a reason why self-reference is more\ncommon than general loop?\").\n\nThanks.\n"},{"id":"507306","messageId":"xmqqv7wp483v.fsf@gitster.g","threadId":"62492","inReplyTo":"xmqqzfm149sv.fsf@gitster.g","subject":"Re: [PATCH] fast-import: avoid making replace refs point to themselves","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-15T01:08:04Z","receivedAt":"2024-11-15T01:08:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I am not sure if a warning is even warranted.  If you decide to\n> replace an object A with the same object A, the result ought to be a\n> no-op.  I wonder if it is makes more sense to\n>\n>  (1) do this unconditionally and silently, and\n>  (2) when we prepare the replace_map, ignore self-referencing ones.\n>\n> instead.  If (2) makes sense entirely depends on the answer of an\n> earlier question (i.e. \"is there a reason why self-reference is more\n> common than general loop?\").\n\nForgot to add.  (1) could be done even at a lower layer, i.e. the\nref API could be told to reject such a replace ref creation/update\nthat maps an object name to itself.\n"},{"id":"507309","messageId":"CABPp-BE2651h6MjPPOvmvgQAp_RrcRW3wuf4_N=DoX3EvJ-muQ@mail.gmail.com","threadId":"62492","inReplyTo":"xmqqzfm149sv.fsf@gitster.g","subject":"Re: [PATCH] fast-import: avoid making replace refs point to themselves","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-11-15T03:15:26Z","receivedAt":"2024-11-15T03:15:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Nov 14, 2024 at 4:31 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Elijah Newren via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > Most git commands that you try to run in such a repository with a\n> > self-pointing replace object will result in an error:\n> >\n> >     $ git log\n> >     fatal: replace depth too high for object fb92ebc654641b310e7d0360d0a5a49316fd7264\n>\n> After reading up to this point, with \"Most git commands\", I was\n> afraid that you were going to say \"... but I found this command that\n> fails to stop gracefully, and instead infinitely spin\".\n>\n> If that is not a case, then I am happy ;-) but in that case probably\n> \"Most\" -> \"All\" is warranted.\n\nSome git commands are not bothered by the existence of the problematic\nreplace ref; e.g.:\n\n$ git show-ref\n64026eaa60e0208a00946cdd3cb41b059c6675bd refs/heads/main\nfb92ebc654641b310e7d0360d0a5a49316fd7264\nrefs/replace/fb92ebc654641b310e7d0360d0a5a49316fd7264\nfb92ebc654641b310e7d0360d0a5a49316fd7264 refs/tags/msg\n14f901a95ebae912feb4805f40ef68f15b0192c2 refs/tags/test\n\nSo, it's not \"all\" git commands.  Maybe I should rephrase as:\n\n\"\"\"\nSome git commands will ignore such replace refs, but many will fail\nwith an error:\n\n$ git log\nfatal: replace depth too high for object\nfb92ebc654641b310e7d0360d0a5a49316fd7264\n\"\"\"\n\n?\n\n> > Avoid such problems by deleting replace refs that will simply end up\n> > pointing to themselves at the end of our writing.  Warn the users when\n> > we do so, unless they specify --quiet.\n>\n> This may need a bit of rephrasing.\n>\n> Even when they specify \"--quiet\", you'd refrain from creating a\n> self-referencing replace entry, which makes sense, but it was not\n> clear with only the above description.  I had to read the patch text\n> to find it out.\n\nWould reordering the two phrases of the last sentence do it?\n\n\"\"\"\nAvoid such problems by deleting replace refs that will simply end up\npointing to themselves at the end of our writing.  Unless users specify --quiet,\nwarn the users when we do so.\n\"\"\"\n\n> Is it reasonable to expect that a self referencing replace entry is\n> the most common thing to happen, or loops with more than one\n> elements are equally plausible to happen but the self-referencing\n> one is singled out by this patch because it is trivial to notice,\n> unlike other forms of equally problematic loops?\n\nLoops with more than one element are possible, but I couldn't see an\nangle by which they are plausible.  In particular, I know how a user\ncan (and did) accidentally trigger a self-referencing replace ref (if\nyou're curious, see\nhttps://github.com/newren/git-filter-repo/issues/401; it's much less\nlikely to trigger now due to an unrelated change to the default for\n`--replace-refs`, but could still be triggered in combination with\nthat option).  There are many variations on their theme of \"do a\nchange, then undo it\" (in combination with the old `--replace-refs`\ndefault), but I don't see how to either vary their testcase or come up\nwith some other way to get git-filter-repo to trigger a replace ref\nloop with more than one element.\n\n> > diff --git a/builtin/fast-import.c b/builtin/fast-import.c\n> > index 76d5c20f141..51c8228cb7b 100644\n> > --- a/builtin/fast-import.c\n> > +++ b/builtin/fast-import.c\n> > @@ -179,6 +179,7 @@ static unsigned long branch_load_count;\n> >  static int failure;\n> >  static FILE *pack_edges;\n> >  static unsigned int show_stats = 1;\n> > +static unsigned int quiet;\n> >  static int global_argc;\n> >  static const char **global_argv;\n> >  static const char *global_prefix;\n> > @@ -1602,7 +1603,19 @@ static int update_branch(struct branch *b)\n> >       struct ref_transaction *transaction;\n> >       struct object_id old_oid;\n> >       struct strbuf err = STRBUF_INIT;\n> > -\n> > +     static const char *replace_prefix = \"refs/replace/\";\n> > +\n> > +     if (starts_with(b->name, replace_prefix) &&\n>\n> Curious how the \"diff\" machinery decided to represent the hunk like\n> this, instead of say\n>\n> >       struct strbuf err = STRBUF_INIT;\n> > +     static const char *replace_prefix = \"refs/replace/\";\n> >\n> > +     if (starts_with(b->name, replace_prefix) &&\n>\n> Of course that has nothing to do with this patch or its review.\n\nOh, that is kinda weird.  It's not a whitespace-change issue either;\nboth the removed and added blank-looking lines really are blank lines.\nNo idea why the diff machinery does that.\n\n> > +         !strcmp(b->name + strlen(replace_prefix),\n> > +                 oid_to_hex(&b->oid))) {\n> > +             if (!quiet)\n> > +                     warning(\"Dropping %s since it would point to \"\n> > +                             \"itself (i.e. to %s)\",\n> > +                             b->name, oid_to_hex(&b->oid));\n> > +             refs_delete_ref(get_main_ref_store(the_repository),\n> > +                             NULL, b->name, NULL, 0);\n> > +             return 0;\n>\n> I am not sure if a warning is even warranted.  If you decide to\n> replace an object A with the same object A, the result ought to be a\n> no-op.  I wonder if it is makes more sense to\n>\n>  (1) do this unconditionally and silently, and\n>  (2) when we prepare the replace_map, ignore self-referencing ones.\n>\n> instead.  If (2) makes sense entirely depends on the answer of an\n> earlier question (i.e. \"is there a reason why self-reference is more\n> common than general loop?\").\n>\n> Thanks.\n\nSince you added more in a subsequent response, I'll go respond to that.\n"},{"id":"507310","messageId":"CABPp-BGOLfeUn6H3cdeMf5tZ1gswoaR51J+PDhMB4akR3jG+LA@mail.gmail.com","threadId":"62492","inReplyTo":"xmqqv7wp483v.fsf@gitster.g","subject":"Re: [PATCH] fast-import: avoid making replace refs point to themselves","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-11-15T03:41:00Z","receivedAt":"2024-11-15T03:41:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Nov 14, 2024 at 5:08 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n> > I am not sure if a warning is even warranted.  If you decide to\n> > replace an object A with the same object A, the result ought to be a\n> > no-op.\n\nDo you mean that the result ought to be that there is no replacement?\nIf so, I agree, but that's not always equivalent to a no-op...\n\n> >  I wonder if it is makes more sense to\n> >\n> >  (1) do this unconditionally and silently, and\n> >  (2) when we prepare the replace_map, ignore self-referencing ones.\n> >\n> > instead.  If (2) makes sense entirely depends on the answer of an\n> > earlier question (i.e. \"is there a reason why self-reference is more\n> > common than general loop?\").\n>\n> Forgot to add.  (1) could be done even at a lower layer, i.e. the\n> ref API could be told to reject such a replace ref creation/update\n> that maps an object name to itself.\n\nPerhaps rejecting the creation or update of a replace ref would make\nsense at a lower level, but a no-op isn't what I want here -- it'd\njust result in a different bug.  In particular, note that my patch\nmodifies fast-import to issue a delete where it would otherwise issue\nan update instead of merely rejecting the update.  If the repository\nhad a value for the replace ref before fast-import was run, and the\nreplace ref was explicitly named in the fast-import stream, I don't\nwant the replace ref to be left with a pre-fast-import value.\n"},{"id":"507311","messageId":"xmqqjzd540j1.fsf@gitster.g","threadId":"62492","inReplyTo":"CABPp-BGOLfeUn6H3cdeMf5tZ1gswoaR51J+PDhMB4akR3jG+LA@mail.gmail.com","subject":"Re: [PATCH] fast-import: avoid making replace refs point to themselves","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-15T03:51:46Z","receivedAt":"2024-11-15T03:51:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> ...  If the repository\n> had a value for the replace ref before fast-import was run, and the\n> replace ref was explicitly named in the fast-import stream, I don't\n> want the replace ref to be left with a pre-fast-import value.\n\nAh, right.  We want to honor the user's latest wish, i.e., if they\ncreate a replace ref that maps A to A, when they ask for object A,\ninstead of returning it, we need to return what the replace ref\nrefers to, which happens to be object A.  So you are right. \"You are\ntrying to map A to A, so we'll ignore that request\" is a wrong thing\nto do.  \"In order to give you A when you ask for A, we will remove\nthe existing mapping for A\" is absolutely the right thing to do,\nwhich is what your patch does.\n\nThanks.\n"},{"id":"507548","messageId":"pull.1824.v2.git.1731968389590.gitgitgadget@gmail.com","threadId":"62492","inReplyTo":"pull.1824.git.1731610074707.gitgitgadget@gmail.com","subject":"[PATCH v2] fast-import: avoid making replace refs point to themselves","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-18T22:19:49Z","receivedAt":"2024-11-18T22:19:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nIf someone replaces a commit with a modified version, then builds on\nthat commit, and then later decides to rewrite history in a format like\n\n    git fast-export --all | CMD_TO_TWEAK_THE_STREAM | git fast-import\n\nand CMD_TO_TWEAK_THE_STREAM undoes the modifications that the\nreplacement did, then at the end you'd get a replace ref that points to\nitself.  For example:\n\n    $ git show-ref | grep replace\n    fb92ebc654641b310e7d0360d0a5a49316fd7264 refs/replace/fb92ebc654641b310e7d0360d0a5a49316fd7264\n\nGit commands which pay attention to replace refs will die with an error\nwhen a self-referencing replace ref is present:\n\n    $ git log\n    fatal: replace depth too high for object fb92ebc654641b310e7d0360d0a5a49316fd7264\n\nAvoid such problems by deleting replace refs that will simply end up\npointing to themselves at the end of our writing.  Unless users specify\n--quiet, warn them when we delete such a replace ref.\n\nTwo notes about this patch:\n  * We are not ignoring the problematic update of the replace ref\n    (turning it into a no-op), we are replacing the update with a delete.\n    The logic here is that if the repository had a value for the replace\n    ref before fast-import was run, and the replace ref was explicitly\n    named in the fast-import stream, we don't want the replace ref to be\n    left with a pre-fast-import value.\n  * While loops with more than one element (e.g. refs/replace/A points\n    to B, and refs/replace/B points to A) are possible, they seem much\n    less plausible.  It is pretty easy to create a sequence of\n    git-filter-repo commands that will trigger a self-referencing replace\n    ref, but I do not know how to trigger a scenario with a cycle length\n    greater than 1.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n    fast-import: avoid making replace refs point to themselves\n    \n    Changes since v1: Clarified wording in the commit message, and added a\n    little more text at the end of the commit message to address questions\n    that came up in review.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1824%2Fnewren%2Ffast-import-self-pointing-replace-ref-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1824/newren/fast-import-self-pointing-replace-ref-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1824\n\nRange-diff vs v1:\n\n 1:  7402518e806 ! 1:  cc77a004cea fast-import: avoid making replace refs point to themselves\n     @@ Commit message\n              $ git show-ref | grep replace\n              fb92ebc654641b310e7d0360d0a5a49316fd7264 refs/replace/fb92ebc654641b310e7d0360d0a5a49316fd7264\n      \n     -    Most git commands that you try to run in such a repository with a\n     -    self-pointing replace object will result in an error:\n     +    Git commands which pay attention to replace refs will die with an error\n     +    when a self-referencing replace ref is present:\n      \n              $ git log\n              fatal: replace depth too high for object fb92ebc654641b310e7d0360d0a5a49316fd7264\n      \n          Avoid such problems by deleting replace refs that will simply end up\n     -    pointing to themselves at the end of our writing.  Warn the users when\n     -    we do so, unless they specify --quiet.\n     +    pointing to themselves at the end of our writing.  Unless users specify\n     +    --quiet, warn them when we delete such a replace ref.\n     +\n     +    Two notes about this patch:\n     +      * We are not ignoring the problematic update of the replace ref\n     +        (turning it into a no-op), we are replacing the update with a delete.\n     +        The logic here is that if the repository had a value for the replace\n     +        ref before fast-import was run, and the replace ref was explicitly\n     +        named in the fast-import stream, we don't want the replace ref to be\n     +        left with a pre-fast-import value.\n     +      * While loops with more than one element (e.g. refs/replace/A points\n     +        to B, and refs/replace/B points to A) are possible, they seem much\n     +        less plausible.  It is pretty easy to create a sequence of\n     +        git-filter-repo commands that will trigger a self-referencing replace\n     +        ref, but I do not know how to trigger a scenario with a cycle length\n     +        greater than 1.\n      \n          Signed-off-by: Elijah Newren <newren@gmail.com>\n      \n\n\n builtin/fast-import.c  | 16 +++++++++++++++-\n t/t9300-fast-import.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 76d5c20f141..51c8228cb7b 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -179,6 +179,7 @@ static unsigned long branch_load_count;\n static int failure;\n static FILE *pack_edges;\n static unsigned int show_stats = 1;\n+static unsigned int quiet;\n static int global_argc;\n static const char **global_argv;\n static const char *global_prefix;\n@@ -1602,7 +1603,19 @@ static int update_branch(struct branch *b)\n \tstruct ref_transaction *transaction;\n \tstruct object_id old_oid;\n \tstruct strbuf err = STRBUF_INIT;\n-\n+\tstatic const char *replace_prefix = \"refs/replace/\";\n+\n+\tif (starts_with(b->name, replace_prefix) &&\n+\t    !strcmp(b->name + strlen(replace_prefix),\n+\t\t    oid_to_hex(&b->oid))) {\n+\t\tif (!quiet)\n+\t\t\twarning(\"Dropping %s since it would point to \"\n+\t\t\t\t\"itself (i.e. to %s)\",\n+\t\t\t\tb->name, oid_to_hex(&b->oid));\n+\t\trefs_delete_ref(get_main_ref_store(the_repository),\n+\t\t\t\tNULL, b->name, NULL, 0);\n+\t\treturn 0;\n+\t}\n \tif (is_null_oid(&b->oid)) {\n \t\tif (b->delete)\n \t\t\trefs_delete_ref(get_main_ref_store(the_repository),\n@@ -3388,6 +3401,7 @@ static int parse_one_option(const char *option)\n \t\toption_export_pack_edges(option);\n \t} else if (!strcmp(option, \"quiet\")) {\n \t\tshow_stats = 0;\n+\t\tquiet = 1;\n \t} else if (!strcmp(option, \"stats\")) {\n \t\tshow_stats = 1;\n \t} else if (!strcmp(option, \"allow-unsafe-features\")) {\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 6224f54d4d2..425a261c161 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -3692,6 +3692,34 @@ test_expect_success ICONV 'X: handling encoding' '\n \tgit log -1 --format=%B encoding | grep $(printf \"\\317\\200\")\n '\n \n+test_expect_success 'X: replace ref that becomes useless is removed' '\n+\tgit init -qb main testrepo &&\n+\tcd testrepo &&\n+\t(\n+\t\ttest_commit test &&\n+\n+\t\ttest_commit msg somename content &&\n+\n+\t\tgit mv somename othername &&\n+\t\tNEW_TREE=$(git write-tree) &&\n+\t\tMSG=\"$(git log -1 --format=%B HEAD)\" &&\n+\t\tNEW_COMMIT=$(git commit-tree -p HEAD^1 -m \"$MSG\" $NEW_TREE) &&\n+\t\tgit replace main $NEW_COMMIT &&\n+\n+\t\techo more >>othername &&\n+\t\tgit add othername &&\n+\t\tgit commit -qm more &&\n+\n+\t\tgit fast-export --all >tmp &&\n+\t\tsed -e s/othername/somename/ tmp >tmp2 &&\n+\t\tgit fast-import --force <tmp2 2>msgs &&\n+\n+\t\tgrep \"Dropping.*since it would point to itself\" msgs &&\n+\t\tgit show-ref >refs &&\n+\t\t! grep refs/replace refs\n+\t)\n+'\n+\n ###\n ### series Y (submodules and hash algorithms)\n ###\n\nbase-commit: 8f8d6eee531b3fa1a8ef14f169b0cb5035f7a772\n-- \ngitgitgadget\n"},{"id":"507558","messageId":"xmqq8qtghx2d.fsf@gitster.g","threadId":"62492","inReplyTo":"pull.1824.v2.git.1731968389590.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] fast-import: avoid making replace refs point to themselves","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-19T00:44:10Z","receivedAt":"2024-11-19T00:44:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> +\t\tgrep \"Dropping.*since it would point to itself\" msgs &&\n> +\t\tgit show-ref >refs &&\n> +\t\t! grep refs/replace refs\n\n\"test_grep !\" may make a failure case easier to diagnose.\n\nOther than that, looking good.\n\nThanks, will queue.\n"}]}