Re: [PATCH JGIT] Compute the author/commiter name and email from the git configuration
- From
Shawn O. Pearce <spearce@spearce.org>
- Date
- Feb 4, 2009, 17:36 UTC
- Message-ID
- <20090204173603.GC26880@spearce.org>
- In-Reply-To
- <49898A06.5040603@gmail.com>
Yann Simon <yann.simon.fr@gmail.com> wrote:
Show 18 quoted lines
> diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryConfigTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryConfigTest.java
> index 34ce04a..113eb1c 100644
> --- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryConfigTest.java
> +++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryConfigTest.java
> @@ -116,4 +120,70 @@ public void test006_readCaseInsensitive() throws IOException {
> assertEquals(true, repositoryConfig.getBoolean("foo", null, "bar", false));
> assertEquals("", repositoryConfig.getString("foo", null, "bar"));
> }
> +
> + private class FakeSystemReader implements SystemReader {
> + Map<String, String> values = new HashMap<String, String>();
> + public String getEnvironmentVariable(String variable) {
> + return values.get(variable);
> + }
> + public String getProperty(String key) {
> + return values.get(key);
> + }
> + }I like this approach. Perhaps this should simply be done in RepositoryTestCase so all of our unit tests have a stable default identity, no matter what. They could change it on a test-by-test basis if needed, especially if the reader is created and populated with default values during the setUp() method.
Show 7 quoted lines
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/Constants.java b/org.spearce.jgit/src/org/spearce/jgit/lib/Constants.java > index 8f093d6..372fba5 100644 > --- a/org.spearce.jgit/src/org/spearce/jgit/lib/Constants.java > +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/Constants.java > + > + /** The environment variable that contains the commiter's name */ > + public static final String GIT_COMMITER_NAME_KEY = "GIT_COMMITER_NAME";
There are 2 't's in GIT_COMMITTER_NAME. Both the variable name and the value are wrong.
> + /** The environment variable that contains the commiter's email */ > + public static final String GIT_COMMITER_EMAIL_KEY = "GIT_COMMITER_EMAIL";
Again, 2 't's.
Show 5 quoted lines
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RepositoryConfig.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RepositoryConfig.java > index 7df90cd..0fa4b1f 100644 > --- a/org.spearce.jgit/src/org/spearce/jgit/lib/RepositoryConfig.java > +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RepositoryConfig.java > @@ -308,6 +323,83 @@ public String getString(final String section, String subsection, final String na
...
> + public String getCommiterName() {2 't's in comitter.
> + public String getCommiterEmail() {There are 2 't's in Committer.
> + private String getUserEmailInternal(String gitVariableKey, boolean author) {...
Show 7 quoted lines
> + // try to construct an email
> + String userName = author ? getAuthorName() : getCommiterName();
> + if (userName != null && userName.length() != 0) {
> + String hostnameTmp = getHostname();
> + if (hostnameTmp != null && hostnameTmp.length() != 0) {
> + email = userName + "@" + hostnameTmp;
> + }If you do what I suggest below, getHostname() will never return null, so this logic will simplify out and we'll always be able to set email if we have a userName.
Also, I think this should be defaulting to the Java "user.name" property and not to the GIT_AUTHOR_NAME environment variable. So if the user.email isn't set, and the *_EMAIL env var isn't set, use the "user.name" property and the hostname to form the email address.
I don't think this should ever produce null. If we don't have a username from user.name, use some generic string like "unknown-user". Then we at least still have some identity data. Its very unlikely the JVM won't have a "user.name" property for us, so its very likely we won't get good tests in application level code for null return values.
Show 6 quoted lines
> @@ -957,4 +1049,28 @@ private static boolean eq(final String a, final String b) {
> +
> + /**
> + * @return the canonical hostname
> + */
> + public static String getHostname() {I don't see a reason for this to be public. Please make it private.
Show 6 quoted lines
> + if (hostname == null) {
> + try {
> + InetAddress localMachine = InetAddress.getLocalHost();
> + hostname = localMachine.getCanonicalHostName();
> + } catch (UnknownHostException e) {
> + // we do nothingSet hostname = "localhost" so we don't incur an UHE on every attempt to get the hostname. If it failed once, its very likely to fail again, and again, and again.
Show 5 quoted lines
> + /**
> + * Overrides the default system reader by a custom one.
> + * @param systemReader new system reader
> + */
> + public void setSystemReader(SystemReader systemReader) {Method should be static.
> + RepositoryConfig.systemReader = systemReader;
There is no need for RepositoryConfig here. Rename the parameter so it doesn't hide the field.
Show 7 quoted lines
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/util/SystemReader.java b/org.spearce.jgit/src/org/spearce/jgit/util/SystemReader.java > new file mode 100644 > index 0000000..89b4021 > --- /dev/null > +++ b/org.spearce.jgit/src/org/spearce/jgit/util/SystemReader.java > @@ -0,0 +1,24 @@ > +package org.spearce.jgit.util;
Missing copyright header.
Show 5 quoted lines
> + /** > + * @param variable system variable to read > + * @return value of the system variable > + */ > + String getEnvironmentVariable(String variable);
Maybe just emulate the System class and call this getenv() ?
-- Shawn.