Re: [GSoC][PATCH v13 04/10] fsck: add refs-related error report function

[shejialuo <shejialuo@gmail.com>]

On Tue, Jul 30, 2024 at 10:31:26AM +0200, Patrick Steinhardt wrote:
> On Mon, Jul 29, 2024 at 09:26:51PM +0800, shejialuo wrote:
> > Add refs-related options to the "fsck_options", create refs-specific
> > "error_func" callback "fsck_refs_error_function".
> 
> We should have an explanation _why_ we are adding these functions in the
> commit message.
> 

Yes, I will improve this in the next version.

> > "fsck_refs_error_function" will use the "oid" parameter. When the caller
> > passes the oid, it will use "oid_to_hex" to get the corresponding hex
> > value to report to the caller.
> > 
> > Last, add "FSCK_REFS_OPTIONS_DEFAULT" and "FSCK_REFS_OPTIONS_STRICT"
> > macros to create refs options easily.
> 
> It is a bit unclear to me what you mean with "create refs options
> easily". Do you mean to say that `git refs check` (or whatever this will
> be called) will have flags like "--strict"?
> 

Yes, when the user passes `--strict`, all the warn type will be seen as
the error type. So I create "FSCK_REFS_OPTIONS_STRICT". However, I
didn't think too much here. I just followed the way the codebase does
for the objects.

> > Mentored-by: Patrick Steinhardt <ps@pks.im>
> > Mentored-by: Karthik Nayak <karthik.188@gmail.com>
> > Signed-off-by: shejialuo <shejialuo@gmail.com>
> > ---
> >  fsck.c | 25 +++++++++++++++++++++++++
> >  fsck.h | 14 ++++++++++++++
> >  2 files changed, 39 insertions(+)
> > 
> > diff --git a/fsck.c b/fsck.c
> > index af61fa90ba..56de29b4c0 100644
> > --- a/fsck.c
> > +++ b/fsck.c
> > @@ -1251,6 +1251,31 @@ int fsck_objects_error_function(struct fsck_options *o,
> >  	return 1;
> >  }
> >  
> > +int fsck_refs_error_function(struct fsck_options *options,
> > +			     const struct object_id *oid,
> > +			     enum object_type object_type UNUSED,
> > +			     const struct fsck_refs_info *refs_info,
> > +			     enum fsck_msg_type msg_type,
> > +			     enum fsck_msg_id msg_id UNUSED,
> > +			     const char *message)
> > +{
> > +	struct strbuf sb = STRBUF_INIT;
> > +	int ret = 0;
> > +
> > +	strbuf_addstr(&sb, refs_info->path);
> > +
> > +	if (oid)
> > +		strbuf_addf(&sb, " -> (%s)", oid_to_hex(oid));
> 
> Okay, so we do end up printing the object ID indeed. But wouldn't we
> want to potentially do the same with symbolic refs?
> 
> Also, would it make more sense to put the `oid` (and potentially the
> `referent` when we also handle symbolic refs) into `struct
> fsck_refs_info`? Like this, the whole state would be self-contained in
> that structure, which would also make my proposal from a preceding
> commit more feasible where the subsystem-specific error functions only
> get a void pointer to this structure. It would require another
> refactoring on top to move the object type and OID into a `struct
> fsck_objects_info`, too, but that shouldn't be too involved, I guess.
> 

Yes, I totally agree here. I didn't consider symrefs here, actually we
should provide "symref -> referent" message if we have providen the
"regular ref -> oid" message.

I think we should do this. And I have commented on "[Patch v13][2/10]",
it is a necessity we should refactor this part.

> Patrick