Re: [PATCH] fast-import: avoid making replace refs point to themselves

[Junio C Hamano <gitster@pobox.com>]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> 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.