Merge @ptitSeb's and @RolandTaverner's branches to master

[DEVoytas]

Before this repository was created, there were at least two independent branches of development based on the prevailing trunk of the original SVN repository (previous ctp2 source code hosting, which is now retired in favor of this github repositry).
Those branches are:

• ptitSeb taken from master of @ptitSeb's ctp2 repository
• RolandTaverner taken from master of @RolandTaverner's ctp2 repository

The goal of this issue is to merge both of those branches to current master, so the work of their authors is not lost.
The decision has to be made with regard of the order that the branches being merged. Both of them are based on the same commit (current master), so the first merge will be fast-forward, but merging second of them, will result in many conflicts since both branches bring changes in the same code areas.
This decision will probably has to be made by new maintainer when issue #2 is resolved.

The discussion on that started in the comments for issue #3 of @ptitSeb's repository, and will be continued here, so if any of the comments bellow seem out of context, it may be helpful to follow original discussion first.

[DEVoytas]

Not sure I understood fully. You want merge all @ptitSeb's commits to master branch, or just use his README as a base?

I meant all @ptitSeb's commits, as in my test they all worked out well.

Personally I agree, but I still think that the decision need to be made by the maintainer.
I already gave my reasons here, but in summary, whoever changes are merged first it's bit unfair to the other author, whose branch will start to have many conflicts.

One reason I would favor merging @ptitSeb's first, is that he is more active in the discussion than @RolandTaverner, but I do not want to impose my personal judgement.

This is why I would strongly encourage both of you @LynxAbraxas and @ptitSeb to reconsider the role of the initial maintainer :-) Both of you seem to have active interest in the project, and both of you have already contributed code to it, so I do not expect to find better candidates soon.

If @ptitSeb agrees, I think it would be completely natural for him to favor his changes and merge them first, but I would hope he would still not ignore @RolandTaverner's branch :)

@LynxAbraxas I know your concern is that you would spend more time then you should on this project, but maybe you could set some limits for time spent on it and go ahead with that?
(Maybe in order to prevent "lapsing back into coding-adiction", create request to github for feature called "temporary self-exclude", aka "timeout" common in gambling services ;-) )

[ptitSeb]

I still would prefer to not be the maintainer, but if no-one step-in, I'll do (but be warned that my failling git-fu will get in the way...). Also, I will not be availble before November anway, so that leave some time to find a proper maintainer.

About @RolandTaverner repo, I had a look. There are some overlapping commit with my repo (like the FT sources for example) but there is also interresting stuffs like the meson build script (no idea if it works) or the switch to VS2017 for Windows build, along with some C++ modernisation.
But it seems is not active on his github account for 6 months..

[LynxAbraxas]

As @ptitSeb is active and I can confirm that his changes work as expected I would first merge his changes and then work on new topic branches where one cherry-picks from those that were not merged, e.g. for VS2017 when someone tries to set up AppVeyor (see #6) or from the linux branch for e.g. support for linux home like ~/.civctp2. However, even though there is already the branch ptitSeb, it would be more in the GH way that @ptitSeb forks https://github.com/civctp2/civctp2 and then opens a PR for merging https://github.com/ptitSeb/civctp2/tree/ptitSeb into https://github.com/civctp2/civctp2/tree/master. Issues and PRs are not the same at GH.

Maybe in order to prevent "lapsing back into coding-adiction", create request to github for feature called "temporary self-exclude", aka "timeout" common in gambling services ;-)

I fear they won't concern about this just for me;-) I am happy to assist, comment and help with git-fu when ever I find the time but should not have a leading/depending role in this any more...
I only ended up here in the civctp2 business again only because I wanted to create a working docker image for myself to have a running state of ctp2 as long as docker is alive.

... whoever changes are merged first it's bit unfair to the other author, whose branch will start to have many conflicts.

I saw your try to merge on the civctp2 network view (https://github.com/civctp2/civctp2/network), which made me wonder if you used git-imerge? The interleaving merges caused that impression.

[ptitSeb]

Well Ok, I can work on that PR, but probably in November, I don't think I'll have time before.

(On a side note, the brank "linux-merge" has many commit with strange generic comment, is that normal?)

[ptitSeb]

Mmmm, I tried quickly, but it wont merge automatically between branch "ptitSeb" and "master". Shall I still make the PR or should I first try to solve that on my side?

[LynxAbraxas]

That is probably only due to the README.md that was added (d3822e6) and is not the same as yours. Do the PR and we can have a look;-)

[DEVoytas]

but if no-one step-in, I'll do

That is fantastic news @ptitSeb ! Thanks a lot.

be warned that my failling git-fu will get in the way...

Do not worry about that! I have some experience with git and I am willing to help. I am sure @LynxAbraxas can help as well. What is important is to have someone who will drive the project forward.
To be on a safe side with git we could agree no direct pushes to branches just pull-requests, which is probably good practice anyway.

[LynxAbraxas]

If you are happy with the current README.md you can do the following:

cd civctp2 # repos clone of your fork https://github.com/ptitSeb/civctp2 as origin
git remote add upstream https://github.com/civctp2/civctp2 # add remote to follow "official repos"
git fetch upstream# just to be current, you shoud see d3822e6e57f in e.g. gitk --all
git checkout ptitSeb # make sure you are on your branch
git checkout d3822e6e57f2f7ba7cc6aead76df86390a05e211 -- README.md # copy README.md from master over to your branch into stage
git commit -m 'using README.md from d3822e6e57f (git checkout d3822e6e57f2f7ba7cc6aead76df86390a05e211 -- README.md)'
git push # and there should be no confilict

[LynxAbraxas]

To be on a safe side with git we could agree no direct pushes to branches just pull-requests, which is probably good practice anyway.

@DEVoytas: I guess you mean no direct pushes to branches existing on the "official" repos? I think that is only important to follow by the maintainer and those collaborators who have write access to the "official" repos (https://github.com/civctp2/civctp2), i.e. currently only you. The work on forks can be done on branches that came from the official repos without any problems as they stay in the fork, but good practice is to choose a new branch name for a new topic, e.g. moved my commit (formerly on linux branch) to docker-compile_ubuntu, to give some more detailed meaning to (possibly) a series of commits.

[DEVoytas]

I saw your try to merge on the civctp2 network view (https://github.com/civctp2/civctp2/network), which made me wonder if you used git-imerge? The interleaving merges caused that impression.

Yes, I tried to merge it using git-imerge. And I managed to finish the merge but there are few issues with it:

• BIG one: it does not build! It stops at error:
  Crater.cpp:32:36: fatal error: os/include/ctp2_config.h: No such file or directory
  This is most likely due to 1c19591 "Refactoring: added paths to #include's" change on @RolandTaverner's branch. This probably is meant to work with meson builds, becasue 9d65143 "inital work on meson build" is the only commit I found that adds os/include to the include patch.
• git-imerge does not preserve original author metadata for rebased commits (I opened new issue for this: Original author not preserved for rebased commits mhagger/git-imerge#135)
• now after @LynxAbraxas pointed me to network view it made me realize that border-with-history makes branch network very cluttered so maybe its better to go with regular merge

Because of the above issue, please treat the branch with merge attempt as temporary one (I will likely delete it at some point).
However if any of you would like to have a look at it for reference you can fetch it (without adding my fork to remotes) with:
git fetch https://github.com/DEVoytas/civctp2.git ptitSeb-RolandTaverner-merge-to-master-WIP-bwh:ptitSeb-RolandTaverner-merge-to-master-WIP-bwh

That all makes me think that merging @ptitSeb branch and cherry-picking good stuff from other branches may indeed be the beast approach at this point.

[DEVoytas]

@DEVoytas: I guess you mean no direct pushes to branches existing on the "official" repos? I think that is only important to follow by the maintainer and those collaborators who have write access to the "official" repos (https://github.com/civctp2/civctp2), i.e. currently only you. The work on forks can be done on branches that came from the official repos without any problems as they stay in the fork, but good practice is to choose a new branch name for a new topic, e.g. moved my commit (formerly on linux branch) to docker-compile_ubuntu, to give some more detailed meaning to (possibly) a series of commits.

That is exactly right! On private forks everyone can adopt whatever workflow they prefer, since they are not affect main repo, in any other way then creating PR, wchic can be evaluated and reviewed.
However, even in personal forks I think it's probably good idea to still never to push to master directly Just merge topic branches (or in case of GH create maybe internal PRs?).

[DEVoytas]

Mmmm, I tried quickly, but it wont merge automatically between branch "ptitSeb" and "master". Shall I still make the PR or should I first try to solve that on my side?

That is probably only due to the README.md that was added (d3822e6) and is not the same as yours. Do the PR and we can have a look;-)

Right, this is most likely due to my new commit with README.md file.
@ptitSeb, I can create PR if you like (and resolve the conflicts in REDME file) but tell me if you want to leave information about our lookout for maintainer until November or are you ok with removing it already?

Also if you care about who created the PR (which does not matter much IMO, it's important who authored commits on a branch to be merged, which is you), I can create PR with your changes from my fork to you fork, and then you can create PR from there to main repo :)

[DEVoytas]

OK, I have created PR with rebase of @ptitSeb changes on top of master of main repo:
DEVoytas#1
Only last commit (DEVoytas@deada6b) bring some updates to README file.
Commit before that (DEVoytas@c634b06) is identical to @ptitSeb's branch:
https://github.com/ptitSeb/civctp2/compare/ptitSeb:ptitSeb..DEVoytas:ptitSeb-rebased^

Please let me know if you want me to create the same PR to your repo, and you will crete PR to main one, or do you want me to create PR directly to main repo.

[DEVoytas]

I wanted to create a working docker image for myself to have a running state of ctp2 as long as docker is alive.

@LynxAbraxas , interesting. Can you share some more info about that? Is it just for building ctp2 inside docker or something more? What's the current status?
Personally I think it is good idea to dockerize any project, because it makes you aware of all non-standard project dependencies.

[ptitSeb]

Well, I have done the PR (I resolved the conflict and yeah, it was the README.md file).

[ptitSeb]

Oh, so there is 3 PR about that merge now. Please pick one so we can delete the 2 others.

[DEVoytas]

@ptitSeb I would like you to pick one :)
Please read my comments to your PR I just added (a minute ago :) ).

[DEVoytas]

If you use gitk and would like to check how those PR looks visually you can do:

git fetch https://github.com/DEVoytas/civctp2.git ptitSeb-rebased:ptitSeb-rebased && gitk ptitSeb-rebased
git fetch https://github.com/DEVoytas/civctp2.git ptitSeb-merged:ptitSeb-merged && gitk ptitSeb-merged

But as I already said in the PR comments, both will result in exactly same code on master, it just the matter of preference: merge vs rebase.

[ptitSeb]

That's why I'm not a good choice for maintainer: the 3 PRs (even mine) looks the same to me, and I really don't care.
So, really pick which ever you want (if you realy need a preference: rebase).

[DEVoytas]

I disagree with, I think you are good choice for maintainer.
Git work-flow can be confusing, especially when you add some extra complexity and terminology from GH work-flow on top of that. It just requires some time and experience to be more comfortable with it.

You are correct, those 3 PRs are indeed very similar: they all bring all your changes to master.
What they are differ in, is the shape of the commits graph, plus in case of mine 2 PRs small updates to README file.

OK, I will merge the rebase PR and decline remaining two PRs, if this is fine with you?

[ptitSeb]

Yes I'm fine with that. (I really rarely look at the git graph... but I do like to watch some "gource" graph animation, but yeah, that's not git graph related).

[DEVoytas]

OK, done :)
Yes, I agree "gource" animations are cool to watch, but I would still encourage you to have a look at git graph with gitk, qgit or something similar sometimes.
In my case, having ability to have visual representation of the git graph, really helps to understand what's going on. Without it it can sometimes be very confusing (well to be honest for complicated graphs it can be confusing even with that :) )

EDIT:
it turns out that GH gives you a choice whether you want to merge or rebase incoming PR, which I was not aware of. I choose rebase because that was you preference.

Now if you want to update your own fork (at the moments your master is behind main repo master), you can do:

(I assume remotes are named: https://github.com/ptitSeb/civctp2.git == origin and https://github.com/civctp2/civctp2.git == upstream)

git remote update && git push origin upstream/master:master

[DEVoytas]

OK, this PR: #9 took care about @ptitSeb portion of changes, but I think we should keep this issue open until we merge or (cherry-pick what we can) from: https://github.com/civctp2/civctp2/tree/RolandTaverner

[LynxAbraxas]

That's great! Thanks @DEVoytas and @ptitSeb!
Just my two cents: In general I prefer rebase for structuring my topic branches (to reduce confusion sources), i.e. in my fork, but merges for PRs (or into master, even with --no-ff) because that preserves the branch structure and the "group" of commits that make up the topic, which can be named in the merge commit. Also, most important, a merge does not cause a change of the SHA (or the committer).
Now, that the commits are in master, I would recommend to delete the ptitSeb branch, to avoid confusion.
I also do recommend to look at the graph/gitk to check that such a rebase did not lead to unwanted changes. E.g. if you click on a998155 in gitk and then right-click on 97d6973 you can choose "Diff this -> selected" in the drop-donw menu which will show you the changes between those two commits (which should be empty in this case). You can do some other cool stuff with gitk that is cumbersome to understand/do with git.

[DEVoytas]

Thankx @LynxAbraxas

Actually I agree with all what you said. In general I also prefer to rebase while I am in my private repo, but after commits are published, rebase is discouraged.
I went with rebase, because that what @ptitSeb preference was, and this were his changes.
Also, in this particular case it's not so bad, because it accurately enough reflects reality how the development proceeded: it really was based on prevailing master (SVN trunk) and mine 'README.md commit' was introduced somewhat artificially, so I actually reverted it and applied @ptitSeb changes on top of that.
The usual downsides of the rebase (that you brought up), i.e. changes in commits' SHA1 and changes in original committer does not apply here, because those commits were taken from different SVN import so its meta-data was not preserved anyway.

E.g. if you click on a998155 in gitk and then right-click on 97d6973 you can choose "Diff this -> selected" in the drop-donw menu which will show you the changes between those two commits

Thanks for sharing that tip! I usually do this in command line (i.e. when I want to make sure that content for two branches or commits is identical I just git diff --name-status a998155 97d6973 ) but good to know you can also do it in gitk! :)

[LynxAbraxas]

Also searching a commit by a string in the message or the changes can be nicely done in gikt. It has the look of being a bit simple but actually has many (hidden) features. The docs are a bit scarce sadly.

Can you share some more info about that? Is it just for building ctp2 inside docker or something more? What's the current status?
Personally I think it is good idea to dockerize any project, because it makes you aware of all non-standard project dependencies.

Docker for building and especially using the image for playing, to not be so dependent anymore on keeping up with new lib-APIs, compiler standards etc. Status is currently in test phase for master (with ptitSeb's commits, that's why I noticed that the SHAs changed) and linux branch, both compile and I can start the game (with sound), save user-settings (e.g. screen res) and games but sadly either version crashes when loading a game (though I have a vague feeling, that that was once solved in the linux branch). Next step would be debugging inside a docker container... Concerning sharing: Sure, but I have to think about how to do this in the best way, possibly only the Dockerfile and run.sh with instructions on how to build a private docker image with ones own CD data.

[DEVoytas]

Also searching a commit by a string in the message or the changes can be nicely done in gikt

That one I already knew, agree it's useful :)

It has the look of being a bit simple but actually has many (hidden) features

One of the features, I am missing (or at least do not know how to do it) is to be able to choose what info is displayed next to the commit message (i.e. upper half of the window). It only shows you comit author and date, but I often would like to see committer and his date.

and especially using the image for playing

Hmm this is even more interesting now... because I know that you need to have data from original game to be able to play.

how to build a private docker image with ones own CD data.

That is what I am most interested how you do it :)
Do you have second docker image containing the data, or do you fetch it inside docker from some fixed location after build finishes? Or something else?

[LynxAbraxas]

Concerning docker, see my PR (#13).
My holidays are over now, so don't expect to hear anything from me any time soon anymore...
It was a great time working with you guys on pushing CTP2 forward a bit more, especially for linux and therefore also for the docker word.

[MartinGuehmann]

I merged in @RolandTaverner's branch as far I could see that it makes sense. I left out the modifications of the anet library, because this way I couldn't build it.

I also skipped the white space changes in three files. These seem to be done automatically and have destroyed some alignments of code. Actually, the readability of the code went down.

Then we have some refactoring, raw pointers were replaced with automatic pointers. That is usually the kind of stuff that needs most care. And these things I may also throw into the dustbin myself. The modified code produced crashes. So to include it should at least be crash free and maybe also the sense of it was unclear to me. As far as I can see the stuff does not leak and we don't have problems here I can detect.

I also dropped the change with the fuller include paths. I think it is supposed to make the game compile faster, because the compiler does not have to look for the includes.

There were also some changes to nowin32.cpp, I merged in. Those may still have to be checked in case of doubt we can just revert.

[LynxAbraxas]

@MartinGuehmann would you say the issues this topic refers to are resloved with your recent efforts and the PRs now included in master?
If so, this issue should be closed and the concerned branches replaced with tags, see also #85.

[MartinGuehmann]

@LynxAbraxas, I think the main issue is done, so that we can leave the remaining to #85.