Re: [JGIT] Request for help

From: Jonas Fonseca <jonas.fonseca@gmail.com>
Date: Sep 4, 2009, 18:50 UTC
Message-ID: <2c6b72b30909041150g6374be2ci4d36bd8ab0824a8d@mail.gmail.com>
In-Reply-To: <658028.86274.qm@web27804.mail.ukl.yahoo.com>

On Fri, Sep 4, 2009 at 13:28, Mark Struberg<struberg@yahoo.de> wrote:

Show 7 quoted lines

> Hi!
>
> Work has been done at
>
> http://github.com/sonatype/JGit/tree/mavenize
>
> Please feel free to pull/fork and share your changes! I'd be happy to pull it in.

IMO, there are a lot of things that can be squashed together and cleaned up. I know that you advocated for incremental introduction, but it seems wrong to for example add a file and then completely reformat it a few commits later. The same thing with the .gitignore fixes in step 5.

Some comments ... Some of them I initially entered in github's codereview, but I ended up writing it all here.

Commit: "mavenizing step 1: moved over the initial poms from Jasons branch"

 * Please always add an empty line between the subject and the body
   of the commit message. Like this:

  mavenizing step 1: moved over the initial poms from Jasons branch

  Signed-off-by: Mark Struberg >struberg@yahoo.de>

 * The .gitignore pattern could be further limited to "target/" ...
but you seem to change this to /target later.

In org.spearce.jgit/pom.xml:

    * The use of maven-surefire-plugin should be removed. This module
does not have any tests.

    * Shouldn't we retain the original ${groupId}:${artifactId} naming
convention, being org.spearce:jgit?

In org.spearce.jgit.test/pom.xml:

    * Dependency on jsch is unecessary since it is derived from
org.spearce.jgit.

    * Maybe name as org.spearce:jgit-test?

In org.spearce.jgit.pgm/pom.xml:

    * Maybe name as org.spearce:jgit-pgm?

Commit: "mavenizing step 2: move the core libs from src to src/main/java"

 * Please also add an empty line to this commit message.

 * You might as well squash the whitespace fixes into the first commit.

Commit: "mavenizing step 3: moving all core tests into the core module"

 * The commit message wrongly states:
    org.spearce.jgit.test/tst/ -> org.spearce.jgit/src/test/java/tst/
   Should be:
    org.spearce.jgit.test/tst/ -> org.spearce.jgit/src/test/java/

Commit: "mavenizing step 4: moving some license files and META-INF"

 * Shouldn't the commit message rather say "remove JSch"?
   Then the moving of META-INF can be put in its own commit.

 * The new NOTICE file has a few typos and the info could fit into the README

Then I got a bit lost in a huge reformatting.

-- 
Jonas Fonseca