Hey Pan, I've merged in the patch, on initial inspection it looks good to me.
However whenever I get a patch from you through codereview.appspot.com, I get lots of warnings and even a few merge failures, which means I have to merge in changes by hand ... a process that is not exactly fault tollerant, so I hope you can check what I've committed against your local copy and make sure I didn't miss anything. Could you please in the future submit a plain patch to JIRA, and if you like, add a link to the codereview in the JIRA issue's description; My suspicion is that codereview might reorder some of the diff blocks which would lead to such failures. (Either that, or your working on a completely different php-shindig that I'm not aware off:) In this case applying the patch gave me this result: patch -p0 < ~/issue41111_1.diff (Stripping trailing CRs from patch.) patching file php/index.php (Stripping trailing CRs from patch.) patching file php/src/common/RemoteContentRequest.php (Stripping trailing CRs from patch.) patching file php/src/common/sample/BasicRemoteContent.php (Stripping trailing CRs from patch.) patching file php/src/gadgets/GadgetSpecParser.php (Stripping trailing CRs from patch.) patching file php/src/gadgets/MakeRequestHandler.php (Stripping trailing CRs from patch.) patching file php/src/gadgets/oauth/GadgetOAuthTokenStore.php Hunk #1 succeeded at 138 with fuzz 2. (Stripping trailing CRs from patch.) patching file php/src/gadgets/oauth/OAuth.php Hunk #1 FAILED at 266. 1 out of 1 hunk FAILED -- saving rejects to file php/src/gadgets/oauth/OAuth.php.rej (Stripping trailing CRs from patch.) patching file php/src/gadgets/oauth/OAuthAccessor.php (Stripping trailing CRs from patch.) patching file php/src/gadgets/oauth/OAuthFetcher.php Hunk #5 succeeded at 248 with fuzz 2. Hunk #14 FAILED at 539. Hunk #15 FAILED at 550. 2 out of 16 hunks FAILED -- saving rejects to file php/src/gadgets/oauth/OAuthFetcher.php.rej (Stripping trailing CRs from patch.) patching file php/src/gadgets/oauth/OAuthService.php (Stripping trailing CRs from patch.) patching file php/src/gadgets/oauth/OAuthStore.php (Stripping trailing CRs from patch.) patching file php/src/gadgets/sample/BasicGadgetSpecFactory.php Hunk #1 succeeded at 21 with fuzz 2. (Stripping trailing CRs from patch.) patching file php/src/gadgets/servlet/OAuthCallbackServlet.php That's a *lot* of warnings, fuzzies and even 3 failed hunks that I had to merge in manually... That takes time and I'm likely to make a mistake with that at some point.. So please use plain patches / svn diff's attached to a JIRA issue, and I suspect these problems will go away. Thanks for the great work on fixing up the OAuth proxy! On Mon, Apr 20, 2009 at 1:29 PM, <panjie....@gmail.com> wrote: > http://codereview.appspot.com/41111/diff/1/14#newcode24 > Line 24: //header('Cache-Control: public,max-age=3600'); > Java shindig using 3600 for max-age here: > > http://svn.apache.org/repos/asf/incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/OAuthCallbackServlet.java > I'm not sure that we should use 3600 here or other default value since > response for this page never changes. When in doubt, I guess following java's lead here will lead to a more consistent situation, so it's fine imo -- Chris
