Re: problem using jgit

["Shawn O. Pearce" <spearce@spearce.org>]

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.