| Message ID | 67401d4fbcb3f07d31589bb8ec10060dcb77545e.1716824518.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | object checking related additions and fixes for bundles in fetches | expand |
On Mon, May 27, 2024 at 03:41:56PM +0000, Xing Xin via GitGitGadget wrote: > 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/ > 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... > + 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; } > 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
Patrick Steinhardt <ps@pks.im> writes: >> +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; > } But do they mean the same thing? In a repository where [fetch] fsckobjects = no is set, no matter what transfer.fsckobjects says (or left unspecified), we want to return "no, we are not doing fsck".
Junio C Hamano <gitster@pobox.com> writes: > Patrick Steinhardt <ps@pks.im> writes: > >>> +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; >> } > > But do they mean the same thing? In a repository where > > [fetch] fsckobjects = no > > is set, no matter what transfer.fsckobjects says (or left unspecified), > we want to return "no, we are not doing fsck". The original before it was made into a helper function was written as a cascade of ?: operators, because it had to be a single expression. As the body of a helper function, we now can sprinkle multiple return statements in it. I think the way that is easiest to understand is /* the most specific, if specified */ if (fetch_fsck_objects >= 0) return fetch_fsck_objects; /* the less specific, catch-all for both directions */ if (transfer_fsck_objects >= 0) return transfer_fsck_objects; /* the fallback hardcoded default */ return 0; without the /* comments */.
On Tue, May 28, 2024 at 10:10:46AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > >> +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; > > } > > But do they mean the same thing? In a repository where > > [fetch] fsckobjects = no > > is set, no matter what transfer.fsckobjects says (or left unspecified), > we want to return "no, we are not doing fsck". Oh, of course they don't. This here would be a faithful conversion: int fetch_pack_fsck_objects(void) { fetch_pack_setup(); if (fetch_fsck_objects >= 0) return fetch_fsck_objects; if (transfer_fsck_objects >= 0) return transfer_fsck_objects; return 0; } Still easier to read in my opinion. Patrick
On Tue, May 28, 2024 at 10:24:35AM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Patrick Steinhardt <ps@pks.im> writes: > > > >>> +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; > >> } > > > > But do they mean the same thing? In a repository where > > > > [fetch] fsckobjects = no > > > > is set, no matter what transfer.fsckobjects says (or left unspecified), > > we want to return "no, we are not doing fsck". > > The original before it was made into a helper function was written > as a cascade of ?: operators, because it had to be a single > expression. As the body of a helper function, we now can sprinkle > multiple return statements in it. I think the way that is easiest > to understand is > > /* the most specific, if specified */ > if (fetch_fsck_objects >= 0) > return fetch_fsck_objects; > /* the less specific, catch-all for both directions */ > if (transfer_fsck_objects >= 0) > return transfer_fsck_objects; > /* the fallback hardcoded default */ > return 0; > > without the /* comments */. Ah, right, didn't see this mail. My revised version looks the same as yours, except for the added comments. Patrick
At 2024-05-29 01:24:35, "Junio C Hamano" <gitster@pobox.com> wrote: [snip] >The original before it was made into a helper function was written >as a cascade of ?: operators, because it had to be a single >expression. As the body of a helper function, we now can sprinkle >multiple return statements in it. I think the way that is easiest >to understand is > > /* the most specific, if specified */ > if (fetch_fsck_objects >= 0) > return fetch_fsck_objects; > /* the less specific, catch-all for both directions */ > if (transfer_fsck_objects >= 0) > return transfer_fsck_objects; > /* the fallback hardcoded default */ > return 0; > >without the /* comments */. Applied, thanks! Xing Xin
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; + 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; +} + 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); + #endif