Git: Don't close underlying repo if it came from from a caller

Since 27ae8bc65 Git has implemented AutoCloseable, which means Eclipse
may warn if close() is never called on a Git instance. For example,
the following would result in a resource warning:

  Repository repo = openRepository(foo);
  Git git = new Git(repo);
  try {
    git.someCommand().call();
  } finally {
    repo.close();
  }

(The same warning would occur if repo were created in a try-with-
resources block.)

The "obvious" fix is to open git in a try-with-resources block:

  try (Repository repo = openRepository(foo);
      Git git = new Git(repo)) {
    git.someCommand().call();
  }

Unfortunately, this construction was subtly broken: it would call both
git.close() and repo.close(), but git.close() would call repo.close()
again. Depending on the repository implementation, this might or might
not be ok. If it's not ok, it might not immediately cause an error, if
the reference count of repo was >2 at the time of closing.

Of course, explicitly calling git.close() followed by repo.close() in
two finally blocks has had the same double-closing problem since
forever. But the problem became worse when Git started implementing
AutoCloseable, because now Eclipse is _actively encouraging_
developers to change working code into broken code.

To work around this, keep track in Git's constructor of whether the
repository was passed in or opened at construction time, and only
close the repository if it was opened by Git.

Note that in the original example, there was not _actually_ a resource
leak, since repo was closed exactly once; git did not _need_ to be
closed in this case. But at least fixing this false-positive warning
no longer introduces a real bug.

Change-Id: Ie927a26ce3ae2bf8c3ef5cb963a60847067db95a

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/Git.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/Git.java
index eab3b36008ca42b319cd5fa4e6db3473c84ad334..1e9fe5c25f57d9caece039b5a6ba6c415d6f9cf8 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/api/Git.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/Git.java
@@ -86,6 +86,8 @@ public class Git implements AutoCloseable {
        /** The git repository this class is interacting with */
        private final Repository repo;
 
+       private final boolean closeRepo;
+
        /**
         * @param dir
         *            the repository to open. May be either the GIT_DIR, or the
@@ -103,44 +105,50 @@ public class Git implements AutoCloseable {
         *            working tree directory that contains {@code .git}.
         * @param fs
         *            filesystem abstraction to use when accessing the repository.
-        * @return a {@link Git} object for the existing git repository
+        * @return a {@link Git} object for the existing git repository. Closing this
+        *         instance will close the repo.
         * @throws IOException
         */
        public static Git open(File dir, FS fs) throws IOException {
                RepositoryCache.FileKey key;
 
                key = RepositoryCache.FileKey.lenient(dir, fs);
-               return wrap(new RepositoryBuilder().setFS(fs).setGitDir(key.getFile())
-                               .setMustExist(true).build());
+               Repository db = new RepositoryBuilder().setFS(fs).setGitDir(key.getFile())
+                               .setMustExist(true).build();
+               return new Git(db, true);
        }
 
        /**
         * @param repo
-        *            the git repository this class is interacting with.
-        *            {@code null} is not allowed
-        * @return a {@link Git} object for the existing git repository
+        *            the git repository this class is interacting with;
+        *            {@code null} is not allowed.
+        * @return a {@link Git} object for the existing git repository. The caller is
+        *         responsible for closing the repository; {@link #close()} on this
+        *         instance does not close the repo.
         */
        public static Git wrap(Repository repo) {
                return new Git(repo);
        }
 
        /**
-        * Frees resources held by the underlying {@link Repository} instance. It is
-        * recommended to call this method as soon as you don't need a reference to
-        * this {@link Git} instance and the underlying {@link Repository} instance
-        * anymore. This method closes the underlying object and ref databases. This
-        * will free memory and file handles. E.g. on Windows the repository will
-        * keep file handles on pack files unless you call this method. Such open
-        * file handles may for example prevent that the repository folder in the
-        * filesystem can be deleted.
+        * Frees resources associated with this instance.
+        * <p>
+        * If the repository was opened by a static factory method in this class, then
+        * this method calls {@link Repository#close()} on the underlying repository
+        * instance. (Whether this actually releases underlying resources, such as
+        * file handles, may vary; see {@link Repository} for more details.)
+        * <p>
+        * If the repository was created by a caller and passed into {@link
+        * #Git(Repository)} or a static factory method in this class, then this
+        * method does not call close on the underlying repository.
         * <p>
-        * After calling close() you should not use this {@link Git} instance and
-        * the underlying {@link Repository} instance anymore.
+        * In all cases, after calling this method you should not use this {@link Git}
+        * instance anymore.
         *
         * @since 3.2
         */
        public void close() {
-               if (repo != null)
+               if (closeRepo)
                        repo.close();
        }
 
@@ -183,17 +191,27 @@ public class Git implements AutoCloseable {
 
        /**
         * Constructs a new {@link Git} object which can interact with the specified
-        * git repository. All command classes returned by methods of this class
-        * will always interact with this git repository.
+        * git repository.
+        * <p>
+        * All command classes returned by methods of this class will always interact
+        * with this git repository.
+        * <p>
+        * The caller is responsible for closing the repository; {@link #close()} on
+        * this instance does not close the repo.
         *
         * @param repo
-        *            the git repository this class is interacting with.
-        *            {@code null} is not allowed
+        *            the git repository this class is interacting with;
+        *            {@code null} is not allowed.
         */
        public Git(Repository repo) {
+               this(repo, false);
+       }
+
+       private Git(Repository repo, boolean closeRepo) {
                if (repo == null)
                        throw new NullPointerException();
                this.repo = repo;
+               this.closeRepo = closeRepo;
        }
 
        /**
@@ -695,7 +713,8 @@ public class Git implements AutoCloseable {
        }
 
        /**
-        * @return the git repository this class is interacting with
+        * @return the git repository this class is interacting with; see {@link
+        *         #close()} for notes on closing this repository.
         */
        public Repository getRepository() {
                return repo;