Fix issue #123 #126

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@chrisbroome

Update git log output to use a consistent format, despite any local
changes made by the user.
Update commit regex to correctly parse the new log format.
Update commit regex to use a named capture.

@chrisbroome chrisbroome Fix issue #123
Update git log output to use a consistent format, despite any local
changes made by the user.
Update commit regex to correctly parse the new log format.
Update commit regex to use a named capture.
fe9f4d3
@spraints spraints commented on the diff Jan 26, 2012
GitTfs/Core/GitRepository.cs
@@ -242,7 +242,7 @@ private List<TfsChangesetInfo> GetParentTfsCommits(string head, bool includeStub
try
{
CommandOutputPipe(stdout => FindTfsCommits(stdout, tfsCommits, includeStubRemotes),
- "log", "--no-color", "--pretty=medium", head);
+ "log", "--no-color", @"--pretty=format:'commit %H%n%b'", head );
@spraints
spraints Jan 26, 2012 Member

Can you change the format to 'commit %H%n%B'? It's not a big difference, but it gets git to dump the raw commit body. It seems like this will be more foolproof.

@chrisbroome
chrisbroome Jan 26, 2012

Yeah I'll change it to yours and push it up.

@chrisbroome

Ok, after further investigation, it appears that the above isn't being passed to git appropriately. Sep.Git.Tfs.Core.Ext.QuoteProcessArgument does naive checking of arguments. It only checks whether the string contains a space, not whether the space is within quotes (as in the above diff). So the method sees that the argument contains a space and puts extra double quotes around it.

What we want to run is this:

git log --no-color --pretty=format:'commit %H%n%B' HEAD

What ends up running is this (sample output shown):

$ git log --no-color "--pretty=format:'commit %H%n%B'"
'commit 118db54ffc62000b8dea02fcfc28f6efa3ffb1b4
Fix wiki link.'
'commit fee92600ca79b2277c37d9af8b1bbcf2dce421de
Merge pull request #122 from johnedmonds/redirected_stdout

Only close stdout if it has been redirected.'
'commit cd4babc00fa11024bfe14b3c60c3aea260bc7fc7
Only close stdout if it has been redirected.

git-tfs will crash if it attempts to close stdout from a process from
which stdout has not been redirected.  This adds a guard against closing
process's stdout that has not been redirected.
'
'commit 8e90acd333c19baaf1e98a56aa85f6b5d49e536a
Added closing of stdout for child processes preventing the hang in some cases (fixes #121).

Also removed close in another place as it is redundant now.
'

For each commit, the formatted output is being single-quoted.

As I see it, there's 2 options to correct this.

  1. Remove the QuoteProcessArgument call in the SetArguments method
  2. Change the parameter to not include a space

Option 1 impacts every command sent to git, so I'm less inclined to want to change that.

Option 2 impacts only the commit regex (and associated tests), and since we're changing the regex anyway, we can work around the issue by using a log parameter that doesn't contain spaces.

So we'll change the parameter from

"--pretty=format:'commit %H%n%B'"

to

"--pretty=format:%H%n%B"

which avoid spaces and single quotes in the parameter.

I'm thinking this would be the best temporary fix for now. However, if the commit message contains a line starting out with a valid sha1 hash (and there's nothing stopping anyone from doing that), then this is going to interpret that as a commit and bomb also.

This is a problem with the existing implementation of Sep.Git.Tfs.Core.GitRepository.FindTfsCommits.

We should probably figure out a way to get the commits and associated commit messages through an API rather than through launching processes. I'd imagine that's possible with GitSharp, but I haven't looked into it. I can create a separate issue for the usage of an API/GitSharp if you want.

@spraints
Member

How about we change the log call to this:

    CommandOutputPipe(stdout => FindTfsCommits(stdout, tfsCommits, includeStubRemotes),
      "log", "--no-color", "--pretty=format:commit %H%n%b", head);

This should work well with the existing argument quoting.

QuoteProcessArgument is definitely naive. I would much prefer to let the framework handle it, but Process.Start doesn't (or didn't used to) accept arguments split out into an array.

Using an API would be excellent. From what I can tell, the momentum is behind libgit2 (project and .net bindings). (Using libgit2 might force git-tfs to be a 32-bit build again, which would be annoying but not the end of the world.) I'd rather not use an issue to track this, but rather a branch or (even better:) just a series of pull requests that swap in libgit2 for some small slice of git-tfs that uses Process.Start or gitsharp. I would definitely accept piecemeal pull requests like this.

@sc68cal
Contributor
sc68cal commented Feb 7, 2012

I'd rather not use an issue to track this, but rather a branch or (even better:) just a series of pull requests that swap in libgit2 for some small slice of git-tfs that uses Process.Start or gitsharp. I would definitely accept piecemeal pull requests like this.

Cool - this is an ideal project for me to get more familiar with the internals. Overall I'm pretty excited about libgit2, with the small amount of work that I've already done.

I've got a branch that has some preliminary work.

@nulltoken

Using libgit2 might force git-tfs to be a 32-bit build again, which would be annoying but not the end of the world

@spraints libgit2 doesn't "officially" support 64 bits on Windows yet as many warnings are being issued during the compilation. However, the warnings have been reviewed by the core team.

@arrbee, one of the libgit2 core contributors, stated in this thread "At this point, I believe that this issue has been examined carefully and all of the remaining warnings are thought to be harmless. The issue is still open because we would really like it to be cleared up, but it is not at the top of anyone's priority list right now."

As a matter of fact, I maintain an experimental LibGit2Sharp hybrid (x86/amd64) branch in my repo with both versions of libgit2 binaries. Each time the vNext branch is updated, the experimental/hybrid one is rebased on top of it. Once I get enough feedback of this branch as being "stable" on 64bits platforms, I'll promote its content to vNext.

@sc68cal sc68cal was assigned Feb 19, 2012
@spraints
Member
spraints commented Dec 7, 2012

This has been open a long time. I'm going to close it, but I'm open to a new pull request for this.

@spraints spraints closed this Dec 7, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment