Re: [PATCH 4/4] fetch-pack: print and use dangling .gitmodules

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 17, 2021, 01:48 UTC
Message-ID: <87blcja2ha.fsf@evledraar.gmail.com>
In-Reply-To: <20210128010320.3880242-1-jonathantanmy@google.com>

On Thu, Jan 28 2021, Jonathan Tan wrote:

Sorry I managed to miss this at the time. Hopefully a late reply is better than never.

Show 14 quoted lines

>> On Sun, Jan 24 2021, Jonathan Tan wrote:
>> 
>> > +void register_found_gitmodules(const struct object_id *oid)
>> > +{
>> > +	oidset_insert(&gitmodules_found, oid);
>> > +}
>> > +
>> 
>> In fsck.c we only use this variable to insert into it, or in fsck_blob()
>> to do the actual check, but then we either abort early if we've found
>> it, or right after that:
>
> By "this variable", do you mean gitmodules_found? fsck_finish() consumes
> it.

Yes, consumes it to emit errors with report(), no?

Show 10 quoted lines

>>         if (object_on_skiplist(options, oid))
>>                 return 0;
>> 
>> So (along with comments I have below...) you could just use the existing
>> "skiplist" option instead, no?
>
> I don't understand this part (in particular, the part you quoted). About
> "skiplist", I'll reply to your other email [1] which has more details.
>
> [1] https://lore.kernel.org/git/87czxu7c15.fsf@evledraar.gmail.com/

*nod*

Show 14 quoted lines

>> This whole thing seems just like the bad path I took in earlier rounds
>> of my in-flight mktag series. You don't need this new custom API. You
>> just setup an error handler for your fsck which ignores / prints / logs
>> / whatever the OIDs you want if you get a FSCK_MSG_GITMODULES_MISSING
>> error, which you then "return 0" on.
>> 
>> If you don't have FSCK_MSG_GITMODULES_MISSING punt and call
>> fsck_error_function().
>
> I tried that first, and the issue is that IDs like
> FSCK_MSG_GITMODULES_MISSING are internal to fsck.c. As for whether we
> should start exposing the IDs publicly, I think we should wait until a
> few new cases like this come up, so that we more fully understand the
> requirements first.

The requirement is that you want the objects ids we'd otherwise error about in fsck_finish(). Yeah we don't pass the "fsck_msg_id" down in the "report()" function, but you can reliably strstr() it out of the message. We document & hard rely on that already, since it's also a config key.

But yeah, we could just change the report function to pass down the id and move the relevant macros from fsck.c to fsck.h. I think that would be a smaller change conceptually than a special-case flag in fsck_options for something we could otherwise do with the error reporting.