threads / patch / 20279

patch, 5 partsSome minor bug fixes

Subject: [JGIT PATCH 0/5] Some minor bug fixes

## tl;dr

6 messages between Jul 29, 2009 and Jul 29, 2009. Diffs are folded; open one to read it.

replies: 5people: 1as markdown or json

Shawn O. Pearce· Jul 29, 2009, 15:50 UTC · lore

I'm just throwing this series into the repository as-is.

The first in the series is needed to fix Gerrit Code Review (busted due to a recent change to ReceivePack). The rest of the series is really minor public API corrections.

Shawn O. Pearce (5):
  Don't dispose of RevFlag used to advertise objects in ReceivePack
  Allow RemoteConfig.getAllRemoteConfigs on any Config object
  Add ConfigInvalidException constructor to take Throwable
  Add no fallback constructor to FileBasedConfig
  Include the file path when FileBasedConfig.load throws an exception

 .../tst/org/spearce/jgit/lib/MockSystemReader.java |    2 +-
 .../org/spearce/jgit/lib/RepositoryTestCase.java   |    2 +-
 .../jgit/errors/ConfigInvalidException.java        |   12 ++++++++++++
 .../src/org/spearce/jgit/lib/FileBasedConfig.java  |   16 ++++++++++++++++
 .../org/spearce/jgit/transport/ReceivePack.java    |    1 -
 .../org/spearce/jgit/transport/RemoteConfig.java   |    5 ++---
 .../src/org/spearce/jgit/util/SystemReader.java    |    3 +--
 7 files changed, 33 insertions(+), 8 deletions(-)

Shawn O. Pearce· Jul 29, 2009, 15:50 UTC · re: Shawn O. Pearce · lore

[JGIT PATCH 1/5] Don't dispose of RevFlag used to advertise objects in ReceivePack

Back in 3ae30ae64d56113a71a07a3ff12d0af71cf538e2 I refactored how the ReceivePack and UploadPack programs advertise the set of refs that the server-side currently has. As part of this change ReceivePack allocated a new RevFlag and marked it on all objects it advertised to the client, because the common code with UploadPack requires the RevFlag to mark objects. It also uses this RevFlag to avoid sending ".have" lines for objects we have already described.

Disposing of the RevFlag however is problematic for any application that uses the ReceivePack's RevWalk instance within a PreReceiveHook or a PostReceiveHook implementation. By disposing of the RevFlag, the flag bit was not actually unset from the advertised objects, it was only marked as being eligible for reallocation in another RevFlag. If the flag bit was reallocated, any advertised objects would automatically have the bit set, which can confuse the new user of the bit.

RevWalk's isMergedInto() really got confused, as it allocates a temporary RevFlag for each input RevCommit in order to compute the merge base.

The simplest fix is to just never dispose of this RevFlag once it has been allocated. We could dispose of it and also unset it from all objects we advertised, but that would require us to keep track of the full advertised set, a potentially expensive thing to do given that most callers will never need to allocate a RevFlag, let alone care about the 1 bit leaked by the protocol.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../org/spearce/jgit/transport/ReceivePack.java    |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

Show changes to org.spearce.jgit/src/org/spearce/jgit/transport/ReceivePack.java +0 −1

diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/ReceivePack.java b/org.spearce.jgit/src/org/spearce/jgit/transport/ReceivePack.java
index ca4a7ec..eb21254 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/ReceivePack.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/ReceivePack.java
@@ -547,7 +547,6 @@ private void sendAdvertisedRefs() throws IOException {
 		if (adv.isEmpty())
 			adv.advertiseId(ObjectId.zeroId(), "capabilities^{}");
 		pckOut.end();
-		walk.disposeFlag(advertised);
 	}
 
 	private void recvCommands() throws IOException {

-- 
1.6.4.rc3.201.gd9d59

Shawn O. Pearce· Jul 29, 2009, 15:50 UTC · re: Shawn O. Pearce · lore

[JGIT PATCH 2/5] Allow RemoteConfig.getAllRemoteConfigs on any Config object

Back in 07c8dc6e94729ff4104f0a1c815dd81f3e71c562 I changed the RemoteConfig class to accept any Config object, but its static getAllRemoteConfigs() utility method was missed and still used a RepositoryConfig. It should take any Config.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../org/spearce/jgit/transport/RemoteConfig.java   |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

Show changes to org.spearce.jgit/src/org/spearce/jgit/transport/RemoteConfig.java +2 −3

diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/RemoteConfig.java b/org.spearce.jgit/src/org/spearce/jgit/transport/RemoteConfig.java
index ca84acf..dd7a50c 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/RemoteConfig.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/RemoteConfig.java
@@ -44,7 +44,6 @@
 import java.util.List;
 
 import org.spearce.jgit.lib.Config;
-import org.spearce.jgit.lib.RepositoryConfig;
 
 /**
  * A remembered remote repository, including URLs and RefSpecs.
@@ -96,8 +95,8 @@
 	 * @throws URISyntaxException
 	 *             one of the URIs within the remote's configuration is invalid.
 	 */
-	public static List<RemoteConfig> getAllRemoteConfigs(
-			final RepositoryConfig rc) throws URISyntaxException {
+	public static List<RemoteConfig> getAllRemoteConfigs(final Config rc)
+			throws URISyntaxException {
 		final List<String> names = new ArrayList<String>(rc
 				.getSubsections(SECTION));
 		Collections.sort(names);

-- 
1.6.4.rc3.201.gd9d59

Shawn O. Pearce· Jul 29, 2009, 15:50 UTC · re: Shawn O. Pearce · lore

[JGIT PATCH 3/5] Add ConfigInvalidException constructor to take Throwable

This permits applications to wrap a ConfigInvalidException with more detail before throwing it to the caller.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../jgit/errors/ConfigInvalidException.java        |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

Show changes to org.spearce.jgit/src/org/spearce/jgit/errors/ConfigInvalidException.java +12 −0

diff --git a/org.spearce.jgit/src/org/spearce/jgit/errors/ConfigInvalidException.java b/org.spearce.jgit/src/org/spearce/jgit/errors/ConfigInvalidException.java
index 6e861fd..28c87a5 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/errors/ConfigInvalidException.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/errors/ConfigInvalidException.java
@@ -50,4 +50,16 @@
 	public ConfigInvalidException(final String message) {
 		super(message);
 	}
+
+	/**
+	 * Construct an invalid configuration error.
+	 *
+	 * @param message
+	 *            why the configuration is invalid.
+	 * @param cause
+	 *            root cause of the error.
+	 */
+	public ConfigInvalidException(final String message, final Throwable cause) {
+		super(message, cause);
+	}
 }

-- 
1.6.4.rc3.201.gd9d59

Shawn O. Pearce· Jul 29, 2009, 15:50 UTC · re: Shawn O. Pearce · lore

[JGIT PATCH 4/5] Add no fallback constructor to FileBasedConfig

Sometimes when reading a FileBasedConfig the caller does not want any fallback lookup. Instead always passing a null first parameter to the constructor, we can use a 1-arg constructor that takes only the File path for this file. This simplifies a number of cases within the JGit library and test cases, as well as some application code.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../tst/org/spearce/jgit/lib/MockSystemReader.java |    2 +-
 .../org/spearce/jgit/lib/RepositoryTestCase.java   |    2 +-
 .../src/org/spearce/jgit/lib/FileBasedConfig.java  |   10 ++++++++++
 .../src/org/spearce/jgit/util/SystemReader.java    |    3 +--
 4 files changed, 13 insertions(+), 4 deletions(-)

Show changes to 4 files +13 −4

org.spearce.jgit.test/tst/org/spearce/jgit/lib/MockSystemReader.java, org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java, org.spearce.jgit/src/org/spearce/jgit/lib/FileBasedConfig.java, org.spearce.jgit/src/org/spearce/jgit/util/SystemReader.java

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/MockSystemReader.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/MockSystemReader.java
index 62862d1..7a65f99 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/MockSystemReader.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/MockSystemReader.java
@@ -53,7 +53,7 @@
 		init(Constants.GIT_AUTHOR_EMAIL_KEY);
 		init(Constants.GIT_COMMITTER_NAME_KEY);
 		init(Constants.GIT_COMMITTER_EMAIL_KEY);
-		userGitConfig = new FileBasedConfig(null, null);
+		userGitConfig = new FileBasedConfig(null);
 	}
 
 	private void init(final String n) {
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java
index 24a99ca..6de9afe 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java
@@ -237,7 +237,7 @@ public void run() {
 		}
 
 		final MockSystemReader mockSystemReader = new MockSystemReader();
-		mockSystemReader.userGitConfig = new FileBasedConfig(null, new File(
+		mockSystemReader.userGitConfig = new FileBasedConfig(new File(
 				trash_git, "usergitconfig"));
 		SystemReader.setInstance(mockSystemReader);
 
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/FileBasedConfig.java b/org.spearce.jgit/src/org/spearce/jgit/lib/FileBasedConfig.java
index a419e7f..adf85c6 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/FileBasedConfig.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/FileBasedConfig.java
@@ -55,6 +55,16 @@
 	private final File configFile;
 
 	/**
+	 * Create a configuration with no default fallback.
+	 *
+	 * @param cfgLocation
+	 *            the location of the configuration file on the file system
+	 */
+	public FileBasedConfig(File cfgLocation) {
+		this(null, cfgLocation);
+	}
+
+	/**
 	 * The constructor
 	 *
 	 * @param base
diff --git a/org.spearce.jgit/src/org/spearce/jgit/util/SystemReader.java b/org.spearce.jgit/src/org/spearce/jgit/util/SystemReader.java
index a30dfef..51a0d29 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/util/SystemReader.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/util/SystemReader.java
@@ -42,7 +42,6 @@
 import java.net.UnknownHostException;
 
 import org.spearce.jgit.lib.FileBasedConfig;
-import org.spearce.jgit.lib.RepositoryConfig;
 
 /**
  * Interface to read values from the system.
@@ -66,7 +65,7 @@ public String getProperty(String key) {
 
 		public FileBasedConfig openUserConfig() {
 			final File home = FS.userHome();
-			return new RepositoryConfig(null, new File(home, ".gitconfig"));
+			return new FileBasedConfig(new File(home, ".gitconfig"));
 		}
 
 		public String getHostname() {

-- 
1.6.4.rc3.201.gd9d59

Shawn O. Pearce· Jul 29, 2009, 15:50 UTC · re: Shawn O. Pearce · lore

[JGIT PATCH 5/5] Include the file path when FileBasedConfig.load throws an exception

This may help an end-user debug the problem, if they know what file the error occurred in. We have to wrap the exception here because the methods throwing the original error do not have access to the file path, and cannot include it in the message.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../src/org/spearce/jgit/lib/FileBasedConfig.java  |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

Show changes to org.spearce.jgit/src/org/spearce/jgit/lib/FileBasedConfig.java +6 −0

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/FileBasedConfig.java b/org.spearce.jgit/src/org/spearce/jgit/lib/FileBasedConfig.java
index adf85c6..518b31d 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/FileBasedConfig.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/FileBasedConfig.java
@@ -98,6 +98,12 @@ public void load() throws IOException, ConfigInvalidException {
 			fromText(RawParseUtils.decode(NB.readFully(getFile())));
 		} catch (FileNotFoundException noFile) {
 			clear();
+		} catch (IOException e) {
+			final IOException e2 = new IOException("Cannot read " + getFile());
+			e2.initCause(e);
+			throw e2;
+		} catch (ConfigInvalidException e) {
+			throw new ConfigInvalidException("Cannot read " + getFile(), e);
 		}
 	}

-- 
1.6.4.rc3.201.gd9d59