Re: Re: [PATCH v3 2/4] unbundle: introduce unbundle_fsck_flags for fsckobjects handling

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2259 bytes --]

On Thu, May 30, 2024 at 02:12:47AM +0800, Xing Xin wrote:
> At 2024-05-28 20:03:25, "Patrick Steinhardt" <ps@pks.im> wrote:
> >On Mon, May 27, 2024 at 03:41:55PM +0000, Xing Xin via GitGitGadget wrote:
> >[snip]
> >> diff --git a/bundle.h b/bundle.h
> >> index 021adbdcbb3..cfa9daddda6 100644
> >> --- a/bundle.h
> >> +++ b/bundle.h
> >> @@ -30,6 +30,11 @@ int create_bundle(struct repository *r, const char *path,
> >>  		  int argc, const char **argv, struct strvec *pack_options,
> >>  		  int version);
> >>  
> >> +enum unbundle_fsck_flags {
> >> +	UNBUNDLE_FSCK_NEVER = 0,
> >> +	UNBUNDLE_FSCK_ALWAYS,
> >> +};
> >> +
> >>  enum verify_bundle_flags {
> >>  	VERIFY_BUNDLE_VERBOSE = (1 << 0),
> >>  	VERIFY_BUNDLE_QUIET = (1 << 1),
> >
> >Wouldn't this have been a natural fit for the new flag, e.g. via
> >something like `VERIFY_BUNDLE_FSCK`?
> 
> It makes sense to me. Currently, verify_bundle_flags controls the amount
> of information displayed when checking a bundle's prerequisites. The
> newly added unbundle_fsck_flags is designed to check for broken objects
> during the unbundle process, which is essentially a form of bundle
> verification. I believe we should extend some object verification
> capabilities to the git bundle verify command as well, perhaps by adding
> a --fsck-objects option.
> 
> With this in mind, I support adding new options to verify_bundle_flags.
> Since bundle.c:unbundle needs to combine multiple options, we must
> define new options using bitwise shifting:
> 
> 	enum verify_bundle_flags {
> 		VERIFY_BUNDLE_VERBOSE = (1 << 0),
> 		VERIFY_BUNDLE_QUIET = (1 << 1),
> 		VERIFY_BUNDLE_FSCK_OBJECTS_ALWAYS = (1 << 2),
> 		VERIFY_BUNDLE_FSCK_OBJECTS_FOLLOW_FETCH = (1 << 3),
> 	};
> 
> How about the naming? I'm not very good at naming :)

I later noticed that you extend the `unbundle_fsck_flags` in a later
patch. With that in mind I don't think it's all that important anymore
to merge those into the `verify_bundle_flags` as you would otherwise
allow for weirdness. What happens for example when both `ALWAYS` and
`FOLLOW_FETCH` are set?

So feel free to ignore this advice. If you still think it's a good idea
then the above naming looks okay to me.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]