Re: [JGIT] Blame functionality for jgit

Manuel Woelker <manuel.woelker@gmail.com> wrote:

Interesting.
 

OK. Fortunately Junio has largely given his blessing to this code being converted to Java under the BSD license.

Unfortunately I can't reach incava.org to download java-diff, but its entry on sourceforge says it uses an LGPL license. Within JGit we do want to stick to BSD and BSD dependencies, so bringing in java-diff as a new dependency isn't something we really want to do.

Fortunately Dscho has been working on a Java diff implementation for JGit, and is considering releasing it under a BSD license so we can include it.

The interface is still to be decided, but in general we have
found that running against a byte[] is much faster than running
against String.  We're probably going to want the diff library to
take a byte[] and an IntList (as created by RawParseUtils.lineMap)
and let it work against the byte array ranges directly, without any
additional allocations, except where it needs to build up its hash
of records.
 

I'm open to a plugin interface like you have done with the IDiff API to hide java-diff, but for that to work we cannot have the java-diff code as part of the JGit classpath. It needs to be something added externally by the user, in a different context, so the user can easily load their preferred diff implementation.

If that happents to be the LGPL java-diff, that's the user's choice. We can provide the shim needed to get it to load, and the adapter, but IMHO we shouldn't distribute the LGPL code, and we shouldn't make it required in order to make the library compile or work.

Eclipes has its own LCS implementation, we should also be able to have the Eclipse plugin provide its own shim to link to the Eclipse EPL licensed LCS, so Eclipse can avoid java-diff entirely. I'm not sure if NetBeans has an LCS available that the NetBeans plugin could easily link to. It probably does.

I think eventually we'll have a BSD licensed LCS that will come with JGit, which would eliminate the need for such a shim. I'd like to see that happen before blame gets added, but if blame is ready and the shim is reasonably well done, I'm inclined to bring blame in early.

There isn't one. Its whatever gets contributed in a state that is ready for inclusion. :-)

Since you are submitting this work, I'd say its on you to get blame added.

Yes.
 

I agree, I use git blame fairly often myself...

Thanks.

Now for some comments about your blame branch.

- Don't use @author tags, we don't use them anywhere else.

- Please include copyright headers on all new files.

- I think the IDiff, IDifference should be in a ".diff" package so
we can reuse them for other diff applications.  Especially if we
wind up using a shim to link to different LCS implementations.

- I think the API for BlameEngine should be more like:

  public class BlameEngine {
    private final RevWalk rw;

    public BlameEngine(final Repository repo) {
      this(new RevWalk(repo));
    }

    public BlameEngine(final RevWalk walk) {
      rw = walk;
    }

    public RevWalk getRevWalk() {
      return rw;
    }

    public List<BlameEntry> blame(RevCommit c, String path);
  }

  One reason for this style of API is we can have the engine cache
  blame records.  This can make a GUI navigator much more efficient
  as it jumps around between commits and asks for blames over the
  same data.

- Internally you should *always* use RevCommit, not Commit.
The RevCommit class can be orders of magnitude faster than Commit.

- Internally you should always use RevTree and TreeWalk to locate
blob IDs.  Its orders of magnitude faster than Tree and TreeEntry.

- Don't use new String(loader.getBytes()) (e.g. in Origin.getData)
to get the data for a file.  We want to do raw diffs, so that the
character encoding is considered as part of the blame.  E.g. if
a file switches character encodings, the blame has to go to the
commit that introduced the new encoding.  This is a huge reason
to use byte[] and IntList over an array of String.

- RawParseUtils.lineMap will be faster than a Pattern of "^(.*)$".

Otherwise, I'm finding your code to be quite clear, and converted rather nicely from C. I'd love to see this integrated into the JGit project.