Working master branch

[jostephd]

As we all know, master is unplayable (#3538).

This PR is a replacement master branch, built by cherry-picking a subset of all commits in current master onto the 1.13.12 tag, which is the point where 1.14 forked from master.

I may have excluded too many or too few commits. If such mistakes have happened please send corrections (and don't bite my head off, it was a huge chunk of work and it's inevitable that mistakes will have been made).

There were quite a few conflicts in the rebase. It is not unlikely that in resolving those conflicts I introduced some bugs. From memory, there were non-trivial conflicts in unique_ptr/shared_ptr changes (particularly in ai/actions.cpp), boost::thread/std::thread changes, boost filesystem / std::filesystem changes. The lua vulnerability fix conflicted too.

The best way to review this branch is by diffing the commits in this branch to the corresponding commits in master.

The project files for VS/XCode/CBP may need to be updated to account for files that have been added/deleted/renamed on master.

This branch should be force-pushed as a new master branch, not merged. Current master should be copied to another name beforehand. Afterwards, any bugs that were fixed on master but are not fixed on this branch should be reopened. Then we should release a 1.15.0 development version from this branch.

edit:

List of commits that differ between master and this branch

[gfgtdf]

This PR is a replacement master branch, built by cherry-picking a subset of all commits in current master onto the 1.13.12 tag, which is the point where 1.14 forked from master. ... Then we should release a 1.15.0 development version from this branch.

I don't think there is much point in doing that currently, the current plan was to turn 1.14. in a rolling-release-like model that allows new features, so what we want is a branch-of from latest 1.14 with non-compatability-breaking features from master (and i suggest that, at least for the main committers (me, vultral, jykive, celmin, mattsc, and you, hopefulyl i didnt forget someone), so most of their commits on theirr own becasue they usually know their commits best).

Then once we have that, we can still think about doing a 1.15 release at some point, when we have something that is actually compatability breaking.

Another disadvantage of your appraoch is that it is basicially impossibel to approve this because one cannot get an overview over this. doing stepsie from 1.14 also solve that.

[Pentarctagon]

the current plan was to turn 1.14. in a rolling-release-like model

I know that was one option that was proposed by @Vultraz , but was that definitely chosen? So far it's mostly been waffling about even needing to do anything with the master branch at all, it seems like.

[gfgtdf]

I know that was one option that was proposed by @Vultraz , but was that definitely chosen?

Well it was proposed by other peole before him and all opinions i see on that thread (Me, vultraz, loonycyborg, celmin, Shiki) support that.

So far it's mostly been waffling about even needing to do anything with the master branch at all, it seems like.

I don't have a strong opinion on what do do with the branch for 1.14 features, we could:

1. call it the "1.14_newfeatures" branch and reserve master for the 1.15 release (incomatible changes)
2. or make the master branch basicialyl the branch for new 1.14 features.
3. or make 1.14 the master branch. and only have featurebranches.

I woudl probably leave the master branch asis and just create the 1.14 featuresbranch, but in the end the only differnce between all these otions is how the branches are called so.. it doesn't change much for me.

[jostephd]

Another disadvantage of your appraoch is that it is basicially impossibel to approve this because one cannot get an overview over this. doing stepsie from 1.14 also solve that.

I explained in the OP how to review the branch. Compare each commit in the branch to the commit in current master that has the same log message. You'll find that the only changes are conflict resolution.

Even if the branch were based on 1.14 it wouldn't be much easier to review, because there are several hundred commits that would need to be cherry-picked from master to such a branch.

edit: Wording

[GregoryLundberg]

I was surprised to see this based of 1.13.12 but I can understand the logic.

If we start from 1.13.12, it's easier to merge the changes since they can be taken in order. There should be fewer merge conflicts. That makes this process a lot faster.

If we start from 1.14.5 we're eliminating the risk of regression.

I don't have an opinion either way because, wherever you start from, you're going to have to audit the entire chain from 1.13.12 to the tip of both.

[jostephd]

@gfgtdf The project needs to have an open, playable branch where changes not suitable for the 1.14 branch can be made. That's true even if we allow more kinds of changes to be made in 1.14 patch releases.

[gfgtdf]

I explained in the OP how to review the branch. Compare each commit in the branch to the commit in current master that has the same log message. You'll find that the only changes are conflict resolution.
Even if the branch were based on 1.14 it wouldn't be much easier to review, because there are several hundred commits that would need to be cherry-picked from master to such a branch.

First, i wouldn't be so sure that commits that should be cherry picked are several hundred: commtis that have equivalences in 1.14. would ofc not be picked, and commits that are probably unsuitable (basicially most of the accelerated rendering) also would not be picked.

In fact If you assume that the ar commits are basicially exactly vultraz commits (which is clearly not true but that one don't really make the following statistic much wrong, becasue there we are also removing his commits in the 1.14 count) the commitcounts excluding the ar commits are:

master commits:
git log --perl-regexp --author='^((?!Charles Dang).*)$' --oneline 0b3f14d63e299..110bab74f0c | wc -l
retult: 1246
1.14 commits:
git log --perl-regexp --author='^((?!Charles Dang).*)$' --oneline 0b3f14d63e299..7d371752923 | wc -l
retult: 1275

further assuming the every 1.14. commit has a master equivalent 'forwardport' and the gap in the non-ar commits bewetten master and 1.14 is the amount of commits that will be backported to 1.14 featurebranch (clearly this is wrong since for some reason there are more commits on 1.14 than on master, but think it's still good as an approximisation). This tell you that there will only a few commits to backport if you start from 1.14.5 afterall. This also matches my memeory which is that there were really just a vrey few features added to master that were unrelated to ar.

Second, It's still easier than the current commit count which github shows as 1.5k.

And third and most importantly, under the assumption that the newfeatures on 1.14 will come, the pr means doing the review twice, one time when porting all commit from currentmaster to newmaster and once when porting them from master to 1.14 . But when you wait until after the 1.14 newfeatures branch, doing the newmaster branch (which would then branch off from 1.14 newfeatures branch) is probably just a few commits.

@gfgtdf The project needs to have an open, playable branch where changes not suitable for the 1.14 branch can be made. That's true even if we allow more kinds of changes to be made in 1.14 patch releases.

Well yes, i agree that there are quite some things that cannot be done in 1.14 in particular raising the requirements like using c++14. or hardware acceleration (if it ever comes to 1.15). Still there is no hurry and also no reason why we need to overwrite master to if we could aswell use any other name in the meantime.

[CelticMinstrel]

Comparing current master to your gitlab fork, I've come across a couple additional commits that I think could be included, though none that absolutely need to be. I got a little lazy towards the end, but that's also when it's least likely that there will be something, I guess.

19c78fc
9fe8631
ba0a12b

These commits remove deprecated things and should definitely be cherry-picked eventually, but perhaps not quite yet; if we plan to push out master as a 1.14.x or a stable 1.15.0, then we probably should not cherry-pick these until we're ready for a 1.16.x.

80e0463
fc6825f
50a3960
3e9424f

I'd also recommend some squashes for no other reason than they have commit messages that reference a no-longer-valid commit hash. Most of them are of the form "fixup ", but there are some others that reference hashes too. Or if you prefer you could reword the fixup to reference the new commit hash, but I figure that's not worth the extra effort. (This is why you should never reference commit hashes in your commit messages, people!)

Finally, is there any reason to keep commit-revert pairs such as 4d0e5f6 and 2f98779?

Apart from that the only other thing is that, obviously, you'd need to also grab the new master commits from the past few days.

My view is that even if you don't apply the changes I've suggested here, we can go right ahead and make this PR the new master branch. I just pushed a duplicate of the old master as accelerated-rendering-attempt, as well as a tentative new gui2_help branch, though it'll need to be greatly cleaned up once it's rebased against this new master. (In particular, all the a_r commits will probably need to be separately dropped on that branch.)

[CelticMinstrel]

Also just for the record @gfgtdf - while I somewhat support the rolling release schedule on 1.14.x, I'd prefer to continue to increase the middle number for API-breaking changes even on a rolling release schedule; so under that model, 1.15.o would be a new stable release instead of a dev release.

[CelticMinstrel]

Another thing that was pointed out to me in the chat is that there was a big PR from singalen was merged to 1.14 but not to master, so that will need to be added here; though it can just as easily be done after this is pushed to master.

[gfgtdf]

Comparing current master to your gitlab fork, I've come across a couple additional commits that I think could be included, though none that absolutely need to be. I got a little lazy towards the end, but that's also when it's least likely that there will be something, I guess.

celnup commits like this that obviously don't break things should also go into 1.14 (if they arent already) otherwise they will just give merging conflicts.

Also just for the record @gfgtdf - while I somewhat support the rolling release schedule on 1.14.x, I'd prefer to continue to increase the middle number for API-breaking changes even on a rolling release schedule; so under that model, 1.15.o would be a new stable release instead of a dev release.

Yes that was my plan aswell, no breaking changes in 1.14 just new api.

edit: well actual, my plan says nothing about how to handle breaking chanes exactly except that they won't come to 1.14 . so whether we name the next stable release 1.15, 1.16, 2.0 or 15.0 not sure. Whats important is though, that we should not make new stable series without a reason, that is: clearly noticable improvements for the user, in particular code clenups, removal of deprecated stuff etc. is irrelvant when it comes to when to make new releases.

[jostephd]

First, i wouldn't be so sure that commits that should be cherry picked are several hundred: commtis that have equivalences in 1.14. would ofc not be picked, and commits that are probably unsuitable (basicially most of the accelerated rendering) also would not be picked.

Yes, I already accounted for these factors. In 1.13.12..origin/master there are 1900 commits, of which 1200 have an equivalent in 1.14.4 and 700 do not. I considered two commits equivalent if their log message's first line was equal (which gives some false positives, particularly for translations).

In fact If you assume that the ar commits are basicially exactly vultraz commits (which is clearly not true but that one don't really make the following statistic much wrong, becasue there we are also removing his commits in the 1.14 count) the commitcounts excluding the ar commits are:

$ git log --pretty='%s %an' 1.13.12..origin/master | sort > master 
$ git log --pretty='%s %an' 1.13.12..origin/1.14 | sort > 1.14
$ comm -13 1.14 master | wc -l 
747
$ 

Of those 750 commits, about 450 are by Vultraz and about 300 by others.

further assuming the every 1.14. commit has a master equivalent 'forwardport'

There are 370 commits in 1.14 that don't have equivalents in master.

$ comm -23 1.14 master | wc -l 
375

and the gap in the non-ar commits bewetten master and 1.14 is the amount of commits that will be backported to 1.14 featurebranch (clearly this is wrong since for some reason there are more commits on 1.14 than on master, but think it's still good as an approximisation). This tell you that there will only a few commits to backport if you start from 1.14.5 afterall. This also matches my memeory which is that there were really just a vrey few features added to master that were unrelated to ar.

Your approximation is inaccurate: there are 292 commits in master that aren't by Vultraz and aren't in 1.14. The reason you got a much smaller figure is that the figure 292 is coincidentally almost equal to the number of commits in 1.14 not in master, 375. You counted only 375-292=83 commits.

Second, It's still easier than the current commit count which github shows as 1.5k.

Can I make something very clear? No one will need to review 1.5k commits. You can review the branch en masse by doing, for each commit X on the branch, git checkout X~1 && git apply ${commit on master equivalent to X} && git diff X. You'll find that about 50 to 100 commits had whitespace conflicts and about 30 commits had non-trivial conflicts (like changing unique_ptr to shared_ptr). That is what will need to be reviewed. (edit: see #3603 (comment) for details)

[jostephd]

Another thing that was pointed out to me in the chat is that there was a big PR from singalen was merged to 1.14 but not to master, so that will need to be added here; though it can just as easily be done after this is pushed to master.

Singalen's PR should be listed under Fwdport A reminder of a bugfix that was added to the stable branch that needs to be duplicated on master. regardless of what happens to this PR. If it's not there already would someone please add it.

[jostephd]

These commits remove deprecated things and should definitely be cherry-picked eventually, but perhaps not quite yet; if we plan to push out master as a 1.14.x or a stable 1.15.0, then we probably should not cherry-pick these until we're ready for a 1.16.x.

3e9424f

According to https://wiki.wesnoth.org/InterfaceActionsWML#.5Bdeprecated_message.5D those macros should have been retained at level 4.

[gfgtdf]

Yes, I already accounted for these factors ...

hmm yes i know my calculation werent that stong and since they werent the main point of my comment anyways i'll ignore this part.

Singalen's PR should be listed under Fwdport A reminder of a bugfix that was added to the stable branch that needs to be duplicated on master. regardless of what happens to this PR

Hmm there is also at least one patch (the theme battery thing) that is also not merged to master, and also not listed there becasue it was assumed not to be appliabel to master.

Can I make something very clear? No one will need to review 1.5k commits. You can review the branch en masse by doing, for each commit X on the branch,

hmm, this would not tell me whether there is aynthing important missed though. i think i'll try to look at the diff from your branch to 1.14 instead, not sure whetehr thats realistic, will have to backport some code cleanup commits before i can really test that though.

[CelticMinstrel]

commits like this that obviously don't break things should also go into 1.14 (if they arent already) otherwise they will just give merging conflicts.

Well sure, if you want to backport them, go right ahead.

those macros should have been retained at level 4.

Huh? I don't understand what you're talking about here. The macros are marked level 3 or 2. The level 2 ones maybe should be retained for the time being, I suppose.

Hmm there is also at least one patch (the theme battery thing) that is also not merged to master, and also not listed there becasue it was assumed not to be appliabel to master.

Good point. If this becomes the new master, the battery branch will be applicable.

Can I make something very clear? No one will need to review 1.5k commits. You can review the branch en masse by doing, for each commit X on the branch, [complicated procedure]

Maybe I'm missing something, but I don't understand how this is different from reviewing 1.5k commits. In any case, if you could somehow point directly at the commits that had conflicts whose resolution you're not confident in, perhaps that would make this easier? If possible giving both the old and new hash, or failing that, at least the commit message's first line.

[Vultraz]

Do not push this yet.

[jostephd]

hmm yes i know my calculation werent that stong and since they werent the main point of my comment anyways i'll ignore this part.

Your argument that we should wait for the rolling release model to be implemented is being discussed on the private thread.

hmm, this would not tell me whether there is aynthing important missed though. i think i'll try to look at the diff from your branch to 1.14 instead, not sure whetehr thats realistic, will have to backport some code cleanup commits before i can really test that though.

Yes, of course, in addition to going through the diffs that are in you'd have to also go through what's been omitted, as celmin already has.

Huh? I don't understand what you're talking about here. The macros are marked level 3 or 2. The level 2 ones maybe should be retained for the time being, I suppose.

That commit removed macros that were deprecated at level 3. If i understand the wiki correctly they shouldn't have been removed altogether but kept as no-ops deprecated at level 4.

Maybe I'm missing something, but I don't understand how this is different from reviewing 1.5k commits. In any case, if you could somehow point directly at the commits that had conflicts whose resolution you're not confident in, perhaps that would make this easier? If possible giving both the old and new hash, or failing that, at least the commit message's first line.

The difference is that you don't have to read 1500 diffs. You'd run a script on 1500 diffs and for 1400 of them it tells you that they're okay, and 90% of the remaining ones will have whitespace differences only.

I explained the procedure because I wanted everyone to be able to review the branch without relying on info provided by me, but I suppose I could produce a list of commits that had conflicts, if it helps.

[jostephd]

If possible giving both the old and new hash, or failing that, at least the commit message's first line.

(edit: see #3603 (comment) for how this list was produced)

Here you go. Most of these are trivial differences such as whitespace; 3533 is an example of a non-trivial difference.

conflicted: 0546b51 b746829 Improved console output on IPF fail
conflicted: 2c623ec 68f4bd1 Used std::exchange for a thing
conflicted: 45f8710 fc2a58f Use std::size_t everywhere instead of plain size_t
conflicted: 42fc801 899b4b7 Update VC project files.
conflicted: 5a017b5 3be39a9 0 -> nullptr in 2 places
conflicted: ca150a9 d4c9db9 Visual Studio: increased warning level to /W4 even for release builds
conflicted: 1e0c357 7621af9 show the oos savegame dialog when receiving a debug command in mp
conflicted: dfce371 1deacd8 Convert custom unicode type aliases to proper types (available as of C++11)
conflicted: a2c5129 e7a8af0 Renamed two t-prefix typedefs
conflicted: a01cde4 c42401a Fix #1736: on GNU/Linux, a hotkey can trigger multiple commands
conflicted: 16535c3 934f032 Revert "Fix #1736: on GNU/Linux, a hotkey can trigger multiple commands"
conflicted: 5eac05a 7f8cb13 help: Use new attack stats separator in unit descriptions
conflicted: cb3ddc3 8667e5b Hotkey manager: drop duplicate commands
conflicted: 98c6111 acc3fe8 Refactor out custom wesnothd_connection_ptr class again
conflicted: 68f066d 17fc9d7 Removed bind_void
conflicted: 1f8ba28 9668115 Expand NORETURN, enable C99 unconditionally on MSVC
conflicted: 58a61af 9977add Removed duplicate config_key_type alias
conflicted: 2a6d945 29fcbd0 Replaced round_double and round_portable with std::round
conflicted: c815fa1 227c8ba Changelog entry for ab54b62
conflicted: dde49f0 8e4de9d Editor: Refresh named locations list when map context changes (fixes #1023)
conflicted: 04837d9 03d80a0 Better changelog entry for the MP password fix
conflicted: 4b84091 a02100a Implement saving MP chat message history (#1194, #2802)
conflicted: 46eadd7 eacbc5f gui2/text_box: Store hint text as a t_string
new: c35ab93
conflicted: 5c8eed3 56e7b01 Refactor synced debug commands prompt to bring the string count down to 2
new: 0228706
conflicted: 44c8034 135921a Re-add boost-thread.
conflicted: 31e6c40 3792612 Removed OpenMP-related code
conflicted: 2af98f7 e583c47 Display: cleaned up overlay map getter interface
conflicted: baf02eb 882aba3 Units/Display: minor code cleanup
conflicted: 2b65a8c 7e442cb Units: refactor display_context parameter out of ability functions
conflicted: 80d44a4 0cdcfac Unit/Animation: emplace_back (mostly) ahoy
conflicted: 151ea62 c7b8694 Unit: replace unit_ability_list::push_back with an emplace_back impl
conflicted: 1af3bcb 098bc1c Unit/Race: made use of std::array
conflicted: 2e31e46 817f612 Update VC project files.
conflicted: d719470 b47837b GUI2/Dialogs: cleaned up a bunch of unnecessary forward declarations
conflicted: a040d52 5e36a90 Updates cmake and scons to be able to compile with OGL.
conflicted: bd5010a 691276a AI: removed duplicate composite AI typedef used a unique_ptr for it
conflicted: 53fb426 18d597e AI/Contexts: deployed std::make_shared in a place I missed
conflicted: c49917c 1c74487 AI/Actions: avoid inline ternaries in place of if blocks
conflicted: 7b1a76a 2174bfc wb: update following moves when a recruit is executed
conflicted: 0b98ddd f1bcdc7 Update text to match changes in dialogues (fixes #2882).
conflicted: 8516e1f d85e2a2 Update text to match game-play changes (fixes #2950).
conflicted: 3b5a9d2 4c937c1 Fixed a crash when using certain invalid color= values
conflicted: 70fb145 8fa3f6a Fix #3065: unit halo remains after undoing a recall
conflicted: ee2b9d2 2a58511 Bump min required Windows version to 7
conflicted: f262321 f5d74cd Fix building with osx+cmake on travis.
conflicted: d9fb6c0 2bf4d68 Catch all exceptions (where possible) as const references
conflicted: 399b589 d268249 Use std::time() instead of plain C time()
conflicted: 19e3afb 0dc5656 Convert C-style casts to static_cast
conflicted: d972ed7 5884aa2 wb: don't clear undo stack when dsu is active
conflicted: b953e31 6a9e2c2 wb: add a assertion
conflicted: 8a4d1ad 8f93d0a Saved Game: reame "starting pos" to "starting point" to avoid confusion
conflicted: f895963 2d600d1 Wesnothd: minor code cleanup
conflicted: 6eb3b7e 23732cf Removed useless Hide Help entry for debug terrain info
conflicted: 29632ae e6183aa Campaign Difficulty: colored description column gray
conflicted: 886156b 67530c4 Campaign Difficulty: rearranged entry layout
conflicted: dca6c8d 47bef43 Campaign Difficulty: restore the description parentheses on request
conflicted: 3e3d353 1413dfd Fix effects being unable to decrease weapon parry/accuracy
conflicted: 075d793 f72f89f Campaign Difficulty: consolidated both lines into a single label
conflicted: 8749b97 eddbaa2 Events: minor cleanup
conflicted: 56a09ab 4b03168 Fixup f72f89f
conflicted: ddd0a2e 8bdccca wb: fix moved becoming invalid after recruit is executed
new: bd06263
conflicted: 1520676 cc2cc29 Deployed std::make_unique and std::make_shared in more places
conflicted: adaa115 569d862 GUI2/Dispatcher: added a connect_signal convenience wrapper for draw callbacks
conflicted: a13791c 34195ae GUI2: removed now-redundant type parameter from build_single_widget_and_cast_to
conflicted: 758fe79 c79e164 fix issues caused by empty save id
conflicted: 9baabd8 70a1cdd Removed duplicate changelog entries under the 1.15.0-dev header
conflicted: d002ce2 b27d1c2 Changelog: moved an entry, fixed a typo
conflicted: 73cb9cf 175aa81 MP Faction Select dialog: Always show the leading unit information, never "Unknown Unit".
conflicted: a5859dc b6a0b2c Drop Down List: added documentation and did some code cleanup
conflicted: b9c2646 aca7d4e Select Orb Colors: used a widget iterator instead of a walker
conflicted: f46e217 4eee386 Stop passing a milion game_config refs around during game initializaton
conflicted: e98ec7e feb99c3 Cleaned up a few game_config_manager.hpp includes
conflicted: 81083e5 862b086 Fixup tests for 4eee386
conflicted: 826a76a 902cf6d Fix weapon specials marked as inactive
conflicted: c6e9efd 458dd28 GUI2: removed 2010 experimental listbox
conflicted: cb88a7a a4b0de5 Help: Use female_name and name as fallback when male_name is empty and don't list hidden traits.
conflicted: 766a0ff fa7c967 GUI2/Tree View: added interface for moving nodes between parent nodes
conflicted: 4dd1463 0f511e8 EI S11: use same approach as in 1e58164
new: 2ea002b
conflicted: fcfd534 637e69f Used the return value of modal_dialog::show instead of an explicit retval check when possible
conflicted: 5f849fc 300197e GUI2/Dispatcher: pass the message parameter for message events around as const
conflicted: 674fda8 bc4d22d Migrate links to https if available - Fwd c18537e
conflicted: a380ba8 61b7e72 Sort the movement costs in the tooltip in ascending order (#3305)
conflicted: f245cd6 d8e2498 Create a named struct for terrain name and movement pair
conflicted: 120e8d9 2554c16 disallow loading lua bytecode via load/dofile (CVE-2018-1999023)
conflicted: 81ea32e 40205c5 Help: document cores (#2703)
conflicted: f6eac47 fb5f46c Help: update the "Installing Add-ons" page (addresses #2703)
conflicted: 96bbebb 50301f8 Fix building with Xcode 10 (#3460)
conflicted: 6800ccd 6f67055 Game Load: Show list of enabled modifications (#3495)
conflicted: 4b29bd7 d99f5b8 Reevaluate [show_if] conditions and delayed variable expansions before displaying objectives at start.
conflicted: d5238ac b64f4b4 Commandline: --campaign-skip-story skips [message]s during prestart and start events
conflicted: e766cdc 3a3b752 WML: Support [filter_side] in [item]. (#3533)
conflicted: 50c85f4 4db9744 Unit Display: When the recruiting or recruited units is invisible, don't scroll to it.
conflicted: c54f4e6 c88a799 Help: Hide abilities with empty names.
conflicted: da95fed 248af05 Update Code::Blocks project (#3585)

[CelticMinstrel]

Do not push this yet.

Uh. You don't need to say that. It's kind of not possible to push unless the protection on the master branch is removed, right?

That commit removed macros that were deprecated at level 3. If i understand the wiki correctly they shouldn't have been removed altogether but kept as no-ops deprecated at level 4.

Ohhhh I see what you mean. That's a different interpretation of the deprecation system than I was envisioning, where level 4 is only used when you discover you have to remove something without going through a deprecation cycle. I'm not sure which is closer to what the original designer of the system had in mind, though.

I'll try to look over your conflicts tomorrow. (Or rather, after getting some sleep.)

[gfgtdf]

ok i checkkpicked some of the cleanups commtis to 1.14 and from looking at the diff to your branch to 1.14 (https://gist.github.com/gfgtdf/acdf3cd4ed88088eb5854d8e73701f94 for the src/ only part (some nose removed) if someone is interrested) it looks good, some thigns were missed though but i assume most of there are marked somehere, some thign which i think might not have a report yet

1. partls of the 'use https anywhere' commit were lost due to files that were removed before that commit
2. 247e08e (was notneeded anymore with ar)
3. ai: use cusotm recall cost

edit: i only looked at /src though, in particular i didnt catch any wml/lua changes.

[CelticMinstrel]

The checkout-apply-diff procedure you mentioned doesn't seem to be working here (maybe you missed some detail?). I'm also not clear on whether X refers to the hash on this branch or the hash on the original master.

[jostephd]

ok i checkkpicked some of the cleanups commtis to 1.14 and from looking at the diff to your branch to 1.14 (https://gist.github.com/gfgtdf/acdf3cd4ed88088eb5854d8e73701f94 for the src/ only part (some nose removed) if someone is interrested) it looks good, some thigns were missed though but i assume most of there are marked somehere, some thign which i think might not have a report yet

1. partls of the 'use https anywhere' commit were lost due to files that were removed before that commit

Good catch.

2. [247e08e](https://github.com/wesnoth/wesnoth/commit/247e08e1d521cfca3279c679f93dd0248ef23832) (was notneeded anymore with ar)

That's a commit on the 1.14 branch. It's like the battery theme changes: it should be added to remaster, but it was never in master.

3. ai: use cusotm recall cost

Sorry, I can't find this one. What's the commit hash?

The checkout-apply-diff procedure you mentioned doesn't seem to be working here (maybe you missed some detail?). I'm also not clear on whether X refers to the hash on this branch or the hash on the original master.

Sorry. Here's a complete example. It builds on the "(cherry-picked from commit hash on master)" annotations I added yesterday.

for remaster_hash in $(git log --reverse --pretty=%H 1.13.12..josteph/remaster) ; do
master_hash=`git log --pretty=%B -1 $remaster_hash | grep '^(cherry-picked from commit' | egrep -o '[0-9a-f]{40}'`
git branch -r --contains=$master_hash | grep -q origin/master || echo "Not recognized $master_hash"
git reset --hard "$remaster_hash"^ -- >/dev/null
git -c rerere.enabled=false cherry-pick "$master_hash" >/dev/null 2>/dev/null || { echo -n "conflicted: $remaster_hash $master_hash "; git log --pretty=%s -1 $master_hash | cat; }
git cherry-pick --abort 2>/dev/null
done

Now, there's one more detail. The branch contains 4 original commits by me, which aren't backports of existing commits. Yesterday, when I added "cherry-picked from commit hash on master" annotations to all commits on the branch, I inadvertently added those annotations to those 4 commits too. To cut a long story short, those 4 commits are printed by the above loop, but I edited the output by hand to add the "new:" labels you saw above.

[gfgtdf]

Sorry, I can't find this one. What's the commit hash?

i don't knmow the exact commit actually i just saw a nontrivial change in src/ai/default/recruitment.cpp and remembered that mattsc did such a thing.

[jostephd]

i don't knmow the exact commit actually i just saw a nontrivial change in src/ai/default/recruitment.cpp and remembered that mattsc did such a thing.

The difference in recruitment.cpp between remaster and master is:

Diff

diff --git a/src/ai/default/recruitment.cpp b/src/ai/default/recruitment.cpp
index b4c01ed1ed2..763c99b9e4f 100644
--- a/src/ai/default/recruitment.cpp
+++ b/src/ai/default/recruitment.cpp
@@ -448,7 +448,7 @@ action_result_ptr recruitment::execute_recall(const std::string& id, data& leade
 		recall_result->execute();
 		++leader_data.recruit_count;
 	}
-	return action_result_ptr(recall_result.release());
+	return recall_result;
 }
 
 /**
@@ -464,7 +464,7 @@ action_result_ptr recruitment::execute_recruit(const std::string& type, data& le
 		LOG_AI_RECRUITMENT << "Recruited " << type << "\n";
 		++leader_data.recruit_count;
 	}
-	return action_result_ptr(recruit_result.release());
+	return recruit_result;
 }
 
 /**

I assume what you're seeing is some commit to master that hasn't been backported to 1.14, or some commit to 1.14 that hasn't been fwdported to master.

[mattsc]

I think @gfgtdf is talking about 0119413 and 1597a2c. Those have only been committed to 1.14 so far because I want to expand on them for master in a way that is not allowed for 1.14. They can be added as they are though, the rest is in addition to this, not changes to these commits.

[Pentarctagon]

Any update on this? There really does need to be a working master branch again.

[gfgtdf]

Any update on this? There really does need to be a working master branch again.

If i understood @jostephd correctly his plan is how to wait until things are decided/this pr is 'merged' before doing the final fixup/forardsports.

I personally stiill think it'd be easiert to branch off from current 1.14 branch, but i'm currently not really against this apporach either.

Is there anythign in partiuclar you want to do on master?

[Pentarctagon]

There's nothing particularly critical I want to do myself or anything like that, but there hasn't been a working master branch for way too long already, and I don't want this to end up forgotten because @Vultraz isn't pushing to get a working master branch like he should be.

[jostephd]

If i understood @jostephd correctly his plan is how to wait until things are decided/this pr is 'merged' before doing the final fixup/forardsports.

I'm waiting to hear if there's consensus that this branch should become the new 1.15 branch.

To me, the goal of this PR is to produce a branch that's as close as possible to the current master branch, minus the portions that make master unplayable. (Those portions are not thrown away; they are preserved on the a-r-a branch and can be resurrected from there in the future.) That means I do not consider forwardports in scope for this PR. If this branch becomes the new 1.15 branch, then we should do forwardports from 1.14 to this branch before creating a release from this branch. Please do not expect me to do all the forwardports by myself; I have done way more than my fair share already.

I personally stiill think it'd be easiert to branch off from current 1.14 branch, but i'm currently not really against this apporach either.

Sure, if someone comes up with a branch that's the 1.14 branch plus interesting stuff from master backported, I would be happy to close this PR and let their branch become the new 1.15 branch. The end result will be the same, after all. Gregory touched on the pros and cons in his comment.

[CelticMinstrel]

Please do not expect me to do all the forwardports by myself; I have done way more than my fair share already.

Agreed. It's perfectly acceptable to get others to handle the forward ports.

I haven't been able to find the time or motivation to properly review the conflicting commits that you've listed. I'll still try to find it, but if anyone can review them and conclude that there's no merge errors, I would consider that sufficient for considering this branch usable.

[Vultraz]

This can be pushed to a new branch (not force-pushed to master) if it's ready. Call it development and I'll set it as the default branch.

[jostephd]

This can be pushed to a new branch (not force-pushed to master) if it's ready. Call it development and I'll set it as the default branch.

This has been done. Thanks @Vultraz.

For posterity let me document again the review procedure: go through the list commits that differ between master and this branch and confirm that the differences are all whitespace / conflict resolution. The script that produced that list can also be used to confirm those commits are the only differences and that every commit to this branch is a cherry-pick of a commit to master (except for four new commits that were needed to resolve conflicts).

Next up is to think about fwdports from 1.14 to development.