/hg/release/icedtea6-1.8: PR632: 6878713.patch breaks shark zero...

[Dr Andrew John Hughes]

On 14:46 Fri 25 Mar     , Mark Wielaard wrote:
> On Fri, 2011-03-25 at 13:28 +0000, Dr Andrew John Hughes wrote:
> > > > Where was this reviewed?
> > > 
> > > See this thread:
> > > http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2011-March/013107.html
> > > 
> > How is that a review when there is no patch attached?
> 
> The actual patch was up earlier in the thread. Matthias just asked if it
> was relevant for backporting and I agreed. Since the patch mentions a
> bug number you could also look that up if you are curious, it has a lot
> more disscusion about the original issue:
> http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=632
> 

But that's a different patch to the one committed as it's on a different tree.
That's what's caused the issue below.

> > There is an issue with the patch that needs correcting.  The NEWS entry is filed
> > under 'Backports' whereas I don't believe this is an upstream backport.  It should
> > be under 'Bug fixes'.
> 
> OK, noted for next time. Thanks.
> 

So is someone going to fix this?

> > Has it been tested on both HotSpot configurations in 1.8?
> 
> That is the responsibility of the person committing to the branch.
> 

Yes, and something I would have asked in review if there had been one.

So, doko, has it been tested on both?

> > What about 1.9?
> 
> I haven't tried personally with any branch, just the trunk which I am
> using.
> 

The question was more aimed at doko as he was backporting the patch.
I don't like this cherry-picking of branches as it causes confusion as
to what has been applied where.  Let's try and keep everything on the
same level unless there's a good reason not to backport somewhere.

> > > And please only quote the relevant bits in your email.
> > > 
> > Again, the whole patch is relevant.  I now can't comment on it because you removed it.
> 
> Sigh.
> 
> Really, I get the feeling you are overly pedantic.
> But I cannot figure out why.
> 

I don't follow.  The comment about NEWS above would make more sense if it was
in the context of the change in the patch.  But you dropped it from the mail.

> I just tried to be helpful when Matthias asked if I thought it was a
> good idea to backport that patch. And I thought it was, even though I
> don't use that branch myself, but obviously Matthias does.
> 

I don't have a problem with that.  But there's a missing stage between you
saying "that sounds a good idea" to a backported patch appearing on the branch
with no review.  The actual patch for 1.8 wasn't posted before commit, and there's
no indication as to what builds have been tested, if any.

I don't think requiring review and testing for patches to a stable branch is
pedantic, especially when that branch is about to have a release, as with 1.10.
There's now been far more time spent on deliberating and arguing, both here on
IRC, than it ever would have taken to just post the patch on the list and
have it oked.

Not doing so means someone else has to do that testing and someone else has to
fix the remaining issues.  Most of that time, it invariably seems to be me.
More time has to be wasted cleaning up the mess that has been created, so
someone else can be lazy in not posting the patch for review.

> Cheers,
> 
> Mark
> 

-- 
Andrew :)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D  0698 0713 C3ED F586 2A37