Re: [PATCH] fast-import: avoid making replace refs point to themselves
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 15, 2024, 00:31 UTC
- Message-ID
- <xmqqzfm149sv.fsf@gitster.g>
- In-Reply-To
- <pull.1824.git.1731610074707.gitgitgadget@gmail.com>
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 5 quoted lines
> 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.
> 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.
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?
Show 20 quoted lines
> 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.
Show 9 quoted lines
> + !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.