diff mbox

[00/14] fsck: API improvements

Message ID 20210217194246.25342-1-avarab@gmail.com (mailing list archive)
State New
Headers show

Commit Message

Ævar Arnfjörð Bjarmason Feb. 17, 2021, 7:42 p.m. UTC
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(-)

Comments

Junio C Hamano Feb. 17, 2021, 9:02 p.m. UTC | #1
Æ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.
Ævar Arnfjörð Bjarmason Feb. 18, 2021, midnight UTC | #2
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.
Junio C Hamano Feb. 18, 2021, 7:12 p.m. UTC | #3
Æ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.
Jeff King Feb. 18, 2021, 7:57 p.m. UTC | #4
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
Junio C Hamano Feb. 18, 2021, 8:27 p.m. UTC | #5
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 ;-)
Junio C Hamano Feb. 18, 2021, 10:36 p.m. UTC | #6
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)?
Ævar Arnfjörð Bjarmason Feb. 19, 2021, 12:54 a.m. UTC | #7
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 mbox

Patch

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");
 }