| Message ID | 20210217194246.25342-1-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Jonathan Tan pointed out that the fsck error_func doesn't pass you the > ID of the fsck failure in [1]. This series improves the API so it > does, and moves the gitmodules_{found,done} variables into the > fsck_options struct. > > The result is that instead of the "print_dangling_gitmodules" member > in that series we can just implement that with the diff at the end of > this cover letter (goes on top of a merge of this series & "seen"), > and without any changes to fsck_finish(). > > This conflicts with other in-flight fsck changes but the conflict is > rather trivial. Jeff King has another concurrent series to add a > couple of new fsck checks, those need to be moved to fsck.h, and > there's another trivial conflict in 2 hunks due to the > gitmodules_{found,done} move. > > 1. https://lore.kernel.org/git/87blcja2ha.fsf@evledraar.gmail.com/ Let's get this reviewed now, but with expectation that it will be rebased after the dust settles.
On Wed, Feb 17 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Jonathan Tan pointed out that the fsck error_func doesn't pass you the >> ID of the fsck failure in [1]. This series improves the API so it >> does, and moves the gitmodules_{found,done} variables into the >> fsck_options struct. >> >> The result is that instead of the "print_dangling_gitmodules" member >> in that series we can just implement that with the diff at the end of >> this cover letter (goes on top of a merge of this series & "seen"), >> and without any changes to fsck_finish(). >> >> This conflicts with other in-flight fsck changes but the conflict is >> rather trivial. Jeff King has another concurrent series to add a >> couple of new fsck checks, those need to be moved to fsck.h, and >> there's another trivial conflict in 2 hunks due to the >> gitmodules_{found,done} move. >> >> 1. https://lore.kernel.org/git/87blcja2ha.fsf@evledraar.gmail.com/ > > Let's get this reviewed now, but with expectation that it will be > rebased after the dust settles. Makes sense. Pending a review of this would you be interested in queuing a v2 of this that doesn't conflict with in-flight topics? Patches 01..09 & 13/14 can live conflict-free with what's in "seen" now (I'd have made the 13th the 10th in v1 if I'd noticed). Then I could re-roll the remainder of this once the other topics land.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> Let's get this reviewed now, but with expectation that it will be >> rebased after the dust settles. > > Makes sense. Pending a review of this would you be interested in queuing > a v2 of this that doesn't conflict with in-flight topics? Not really. I am not sure your recent patches are getting sufficient review bandwidth they deserve. > Patches 01..09 & 13/14 can live conflict-free with what's in "seen" now > (I'd have made the 13th the 10th in v1 if I'd noticed). Then I could > re-roll the remainder of this once the other topics land.
On Thu, Feb 18, 2021 at 11:12:26AM -0800, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > > >> Let's get this reviewed now, but with expectation that it will be > >> rebased after the dust settles. > > > > Makes sense. Pending a review of this would you be interested in queuing > > a v2 of this that doesn't conflict with in-flight topics? > > Not really. I am not sure your recent patches are getting > sufficient review bandwidth they deserve. FWIW, I just read through v2 (without having looked at all at v1 yet!), and they all seemed like quite reasonable cleanups. I left a few small comments that might be worth a quick re-roll, but I would also be OK with the patches being picked up as-is. -Peff
Jeff King <peff@peff.net> writes: > On Thu, Feb 18, 2021 at 11:12:26AM -0800, Junio C Hamano wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >> >> Let's get this reviewed now, but with expectation that it will be >> >> rebased after the dust settles. >> > >> > Makes sense. Pending a review of this would you be interested in queuing >> > a v2 of this that doesn't conflict with in-flight topics? >> >> Not really. I am not sure your recent patches are getting >> sufficient review bandwidth they deserve. > > FWIW, I just read through v2 (without having looked at all at v1 yet!), > and they all seemed like quite reasonable cleanups. I left a few small > comments that might be worth a quick re-roll, but I would also be OK > with the patches being picked up as-is. That's good to hear. I shouldn't even have bothered to answer the question, if the v2 were to have sent to the list without waiting for my reply ;-)
Jeff King <peff@peff.net> writes: > On Thu, Feb 18, 2021 at 11:12:26AM -0800, Junio C Hamano wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >> >> Let's get this reviewed now, but with expectation that it will be >> >> rebased after the dust settles. >> > >> > Makes sense. Pending a review of this would you be interested in queuing >> > a v2 of this that doesn't conflict with in-flight topics? >> >> Not really. I am not sure your recent patches are getting >> sufficient review bandwidth they deserve. > > FWIW, I just read through v2 (without having looked at all at v1 yet!), > and they all seemed like quite reasonable cleanups. I left a few small > comments that might be worth a quick re-roll, but I would also be OK > with the patches being picked up as-is. Yeah, all except for a handful minor nits looked good. Thanks for writing and reviewing. Perhaps a final reroll to tie the loose ends, or is it just a matter of signing off one of them and droping a couple of other ones (which other ones)?
On Thu, Feb 18 2021, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > >> On Thu, Feb 18, 2021 at 11:12:26AM -0800, Junio C Hamano wrote: >> >>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >>> >>> >> Let's get this reviewed now, but with expectation that it will be >>> >> rebased after the dust settles. >>> > >>> > Makes sense. Pending a review of this would you be interested in queuing >>> > a v2 of this that doesn't conflict with in-flight topics? >>> >>> Not really. I am not sure your recent patches are getting >>> sufficient review bandwidth they deserve. >> >> FWIW, I just read through v2 (without having looked at all at v1 yet!), >> and they all seemed like quite reasonable cleanups. I left a few small >> comments that might be worth a quick re-roll, but I would also be OK >> with the patches being picked up as-is. > > That's good to hear. I shouldn't even have bothered to answer the > question, if the v2 were to have sent to the list without waiting > for my reply ;-) FWIW it's not that I didn't care about the reply, but I'm somewhat intermittently available time/network wise in the coming days. And there's the TZ difference between us. I sent v1 thinking you might be willing to pick it up & resolve the conflict, but since you expressed an interest in deferring it until conflicting work landed figured I'd ask (and then just sent the patches) if you'd be interested in a conflict-free version to queue alongside those changes. If it was still "nah" fair enough, I'd just wait. But if not those patches would be there to pickup. Thanks a lot to you & Jeff for the review on v2. I won't have time to address all that today, and in any case I got the message that maybe I should stop firehosing the list with patch series's for a bit :)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 82f381f854..22dfcfc5de 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1713,6 +1713,20 @@ static void show_pack_info(int stat_only) } } +static int index_pack_fsck_error_func(struct fsck_options *o, + const struct object_id *oid, + enum object_type object_type, + enum fsck_msg_type msg_type, + enum fsck_msg_id msg_id, + const char *message) +{ + if (msg_id == FSCK_MSG_GITMODULES_MISSING) { + puts(oid_to_hex(oid)); + return 0; + } + return fsck_error_function(o, oid, object_type, msg_type, msg_id, message); +} + int cmd_index_pack(int argc, const char **argv, const char *prefix) { int i, fix_thin_pack = 0, verify = 0, stat_only = 0, rev_index; @@ -1934,10 +1948,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) close(input_fd); if (do_fsck_object) { - struct fsck_options fo = FSCK_OPTIONS_STRICT; - - fo.print_dangling_gitmodules = 1; - if (fsck_finish(&fo)) + fsck_options.error_func = index_pack_fsck_error_func; + if (fsck_finish(&fsck_options)) die(_("fsck error in pack objects")); } diff --git a/fetch-pack.c b/fetch-pack.c index 0a337a04f1..9fc2ce86e4 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -40,6 +40,7 @@ static struct shallow_lock shallow_lock; static const char *alternate_shallow_file; static struct strbuf fsck_msg_types = STRBUF_INIT; static struct string_list uri_protocols = STRING_LIST_INIT_DUP; +static struct fsck_options fsck_options = FSCK_OPTIONS_STRICT; /* Remember to update object flag allocation in object.h */ #define COMPLETE (1U << 0) @@ -993,19 +994,34 @@ static int cmp_ref_by_name(const void *a_, const void *b_) return strcmp(a->name, b->name); } +static int fetch_pack_fsck_error_func(struct fsck_options *o, + const struct object_id *oid, + enum object_type object_type, + enum fsck_msg_type msg_type, + enum fsck_msg_id msg_id, + const char *message) +{ + if (msg_id == FSCK_MSG_GITMODULES_MISSING) { + puts(oid_to_hex(oid)); + return 0; + } + return fsck_error_function(o, oid, object_type, msg_type, msg_id, message); +} + static void fsck_gitmodules_oids(struct oidset *gitmodules_oids) { struct oidset_iter iter; const struct object_id *oid; - struct fsck_options fo = FSCK_OPTIONS_STRICT; if (!oidset_size(gitmodules_oids)) return; oidset_iter_init(gitmodules_oids, &iter); while ((oid = oidset_iter_next(&iter))) - register_found_gitmodules(oid); - if (fsck_finish(&fo)) + oidset_insert(&fsck_options.gitmodules_found, oid); + + fsck_options.error_func = fetch_pack_fsck_error_func; + if (fsck_finish(&fsck_options)) die("fsck failed"); }
Jonathan Tan pointed out that the fsck error_func doesn't pass you the ID of the fsck failure in [1]. This series improves the API so it does, and moves the gitmodules_{found,done} variables into the fsck_options struct. The result is that instead of the "print_dangling_gitmodules" member in that series we can just implement that with the diff at the end of this cover letter (goes on top of a merge of this series & "seen"), and without any changes to fsck_finish(). This conflicts with other in-flight fsck changes but the conflict is rather trivial. Jeff King has another concurrent series to add a couple of new fsck checks, those need to be moved to fsck.h, and there's another trivial conflict in 2 hunks due to the gitmodules_{found,done} move. 1. https://lore.kernel.org/git/87blcja2ha.fsf@evledraar.gmail.com/ Ævar Arnfjörð Bjarmason (14): fsck.h: indent arguments to of fsck_set_msg_type fsck.h: use use "enum object_type" instead of "int" fsck.c: rename variables in fsck_set_msg_type() for less confusion fsck.c: move definition of msg_id into append_msg_id() fsck.c: rename remaining fsck_msg_id "id" to "msg_id" fsck.h: move FSCK_{FATAL,INFO,ERROR,WARN,IGNORE} into an enum fsck.c: call parse_msg_type() early in fsck_set_msg_type() fsck.c: undefine temporary STR macro after use fsck.c: give "FOREACH_MSG_ID" a more specific name fsck.[ch]: move FOREACH_FSCK_MSG_ID & fsck_msg_id from *.c to *.h fsck.c: pass along the fsck_msg_id in the fsck_error callback fsck.c: add an fsck_set_msg_type() API that takes enums fsck.h: update FSCK_OPTIONS_* for object_name fsck.c: move gitmodules_{found,done} into fsck_options builtin/fsck.c | 7 +- builtin/index-pack.c | 3 +- builtin/mktag.c | 7 +- builtin/unpack-objects.c | 3 +- fsck.c | 160 ++++++++++++--------------------------- fsck.h | 98 +++++++++++++++++++++--- 6 files changed, 152 insertions(+), 126 deletions(-)