diff mbox series

[v5,3/4] unbundle: extend options to support object verification

Message ID 0a18d7839be67d6c0be137c7e15dff9663a161a8.1718088127.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 June 11, 2024, 6:42 a.m. UTC 
From: Xing Xin <xingxin.xx@bytedance.com>

This commit extends object verification support in `bundle.c:unbundle`
by adding two new options to `verify_bundle_flags`:

- `VERIFY_BUNDLE_FSCK_ALWAYS` explicitly enables checks for broken
  objects. It will be used to add "--fsck-objects" support for "git
  bundle unbundle" in a separate series.
- `VERIFY_BUNDLE_FSCK_FOLLOW_FETCH` is designed to be used during fetch
  operations, specifically for direct bundle fetches and _bundle-uri_
  enabled fetches. When enabled, `bundle.c:unbundle` invokes
  `fetch-pack.c:fetch_pack_fsck_objects` to determine whether to enable
  checks for broken objects. Passing this flag during fetching will be
  implemented in a subsequent commit.

Note that the option `VERIFY_BUNDLE_FSCK_ALWAYS` takes precedence over
`VERIFY_BUNDLE_FSCK_FOLLOW_FETCH`.

Reviewed-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
---
 bundle.c | 10 ++++++++++
 bundle.h |  2 ++
 2 files changed, 12 insertions(+)

Comments

Patrick Steinhardt June 11, 2024, 9:11 a.m. UTC | #1 
On Tue, Jun 11, 2024 at 06:42:05AM +0000, Xing Xin via GitGitGadget wrote:
> From: Xing Xin <xingxin.xx@bytedance.com>
> 
> This commit extends object verification support in `bundle.c:unbundle`
> by adding two new options to `verify_bundle_flags`:
> 
> - `VERIFY_BUNDLE_FSCK_ALWAYS` explicitly enables checks for broken
>   objects. It will be used to add "--fsck-objects" support for "git
>   bundle unbundle" in a separate series.
> - `VERIFY_BUNDLE_FSCK_FOLLOW_FETCH` is designed to be used during fetch
>   operations, specifically for direct bundle fetches and _bundle-uri_
>   enabled fetches. When enabled, `bundle.c:unbundle` invokes
>   `fetch-pack.c:fetch_pack_fsck_objects` to determine whether to enable
>   checks for broken objects. Passing this flag during fetching will be
>   implemented in a subsequent commit.
> 
> Note that the option `VERIFY_BUNDLE_FSCK_ALWAYS` takes precedence over
> `VERIFY_BUNDLE_FSCK_FOLLOW_FETCH`.

Thanks, the new sequence of commits is much easier to follow. It also
shows that there is no user of `VERIFY_BUNDLE_FSCK_ALWAYS` at the end of
this series. So maybe we should drop that flag?

If you do that, then I'd also propose to merge patches 2 and 3 into one
given that both are quite trivial and related to each other.

Other than that this series looks good to me.

Patrick

> Reviewed-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
> ---
>  bundle.c | 10 ++++++++++
>  bundle.h |  2 ++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/bundle.c b/bundle.c
> index 95367c2d0a0..53ac73834ea 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";
> @@ -615,6 +616,7 @@ int unbundle(struct repository *r, struct bundle_header *header,
>  	     enum verify_bundle_flags flags)
>  {
>  	struct child_process ip = CHILD_PROCESS_INIT;
> +	int fsck_objects = 0;
>  
>  	if (verify_bundle(r, header, flags))
>  		return -1;
> @@ -625,6 +627,14 @@ int unbundle(struct repository *r, struct bundle_header *header,
>  	if (header->filter.choice)
>  		strvec_push(&ip.args, "--promisor=from-bundle");
>  
> +	if (flags & VERIFY_BUNDLE_FSCK_ALWAYS)
> +		fsck_objects = 1;
> +	else if (flags & VERIFY_BUNDLE_FSCK_FOLLOW_FETCH)
> +		fsck_objects = fetch_pack_fsck_objects();
> +
> +	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 021adbdcbb3..a39d8ea1a7e 100644
> --- a/bundle.h
> +++ b/bundle.h
> @@ -33,6 +33,8 @@ int create_bundle(struct repository *r, const char *path,
>  enum verify_bundle_flags {
>  	VERIFY_BUNDLE_VERBOSE = (1 << 0),
>  	VERIFY_BUNDLE_QUIET = (1 << 1),
> +	VERIFY_BUNDLE_FSCK_ALWAYS = (1 << 2),
> +	VERIFY_BUNDLE_FSCK_FOLLOW_FETCH = (1 << 3),
>  };
>  
>  int verify_bundle(struct repository *r, struct bundle_header *header,
> -- 
> gitgitgadget
>
Xing Xin June 11, 2024, 12:47 p.m. UTC | #2 
At 2024-06-11 17:11:09, "Patrick Steinhardt" <ps@pks.im> wrote:
>On Tue, Jun 11, 2024 at 06:42:05AM +0000, Xing Xin via GitGitGadget wrote:
>> From: Xing Xin <xingxin.xx@bytedance.com>
>> 
>> This commit extends object verification support in `bundle.c:unbundle`
>> by adding two new options to `verify_bundle_flags`:
>> 
>> - `VERIFY_BUNDLE_FSCK_ALWAYS` explicitly enables checks for broken
>>   objects. It will be used to add "--fsck-objects" support for "git
>>   bundle unbundle" in a separate series.
>> - `VERIFY_BUNDLE_FSCK_FOLLOW_FETCH` is designed to be used during fetch
>>   operations, specifically for direct bundle fetches and _bundle-uri_
>>   enabled fetches. When enabled, `bundle.c:unbundle` invokes
>>   `fetch-pack.c:fetch_pack_fsck_objects` to determine whether to enable
>>   checks for broken objects. Passing this flag during fetching will be
>>   implemented in a subsequent commit.
>> 
>> Note that the option `VERIFY_BUNDLE_FSCK_ALWAYS` takes precedence over
>> `VERIFY_BUNDLE_FSCK_FOLLOW_FETCH`.
>
>Thanks, the new sequence of commits is much easier to follow. It also
>shows that there is no user of `VERIFY_BUNDLE_FSCK_ALWAYS` at the end of
>this series. So maybe we should drop that flag?

OK, let's focus on the fetching scenarios in this patch series.

>If you do that, then I'd also propose to merge patches 2 and 3 into one
>given that both are quite trivial and related to each other.

I merged patches 3 and 4 instead to combine the new option definition and usage
logic, as the two are more closely related and more trivial. :)

Xing Xin

>Other than that this series looks good to me.
>
>Patrick
>
diff mbox series

Patch

diff --git a/bundle.c b/bundle.c
index 95367c2d0a0..53ac73834ea 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";
@@ -615,6 +616,7 @@  int unbundle(struct repository *r, struct bundle_header *header,
 	     enum verify_bundle_flags flags)
 {
 	struct child_process ip = CHILD_PROCESS_INIT;
+	int fsck_objects = 0;
 
 	if (verify_bundle(r, header, flags))
 		return -1;
@@ -625,6 +627,14 @@  int unbundle(struct repository *r, struct bundle_header *header,
 	if (header->filter.choice)
 		strvec_push(&ip.args, "--promisor=from-bundle");
 
+	if (flags & VERIFY_BUNDLE_FSCK_ALWAYS)
+		fsck_objects = 1;
+	else if (flags & VERIFY_BUNDLE_FSCK_FOLLOW_FETCH)
+		fsck_objects = fetch_pack_fsck_objects();
+
+	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 021adbdcbb3..a39d8ea1a7e 100644
--- a/bundle.h
+++ b/bundle.h
@@ -33,6 +33,8 @@  int create_bundle(struct repository *r, const char *path,
 enum verify_bundle_flags {
 	VERIFY_BUNDLE_VERBOSE = (1 << 0),
 	VERIFY_BUNDLE_QUIET = (1 << 1),
+	VERIFY_BUNDLE_FSCK_ALWAYS = (1 << 2),
+	VERIFY_BUNDLE_FSCK_FOLLOW_FETCH = (1 << 3),
 };
 
 int verify_bundle(struct repository *r, struct bundle_header *header,