Re: [PATCH v3 3/4] fetch-pack: expose fsckObjects configuration logic

From: Patrick Steinhardt <ps@pks.im>
Date: May 28, 2024, 12:03 UTC
Message-ID: <ZlXIBrs5Q91r_-qU@tanuki>
In-Reply-To: <67401d4fbcb3f07d31589bb8ec10060dcb77545e.1716824518.git.gitgitgadget@gmail.com>

On Mon, May 27, 2024 at 03:41:56PM +0000, Xing Xin via GitGitGadget wrote:

Show 6 quoted lines

> From: Xing Xin <xingxin.xx@bytedance.com>
> 
> Currently we can use "transfer.fsckObjects" or "fetch.fsckObjects" to
> control whether to enable checks for broken objects during fetching. But
> these configs are only acknowledged by `fetch-pack.c:get_pack` and do
> not make sense when fetching from bundles or using bundle-uris.

Do they not make sense, or are they not effective? I assume you mean the latter, right?

> This commit exposed the fetch-then-transfer configuration logic by

s/exposed/exposes/

Show 24 quoted lines

> adding a new function `fetch_pack_fsck_objects` in fetch-pack.h. In next
> commit, this new function will be used by `unbundle` in fetching
> scenarios.
> 
> Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
> ---
>  fetch-pack.c | 18 ++++++++++++------
>  fetch-pack.h |  2 ++
>  2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 7d2aef21add..81a64be6951 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -954,12 +954,7 @@ static int get_pack(struct fetch_pack_args *args,
>  		strvec_push(&cmd.args, alternate_shallow_file);
>  	}
>  
> -	if (fetch_fsck_objects >= 0
> -	    ? fetch_fsck_objects
> -	    : transfer_fsck_objects >= 0
> -	    ? transfer_fsck_objects
> -	    : 0)
> -		fsck_objects = 1;

This statement is really weird to read, but that is certainly not the fault of this patch, but...

Show 18 quoted lines

> +	fsck_objects = fetch_pack_fsck_objects();
>  
>  	if (do_keep || args->from_promisor || index_pack_args || fsck_objects) {
>  		if (pack_lockfiles || fsck_objects)
> @@ -2046,6 +2041,17 @@ static const struct object_id *iterate_ref_map(void *cb_data)
>  	return &ref->old_oid;
>  }
>  
> +int fetch_pack_fsck_objects(void)
> +{
> +	fetch_pack_setup();
> +
> +	return fetch_fsck_objects >= 0
> +	       ? fetch_fsck_objects
> +	       : transfer_fsck_objects >= 0
> +	       ? transfer_fsck_objects
> +	       : 0;
> +}

... can we maybe rewrite it to something more customary here? The following is way easier to read, at least for me.

	int fetch_pack_fsck_objects(void)
	{
		fetch_pack_setup();
		if (fetch_fsck_objects >= 0 ||
		    transfer_fsck_objects >= 0)
			return 1;
		return 0;
	}

Show 12 quoted lines

>  struct ref *fetch_pack(struct fetch_pack_args *args,
>  		       int fd[],
>  		       const struct ref *ref,
> diff --git a/fetch-pack.h b/fetch-pack.h
> index 6775d265175..38956d9b748 100644
> --- a/fetch-pack.h
> +++ b/fetch-pack.h
> @@ -101,4 +101,6 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,
>   */
>  int report_unmatched_refs(struct ref **sought, int nr_sought);
>  
> +int fetch_pack_fsck_objects(void);

Let's add a comment here saying what this function does.

Patrick