At 2024-05-21 01:19:02, "Junio C Hamano" <gitster@xxxxxxxxx> wrote:
>"Xing Xin" <bupt_xingxin@xxxxxxx> writes:
>
>> Personally I think data from bundles and data received via network
>> should be treated equally.
>
>Yup, that is not personal ;-) but is universally accepted as a good
>discipline. In the case of bundle-uri, the bundle came over the
>network so it is even more true that they should be treated the
>same.
>
>> For "fetch-pack" we now have some configs
>> such as "fetch.fsckobjects" and "transfer.fsckobjects" to decide the
>> behavior, these configs are invisible when we are fetching bundles.
>
>When fetching over network, transport.c:fetch_refs_via_pack() calls
>fetch_pack.c:fetch_pack(), which eventually calls get_pack() and the
>configuration variables are honored there. It appears that the
>transport layer is unaware of the .fsckobjects configuration knobs.
>
>When fetching from a bundle, transport.c:fetch_refs_from_bundle()
>calls bundle.c:unbundle(). This function has three callers, i.e.
>"git bundle unbundle", normal fetching from a bundle, and more
>recently added bundle-uri codepaths.
>
>I think one reasonable approach to take is to add an extra parameter
>that takes one of three values: (never, use-config, always), and
>conditionally add "--fsck-objects" to the command line of the
>index-pack. Teach "git bundle unbundle" the "--fsck-objects" option
>so that it can pass 'never' or 'always' from the command line, and
>pass 'use-config' from the code paths for normal fetching from a
>budnle and bundle-uri.
>
>To implement use-config, you'd probably need to refactor a small
>part of fetch-pack.c:get_pack()
>
> if (fetch_fsck_objects >= 0
> ? fetch_fsck_objects
> : transfer_fsck_objects >= 0
> ? transfer_fsck_objects
> : 0)
> fsck_objects = 1;
>
>into a public function (to support a caller like unbundle() that
>comes from sideways, the new function may also need to call
>fetch_pack_setup() to prime them).
>
>A patch series may take a structure like so:
>
> * define enum { UNBUNDLE_FSCK_NEVER, UNBUNDLE_FSCK_ALWAYS } in
> bundle.h, have bundle.c:unbundle() accept a new parameter of that
> type, and conditionally add "--fsck-objects" to its call to
> "index-pack". "git bundle unbundle" can pass 'never' to its
> invocation to unbundle() as an easy way to test it. For the
> other two callers, we can start by passing 'always'.
>
> * (optional) teach "git bundle unbundle" a new "--fsck-objects"
> option to allow passing 'always' to its call to unbundle(). With
> that, add tests to feed it a bundle with questionable objects in
> it and make sure that unbundling notices.
I just submitted a new series mainly focusing on the unbundle handling during
fetches. I would like to submit a new one for teaching "git bundle unbundle" a
"--fsck-objects" option after this to make changes more targeted.
> * refactor fetch-pack.c:get_pack() to make the fetch-then-transfer
> configuration logic available to external callers.
>
> * Add UNBUNDLE_FSCK_USE_CONFIG to the enum, enhance unbundle() to
I tend to use `UNBUNDLE_FSCK_FOLLOW_FETCH` because this option is only
used in fetches, though the current implementation is indeed reading configs.
> react to the value by calling the helper function you introduced
> in the previous step.
The new patch series is constructed right as you suggested, thanks a lot for
your help.
Xing Xin
