git tfs verify regex check broken #123

Closed
chrisbroome opened this Issue Dec 22, 2011 · 3 comments

Comments

Projects
None yet
2 participants

I was getting the dreaded "object reference not set to an instance of an object" error using the latest git-tfs on git version 1.7.8.msysgit.0

It turns out that the Regular expression that looks for commits wasn't allowing for extra stuff after the commit id. I set my git logs to automatically decorate commits and this was causing the Regex to not be able to parse the commit. I've posted my fix along with a new unit test below in the form of diffs:

diff --git a/GitTfs/Core/GitRepository.cs b/GitTfs/Core/GitRepository.cs
index 6d1f0c6..dda0436 100644
--- a/GitTfs/Core/GitRepository.cs
+++ b/GitTfs/Core/GitRepository.cs
@@ -260,7 +260,7 @@ namespace Sep.Git.Tfs.Core
                 var match = GitTfsConstants.CommitRegex.Match(line);
                 if (match.Success)
                 {
-                    currentCommit = match.Groups[1].Value;
+                    currentCommit = match.Groups["commit"].Value;
                     continue;
                 }
                 var changesetInfo = TryParseChangesetInfo(line, currentCommit, includeStubRemotes);
diff --git a/GitTfs/GitTfsConstants.cs b/GitTfs/GitTfsConstants.cs
index 543d3d6..6cd6c5b 100644
--- a/GitTfs/GitTfsConstants.cs
+++ b/GitTfs/GitTfsConstants.cs
@@ -6,7 +6,7 @@ namespace Sep.Git.Tfs
     {
         public static readonly Regex Sha1 = new Regex("[a-f\\d]{40}", RegexOptions.IgnoreCase);
         public static readonly Regex Sha1Short = new Regex("[a-f\\d]{4,40}", RegexOptions.IgnoreCase);
-        public static readonly Regex CommitRegex = new Regex("^commit (" + Sha1 + ")\\s*$");
+        public static readonly Regex CommitRegex = new Regex(string.Concat( @"^commit (?<commit>", Sha1, @")" ), RegexOptions.Compiled);

         public const string DefaultRepositoryId = "default";

diff --git a/GitTfsTest/GitTfsRegexTests.cs b/GitTfsTest/GitTfsRegexTests.cs
index 1711ac0..62aca19 100644
--- a/GitTfsTest/GitTfsRegexTests.cs
+++ b/GitTfsTest/GitTfsRegexTests.cs
@@ -14,6 +14,14 @@ namespace Sep.Git.Tfs.Test
         }

         [TestMethod]
+        public void CommitRegexShouldApproveGitCommitTitleWithExtraInfoAfterCommit()
+        {
+            const string line = "commit 9b655abe865ef0e4048aba904b79c7a2f10bdfce (HEAD, origin/master)";
+            var match = GitTfsConstants.CommitRegex.Match( line );
+            Assert.IsTrue( match.Success );
+        }
+
+        [TestMethod]
         public void CommitRegexShouldDeclineCommitRevertMessage()
         {
             const string line = "    This reverts commit e096daaf57d937fef3c0c639c3a59232310c6a20.";

Strictly speaking, the named capture group, "commit", is not necessary, but I find it easier to read when used vs match.Groups[1].Value

I could create a fork and submit a pull request if that's the better way to submit the fix for the issue.

Owner

spraints commented Dec 23, 2011

I like the named capture group. I agree that it's easier to read.

As for the fix, this is good, but it'd be even better to get git log to output a consistent format, despite any local customizations made by a user.

Also, a pull request is the way to go for patch submission.

Sorry it has taken me so long to reply here. Over the past few months, I've started doing things a bit differently with my git setup. I no longer alias over any of the builtin commands, so this issue is no longer a problem for me.

Instead of configuring alias.log, I now set use the alias hist = log --oneline --decorate --graph --branches. I've decided that aliasing core commands is probably a Bad Idea, so I just don't do it anymore, regardless of whether or not I'm using git-tfs.

I'm going to close this issue, but I wanted to ask: do you think this deserves a mention on the wiki? It might be useful for others to know that aliasing over any of the git commands will be problematic for git-tfs because of the way it runs commands and captures output. Obviously this will no longer be a problem when everything is implementing using a library, but it might be worth mentioning.

Owner

spraints commented Apr 19, 2012

i would like to make git-tfs handle it gracefully... ideally, we'll get away from parsing command output.

in the meantime, yes, it is worth a mention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment