Re: [PATCH 01/18] Introduce fsck options

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Dec 22, 2014, 17:26 UTC
Message-ID: <alpine.DEB.1.00.1412221822310.21312@s15462909.onlinehome-server.info>
In-Reply-To: <xmqqk31zjz8o.fsf@gitster.dls.corp.google.com>

Hi Junio,

On Wed, 10 Dec 2014, Junio C Hamano wrote:

Show 8 quoted lines

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > Subject: Re: [PATCH 01/18] Introduce fsck options
> 
> please make it easier to cluster and spot the series in the eventual
> shortlog by giving a common prefix to the patches, e.g.
> 
> 	fsck: introduce fsck_options struct

I use the fsck: prefix consistently now.

Show 6 quoted lines

> > +static struct fsck_options fsck_walk_options = FSCK_OPTIONS_DEFAULT;
> > +static struct fsck_options fsck_obj_options = FSCK_OPTIONS_DEFAULT;
> 
> Is it a good idea to allow walker to be strict but obj verifier to
> be not (or vice versa)?  I am wondering why this is not a single
> struct with two callback function pointers.

Unfortunately not. There are two different walkers used, and in fact, fsck_walk_options() is only used to walk the objects, not to fsck them.

Now, I could use only one struct and set the walker, but that is not thread-safe, and while code is not threaded yet AFAICT, it might be in the future. That is why I decided to be rather safe than sorry. If you want it differently, please just say the word, I will make it so.

Show 9 quoted lines

> > +struct fsck_options {
> > +	fsck_walk_func walk;
> > +	fsck_error error_func;
> > +	int strict:1;
> 
> A signed 1-bit-wide bitfield can hold its sign-bit and nothing else,
> no?
> 
>     unsigned strict:1;

Oops. Right. For some reason, it worked here, though... Fixed!

Ciao, Dscho