"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.
> 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?
> 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.
> + !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.
