From 013cb8de3824c304645a9c5db87c2e80286872d1 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Mon, 13 Dec 2010 14:18:05 -0800 Subject: [PATCH] Reduce calls to Repository.getConfig Each time getConfig() is called on FileRepository, it checks the last modified time of both ~/.gitconfig and $GIT_DIR?config. If $GIT_DIR/config appears to have been modified, it is read back in from disk and the current config is wiped out. When mutating a configuration file, this may cause in-memory edits to disappear. To avoid that callers need to avoid calling getConfig until after the configuration has been saved to disk. Unfortunately the API is still horribly broken. Configuration should be modified only while a lock is held on the configuration file, very similar to the way a ref is updated via its locking protocol. But our existing API is really broken for that so we'll have to defer cleaning up the edit path for a future change. Change-Id: I5888dd97bac20ddf60456c81ffc1eb8df04ef410 Signed-off-by: Shawn O. Pearce --- .../jgit/http/test/SmartClientSmartServerTest.java | 7 ++++--- .../src/org/eclipse/jgit/pgm/Clone.java | 13 ++++++++----- .../org/eclipse/jgit/storage/file/T0003_Basic.java | 12 ++++++------ .../org/eclipse/jgit/api/DeleteBranchCommand.java | 6 ++++-- .../src/org/eclipse/jgit/api/PullCommand.java | 2 +- .../org/eclipse/jgit/api/RenameBranchCommand.java | 14 ++++++++------ .../eclipse/jgit/storage/file/FileRepository.java | 2 +- 7 files changed, 32 insertions(+), 24 deletions(-) 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 491094ca6..f73f54ffb 100644 --- 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 @@ -500,9 +500,10 @@ public void testPush_ChunkedEncoding() throws Exception { enableReceivePack(); - db.getConfig().setInt("core", null, "compression", 0); - db.getConfig().setInt("http", null, "postbuffer", 8 * 1024); - db.getConfig().save(); + final FileBasedConfig cfg = db.getConfig(); + cfg.setInt("core", null, "compression", 0); + cfg.setInt("http", null, "postbuffer", 8 * 1024); + cfg.save(); t = Transport.open(db, remoteURI); try { diff --git a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Clone.java b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Clone.java index 0bc72a71e..8eccdedc3 100644 --- a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Clone.java +++ b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Clone.java @@ -65,6 +65,7 @@ import org.eclipse.jgit.lib.WorkDirCheckout; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; +import org.eclipse.jgit.storage.file.FileBasedConfig; import org.eclipse.jgit.storage.file.FileRepository; import org.eclipse.jgit.transport.FetchResult; import org.eclipse.jgit.transport.RefSpec; @@ -110,8 +111,9 @@ protected void run() throws Exception { dst = new FileRepository(gitdir); dst.create(); - dst.getConfig().setBoolean("core", null, "bare", false); - dst.getConfig().save(); + final FileBasedConfig dstcfg = dst.getConfig(); + dstcfg.setBoolean("core", null, "bare", false); + dstcfg.save(); db = dst; out.print(MessageFormat.format( @@ -128,13 +130,14 @@ protected void run() throws Exception { private void saveRemote(final URIish uri) throws URISyntaxException, IOException { - final RemoteConfig rc = new RemoteConfig(dst.getConfig(), remoteName); + final FileBasedConfig dstcfg = dst.getConfig(); + final RemoteConfig rc = new RemoteConfig(dstcfg, remoteName); rc.addURI(uri); rc.addFetchRefSpec(new RefSpec().setForceUpdate(true) .setSourceDestination(Constants.R_HEADS + "*", Constants.R_REMOTES + remoteName + "/*")); - rc.update(dst.getConfig()); - dst.getConfig().save(); + rc.update(dstcfg); + dstcfg.save(); } private FetchResult runFetch() throws NotSupportedException, diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/T0003_Basic.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/T0003_Basic.java index 2057020cd..6044f2599 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/T0003_Basic.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/T0003_Basic.java @@ -182,9 +182,9 @@ public void test000_openrepo_default_absolute_workdirconfig() workdir.mkdir(); FileRepository repo1initial = new FileRepository(new File(repo1Parent, Constants.DOT_GIT)); repo1initial.create(); - repo1initial.getConfig().setString("core", null, "worktree", - workdir.getAbsolutePath()); - repo1initial.getConfig().save(); + final FileBasedConfig cfg = repo1initial.getConfig(); + cfg.setString("core", null, "worktree", workdir.getAbsolutePath()); + cfg.save(); repo1initial.close(); File theDir = new File(repo1Parent, Constants.DOT_GIT); @@ -207,9 +207,9 @@ public void test000_openrepo_default_relative_workdirconfig() workdir.mkdir(); FileRepository repo1initial = new FileRepository(new File(repo1Parent, Constants.DOT_GIT)); repo1initial.create(); - repo1initial.getConfig() - .setString("core", null, "worktree", "../../rw"); - repo1initial.getConfig().save(); + final FileBasedConfig cfg = repo1initial.getConfig(); + cfg.setString("core", null, "worktree", "../../rw"); + cfg.save(); repo1initial.close(); File theDir = new File(repo1Parent, Constants.DOT_GIT); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/DeleteBranchCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/DeleteBranchCommand.java index fa00581d0..a88dd2a7e 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/DeleteBranchCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/DeleteBranchCommand.java @@ -59,6 +59,7 @@ import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.lib.StoredConfig; import org.eclipse.jgit.lib.RefUpdate.Result; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; @@ -156,10 +157,11 @@ public List call() throws JGitInternalException, String shortenedName = fullName .substring(Constants.R_HEADS.length()); // remove upstream configuration if any - repo.getConfig().unsetSection( + final StoredConfig cfg = repo.getConfig(); + cfg.unsetSection( ConfigConstants.CONFIG_BRANCH_SECTION, shortenedName); - repo.getConfig().save(); + cfg.save(); } } else throw new JGitInternalException(MessageFormat.format( diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/PullCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/PullCommand.java index 4fe050b2a..adcea90f5 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/PullCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/PullCommand.java @@ -186,7 +186,7 @@ public PullResult call() throws WrongRepositoryStateException, String remoteUri; FetchResult fetchRes; if (isRemote) { - remoteUri = repo.getConfig().getString("remote", remote, + remoteUri = repoConfig.getString("remote", remote, ConfigConstants.CONFIG_KEY_URL); if (remoteUri == null) { String missingKey = ConfigConstants.CONFIG_REMOTE_SECTION + DOT diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/RenameBranchCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/RenameBranchCommand.java index 465498132..04b7791d6 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/RenameBranchCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/RenameBranchCommand.java @@ -58,6 +58,7 @@ import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.RefRename; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.lib.StoredConfig; import org.eclipse.jgit.lib.RefUpdate.Result; /** @@ -144,27 +145,28 @@ public Ref call() throws RefNotFoundException, InvalidRefNameException, // move the upstream configuration over to the new branch String shortOldName = fullOldName .substring(Constants.R_HEADS.length()); - String oldRemote = repo.getConfig().getString( + final StoredConfig repoConfig = repo.getConfig(); + String oldRemote = repoConfig.getString( ConfigConstants.CONFIG_BRANCH_SECTION, shortOldName, ConfigConstants.CONFIG_KEY_REMOTE); if (oldRemote != null) { - repo.getConfig().setString( + repoConfig.setString( ConfigConstants.CONFIG_BRANCH_SECTION, newName, ConfigConstants.CONFIG_KEY_REMOTE, oldRemote); } - String oldMerge = repo.getConfig().getString( + String oldMerge = repoConfig.getString( ConfigConstants.CONFIG_BRANCH_SECTION, shortOldName, ConfigConstants.CONFIG_KEY_MERGE); if (oldMerge != null) { - repo.getConfig().setString( + repoConfig.setString( ConfigConstants.CONFIG_BRANCH_SECTION, newName, ConfigConstants.CONFIG_KEY_MERGE, oldMerge); } - repo.getConfig() + repoConfig .unsetSection( ConfigConstants.CONFIG_BRANCH_SECTION, shortOldName); - repo.getConfig().save(); + repoConfig.save(); } } else diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileRepository.java b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileRepository.java index a145e7725..417fa4b53 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileRepository.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileRepository.java @@ -160,7 +160,7 @@ public FileRepository(final BaseRepositoryBuilder options) throws IOException { loadUserConfig(); loadRepoConfig(); - getConfig().addChangeListener(new ConfigChangedListener() { + repoConfig.addChangeListener(new ConfigChangedListener() { public void onConfigChanged(ConfigChangedEvent event) { fireEvent(event); }