On Thu, Nov 14, 2024 at 4:31 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > "Elijah Newren via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > > > Most git commands that you try to run in such a repository with a > > self-pointing replace object will result in an error: > > > > $ git log > > fatal: replace depth too high for object fb92ebc654641b310e7d0360d0a5a49316fd7264 > > After reading up to this point, with "Most git commands", I was > afraid that you were going to say "... but I found this command that > fails to stop gracefully, and instead infinitely spin". > > If that is not a case, then I am happy ;-) but in that case probably > "Most" -> "All" is warranted. Some git commands are not bothered by the existence of the problematic replace ref; e.g.: $ git show-ref 64026eaa60e0208a00946cdd3cb41b059c6675bd refs/heads/main fb92ebc654641b310e7d0360d0a5a49316fd7264 refs/replace/fb92ebc654641b310e7d0360d0a5a49316fd7264 fb92ebc654641b310e7d0360d0a5a49316fd7264 refs/tags/msg 14f901a95ebae912feb4805f40ef68f15b0192c2 refs/tags/test So, it's not "all" git commands. Maybe I should rephrase as: """ Some git commands will ignore such replace refs, but many will fail with an error: $ git log fatal: replace depth too high for object fb92ebc654641b310e7d0360d0a5a49316fd7264 """ ? > > Avoid such problems by deleting replace refs that will simply end up > > pointing to themselves at the end of our writing. Warn the users when > > we do so, unless they specify --quiet. > > This may need a bit of rephrasing. > > Even when they specify "--quiet", you'd refrain from creating a > self-referencing replace entry, which makes sense, but it was not > clear with only the above description. I had to read the patch text > to find it out. Would reordering the two phrases of the last sentence do it? """ Avoid such problems by deleting replace refs that will simply end up pointing to themselves at the end of our writing. Unless users specify --quiet, warn the users when we do so. """ > Is it reasonable to expect that a self referencing replace entry is > the most common thing to happen, or loops with more than one > elements are equally plausible to happen but the self-referencing > one is singled out by this patch because it is trivial to notice, > unlike other forms of equally problematic loops? Loops with more than one element are possible, but I couldn't see an angle by which they are plausible. In particular, I know how a user can (and did) accidentally trigger a self-referencing replace ref (if you're curious, see https://github.com/newren/git-filter-repo/issues/401; it's much less likely to trigger now due to an unrelated change to the default for `--replace-refs`, but could still be triggered in combination with that option). There are many variations on their theme of "do a change, then undo it" (in combination with the old `--replace-refs` default), but I don't see how to either vary their testcase or come up with some other way to get git-filter-repo to trigger a replace ref loop with more than one element. > > diff --git a/builtin/fast-import.c b/builtin/fast-import.c > > index 76d5c20f141..51c8228cb7b 100644 > > --- a/builtin/fast-import.c > > +++ b/builtin/fast-import.c > > @@ -179,6 +179,7 @@ static unsigned long branch_load_count; > > static int failure; > > static FILE *pack_edges; > > static unsigned int show_stats = 1; > > +static unsigned int quiet; > > static int global_argc; > > static const char **global_argv; > > static const char *global_prefix; > > @@ -1602,7 +1603,19 @@ static int update_branch(struct branch *b) > > struct ref_transaction *transaction; > > struct object_id old_oid; > > struct strbuf err = STRBUF_INIT; > > - > > + static const char *replace_prefix = "refs/replace/"; > > + > > + if (starts_with(b->name, replace_prefix) && > > Curious how the "diff" machinery decided to represent the hunk like > this, instead of say > > > struct strbuf err = STRBUF_INIT; > > + static const char *replace_prefix = "refs/replace/"; > > > > + if (starts_with(b->name, replace_prefix) && > > Of course that has nothing to do with this patch or its review. Oh, that is kinda weird. It's not a whitespace-change issue either; both the removed and added blank-looking lines really are blank lines. No idea why the diff machinery does that. > > + !strcmp(b->name + strlen(replace_prefix), > > + oid_to_hex(&b->oid))) { > > + if (!quiet) > > + warning("Dropping %s since it would point to " > > + "itself (i.e. to %s)", > > + b->name, oid_to_hex(&b->oid)); > > + refs_delete_ref(get_main_ref_store(the_repository), > > + NULL, b->name, NULL, 0); > > + return 0; > > I am not sure if a warning is even warranted. If you decide to > replace an object A with the same object A, the result ought to be a > no-op. I wonder if it is makes more sense to > > (1) do this unconditionally and silently, and > (2) when we prepare the replace_map, ignore self-referencing ones. > > instead. If (2) makes sense entirely depends on the answer of an > earlier question (i.e. "is there a reason why self-reference is more > common than general loop?"). > > Thanks. Since you added more in a subsequent response, I'll go respond to that.
