Subject: Re: [GSoC][PATCH v3 1/7] fsck: add refs check interfaces to interface with fsck error levels

From: shejialuo <[email-masked]>
Date: Wed, 19 Jun 2024 10:39:53 +0800

On Tue, Jun 18, 2024 at 08:25:34AM -0700, Junio C Hamano wrote:
> shejialuo <[email-masked]> writes:
> 
> > On Tue, Jun 18, 2024 at 04:38:07AM -0400, Karthik Nayak wrote:
> >> shejialuo <[email-masked]> writes:
> >> 
> >> [snip]
> >> 
> >> >  struct fsck_options {
> >> > +	/*
> >> > +	 * Reorder the fields to allow `fsck_ref_options` to use
> >> > +	 * the interfaces using `struct fsck_options`.
> >> > +	 */
> >> 
> >> Why is this added? It makes sense to have it in the commit message
> >> because it talks about the change, but why make it persistent in the
> >> code?
> >> 
> >
> > I explicitly add this comments due to the following reasons:
> >
> > 1. If someone needs to change the `fsck_options`, without this comment,
> > he might be just add some new fields at top. Although the change will
> > fail the tests here, I think we should mention this in code.
> 
> Do you mean you plan to take advantage of the fact that early
> members of two structures are the same?  IOW, if there is a function
> that takes a pointer to smaller fsck_refs_options, you plan to pass
> a pointer to fsck_options from some callers, e.g.
> 
>     extern void func(struct fsck_refs_options *);
>     void a_caller(struct fsck_options *o)
>     {
> 	func((struct fsck_options *)o);
> 	...
> 
> If that is the case, then ...
> 
> Do not do that.
> 
> Your data structure design is broken.  Instead you would do this:
> 
> 	struct fsck_options {
> 		struct fsck_refs_options refs;
> 		... other members ...
> 	};
> 	void a_caller(struct fsck_options *o)
> 	{
> 		func(&o->refs);
> 		...

Well, I totally agree with this. It's bad to convert the pointer type. I
will change the code in this patch to make it OK.

Thanks for your advice.