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

build on mingw compiler #42

Closed
wants to merge 17 commits into from
Closed

build on mingw compiler #42

wants to merge 17 commits into from

Conversation

mattn
Copy link
Contributor

@mattn mattn commented Apr 10, 2018

include some fixes

@mattn mattn mentioned this pull request Apr 10, 2018
@craigwi
Copy link
Contributor

craigwi commented Apr 10, 2018

Lots of good suggestions here. Please break them down into specific fixes.

@mattn
Copy link
Contributor Author

mattn commented Apr 10, 2018

Thanks. Which commit you want to separate?

@craigwi
Copy link
Contributor

craigwi commented Apr 10, 2018

Each commit should be simple and focused on a specific issue. Just scanning the comments, it looks like most can be separate commits with those about compilation warnings grouped together and those about ^Z grouped together.

Look at it from my point of view. I really appreciate the help. The more you can make the changes in bite sized, related pieces the better for you and for me!

@mattn
Copy link
Contributor Author

mattn commented Apr 10, 2018

I separeted some commit. And I noticed usage of CharUpper is not wrong. So removed commits about CharUpper from this PR.

@mattn
Copy link
Contributor Author

mattn commented Apr 10, 2018

If you want to squash commits for the groups, I'll do it.

@craigwi
Copy link
Contributor

craigwi commented Apr 10, 2018

Thanks; I'll go over the changes again.

@craigwi
Copy link
Contributor

craigwi commented Apr 12, 2018

Hi @mattn, I took a bunch of the fixes from your PR that were not specific to mingw.

Can you now resubmit your PR with just the remaining changes -- which presumably are just those for mingw?

Thanks.

@mattn
Copy link
Contributor Author

mattn commented Apr 12, 2018

Sorry, GitHub merge tool break encoding of source code. I'll fix in later.

@mattn
Copy link
Contributor Author

mattn commented Apr 12, 2018

Hmm, you've better to use cherry-pick to avoid conflicting. Also the commit you did lost my contributions.

@mattn
Copy link
Contributor Author

mattn commented Apr 12, 2018

Now this is remaining diffs.

@craigwi
Copy link
Contributor

craigwi commented Apr 12, 2018

Hi @mattn,

I need you to take a closer look at some things:

  1. Why might PARTIALKEY be defined?
  2. Why might GUID_DEFINED be defined?
  3. the changes to treectl.c are no longer needed since I changed the type of i.
  4. the change to wfdrives.c and wfinit.c to include shlobj.h are redundant.
  5. the change to wfdirrd.c is wrong since I changed the type of the second parameter to AppendToPath.

Thanks.

@mattn
Copy link
Contributor Author

mattn commented Apr 12, 2018

1, Why might PARTIALKEY be defined?

Recent compiler set have HELP_PARTIALKEY in winuser.h

Why might GUID_DEFINED be defined?

DEFINE_GUID is defined in another implementation in guidef.h on mingw compiler. So need to avoid re-define.

the change to wfdrives.c and wfinit.c to include shlobj.h are redundant.

No. IsNetDrive is not defined without shlobj.h

@mattn
Copy link
Contributor Author

mattn commented Apr 12, 2018

Now reverted some changes in this PR to match master branch. But the changes you did is changes I did. I say again. You had to use git cherry-pick.

craigwi added a commit that referenced this pull request Apr 12, 2018
@craigwi
Copy link
Contributor

craigwi commented Apr 12, 2018

Thanks; all your changes are now merged.

@craigwi craigwi closed this Apr 12, 2018
@craigwi craigwi added the fixed differently Spirit of PR was taken if not all of the details; credit given to original author. label Apr 12, 2018
@mattn
Copy link
Contributor Author

mattn commented Apr 12, 2018

Please revert the change b51bf08. That is commit I did not you.

@craigwi
Copy link
Contributor

craigwi commented Apr 12, 2018

Correct. I cherry-picked your changes and committed some of them.

Let me know specifically what changes I committed that do not meet your needs.

@craigwi craigwi reopened this Apr 12, 2018
@mattn
Copy link
Contributor Author

mattn commented Apr 12, 2018

Why I don't listed in here https://github.com/Microsoft/winfile/graphs/contributors ? Because you didn't cherry-pick correctly.

445ea8e and a06f6f1, b51bf08 are commits I did.

@craigwi
Copy link
Contributor

craigwi commented Apr 12, 2018

I didn't cherry-pick them. I used a merge --squash and reformatted the commit comment. I guess that didn't get you the correct attribution. Sorry about that.

Well, in any case your changes are part of the project. I appreciate your help!

@craigwi craigwi closed this Apr 12, 2018
@wataash
Copy link

wataash commented Apr 12, 2018

@craigwi, you should revert b51bf08; otherwise you stole code @mattn wrote.

Commit history must be like this or this.
You can cherry-pick them by:

git checkout master
git remote add tmpremote git@github.com:wataash/winfile.git
git fetch tmpremote rewrite-author
git cherry-pick master..rewrite-author
git push
git remote remove tmpremote

@craigwi
Copy link
Contributor

craigwi commented Apr 13, 2018

I feel caught in a bind because I want both high quality commits to a very complex code base and definitely wanting to provide attribution to all who contribute. Help me solve that.

@mattn
Copy link
Contributor Author

mattn commented Apr 13, 2018

@craigwi Sorry for confusing. I do not want to say opinions any more because I will be seem I'm talking about childishly. I value the respect for contributors in OSS. For example, I'll keep author name in commit even if I must make squash the commit. If GitHub is gone and I must move my repositories into another VCS, I'll move commit history into the VCS to remain the information of contributions. I feel that this is a difference in respect to contributions. At least if you instructed me, I might make same commit as your commit. If you feel that I don't have the ability to do it, please close this PR.

mattn added 16 commits April 13, 2018 14:57
Signed-off-by: Yasuhiro Matsumoto <mattn.jp@gmail.com>
Signed-off-by: Yasuhiro Matsumoto <mattn.jp@gmail.com>
Signed-off-by: Yasuhiro Matsumoto <mattn.jp@gmail.com>
Signed-off-by: Yasuhiro Matsumoto <mattn.jp@gmail.com>
Signed-off-by: Yasuhiro Matsumoto <mattn.jp@gmail.com>
Signed-off-by: Yasuhiro Matsumoto <mattn.jp@gmail.com>
Signed-off-by: Yasuhiro Matsumoto <mattn.jp@gmail.com>
Signed-off-by: Yasuhiro Matsumoto <mattn.jp@gmail.com>
Signed-off-by: Yasuhiro Matsumoto <mattn.jp@gmail.com>
Signed-off-by: Yasuhiro Matsumoto <mattn.jp@gmail.com>
Signed-off-by: Yasuhiro Matsumoto <mattn.jp@gmail.com>
Signed-off-by: Yasuhiro Matsumoto <mattn.jp@gmail.com>
Signed-off-by: Yasuhiro Matsumoto <mattn.jp@gmail.com>
Signed-off-by: Yasuhiro Matsumoto <mattn.jp@gmail.com>
Signed-off-by: Yasuhiro Matsumoto <mattn.jp@gmail.com>
add -lversion to makefile

Signed-off-by: Yasuhiro Matsumoto <mattn.jp@gmail.com>
@mattn
Copy link
Contributor Author

mattn commented Apr 13, 2018

@craigwi Which commits do you want to squash? If you want, I'll separate this PR to several PRs.

craigwi pushed a commit that referenced this pull request Apr 13, 2018
commit 71bce76
Author: Matthew Justice <mattjustice@hotmail.com>
Date:   Thu Apr 12 16:38:41 2018 -0400

    Revert b51bf08

commit 00db12b
Author: Matthew Justice <mattjustice@hotmail.com>
Date:   Thu Apr 12 14:07:01 2018 -0400

    Use TCHAR and TEXT() consistently

commit 0f3ce87
Author: Matthew Justice <mattjustice@hotmail.com>
Date:   Thu Apr 12 11:26:25 2018 -0400

    Add newline to end of res.rc

commit aece59f
Author: Matthew Justice <mattjustice@hotmail.com>
Date:   Thu Apr 12 11:18:48 2018 -0400

    Tabs to spaces in res.rc

commit 806647a
Author: Matthew Justice <mattjustice@hotmail.com>
Date:   Thu Apr 12 11:12:43 2018 -0400

    Define version in RC file and display in about box

commit b51bf08
Author: Craig Wittenberg <craigwi@seanet.com>
Date:   Wed Apr 11 21:57:21 2018 -0700

    Squashed commits for PR #42.
@craigwi
Copy link
Contributor

craigwi commented Apr 13, 2018

As I feebly tried to say above, I really, really, really want you all to get the credit for changes. I've been working on WinFile for 10 years and personally pushed to get this project out so you could all benefit. I have enough credit to my name here.

In the end, I will:

  • make sure the author attribution is correct, no matter how the commit happens
  • work more closely with willing authors to shape PR before integration

Given that I already screwed up the commit history, I made one last change so that the most recent commit is authored by Matthew and that he gets the credit on the contributors board.

Again, I want you folks to get the credit and want us to have a great, high quality result.

Sincerely,
Craig.

@mattn
Copy link
Contributor Author

mattn commented Apr 13, 2018

@craigwi Thank you for the faithful and wonderful your work. About twenty years ago, I often used WinFile on Windows 3.1. And it worked very well and charms. I believe that your work was great. Thank you. We are not doubting you. Just simple thing, We hope to share the pleasure of being merge of commits from contributors. So let's make a constructive discuss.

@mattn
Copy link
Contributor Author

mattn commented Apr 13, 2018

@craigwi I'll separete this PR to several PRs. If you like single commit, please let us known, I'll squash several commits in the PR before you will merge.

For about separaing PRs, How about this?

  • fix syntax of res.rc
  • remove compilation warnings
  • add Makefile.mingw
  • fix build errors related re-define macros.

@wataash
Copy link

wataash commented Apr 14, 2018

@craigwi
Thank you for your kind cooperation :)

Please keep in mind that git has the concept of "author" and "committer". The committer is just a proxy for a commit, while the author should be kept as the original one.

If the commits in PRs have so good quality that they don't need to be squashed, I recommend merging them by "Create a merge commit" or "Rebase and merge" on GitHub UI. But please don't use "Squash and merge". I wrote the details in here.

If you need to squash commits, I recommend the following procedure:

git checkout master
git branch master-backup
git fetch origin pull/42/head:tmp
git reset --hard tmp
git rebase -i master-backup
git branch -d master-backup
git branch -D tmp

I wrote more helps in here.

While this procedure, we often get confused -- what is the branch status? and where am I? (where is HEAD?) So I strongly recommend using a commit-tree visualizing tool like Sourcetree (or, of course, git log --graph if you think it's enough).

If you mistakenly rewrite the author, rewrite it back again by git commit --author='Author Name <author@example.com>' --amend.

This was referenced Apr 14, 2018
@mattn
Copy link
Contributor Author

mattn commented Apr 14, 2018

Now this PR is separated to #93 and #94.

@mattn mattn closed this Apr 14, 2018
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

5 participants