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

quoted text
> > -	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.

quoted text
> > +	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
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html