On Tue, Nov 13, 2007 at 08:04:47PM +0100, Alex Riesen wrote:
> > - 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.
> > + 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
