problem using jgit

problem using jgit

[Stephen Bannasch]

I've setup a simple test class that integrates jgit to clone a git 
repository. However I'm getting a NullPointerError when 
RevWalk.parseAny ends up producing a null object id.

The code and the stack trace for the error are here:

   http://pastie.org/237711

This problem occurs using the jgit from the master branch from this repo:

   git://repo.or.cz/egit.git

Re: problem using jgit

[Marek Zawirski]

Stephen Bannasch wrote:
> I've setup a simple test class that integrates jgit to clone a git 
> repository. However I'm getting a NullPointerError when 
> RevWalk.parseAny ends up producing a null object id.
>
> The code and the stack trace for the error are here:
>
>   http://pastie.org/237711
>
> This problem occurs using the jgit from the master branch from this repo:
>
>   git://repo.or.cz/egit.git
Hello Stephen,

I think you've experienced error caused by the same bug as me, during my 
latest fetch/push GUI works few days ago.
Your code looks fine, probably  it's actually bug in jgit. I think it's 
some regression. Thanks for reporting.
I'll try to look at this problem or jgit fetch experts (Robin and Shawn) 
will have a look when they come back. 

Marek

Re: problem using jgit

[Marek Zawirski]

Marek Zawirski wrote:
> Stephen Bannasch wrote:
>> I've setup a simple test class that integrates jgit to clone a git 
>> repository. However I'm getting a NullPointerError when 
>> RevWalk.parseAny ends up producing a null object id.
>>
>> The code and the stack trace for the error are here:
>>
>>   http://pastie.org/237711
>>
>> This problem occurs using the jgit from the master branch from this 
>> repo:
>>
>>   git://repo.or.cz/egit.git
> Hello Stephen,
>
> I think you've experienced error caused by the same bug as me, during 
> my latest fetch/push GUI works few days ago.
> Your code looks fine, probably  it's actually bug in jgit. I think 
> it's some regression. Thanks for reporting.
It's caused by 14a630c3: Cached modification times for symbolic refs too
Changes introduced by this patch made Repository#getAllRefs() including 
Ref objects with null ObjectId in case of unresolvable (invalid?) HEAD 
symbolic ref, and null Ref for HEAD  when it doesn't exist. Previous 
behavior was just not including such refs in result.

Fix for null Ref is just a matter of simple filtering out null Ref 
object for HEAD, if it doesn't exist (just is it considered to be legal 
state of repository when HEAD doesn't exist?).

To fix null ObjectId issue, we have to either change all clients of this 
method or revert method to previous behavior. Now it's just unspecified 
in javadoc.
Robin, Shawn, what do you think? If we want to have unresolvable refs 
included, IMO it may be sensible to provide argument includeUnresolbable 
for Repository#getAllRefs() to let clients avoid burden of filtering 
them out when they don't need them (most cases, perhaps).
I can prepare fix for it (rather easy one) as you are unavailable now, 
let me now what's your opinion.

Re: problem using jgit

[Stephen Bannasch]

At 2:35 PM +0200 7/21/08, Marek Zawirski wrote:
>Marek Zawirski wrote:
>>Stephen Bannasch wrote:
>>>I've setup a simple test class that integrates jgit to clone a git repository. However I'm getting a NullPointerError when RevWalk.parseAny ends up producing a null object id.
>>>
>>>The code and the stack trace for the error are here:
>>>
>>>  http://pastie.org/237711
>>>
>>>This problem occurs using the jgit from the master branch from this repo:
>>>
>>>  git://repo.or.cz/egit.git
>>Hello Stephen,
>>
>>I think you've experienced error caused by the same bug as me, during my latest fetch/push GUI works few days ago.
>>Your code looks fine, probably  it's actually bug in jgit. I think it's some regression. Thanks for reporting.
>It's caused by 14a630c3: Cached modification times for symbolic refs too
>Changes introduced by this patch made Repository#getAllRefs() including Ref objects with null ObjectId in case of unresolvable (invalid?) HEAD symbolic ref, and null Ref for HEAD  when it doesn't exist. Previous behavior was just not including such refs in result.
>
>Fix for null Ref is just a matter of simple filtering out null Ref object for HEAD, if it doesn't exist (just is it considered to be legal state of repository when HEAD doesn't exist?).
>
>To fix null ObjectId issue, we have to either change all clients of this method or revert method to previous behavior. Now it's just unspecified in javadoc.
>Robin, Shawn, what do you think? If we want to have unresolvable refs included, IMO it may be sensible to provide argument includeUnresolbable for Repository#getAllRefs() to let clients avoid burden of filtering them out when they don't need them (most cases, perhaps).
>I can prepare fix for it (rather easy one) as you are unavailable now, let me now what's your opinion.

Thanks for looking at this problem Marek. 

If you get a change working for jgit that might not be available in a branch on git://repo.or.cz/egit.gi will you send a patch.

I am looking forward to continuing my tests using jgit from Java and JRuby.

Re: problem using jgit

[Marek Zawirski]

Stephen Bannasch wrote:
> At 2:35 PM +0200 7/21/08, Marek Zawirski wrote:
>   
>> Marek Zawirski wrote:
>>     
>>> Stephen Bannasch wrote:
>>>       
>>>> I've setup a simple test class that integrates jgit to clone a git repository. However I'm getting a NullPointerError when RevWalk.parseAny ends up producing a null object id.
>>>>
>>>> The code and the stack trace for the error are here:
>>>>
>>>>  http://pastie.org/237711
>>>>
>>>> This problem occurs using the jgit from the master branch from this repo:
>>>>
>>>>  git://repo.or.cz/egit.git
>>>>         
>>> Hello Stephen,
>>>
>>> I think you've experienced error caused by the same bug as me, during my latest fetch/push GUI works few days ago.
>>> Your code looks fine, probably  it's actually bug in jgit. I think it's some regression. Thanks for reporting.
>>>       
>> It's caused by 14a630c3: Cached modification times for symbolic refs too
>> Changes introduced by this patch made Repository#getAllRefs() including Ref objects with null ObjectId in case of unresolvable (invalid?) HEAD symbolic ref, and null Ref for HEAD  when it doesn't exist. Previous behavior was just not including such refs in result.
>>
>> Fix for null Ref is just a matter of simple filtering out null Ref object for HEAD, if it doesn't exist (just is it considered to be legal state of repository when HEAD doesn't exist?).
>>
>> To fix null ObjectId issue, we have to either change all clients of this method or revert method to previous behavior. Now it's just unspecified in javadoc.
>> Robin, Shawn, what do you think? If we want to have unresolvable refs included, IMO it may be sensible to provide argument includeUnresolbable for Repository#getAllRefs() to let clients avoid burden of filtering them out when they don't need them (most cases, perhaps).
>> I can prepare fix for it (rather easy one) as you are unavailable now, let me now what's your opinion.
>>     
>
> Thanks for looking at this problem Marek. 
>
> If you get a change working for jgit that might not be available in a branch on git://repo.or.cz/egit.gi will you send a patch.
>
> I am looking forward to continuing my tests using jgit from Java and JRuby.
>   

You can find temporary workaround for that in "workaround" branch at 
git://repo.or.cz/egit/zawir.git
It's rather not a final solution (disables unresolvable HEADs), but I 
hope it let you continue using jgit for a while, as it does for me ;)

Re: problem using jgit

[Shawn O. Pearce]

Marek Zawirski <marek.zawirski@gmail.com> wrote:
> Marek Zawirski wrote:
>> Stephen Bannasch wrote:
>>> I've setup a simple test class that integrates jgit to clone a git  
>>> repository. However I'm getting a NullPointerError when  
>>> RevWalk.parseAny ends up producing a null object id.
...
> It's caused by 14a630c3: Cached modification times for symbolic refs too
> Changes introduced by this patch made Repository#getAllRefs() including  
> Ref objects with null ObjectId in case of unresolvable (invalid?) HEAD  
> symbolic ref, and null Ref for HEAD  when it doesn't exist. Previous  
> behavior was just not including such refs in result.

My intention here was that if a ref cannot be resolved, it should
not be reported.  So Ref.getObjectId should never return null, and
it should also never return an ObjectId for which the object does
not exist in the Repository's object database(s).  (Though that can
happen in the face of repository corruption, but lets not go there
just yet).

So IMHO the RefDatabase code is _wrong_ for returning HEAD with a
null objectId.

Now this case can happen if HEAD points at a stillborn branch.  This
is easily reproduced in any repository, e.g. just do:

	git symbolic-ref HEAD refs/heads/`date`

You'll wind up on a branch which doesn't exist.  In this case HEAD
shouldn't be reported back from RefDatabase, it doesn't exist, as
branch `date` does not exist either.

-- 
Shawn.

Re: problem using jgit

[Marek Zawirski]

Shawn O. Pearce wrote:
> Marek Zawirski <marek.zawirski@gmail.com> wrote:
>   
>> Marek Zawirski wrote:
>>     
>>> Stephen Bannasch wrote:
>>>       
>>>> I've setup a simple test class that integrates jgit to clone a git  
>>>> repository. However I'm getting a NullPointerError when  
>>>> RevWalk.parseAny ends up producing a null object id.
>>>>         
> ...
>   
>> It's caused by 14a630c3: Cached modification times for symbolic refs too
>> Changes introduced by this patch made Repository#getAllRefs() including  
>> Ref objects with null ObjectId in case of unresolvable (invalid?) HEAD  
>> symbolic ref, and null Ref for HEAD  when it doesn't exist. Previous  
>> behavior was just not including such refs in result.
>>     
>
> My intention here was that if a ref cannot be resolved, it should
> not be reported.  So Ref.getObjectId should never return null, and
> it should also never return an ObjectId for which the object does
> not exist in the Repository's object database(s).  (Though that can
> happen in the face of repository corruption, but lets not go there
> just yet).
>
> So IMHO the RefDatabase code is _wrong_ for returning HEAD with a
> null objectId.
>
> Now this case can happen if HEAD points at a stillborn branch.  This
> is easily reproduced in any repository, e.g. just do:
>
> 	git symbolic-ref HEAD refs/heads/`date`
>
> You'll wind up on a branch which doesn't exist.  In this case HEAD
> shouldn't be reported back from RefDatabase, it doesn't exist, as
> branch `date` does not exist either.
>
>   
Beside of my temporary fix for that that filters null Ref and Ref with 
null objectId, I think that 2 more issues may need to be resolved:

1) readRefBasic() method is used for reading arbitrary refs, potentially 
not only those from well-known prefixes as readRefs() does. Is calling 
setModified()  appropriate for those other refs?

2) Am I wrong that setModified() is not called in all cases? Consider 
empty ref file and just...
if (line == null || line.length() == 0)
            return new Ref(Ref.Storage.LOOSE, name, null);

Re: problem using jgit

[Shawn O. Pearce]

Marek Zawirski <marek.zawirski@gmail.com> wrote:
> Shawn O. Pearce wrote:
>> Marek Zawirski <marek.zawirski@gmail.com> wrote:
>>> 
>>> It's caused by 14a630c3: Cached modification times for symbolic refs too
>>> Changes introduced by this patch made Repository#getAllRefs() 
>>> including  Ref objects with null ObjectId in case of unresolvable 
>>> (invalid?) HEAD  symbolic ref, and null Ref for HEAD  when it doesn't 
>>> exist. Previous  behavior was just not including such refs in result.
>>
>> My intention here was that if a ref cannot be resolved, it should
>> not be reported. [...]
>  
> Beside of my temporary fix for that that filters null Ref and Ref with  
> null objectId, I think that 2 more issues may need to be resolved:

Well, I think your temporary fix is correct.  I don't see another way
to implement around it.

> 1) readRefBasic() method is used for reading arbitrary refs, potentially  
> not only those from well-known prefixes as readRefs() does. Is calling  
> setModified()  appropriate for those other refs?

Yes.  If we read something and it is different from what we have cached
we need to signal that the cached data is invalid (which is setModified).
Doing so allows listeners to (eventually) find out that there are changes
made on disk which their subscribers don't know about, but may need to be
informed of.  This way we can identify changes made by command line tools
to a repository that egit has open in the workbench.

> 2) Am I wrong that setModified() is not called in all cases? Consider  
> empty ref file and just...
> if (line == null || line.length() == 0)
>            return new Ref(Ref.Storage.LOOSE, name, null);
>

In this case (and the other like it) we don't call setModified
because there hasn't been a change on disk.  The file wasn't
previously in our cache and the file is empty now.  Either way
the ref has no data so there is no need to notify listeners.

Now there may be other cases that are missing, but this one
is fine as is.

So I'm thinking of applying this:

--8<--
From: Marek Zawirski <marek.zawirski@gmail.com>
Subject: [PATCH] Fix Repository.getAllRefs to skip HEAD if it points to an unborn branch

If HEAD is a symbolic reference pointing to an unborn branch (branch
not yet created) we get a Ref object back for it supplying the name
of the branch, but its ObjectId is null as the branch file itself is
not present in the repository.  In such cases we should not include
the HEAD reference in the returned output.

Signed-off-by: Marek Zawirski <marek.zawirski@gmail.com>
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../src/org/spearce/jgit/lib/RefDatabase.java      |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
index 82b995e..b9c8c8c 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
@@ -209,7 +209,9 @@ class RefDatabase {
 		readPackedRefs(avail);
 		readLooseRefs(avail, REFS_SLASH, refsDir);
 		try {
-			avail.put(Constants.HEAD, readRefBasic(Constants.HEAD, 0));
+			final Ref r = readRefBasic(Constants.HEAD, 0);
+			if (r != null && r.getObjectId() != null)
+				avail.put(Constants.HEAD, r);
 		} catch (IOException e) {
 			// ignore here
 		}
-- 
1.6.0.rc0.182.gb96c7


-- 
Shawn.