Re: [JGIT] Request for help

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Sep 4, 2009 at 13:28, Mark Struberg<struberg@xxxxxxxx> wrote:
> 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@xxxxxxxx>

 * 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]