From 6d24aaf497edd2a54c2d9d50499fd586fea46b65 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 20 Feb 2015 17:53:14 -0800 Subject: [PATCH 1/7] Make sure that GitRepositoryProvider.repositoryForDirectorySync() returns null rather than throws. The UI locks up if this method does not return. --- spec/git-repository-provider-spec.coffee | 30 ++++++++++++++++++++++++ src/git-repository-provider.coffee | 8 +++++++ 2 files changed, 38 insertions(+) diff --git a/spec/git-repository-provider-spec.coffee b/spec/git-repository-provider-spec.coffee index 7d77adc36..c61327321 100644 --- a/spec/git-repository-provider-spec.coffee +++ b/spec/git-repository-provider-spec.coffee @@ -54,3 +54,33 @@ describe "GitRepositoryProvider", -> directory = new Directory dirPath provider.repositoryForDirectory(directory).then (result) -> expect(result).toBe null + + describe "when specified a Directory that throws an exception when explored", -> + it "catches the exception and returns null for the sync implementation", -> + provider = new GitRepositoryProvider atom.project + + # Tolerate an implementation of Directory whose sync methods are unsupported. + subdirectory = existsSync: -> + spyOn(subdirectory, "existsSync").andThrow("sync method not supported") + directory = getSubdirectory: -> + spyOn(directory, "getSubdirectory").andReturn(subdirectory) + + provider = new GitRepositoryProvider atom.project + repo = provider.repositoryForDirectorySync(directory) + expect(repo).toBe null + expect(directory.getSubdirectory).toHaveBeenCalledWith(".git") + + it "catches the exception and returns a Promise that resolves to null for the async implementation", -> + provider = new GitRepositoryProvider atom.project + + # Tolerate an implementation of Directory whose sync methods are unsupported. + subdirectory = existsSync: -> + spyOn(subdirectory, "existsSync").andThrow("sync method not supported") + directory = getSubdirectory: -> + spyOn(directory, "getSubdirectory").andReturn(subdirectory) + + waitsForPromise -> + provider = new GitRepositoryProvider atom.project + provider.repositoryForDirectory(directory).then (repo) -> + expect(repo).toBe null + expect(directory.getSubdirectory).toHaveBeenCalledWith(".git") diff --git a/src/git-repository-provider.coffee b/src/git-repository-provider.coffee index 210197599..d26b9301a 100644 --- a/src/git-repository-provider.coffee +++ b/src/git-repository-provider.coffee @@ -51,6 +51,14 @@ class GitRepositoryProvider # * {GitRepository} if the given directory has a Git repository. # * `null` if the given directory does not have a Git repository. repositoryForDirectorySync: (directory) -> + # Ensure that this method does not throw. + try + @repositoryForDirectorySyncInternal(directory) + catch e + # TODO: Log error. + null + + repositoryForDirectorySyncInternal: (directory) -> # Only one GitRepository should be created for each .git folder. Therefore, # we must check directory and its parent directories to find the nearest # .git folder. From 947df9a6cca39d64116b896eaa5c14e1e64e9321 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Sun, 22 Feb 2015 20:35:51 -0800 Subject: [PATCH 2/7] Print exception via console.warn(). --- src/git-repository-provider.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/git-repository-provider.coffee b/src/git-repository-provider.coffee index d26b9301a..66f53b5f5 100644 --- a/src/git-repository-provider.coffee +++ b/src/git-repository-provider.coffee @@ -55,7 +55,7 @@ class GitRepositoryProvider try @repositoryForDirectorySyncInternal(directory) catch e - # TODO: Log error. + console.warn "Error checking for Git repository for '#{directory.getPath()}'", e.stack null repositoryForDirectorySyncInternal: (directory) -> From eb06cb7f97157ba1d08a4f88410b81d2a0fc99c9 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Sun, 22 Feb 2015 20:36:14 -0800 Subject: [PATCH 3/7] On second thought, don't print anything. This can be a common, expected occurrence when using special implementations of Directory, so it creates a lot of distracting noise for the user. --- src/git-repository-provider.coffee | 1 - 1 file changed, 1 deletion(-) diff --git a/src/git-repository-provider.coffee b/src/git-repository-provider.coffee index 66f53b5f5..c9e96f908 100644 --- a/src/git-repository-provider.coffee +++ b/src/git-repository-provider.coffee @@ -55,7 +55,6 @@ class GitRepositoryProvider try @repositoryForDirectorySyncInternal(directory) catch e - console.warn "Error checking for Git repository for '#{directory.getPath()}'", e.stack null repositoryForDirectorySyncInternal: (directory) -> From 07039ba47a9834f2cdd6083fee9ab3f713505f9b Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 23 Feb 2015 09:30:43 -0800 Subject: [PATCH 4/7] Check whether existsSync() is available in GitRepositoryProvider before trying to call it. --- spec/git-repository-provider-spec.coffee | 22 ++++++++++++---------- src/git-repository-provider.coffee | 15 ++++----------- 2 files changed, 16 insertions(+), 21 deletions(-) diff --git a/spec/git-repository-provider-spec.coffee b/spec/git-repository-provider-spec.coffee index c61327321..5294678ef 100644 --- a/spec/git-repository-provider-spec.coffee +++ b/spec/git-repository-provider-spec.coffee @@ -55,14 +55,15 @@ describe "GitRepositoryProvider", -> provider.repositoryForDirectory(directory).then (result) -> expect(result).toBe null - describe "when specified a Directory that throws an exception when explored", -> - it "catches the exception and returns null for the sync implementation", -> + describe "when specified a Directory without existsSync()", -> + it "returns null", -> provider = new GitRepositoryProvider atom.project - # Tolerate an implementation of Directory whose sync methods are unsupported. - subdirectory = existsSync: -> - spyOn(subdirectory, "existsSync").andThrow("sync method not supported") - directory = getSubdirectory: -> + # Tolerate an implementation of Directory that does not implement existsSync(). + subdirectory = {} + directory = + getSubdirectory: -> + isRoot: -> true spyOn(directory, "getSubdirectory").andReturn(subdirectory) provider = new GitRepositoryProvider atom.project @@ -70,13 +71,14 @@ describe "GitRepositoryProvider", -> expect(repo).toBe null expect(directory.getSubdirectory).toHaveBeenCalledWith(".git") - it "catches the exception and returns a Promise that resolves to null for the async implementation", -> + it "returns a Promise that resolves to null for the async implementation", -> provider = new GitRepositoryProvider atom.project # Tolerate an implementation of Directory whose sync methods are unsupported. - subdirectory = existsSync: -> - spyOn(subdirectory, "existsSync").andThrow("sync method not supported") - directory = getSubdirectory: -> + subdirectory = {} + directory = + getSubdirectory: -> + isRoot: -> true spyOn(directory, "getSubdirectory").andReturn(subdirectory) waitsForPromise -> diff --git a/src/git-repository-provider.coffee b/src/git-repository-provider.coffee index c9e96f908..fcf4a8df5 100644 --- a/src/git-repository-provider.coffee +++ b/src/git-repository-provider.coffee @@ -11,7 +11,7 @@ findGitDirectorySync = (directory) -> # can return cached values rather than always returning new objects: # getParent(), getFile(), getSubdirectory(). gitDir = directory.getSubdirectory('.git') - if gitDir.existsSync() and isValidGitDirectorySync gitDir + if gitDir.existsSync?() and isValidGitDirectorySync gitDir gitDir else if directory.isRoot() return null @@ -26,9 +26,9 @@ isValidGitDirectorySync = (directory) -> # To decide whether a directory has a valid .git folder, we use # the heuristic adopted by the valid_repository_path() function defined in # node_modules/git-utils/deps/libgit2/src/repository.c. - return directory.getSubdirectory('objects').existsSync() and - directory.getFile('HEAD').existsSync() and - directory.getSubdirectory('refs').existsSync() + return directory.getSubdirectory('objects').existsSync?() and + directory.getFile('HEAD').existsSync?() and + directory.getSubdirectory('refs').existsSync?() # Provider that conforms to the atom.repository-provider@0.1.0 service. module.exports = @@ -51,13 +51,6 @@ class GitRepositoryProvider # * {GitRepository} if the given directory has a Git repository. # * `null` if the given directory does not have a Git repository. repositoryForDirectorySync: (directory) -> - # Ensure that this method does not throw. - try - @repositoryForDirectorySyncInternal(directory) - catch e - null - - repositoryForDirectorySyncInternal: (directory) -> # Only one GitRepository should be created for each .git folder. Therefore, # we must check directory and its parent directories to find the nearest # .git folder. From b015e1bfa7f20405d85e3f4afb359cc949b7d84e Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 23 Feb 2015 09:56:57 -0800 Subject: [PATCH 5/7] revert changes to isValidGitDirectorySync() --- src/git-repository-provider.coffee | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/git-repository-provider.coffee b/src/git-repository-provider.coffee index fcf4a8df5..850d30f22 100644 --- a/src/git-repository-provider.coffee +++ b/src/git-repository-provider.coffee @@ -26,9 +26,9 @@ isValidGitDirectorySync = (directory) -> # To decide whether a directory has a valid .git folder, we use # the heuristic adopted by the valid_repository_path() function defined in # node_modules/git-utils/deps/libgit2/src/repository.c. - return directory.getSubdirectory('objects').existsSync?() and - directory.getFile('HEAD').existsSync?() and - directory.getSubdirectory('refs').existsSync?() + return directory.getSubdirectory('objects').existsSync() and + directory.getFile('HEAD').existsSync() and + directory.getSubdirectory('refs').existsSync() # Provider that conforms to the atom.repository-provider@0.1.0 service. module.exports = From 6bd6a8ccddde8fbe000edb0a2db36ce3d4bec9e6 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 23 Feb 2015 09:59:16 -0800 Subject: [PATCH 6/7] Introduce use of beforeEach() in unit test. --- spec/git-repository-provider-spec.coffee | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/spec/git-repository-provider-spec.coffee b/spec/git-repository-provider-spec.coffee index 5294678ef..817e6b887 100644 --- a/spec/git-repository-provider-spec.coffee +++ b/spec/git-repository-provider-spec.coffee @@ -56,7 +56,9 @@ describe "GitRepositoryProvider", -> expect(result).toBe null describe "when specified a Directory without existsSync()", -> - it "returns null", -> + directory = null + provider = null + beforeEach -> provider = new GitRepositoryProvider atom.project # Tolerate an implementation of Directory that does not implement existsSync(). @@ -66,23 +68,13 @@ describe "GitRepositoryProvider", -> isRoot: -> true spyOn(directory, "getSubdirectory").andReturn(subdirectory) - provider = new GitRepositoryProvider atom.project + it "returns null", -> repo = provider.repositoryForDirectorySync(directory) expect(repo).toBe null expect(directory.getSubdirectory).toHaveBeenCalledWith(".git") it "returns a Promise that resolves to null for the async implementation", -> - provider = new GitRepositoryProvider atom.project - - # Tolerate an implementation of Directory whose sync methods are unsupported. - subdirectory = {} - directory = - getSubdirectory: -> - isRoot: -> true - spyOn(directory, "getSubdirectory").andReturn(subdirectory) - waitsForPromise -> - provider = new GitRepositoryProvider atom.project provider.repositoryForDirectory(directory).then (repo) -> expect(repo).toBe null expect(directory.getSubdirectory).toHaveBeenCalledWith(".git") From 61fb408777fc260fdb2311770ca424426bfe1256 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 23 Feb 2015 10:10:37 -0800 Subject: [PATCH 7/7] Tighten up comment. --- spec/git-repository-provider-spec.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/git-repository-provider-spec.coffee b/spec/git-repository-provider-spec.coffee index 817e6b887..59e3f55af 100644 --- a/spec/git-repository-provider-spec.coffee +++ b/spec/git-repository-provider-spec.coffee @@ -61,7 +61,7 @@ describe "GitRepositoryProvider", -> beforeEach -> provider = new GitRepositoryProvider atom.project - # Tolerate an implementation of Directory that does not implement existsSync(). + # An implementation of Directory that does not implement existsSync(). subdirectory = {} directory = getSubdirectory: ->