Cleanup: message reporting for HTTP redirect handling

The addition of "tooManyRedirects" in commit 7ac1bfc ("Do
authentication re-tries on HTTP POST") was an error I didn't
catch after rebasing that change. That message had been renamed
in the earlier commit e17bfc9 ("Add support to follow HTTP
redirects") to "redirectLimitExceeded".

Also make sure we always use the TransportException(URIish, ...)
constructor; it'll prefix the message given with the sanitized URI.
Change messages to remove the explicit mention of that URI inside the
message. Adapt tests that check the expected exception message text.

For the info logging of redirects, remove a potentially present
password component in the URI to avoid leaking it into the log.

Change-Id: I517112404757a9a947e92aaace743c6541dce6aa
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

diff --git a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java
index 1b0c6949a97d1dc0c7127cea67c7d3185dd105d7..51b79903c6514337cfd52331939dab1bd7c193c2 100644 (file)
--- a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java
+++ b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java
@@ -595,9 +595,9 @@ public class SmartClientSmartServerTest extends HttpTestCase {
                        t.fetch(NullProgressMonitor.INSTANCE, mirror(master));
                        fail("Should have failed (too many redirects)");
                } catch (TransportException e) {
-                       String expectedMessageBegin = MessageFormat.format(
-                                       JGitText.get().redirectLimitExceeded, remoteUri, "3",
-                                       remoteUri.replace("/4/", "/1/") + '/', "");
+                       String expectedMessageBegin = remoteUri.toString() + ": "
+                                       + MessageFormat.format(JGitText.get().redirectLimitExceeded,
+                                                       "3", remoteUri.replace("/4/", "/1/") + '/', "");
                        String message = e.getMessage();
                        if (message.length() > expectedMessageBegin.length()) {
                                message = message.substring(0, expectedMessageBegin.length());
@@ -616,7 +616,7 @@ public class SmartClientSmartServerTest extends HttpTestCase {
                        t.fetch(NullProgressMonitor.INSTANCE, mirror(master));
                        fail("Should have failed (redirect loop)");
                } catch (TransportException e) {
-                       assertTrue(e.getMessage().contains("redirected more than"));
+                       assertTrue(e.getMessage().contains("Redirected more than"));
                }
        }
 

diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
index c532b328db6347fb091f70c0c0cb27fcea2b022e..8d3931493c7305079e583c9879c57ec47a601740 100644 (file)
--- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
+++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
@@ -365,7 +365,7 @@ invalidPathContainsSeparator=Invalid path (contains separator ''{0}''): {1}
 invalidPathPeriodAtEndWindows=Invalid path (period at end is ignored by Windows): {0}
 invalidPathSpaceAtEndWindows=Invalid path (space at end is ignored by Windows): {0}
 invalidPathReservedOnWindows=Invalid path (''{0}'' is reserved on Windows): {1}
-invalidRedirectLocation=Invalid redirect location {1} -> {2}
+invalidRedirectLocation=Invalid redirect location {0} -> {1}
 invalidReflogRevision=Invalid reflog revision: {0}
 invalidRefName=Invalid ref name: {0}
 invalidReftableBlock=Invalid reftable block
@@ -535,11 +535,11 @@ receivePackObjectTooLarge2=Object too large ({0} bytes), rejecting the pack. Max
 receivePackInvalidLimit=Illegal limit parameter value {0}
 receivePackTooLarge=Pack exceeds the limit of {0} bytes, rejecting the pack
 receivingObjects=Receiving objects
-redirectBlocked=URI ''{0}'' redirection blocked: redirect {1} -> {2} not allowed
+redirectBlocked=Redirection blocked: redirect {0} -> {1} not allowed
 redirectHttp=URI ''{0}'': following HTTP redirect #{1}  {2} -> {3}
-redirectLimitExceeded=URI ''{0}'' redirected more than {1} times; aborted at {2} -> {3}
-redirectLocationMissing=Invalid redirect of ''{0}'': no redirect location for {1}
-redirectsOff=Cannot redirect ''{0}'': http.followRedirects is false (HTTP status {1})
+redirectLimitExceeded=Redirected more than {0} times; aborted at {1} -> {2}
+redirectLocationMissing=Invalid redirect: no redirect location for {0}
+redirectsOff=Cannot redirect because http.followRedirects is false (HTTP status {0})
 refAlreadyExists=already exists
 refAlreadyExists1=Ref {0} already exists
 reflogEntryNotFound=Entry {0} not found  in reflog for ''{1}''
@@ -636,7 +636,6 @@ timeIsUncertain=Time is uncertain
 timerAlreadyTerminated=Timer already terminated
 tooManyCommands=Too many commands
 tooManyIncludeRecursions=Too many recursions; circular includes in config file(s)?
-tooManyRedirects=Too many redirects; stopped after {0} redirects at ''{1}''
 topologicalSortRequired=Topological sort required.
 transactionAborted=transaction aborted
 transportExceptionBadRef=Empty ref: {0}: {1}

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
index 5acf0587645f5a7fa1117330d26dff44078d5615..07666eb934d58abaeafa29267a00bebb00a8a557 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
@@ -696,7 +696,6 @@ public class JGitText extends TranslationBundle {
        /***/ public String timerAlreadyTerminated;
        /***/ public String tooManyCommands;
        /***/ public String tooManyIncludeRecursions;
-       /***/ public String tooManyRedirects;
        /***/ public String topologicalSortRequired;
        /***/ public String transportExceptionBadRef;
        /***/ public String transportExceptionEmptyRef;

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java
index 1647200e65adbbaafcd6036ca18540a04bf0ffcb..26612120c1c15ecb056c350a29b4e99de4e3269e 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java
@@ -604,7 +604,8 @@ public class TransportHttp extends HttpTransport implements WalkTransport,
 
                                case HttpConnection.HTTP_FORBIDDEN:
                                        throw new TransportException(uri, MessageFormat.format(
-                                                       JGitText.get().serviceNotPermitted, service));
+                                                       JGitText.get().serviceNotPermitted, baseUrl,
+                                                       service));
 
                                case HttpConnection.HTTP_MOVED_PERM:
                                case HttpConnection.HTTP_MOVED_TEMP:
@@ -614,9 +615,10 @@ public class TransportHttp extends HttpTransport implements WalkTransport,
                                        // and in general should occur only on POST requests. But it
                                        // doesn't hurt to accept it here as a redirect.
                                        if (http.followRedirects == HttpRedirectMode.FALSE) {
-                                               throw new TransportException(MessageFormat.format(
-                                                               JGitText.get().redirectsOff, uri,
-                                                               Integer.valueOf(status)));
+                                               throw new TransportException(uri,
+                                                               MessageFormat.format(
+                                                                               JGitText.get().redirectsOff,
+                                                                               Integer.valueOf(status)));
                                        }
                                        URIish newUri = redirect(conn.getHeaderField(HDR_LOCATION),
                                                        Constants.INFO_REFS, redirects++);
@@ -655,31 +657,34 @@ public class TransportHttp extends HttpTransport implements WalkTransport,
        private URIish redirect(String location, String checkFor, int redirects)
                        throws TransportException {
                if (location == null || location.isEmpty()) {
-                       throw new TransportException(MessageFormat.format(
-                                       JGitText.get().redirectLocationMissing, uri, baseUrl));
+                       throw new TransportException(uri,
+                                       MessageFormat.format(JGitText.get().redirectLocationMissing,
+                                                       baseUrl));
                }
                if (redirects >= http.maxRedirects) {
-                       throw new TransportException(MessageFormat.format(
-                                       JGitText.get().redirectLimitExceeded, uri,
+                       throw new TransportException(uri,
+                                       MessageFormat.format(JGitText.get().redirectLimitExceeded,
                                        Integer.valueOf(http.maxRedirects), baseUrl, location));
                }
                try {
                        if (!isValidRedirect(baseUrl, location, checkFor)) {
-                               throw new TransportException(
+                               throw new TransportException(uri,
                                                MessageFormat.format(JGitText.get().redirectBlocked,
-                                                               uri, baseUrl, location));
+                                                               baseUrl, location));
                        }
                        location = location.substring(0, location.indexOf(checkFor));
                        URIish result = new URIish(location);
                        if (LOG.isInfoEnabled()) {
-                               LOG.info(MessageFormat.format(JGitText.get().redirectHttp, uri,
+                               LOG.info(MessageFormat.format(JGitText.get().redirectHttp,
+                                               uri.setPass(null),
                                                Integer.valueOf(redirects), baseUrl, result));
                        }
                        return result;
                } catch (URISyntaxException e) {
-                       throw new TransportException(MessageFormat.format(
-                                       JGitText.get().invalidRedirectLocation,
-                                       uri, baseUrl, location), e);
+                       throw new TransportException(uri,
+                                       MessageFormat.format(JGitText.get().invalidRedirectLocation,
+                                                       baseUrl, location),
+                                       e);
                }
        }