On Mon, Mar 14, 2011 at 06:34:15PM +0200, Daniel Shahaf wrote:
> Kamesh Jayachandran wrote on Fri, Mar 11, 2011 at 18:12:21 +0530:
> > Unless there are no objection I will commit this patch post I
> > receive a r1075802 regression fix(and testcase) from either Daniel
> > Becroft or Arwin.
>
> Hold on, are you suggesting to commit the two thousand, six hundred and
> sixty-three line diff that started this thread?
So what if it's long? I'd be much more worried if this diff was shorter.
It needs to make changes in lots of places.
It does a lot of things, including running every update during tests
with --dry-run, and checking whether the dry-run modified the working
copy, and making the test fail if it did.
And it checks if the dry-run output matches the output for normal update.
If all the tests pass with this, that's fine, isn't it?
Because how else would you do it? Not test it, just to make the diff shorter?
And I don't agree that adding a separate editor for this is going to
make things simpler. We're going to transition to editorv2 at some point.
Let's try not to make that transition more difficult by adding yet
another editor.
From my point of view, we can either add this feature and apply
this diff, or not have the feature. I'd say we should go for it,
unless we decide that we really don't want this feature.
But rejecting this diff based on line counts seems silly.
The only minor thing I noticed scrolling through the diff was that
the new dry_run paramter of run_and_verify_update isn't documented
in the docstring. That can be fixed later.
Received on 2011-03-14 17:55:39 CET