diff mbox series

[v3,4/4] unbundle: introduce new option UNBUNDLE_FSCK_FOLLOW_FETCH

Message ID c19b8f633cb9f851948b3a17b5425f3310d900d5.1716824518.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series object checking related additions and fixes for bundles in fetches | expand

Commit Message

Xing Xin May 27, 2024, 3:41 p.m. UTC 
From: Xing Xin <xingxin.xx@bytedance.com>

This commit adds a new option `UNBUNDLE_FSCK_FOLLOW_FETCH` to
`unbundle_fsck_flags`, this new flag is currently used in the _fetch_
process by

- `transport.c:fetch_refs_from_bundle` for fetching directly from a
  bundle.
- `bundle-uri.c:unbundle_from_file` for unbundling bundles downloaded
  from bundle-uri.

So we now have a relatively consistent logic for checking objects during
fetching. Add tests for the above two situations are added.

Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
---
 bundle-uri.c                |  2 +-
 bundle.c                    | 10 +++++++++-
 bundle.h                    |  1 +
 t/t5558-clone-bundle-uri.sh | 36 +++++++++++++++++++++++++++++++-----
 t/t5607-clone-bundle.sh     | 23 +++++++++++++++++++++++
 transport.c                 |  2 +-
 6 files changed, 66 insertions(+), 8 deletions(-)

Comments

Patrick Steinhardt May 28, 2024, 12:05 p.m. UTC | #1 
On Mon, May 27, 2024 at 03:41:57PM +0000, Xing Xin via GitGitGadget wrote:
> From: Xing Xin <xingxin.xx@bytedance.com>
> diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh
> index 0d1e92d9963..423b35ac237 100755
> --- a/t/t5607-clone-bundle.sh
> +++ b/t/t5607-clone-bundle.sh
> @@ -138,6 +138,29 @@ test_expect_success 'fetch SHA-1 from bundle' '
>  	git fetch --no-tags foo/tip.bundle "$(cat hash)"
>  '
>  
> +test_expect_success 'clone bundle with fetch.fsckObjects' '
> +	test_create_repo bundle-fsck &&
> +	(
> +		cd bundle-fsck &&
> +		test_commit first &&
> +		cat >data <<-EOF &&
> +		tree $(git rev-parse HEAD^{tree})
> +		parent $(git rev-parse HEAD)
> +		author A U Thor
> +		committer A U Thor
> +
> +		commit: this is a commit with bad emails
> +
> +		EOF
> +		git hash-object --literally -t commit -w --stdin <data >commit &&
> +		git branch bad $(cat commit) &&
> +		git bundle create bad.bundle bad
> +	) &&
> +	test_must_fail git -c fetch.fsckObjects=true \
> +		clone bundle-fsck/bad.bundle bundle-fsck-clone 2>err &&
> +	test_grep "missingEmail" err
> +'

Do we also want to have a test for `transfer.fsckObjects`?

Patrick
Xing Xin May 30, 2024, 8:54 a.m. UTC | #2 
At 2024-05-28 20:05:48, "Patrick Steinhardt" <ps@pks.im> wrote:
[snip]
>>  
>> +test_expect_success 'clone bundle with fetch.fsckObjects' '
>> +	test_create_repo bundle-fsck &&
>> +	(
>> +		cd bundle-fsck &&
>> +		test_commit first &&
>> +		cat >data <<-EOF &&
>> +		tree $(git rev-parse HEAD^{tree})
>> +		parent $(git rev-parse HEAD)
>> +		author A U Thor
>> +		committer A U Thor
>> +
>> +		commit: this is a commit with bad emails
>> +
>> +		EOF
>> +		git hash-object --literally -t commit -w --stdin <data >commit &&
>> +		git branch bad $(cat commit) &&
>> +		git bundle create bad.bundle bad
>> +	) &&
>> +	test_must_fail git -c fetch.fsckObjects=true \
>> +		clone bundle-fsck/bad.bundle bundle-fsck-clone 2>err &&
>> +	test_grep "missingEmail" err
>> +'
>
>Do we also want to have a test for `transfer.fsckObjects`?

Sure, some more combinations of "fetch.fsckObjects" and "transfer.fsckObjects"
are added in tests.

Xing Xin
diff mbox series

Patch

diff --git a/bundle-uri.c b/bundle-uri.c
index 80f02aac6f1..0da3e5a61b9 100644
--- a/bundle-uri.c
+++ b/bundle-uri.c
@@ -373,7 +373,7 @@  static int unbundle_from_file(struct repository *r, const char *file)
 	 * the prerequisite commits.
 	 */
 	if ((result = unbundle(r, &header, bundle_fd, NULL,
-			       VERIFY_BUNDLE_QUIET, UNBUNDLE_FSCK_ALWAYS)))
+			       VERIFY_BUNDLE_QUIET, UNBUNDLE_FSCK_FOLLOW_FETCH)))
 		return 1;
 
 	/*
diff --git a/bundle.c b/bundle.c
index a922d592782..c7344543aa4 100644
--- a/bundle.c
+++ b/bundle.c
@@ -17,6 +17,7 @@ 
 #include "list-objects-filter-options.h"
 #include "connected.h"
 #include "write-or-die.h"
+#include "fetch-pack.h"
 
 static const char v2_bundle_signature[] = "# v2 git bundle\n";
 static const char v3_bundle_signature[] = "# v3 git bundle\n";
@@ -616,6 +617,7 @@  int unbundle(struct repository *r, struct bundle_header *header,
 	     enum unbundle_fsck_flags fsck_flags)
 {
 	struct child_process ip = CHILD_PROCESS_INIT;
+	int fsck_objects = 0;
 
 	if (verify_bundle(r, header, flags))
 		return -1;
@@ -628,13 +630,19 @@  int unbundle(struct repository *r, struct bundle_header *header,
 
 	switch (fsck_flags) {
 	case UNBUNDLE_FSCK_ALWAYS:
-		strvec_push(&ip.args, "--fsck-objects");
+		fsck_objects = 1;
+		break;
+	case UNBUNDLE_FSCK_FOLLOW_FETCH:
+		fsck_objects = fetch_pack_fsck_objects();
 		break;
 	case UNBUNDLE_FSCK_NEVER:
 	default:
 		break;
 	}
 
+	if (fsck_objects)
+		strvec_push(&ip.args, "--fsck-objects");
+
 	if (extra_index_pack_args) {
 		strvec_pushv(&ip.args, extra_index_pack_args->v);
 		strvec_clear(extra_index_pack_args);
diff --git a/bundle.h b/bundle.h
index cfa9daddda6..c46488422ce 100644
--- a/bundle.h
+++ b/bundle.h
@@ -33,6 +33,7 @@  int create_bundle(struct repository *r, const char *path,
 enum unbundle_fsck_flags {
 	UNBUNDLE_FSCK_NEVER = 0,
 	UNBUNDLE_FSCK_ALWAYS,
+	UNBUNDLE_FSCK_FOLLOW_FETCH,
 };
 
 enum verify_bundle_flags {
diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh
index a5b04d6f187..3df4d44e78f 100755
--- a/t/t5558-clone-bundle-uri.sh
+++ b/t/t5558-clone-bundle-uri.sh
@@ -19,13 +19,30 @@  test_expect_success 'fail to clone from non-bundle file' '
 
 test_expect_success 'create bundle' '
 	git init clone-from &&
-	git -C clone-from checkout -b topic &&
+	(
+		cd clone-from &&
+		git checkout -b topic &&
+
+		test_commit A &&
+		git bundle create A.bundle topic &&
+
+		test_commit B &&
+		git bundle create B.bundle topic &&
+
+		cat >data <<-EOF &&
+		tree $(git rev-parse HEAD^{tree})
+		parent $(git rev-parse HEAD)
+		author A U Thor
+		committer A U Thor
 
-	test_commit -C clone-from A &&
-	git -C clone-from bundle create A.bundle topic &&
+		commit: this is a commit with bad emails
 
-	test_commit -C clone-from B &&
-	git -C clone-from bundle create B.bundle topic
+		EOF
+		git hash-object --literally -t commit -w --stdin <data >commit &&
+		git branch bad $(cat commit) &&
+		git bundle create bad.bundle bad &&
+		git update-ref -d refs/heads/bad
+	)
 '
 
 test_expect_success 'clone with path bundle' '
@@ -36,6 +53,15 @@  test_expect_success 'clone with path bundle' '
 	test_cmp expect actual
 '
 
+test_expect_success 'clone with bad bundle' '
+	git -c fetch.fsckObjects=true clone --bundle-uri="clone-from/bad.bundle" \
+		clone-from clone-bad 2>err &&
+	# Unbundle fails, but clone can still proceed.
+	test_grep "missingEmail" err &&
+	git -C clone-bad for-each-ref --format="%(refname)" >refs &&
+	! grep "refs/bundles/" refs
+'
+
 test_expect_success 'clone with path bundle and non-default hash' '
 	test_when_finished "rm -rf clone-path-non-default-hash" &&
 	GIT_DEFAULT_HASH=sha256 git clone --bundle-uri="clone-from/B.bundle" \
diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh
index 0d1e92d9963..423b35ac237 100755
--- a/t/t5607-clone-bundle.sh
+++ b/t/t5607-clone-bundle.sh
@@ -138,6 +138,29 @@  test_expect_success 'fetch SHA-1 from bundle' '
 	git fetch --no-tags foo/tip.bundle "$(cat hash)"
 '
 
+test_expect_success 'clone bundle with fetch.fsckObjects' '
+	test_create_repo bundle-fsck &&
+	(
+		cd bundle-fsck &&
+		test_commit first &&
+		cat >data <<-EOF &&
+		tree $(git rev-parse HEAD^{tree})
+		parent $(git rev-parse HEAD)
+		author A U Thor
+		committer A U Thor
+
+		commit: this is a commit with bad emails
+
+		EOF
+		git hash-object --literally -t commit -w --stdin <data >commit &&
+		git branch bad $(cat commit) &&
+		git bundle create bad.bundle bad
+	) &&
+	test_must_fail git -c fetch.fsckObjects=true \
+		clone bundle-fsck/bad.bundle bundle-fsck-clone 2>err &&
+	test_grep "missingEmail" err
+'
+
 test_expect_success 'git bundle uses expected default format' '
 	git bundle create bundle HEAD^.. &&
 	cat >expect <<-EOF &&
diff --git a/transport.c b/transport.c
index 6799988f10c..a140d4b03c0 100644
--- a/transport.c
+++ b/transport.c
@@ -184,7 +184,7 @@  static int fetch_refs_from_bundle(struct transport *transport,
 	if (!data->get_refs_from_bundle_called)
 		get_refs_from_bundle_inner(transport);
 	ret = unbundle(the_repository, &data->header, data->fd,
-		       &extra_index_pack_args, 0, UNBUNDLE_FSCK_ALWAYS);
+		       &extra_index_pack_args, 0, UNBUNDLE_FSCK_FOLLOW_FETCH);
 	transport->hash_algo = data->header.hash_algo;
 	return ret;
 }