[
Date Prev][
Date Next][
Thread Prev][
Thread Next][
Date Index][
Thread Index]
[
List Home]
|
Re: [jgit-dev] NPE when calling RepositoryCache.FileKey.isGitRepository on a relative path
|
Whoops, JGit is actually calling Path.getParent(), so the javadoc of File and File.getParent() may not be directly applicable. Nevetheless, Path.getParent() is also documented to return null.
The Path.getParent() javadoc is not explicit about this case like File.getParent(), but it does say that Path.getParent() is equivalent to calling Path.subpath(0, getNameCount()-1), which means new File("repo.git").toPath().getParent() is equivalent to new File("repo.git").toPath().subpath(0, 1-1). Path.subpath() is documented to throw an IllegalArgumentException "If endIndex is less than or equal to beginIndex". Because Path.getParent() is not documented to throw an IAE, it must return null in this case.
You can easily confirm the behavior of Path is consistent with the behavior of File:
System.out.println("new File(\"repo.git\") .toPath().getParent() = " + new File(".") .toPath().getParent());
System.out.println("new File(\".\") .toPath().getParent() = " + new File(".") .toPath().getParent());
System.out.println("new File(\"/\") .toPath().getParent() = " + new File("/") .toPath().getParent());
System.out.println("new File(\"/usr\") .toPath().getParent() = " + new File("/usr") .toPath().getParent());
System.out.println("new File(\"./repo.git\").toPath().getParent() = " + new File("./repo.git").toPath().getParent());
System.out.println("new File(\"./repo.git\").exists() = " + new File("./repo.git").exists());
which yields:
new File("repo.git") .toPath().getParent() = null
new File(".") .toPath().getParent() = null
new File("/") .toPath().getParent() = null
new File("/usr") .toPath().getParent() = /
new File("./repo.git").toPath().getParent() = .
new File("./repo.git").exists() = false
Thus the root cause (empty path sequence is not a valid parent) and possible approaches to fix or work around the NPE with Path are the same as what I said for File.
Thanks,
Will
--
William Kilian
Computer Software Engineer
TargetStream Technologies
> On Jun 28, 2019, at 07:20:49, William Kilian <william.kilian@xxxxxxxxxxxxxxxx> wrote:
>
> Per the File class and getParent() javadoc, new File("repo.git").getParent() doesn't return null because the path is relative, but because new File("repo.git") has only one name in its sequence, which is the "empty abstract pathname". getParent() is documented to return null in this case.
>
> Running the following:
>
> System.out.println("new File(\"repo.git\") .getParent() = " + new File(".") .getParent());
> System.out.println("new File(\".\") .getParent() = " + new File(".") .getParent());
> System.out.println("new File(\"/\") .getParent() = " + new File("/") .getParent());
> System.out.println("new File(\"/usr\") .getParent() = " + new File("/usr") .getParent());
> System.out.println("new File(\"./repo.git\").getParent() = " + new File("./repo.git").getParent());
> System.out.println("new File(\"./repo.git\").exists() = " + new File("./repo.git").exists());
>
> Should yield on any *nix system where ./repo.git doesn't exist:
>
> new File("repo.git") .getParent() = null
> new File(".") .getParent() = null
> new File("/") .getParent() = null
> new File("/usr") .getParent() = /
> new File("./repo.git").getParent() = .
> new File("./repo.git").exists() = false
>
> This shows that using "./repo.git" is a workaround for the NPE using "repo.git".
>
> Making all pathnames absolute and then showing relative paths to the user has the alternate problems of either tracking whether the user passed an absolute or relative path at the beginning or outputting relative paths when the user passed an absolute path. You could ensure relative paths start with "./" to avoid this NPE, but the best approach is to simply handle the documented possible null from File.getParent().
>
> See https://stackoverflow.com/questions/5883808/new-file-vs-new-file-feature-or-bug for related discussion of new File("") vs new File(".").
>
> Thanks,
>
> Will
>
> --
> William Kilian
> Computer Software Engineer
> TargetStream Technologies
>
>> On Jun 28, 2019, at 05:42:40, Dmitry Pavlenko <pavlenko@xxxxxxxxxxxxx> wrote:
>>
>> 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
>>
>>
>>
>>
>> _______________________________________________
>> jgit-dev mailing list
>> jgit-dev@xxxxxxxxxxx
>> To change your delivery options, retrieve your password, or unsubscribe from this list, visit
>> https://www.eclipse.org/mailman/listinfo/jgit-dev
>
> _______________________________________________
> jgit-dev mailing list
> jgit-dev@xxxxxxxxxxx
> To change your delivery options, retrieve your password, or unsubscribe from this list, visit
> https://www.eclipse.org/mailman/listinfo/jgit-dev
