Re: [jgit-dev] NPE when calling RepositoryCache.FileKey.isGitRepository

[Dmitry Pavlenko <pavlenko@xxxxxxxxxxxxx>]

My environment is
  JDK 9.0.1 by Oracle
  Debian Linux 10

When I run the following code:
        System.out.println("new File(\"repo.git\").exists() = " + new File("repo.git").exists());
        System.out.println("new File(\"repo.git\").getParent() = " + new File("repo.git").getParent());
        System.out.println("new File(\"repo.git\").getAbsolutePath() = " + new File("repo.git").getAbsolutePath());
        System.out.println("new File(\"repo.git\").getAbsoluteFile().getParent() = " + new File("repo.git").getAbsoluteFile().getParent());
        System.out.println();
        System.out.println("new File(\"repo.git\").toPath() = " + new File("repo.git").toPath());
        System.out.println("new File(\"repo.git\").toPath().getParent() = " + new File("repo.git").toPath().getParent());
        System.out.println("new File(\"repo.git\").toPath().toAbsolutePath() = " + new File("repo.git").toPath().toAbsolutePath());
        System.out.println("new File(\"repo.git\").toPath().toAbsolutePath().getParent() = " + new File("repo.git").toPath().toAbsolutePath().getParent());

It prints

new File("repo.git").exists() = false
new File("repo.git").getParent() = null
new File("repo.git").getAbsolutePath() = /home/dmit10/work/translator/repo.git
new File("repo.git").getAbsoluteFile().getParent() = /home/dmit10/work/translator

new File("repo.git").toPath() = repo.git
new File("repo.git").toPath().getParent() = null
new File("repo.git").toPath().toAbsolutePath() = /home/dmit10/work/translator/repo.git
new File("repo.git").toPath().toAbsolutePath().getParent() = /home/dmit10/work/translator

FS.FileStoreAttributeCache.getFsTimestampResolution() relies on the fact that

  Path dir = Files.isDirectory(file) ? file : file.getParent();

is never null, which appear to be wrong in my environment if the path is relative and doesn't exist.

My suggestion would be to convert the paths (and maybe all the paths in JGit) to absolute as soon as possible OR
add to JGit documentation/javadocs that only absolute paths are supported and maybe enforce that with "asserts".

In SVNKit (Java library for SVN support) we do that in the following way: convert the paths to absolute very soon,
then work with absolute paths only. Finally, at the moment when the user expects to see the relative paths (i.e. just before printing them to stdout)
we convert them back to relative paths.
-- 
Dmitry Pavlenko,
TMate Software,
http://subgit.com/ - git-svn bridge

> your line numbers in FS are off by 2 lines
> 
> in FS line 202 dir evaluates to
> /var/folders/16/nqvdxf5s45309nxkv4k4r20r0000gq/T/tmp_1112776041349876411
> when debugging this test
> 
> I set a breakpoint in line 185 of FileSnapshot which isn't reached when
> running this test
> 
> On Thu, Jun 27, 2019 at 8:51 AM Dmitry Pavlenko <pavlenko@xxxxxxxxxxxxx>
> 
> wrote:
> > Hello Matthias,
> > it does fail for me (though I tried to run it not from JGit but posted the
> > code to the dependent project).
> > 
> > The stack trace is
> > 
> > java.lang.NullPointerException
> > 
> >         at
> > 
> > org.eclipse.jgit.util.FS$FileStoreAttributeCache.getFsTimestampResolution(
> > FS.java:205)> 
> >         at org.eclipse.jgit.util.FS.getFsTimerResolution(FS.java:323)
> >         at
> > 
> > org.eclipse.jgit.internal.storage.file.FileSnapshot.<init>(FileSnapshot.ja
> > va:186)> 
> >         at
> > 
> > org.eclipse.jgit.internal.storage.file.FileSnapshot.save(FileSnapshot.java
> > :122)> 
> >         at
> > 
> > org.eclipse.jgit.storage.file.FileBasedConfig.load(FileBasedConfig.java:15
> > 9)> 
> >         at
> > 
> > org.eclipse.jgit.lib.BaseRepositoryBuilder.loadConfig(BaseRepositoryBuilde
> > r.java:771)> 
> >         at
> > 
> > org.eclipse.jgit.lib.BaseRepositoryBuilder.getConfig(BaseRepositoryBuilder
> > .java:748)> 
> >         at
> > 
> > org.eclipse.jgit.lib.BaseRepositoryBuilder.guessWorkTreeOrFail(BaseReposit
> > oryBuilder.java:784)> 
> >         at
> > 
> > org.eclipse.jgit.lib.BaseRepositoryBuilder.setupWorkTree(BaseRepositoryBui
> > lder.java:709)> 
> >         at
> > 
> > org.eclipse.jgit.lib.BaseRepositoryBuilder.setup(BaseRepositoryBuilder.jav
> > a:624)> 
> >         at
> > 
> > org.eclipse.jgit.lib.RepositoryCache$FileKey.getGitRepository(RepositoryCa
> > che.java:519)> 
> >         at
> > 
> > org.eclipse.jgit.lib.RepositoryCache$FileKey.isGitRepository(RepositoryCac
> > he.java:500)
> > 
> > I wonder how does
> > 
> >         new File("repo.git").toPath().getParent()
> > 
> > line evaluate in your environment?
> > 
> > I've attached several screenshots of my debugger, the last one shows why
> > the NPE happens.
> > --
> > Dmitry Pavlenko,
> > TMate Software,
> > http://subgit.com/ - git-svn bridge
> > 
> > > On Wed, Jun 26, 2019 at 11:21 PM Dmitry Pavlenko <pavlenko@xxxxxxxxxxxxx
> > > 
> > > wrote:
> > > > Hello,
> > > > 
> > > > I can reproduce the NPE with a single line:
> > > >   RepositoryCache.FileKey.isGitRepository(new File("repo.git"),
> > > > 
> > > > FS.DETECTED)
> > > > 
> > > > if the relative path "repo.git" doesn't exist yet.
> > > > 
> > > > It's a regression, the previous (File-based, not Path-based) JGit
> > 
> > versions
> > 
> > > > worked fine in this situation.
> > > > 
> > > > The problem happens because
> > > > 
> > > >   new File("repo.git").toPath().getParent()
> > > > 
> > > > evaluates to null in
> > > > 
> > > >   FS.FileStoreAttributeCache.getFsTimestampResolution()
> > > > 
> > > > (The corresponding line is:  Path dir = Files.isDirectory(file) ? file
> > > > 
> > > > file.getParent();  )
> > > > 
> > > > The documentation doesn't mention whether the path can or cannot be
> > > > relative.
> > > > 
> > > > So I wonder whether this is expected behaviour, and I should use the
> > > > absolute path or is it a bug in JGit and it should work with relative
> > > > paths
> > > > as well?
> > > 
> > > I added a test with your example but it doesn't fail on Mac:
> > > https://git.eclipse.org/r/#/c/144960/
> > > 
> > > Can you try that in your environment?
> > > 
> > > -Matthias