[Ovirt-devel] [Patch] History Graphs Axis

[Perry N. Myers]

mark wagner wrote:
> Well, I'm not really happy with this, it doesn't address some of the 
> issues that you claimed, but it doesn't really break anything either and 
> it applied cleanly. If it was early in the devel cycle I would ack it 
> and then we can just fix the remaining stuff including some visual 
> things introduced here. However, if it was against a BZ and you didn't 
> satisfy all of the BZ it would be NACK'd

Even though it is not against a BZ, I would be inclined to NACK a patch at 
this point in the development cycle that either:
a) Does not do all that it claims to do
b) Breaks other things

That being said, we should start tracking bugs in BZ now.  We're far 
enough along that doing this will help us keep organized and prevent bugs 
from falling off of our radar.

> Also, the fact that this patch is depends on a previous patch of yours 
> and there is a follow on patch to correct something in this patch 
> concerns me.  We almost need to start acking things in order to not get 
> messed up or lose work.

It is ok to post a set of patches that are dependent on each other and 
require patching in order.  Just post them with [PATCH 0/3] as the 
introduction to the series, and then each individual patch as separate 
emails like [PATCH 1/3], etc...  If patches are batched in a set like that 
it is made very clear what the dependencies and order is.

However, if you post a patch and then find something wrong with it I don't 
think posting a second patch to correct the first is the right thing to 
do.  Instead, fix your original patch and repost the original patch with 
the fix in the new one.  Indicate in the repost that this patch obviates 
the earlier one.

> Hugh, Perry, should we be modifying the criteria as we get closer to beta ?
> Any comments / suggestions on how to proceed?

We're at the point now in the development cycle that patches should not be 
acked unless they work as claimed and do not introduce regressions.

Unless the patch is trivial or a NFC, at least one ACK is required to 
check it into the repository.  (Use your own judgement on this)

Just so everyone knows, an ACK is more than just, "I read your patch and 
it looks like it should work ok".  An ACK is you telling the rest of the 
development team that:
1. You've reviewed the patch and agree with the intent and implementation
2. You've tested the patch and seen that it applies, does what it says it
    should do and doesn't introduce any obvious regressions.

Of course regressions are bound to happen, and not all of them will be 
caught by the ACK testing process (that's why we have testers doing more 
rigorous testing for us).  But at no time should a patch committed to the 
repository surprise us by breaking major functionality or making the 
system entirely unusable.

Perry