harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Hindess <mark.hind...@googlemail.com>
Subject Re: svn commit: r935847 - in /harmony/enhanced/java/trunk/classlib/modules/luni/src: main/java/org/apache/harmony/luni/internal/net/www/protocol/file/ test/api/common/org/apache/harmony/luni/tests/internal/net/www/protocol/file/
Date Tue, 20 Apr 2010 12:13:31 GMT

In message <o2h70c713191004200431s6b6fb572i30726915644cb4cf@mail.gmail.com>,
Kevin Zhou writes:
>
> Thanks, Mark and Sebb
> 
> On Tue, Apr 20, 2010 at 6:37 PM, sebb <sebbaz@gmail.com> wrote:
> 
> > Also, consider whether the header field could be made final - it looks
> > like it is only set in the constructor.

Kevin,

I'm still not happy about your commit r935873.  Your commit message says:

  Apply patch for HARMONY-6504: [classlib][luni]Harmony
  returns null for header related function when file URL is
  handled. This patch implements the header related functions
  [getHeaderField(int)/getHeaderFieldKey(int)/getHeaderField(
  String)/getLastModified()] to prevent Harmony to return null when file
  URL is handled.

and contains the change:

  +    private final LinkedHashMap<String, String> header;

but Mohanraj's patch contains the change:

  +    private LinkedHashMap<String, String> header;

So, at the very least, your comment should have said that the commit was
a modified version of the patch with an explanation for the modification
either in the commit message or on the JIRA comment associated with the
commit[0].  (You should probably give credit for the change in this
case as well.)

Personally, if a modification isn't required to integrate the patch then
I prefer to see two commits.  One with the patch and one with subsequent
improvements.

Regards,
 Mark.

[0] See Oliver's commit r935528 of  HARMONY-6476 for example.



Mime
View raw message