fast-import: avoid making replace refs point to themselves

Commit Message

Elijah Newren Nov. 14, 2024, 6:47 p.m. UTC

From: Elijah Newren <[e-mail]>

If someone replaces a commit with a modified version, then builds on
that commit, and then later decides to rewrite history in a format like

    git fast-export --all | CMD_TO_TWEAK_THE_STREAM | git fast-import

and CMD_TO_TWEAK_THE_STREAM undoes the modifications that the
replacement did, then at the end you'd get a replace ref that points to
itself.  For example:

    $ git show-ref | grep replace
    fb92ebc654641b310e7d0360d0a5a49316fd7264 refs/replace/fb92ebc654641b310e7d0360d0a5a49316fd7264

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

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.

Signed-off-by: Elijah Newren <[e-mail]>
---
    fast-import: avoid making replace refs point to themselves

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1824%2Fnewren%2Ffast-import-self-pointing-replace-ref-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1824/newren/fast-import-self-pointing-replace-ref-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1824

 builtin/fast-import.c  | 16 +++++++++++++++-
 t/t9300-fast-import.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)


base-commit: 8f8d6eee531b3fa1a8ef14f169b0cb5035f7a772

Comments

Junio C Hamano Nov. 15, 2024, 12:31 a.m. UTC | #1

"Elijah Newren via GitGitGadget" <[e-mail]> 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.

Junio C Hamano Nov. 15, 2024, 1:08 a.m. UTC | #2

Junio C Hamano <[e-mail]> writes:

> 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?").

Forgot to add.  (1) could be done even at a lower layer, i.e. the
ref API could be told to reject such a replace ref creation/update
that maps an object name to itself.

Patch

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) &&
+	    !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;
+	}
 	if (is_null_oid(&b->oid)) {
 		if (b->delete)
 			refs_delete_ref(get_main_ref_store(the_repository),
@@ -3388,6 +3401,7 @@  static int parse_one_option(const char *option)
 		option_export_pack_edges(option);
 	} else if (!strcmp(option, "quiet")) {
 		show_stats = 0;
+		quiet = 1;
 	} else if (!strcmp(option, "stats")) {
 		show_stats = 1;
 	} else if (!strcmp(option, "allow-unsafe-features")) {
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 6224f54d4d2..425a261c161 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -3692,6 +3692,34 @@  test_expect_success ICONV 'X: handling encoding' '
 	git log -1 --format=%B encoding | grep $(printf "\317\200")
 '
 
+test_expect_success 'X: replace ref that becomes useless is removed' '
+	git init -qb main testrepo &&
+	cd testrepo &&
+	(
+		test_commit test &&
+
+		test_commit msg somename content &&
+
+		git mv somename othername &&
+		NEW_TREE=$(git write-tree) &&
+		MSG="$(git log -1 --format=%B HEAD)" &&
+		NEW_COMMIT=$(git commit-tree -p HEAD^1 -m "$MSG" $NEW_TREE) &&
+		git replace main $NEW_COMMIT &&
+
+		echo more >>othername &&
+		git add othername &&
+		git commit -qm more &&
+
+		git fast-export --all >tmp &&
+		sed -e s/othername/somename/ tmp >tmp2 &&
+		git fast-import --force <tmp2 2>msgs &&
+
+		grep "Dropping.*since it would point to itself" msgs &&
+		git show-ref >refs &&
+		! grep refs/replace refs
+	)
+'
+
 ###
 ### series Y (submodules and hash algorithms)
 ###