You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by kk...@apache.org on 2012/08/13 15:06:43 UTC
svn commit: r1372415 - /tomcat/tc6.0.x/trunk/STATUS.txt
Author: kkolinko
Date: Mon Aug 13 13:06:43 2012
New Revision: 1372415
URL: http://svn.apache.org/viewvc?rev=1372415&view=rev
Log:
Change votes where my concerns were addressed.
Add formatting fix to schultz's proposal.
Modified:
tomcat/tc6.0.x/trunk/STATUS.txt
Modified: tomcat/tc6.0.x/trunk/STATUS.txt
URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/STATUS.txt?rev=1372415&r1=1372414&r2=1372415&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/STATUS.txt (original)
+++ tomcat/tc6.0.x/trunk/STATUS.txt Mon Aug 13 13:06:43 2012
@@ -145,31 +145,14 @@ PATCHES PROPOSED TO BACKPORT:
http://svn.apache.org/viewvc?rev=1370537&view=rev
http://svn.apache.org/viewvc?rev=1372390&view=rev (addresses kkolinko's -1)
+1: markt, schultz
- -1: kkolinko:
- Regarding FormAuthenticator.restoreRequest(..):
- My -1 is because decodedURI is saved into SavedRequest in #saveRequest(..)
- but is restored into requestURI field in #restoreRequest(..).
-
- The following are my concerns:
- 1. The web application protected by FORM auth might have expected path
- parameters, and now those are lost from requestURI.
- 2. The decodedURI value is url-decoded in CoyoteAdapter.postParseRequest(..),
- while requestURI is not. Using one for the other changes behaviour.
-
- 3. An issue that exists in the old code as well: I wonder why
- decodedURI value is not restored by restoreRequest(). It looks like a
- bug. I think an observable consequence is that o.a.c.connector.Request#toAbsolute()
- will return different values because of different values of decodedURI.
-
- The BZ 53584 bug is essentially in matchRequest(..) and I agree that it should
- be changed to compare decodedURI values.
- Can SavedRequest store both requestURI and decodedURI values and
- restore both of them?
+ +1: kkolinko (OK, my concerns were addressed)
+ -1:
* Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=53481
Add support for SSLHonorCipherOrder
http://svn.apache.org/viewvc?view=revision&revision=1371298
http://svn.apache.org/viewvc?view=revision&revision=1371302 (rolls-back inadvertent addition of TOMCAT-NEXT.txt)
+ http://svn.apache.org/viewvc?view=revision&revision=1371620 (tab -> spaces)
+1: schultz
-1:
@@ -178,12 +161,14 @@ PATCHES PROPOSED TO BACKPORT:
Patch from 7.0.x should apply relatively cleanly, as it is very small:
http://svn.apache.org/viewvc?view=revision&revision=1041892
+1: schultz
- -1: kkolinko: r1041892 is not enough. It has bad formatting, a HashMap
+ 0: kkolinko: r1041892 is not enough. It has bad formatting, a HashMap
protected field (should be 'Map'), etc. See the current code of Tomcat 7.
schultz: current trunk and TC7 code has HashSet<String>. Am I misunderstanding?
I will also add http://svn.apache.org/viewvc?view=revision&revision=1043983
and http://svn.apache.org/viewvc?view=revision&revision=1049264
to fix the formatting issues.
+ kkolinko: If we missed it, then it has to be fixed in trunk. I think
+ a patch for 6.0 is needed to properly review the change here.
* Further fixes for https://issues.apache.org/bugzilla/show_bug.cgi?id=53071
- Use standard text for standard HTTP error codes
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1372415 - /tomcat/tc6.0.x/trunk/STATUS.txt
Posted by Konstantin Kolinko <kn...@gmail.com>.
2012/8/13 Christopher Schultz
>
> Can I ask why the use of HashSet<String> rather than Set<String> is a
> problem? I intentionally used the implementation class instead of an
> interface for a (small) performance improvement. I'm happy to patch
> trunk/7.0.x/6.0.x to fix this if you feel that it is important.
>
Only because it is a 'protected' field and thus is a part of API. If
it were a private field I wouldn't care.
I mean we are limiting ourselves from replacing it by TreeMap, or
Flat3Map, or whetever other implementation in the future.
I think it is too small concern to make us change released code (I
mean, leave 7.0 as is because we have a released version containing
this code), but it is a valid concern to fix 8 and avoid it in 6.
There are cases when concrete implementation class has additional
methods that are useful, e.g. ArrayList has trimToSize() method. If
you intend to use those, then it is OK to leave concrete
implementation class in the declaration.
Just my preference.
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1372415 - /tomcat/tc6.0.x/trunk/STATUS.txt
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Konstantin,
On 8/13/12 9:06 AM, kkolinko@apache.org wrote:
> Author: kkolinko
> @@ -178,12 +161,14 @@ PATCHES PROPOSED TO BACKPORT:
> Patch from 7.0.x should apply relatively cleanly, as it is very small:
> http://svn.apache.org/viewvc?view=revision&revision=1041892
> +1: schultz
> - -1: kkolinko: r1041892 is not enough. It has bad formatting, a HashMap
> + 0: kkolinko: r1041892 is not enough. It has bad formatting, a HashMap
> protected field (should be 'Map'), etc. See the current code of Tomcat 7.
> schultz: current trunk and TC7 code has HashSet<String>. Am I misunderstanding?
> I will also add http://svn.apache.org/viewvc?view=revision&revision=1043983
> and http://svn.apache.org/viewvc?view=revision&revision=1049264
> to fix the formatting issues.
> + kkolinko: If we missed it, then it has to be fixed in trunk. I think
> + a patch for 6.0 is needed to properly review the change here.
Can I ask why the use of HashSet<String> rather than Set<String> is a
problem? I intentionally used the implementation class instead of an
interface for a (small) performance improvement. I'm happy to patch
trunk/7.0.x/6.0.x to fix this if you feel that it is important.
Thanks,
-chris