Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix handling of blob mode change #201

Closed
wants to merge 1 commit into from
Closed

Fix handling of blob mode change #201

wants to merge 1 commit into from

Conversation

nulltoken
Copy link
Member

Fix #196

@travisbot
Copy link

This pull request passes (merged e740bd0e into 4bda9ff).

@nulltoken
Copy link
Member Author

@carlosmn This covers the TreeToTreeDiff and the Status related issues we discussed in #196. However, I have very mixed feelings about this. See below.

Pros:

Cons:

  • 🔥 Fails on Windows/amd64 (Huh?) see the build log
  • This makes some members of the API return IEnumerable<> whereas only a single entry will be filled in +80% of the cases.
  • I'm not very glad with the resulting code (@dahlbyk any idea to make it look better?)

HeadBangings:

  • I can't understand why "include/Nu/Nu.h" is now seen as Untracked in CanHandleTwoStatusEntryChangesWithTheSamePath(). Could you run git status on you Linux box when line LibGit2Sharp.Tests/StatusFixture.cs:273 is reached?

/cc @arrbee

@dahlbyk
Copy link
Member

dahlbyk commented Aug 14, 2012

I'm not very glad with the resulting code (@dahlbyk any idea to make it look better?)

https://github.com/dahlbyk/libgit2sharp/compare/fix/issue196

I didn't look too closely at the tests, but I think this cleans up TreeChanges a bit.

@travisbot
Copy link

This pull request passes (merged c80a9f0 into 4bda9ff).

@nulltoken
Copy link
Member Author

@dahlbyk Thanks for these. They indeed make the code cleaner! I've pushed them on vNext and rebased this PR on top on them.

Below the current build logs with the updated code:

@dahlbyk
Copy link
Member

dahlbyk commented Aug 14, 2012

Rather than checking for Linux explicitly, try switching on status.Count() or tecs.Length: dahlbyk/libgit2sharp@b8e2154

@dahlbyk
Copy link
Member

dahlbyk commented Aug 14, 2012

Are you able to reproduce the amd64 failure locally? I'm not sure how to switch the build target...

@nulltoken
Copy link
Member Author

@dahlbyk unfortunately I'm running a x86 box :-/

@dahlbyk
Copy link
Member

dahlbyk commented Aug 14, 2012

I'm on amd64 - how do I switch build target?

@nulltoken
Copy link
Member Author

What happens when you run build.libgit2sharp.cmd? What does msbuild say/log about the bitness of the hosting process?

@dahlbyk
Copy link
Member

dahlbyk commented Aug 14, 2012

Regardless of 32- or 64-bit host, I get environment="32-bit .NET 4.0.30319.17020".

@nulltoken
Copy link
Member Author

Regardless of 32- or 64-bit host, I get environment="32-bit .NET 4.0.30319.17020".

Hmmm. That's very weird.

Could you try to update the content of the .cmd file to add the following paramater to the msbuild command /property:Platform=x64?

This should "force" the build in x64 mode. In the build log, xUnit should also be run in 64bits mode and display something like xUnit.net MSBuild runner (64-bit .NET 4.0.30319.269).

@nulltoken
Copy link
Member Author

@dahlbyk

Rather than checking for Linux explicitly, try switching on status.Count() or tecs.Length: dahlbyk/libgit2sharp@b8e2154

From what I understood of this comment, the behavior depends on whether the platform supports symlinks or not, this is why I was explicitly checking the hosting platform. I may be missing something, though? What are you aiming at?

@dahlbyk
Copy link
Member

dahlbyk commented Aug 15, 2012

The only Platform the build will accept is Any CPU. Maybe it just needs to be forced at the xUnit level - will see what I can come up with.

From what I understood of this comment, the behavior depends on whether the platform supports symlinks or not, this is why I was explicitly checking the hosting platform. I may be missing something, though? What are you aiming at?

At minimum, tweaking the tests will let us see better what's really happening on amd64 - currently we know the counts are wrong but that's it.

@dahlbyk
Copy link
Member

dahlbyk commented Aug 15, 2012

Got it - using the Framework64 version of msbuild does the trick (and tests fail). Will let you know what I can figure out...

@dahlbyk
Copy link
Member

dahlbyk commented Aug 15, 2012

https://github.com/dahlbyk/libgit2sharp/compare/fix/issue196

  1. Added build.libgit2sharp.x64.cmd.
  2. With a slight tweak due to path delimiters (sigh), all tests now pass. Part of me thinks this is fine - we'll accept either modified or add+delete as correct answers, so as long as we get one or the other we're cool. The other part wonders why the symlink works on amd64 but not on x86. Since we're just dumping what we get from libgit2, is this a bug there? Or a known/expected Windows quirk?

@nulltoken
Copy link
Member Author

is this a bug there?

I can't help but feel this is very unexpected. As far as I know the symlink feature does not depend on the bitness of the platform.

@arrbee @aroben We'd definitely need some help here. Any advice? If I ported the test to clar would one of you agree to test drive it (and potentially debug it) in a Win64 environment?

@nulltoken
Copy link
Member Author

@carlosmn ran the C version of CanHandleTwoTreeEntryChangesWithTheSamePath() on a 64bits VM and encountered a similar outcome.

@nulltoken
Copy link
Member Author

Closing this in favor of #1004

@nulltoken nulltoken closed this Mar 18, 2015
@nulltoken nulltoken deleted the fix/issue-196 branch March 18, 2015 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants