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

Bad Git-LFS reversion test #100

Merged
merged 1 commit into from
Feb 26, 2017
Merged

Conversation

robertkety
Copy link
Contributor

I attempted to commit to my v0.1.0 branch with an outstanding pull request. Somehow the second commit was linked to the pull request, which inadvertently caused failure. I suspect this was the result of how we're setting up the git push endpoints in combination with VS2015 git operations. I reverted the changes and performed a 'git lfs pull' it is my hope that this successfully reverted the problem and I'd like to see if a second pull request causes failure.

@robertkety
Copy link
Contributor Author

The reversion was a success so I decided to take advantage of the commit sensitivity of a pull request and attempt to commit those verb icons again. Everything appears to be working, but I'm still waiting on Travis CI for Mac build results.

@robertkety
Copy link
Contributor Author

Also:
YAY, 100!

Copy link
Contributor

@persn persn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need 3 commits for uploading two images, please squash it down to a single commit.

@@ -15,6 +15,7 @@
<MonoGameContentBuilderExe Condition=" '$(OS)' != 'Windows_NT' And Exists ('/opt/monogame-pipeline/MGCB.exe') ">/opt/monogame-pipeline/MGCB.exe</MonoGameContentBuilderExe>
<TargetFrameworkVersion>v4.5.1</TargetFrameworkVersion>
<TargetFrameworkProfile />
<AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is completely unrelated to the images, please make a separate PR for it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You literally made PR #99 for this purpose but closed it for unknown reasons, please open it again and do it there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure why you're doing this change in the first place, this property seems to be related to .NET 4.5.1, but all our projects is using that version, so why is only this single project getting this property?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I abused our repo with some personal git experimentation. I'll try to not do that in the future. The transition from TFS to Git is much more difficult since I transitioned from Git to TFS 2 years ago.

I thought I explained in the comments why I closed #99. I'll see what I can do about correcting this. Pull requests do not work the way I thought I understood them.

Hero6.DesktopGL uses the .Net 4.5.1 framework.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're willing to accept this PR, the drop down has an option to squash and merge.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't help but feel, that if we're going to add this property, we should add them in other projects as well.

@persn
Copy link
Contributor

persn commented Feb 24, 2017

The actual images are working great, but there is some messy execution in accordance with standard git practice so I'll need to be deliberately tedious in my reviews so you get it right.

@robertkety
Copy link
Contributor Author

robertkety commented Feb 25, 2017 via email

@persn
Copy link
Contributor

persn commented Feb 25, 2017

All the other projects uses 4.5.1 to my knowledge, personally I didn't get any warning off the sort, curious

@robertkety
Copy link
Contributor Author

robertkety commented Feb 25, 2017 via email

@persn
Copy link
Contributor

persn commented Feb 25, 2017

I don't think so, but neither is DesktopGL to my knowledge

@persn
Copy link
Contributor

persn commented Feb 25, 2017

Okay, I've been digging around a little and I'm guessing that DesktopGL needs binding redirects because it contains multiple entry point assemblies to the Eto framework. Things suddenly make more sense now so I don't feel we need to to introduce this property for every other project.

Since this PR now has a merge conflict anyways I'm going to suggest that you take the following course of action.

  1. Checkout a new local branch at LateStartStudio:v0.1.0.
  2. Commit the two new images at their proper directory.
  3. Force push the branch to robertkety:v0.1.0
  4. Wait for review.

I still want the binding redirects in a separate PR, preferably in the already existing #99 so that we don't clutter our GitHub history further. But that will not be possible until this PR is concluded since you used the same branch name for both PRs.

@robertkety
Copy link
Contributor Author

I've made the verb icon changes you recommended to this branch. The Mac build appears to not understand 'git lfs' commands; otherwise, everything looks good on this end. Once we finalize this, I'll update and reopen #99

@persn persn merged commit e58ac6c into LateStartStudio:v0.1.0 Feb 26, 2017
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.

2 participants