Re: [PATCH/RFC 1/3] send-pack: track errors for each ref

From: Jeff King <peff@peff.net>
Date: Nov 13, 2007, 22:30 UTC
Message-ID: <20071113223006.GA19032@sigill.intra.peff.net>
In-Reply-To: <20071113190447.GB3268@steel.home>

On Tue, Nov 13, 2007 at 08:04:47PM +0100, Alex Riesen wrote:

Show 7 quoted lines

> > -	unsigned char force;
> > -	unsigned char merge;
> > +	unsigned char force : 1;
> > +	unsigned char merge : 1;
> > +	unsigned char nonff : 1;
> 
>         unsigned char fastforward : 1; ? ffwd?

I agree it is a little funny to have a negative flag, but it is the exceptional case to be nonff, so I think it makes the code flow a little better (you would just be asking for !ref->fastforward all the time). I am fine with a more readable name.

Show 7 quoted lines

> > +	unsigned char deletion : 1;
> > +	enum {
> > +		REF_STATUS_NONE = 0,
> > +		REF_STATUS_OK,
> > +		REF_STATUS_NONFF,
> 
> isn't it a duplication of nonff?

No. The nonff flag is "is this push a non-fast forward". The REF_STATUS_NONFF state is "we didn't push because this is a nonff case". So you can have nonff = 1 and status = REF_STATUS_OK, which means that the push was forced (either by ->force, or args.force).

So perhaps a better name is REF_STATUS_REJECT_NONFF (or something longer if NONFF is too non-obvious).

An alternative is that the deletion and nonff flags can be folded into the status. But then checking for "is everything OK" becomes non-obvious (it's something like REF_STATUS_NONFF_FORCED || REF_STATUS_DELETION_OK || REF_STATUS_PUSH_OK). I tried it that way and found the code is a bit more readable by pulling those features away from status (so status is "did we succeed, or did we fail, and if the latter, for what reason?").

-Peff