At 2024-05-30 12:38:49, "Patrick Steinhardt" [off-list ref] wrote:
[snip]

quoted

quoted

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.

With the idea of extending "--fsck-objects" support for "git bundle verify" and
"git bundle unbundle", I prefer to grouping these options together. Especially
in the "git bundle verify" scenario, adding a new parameter like
`unbundle_fsck_flags` for `bundle.c:verify_bundle` is confusing.

Xing Xin