nice catch, especially for the mauryan elephant archer template.

Actually I think this is the remnant of an old feature where celt blacksmiths would be available in the first phase. The person who added those lines must just have changed that without looking to the parent template. Nice catch.

In the test plan there should be what the reviewer must do to check that the patch is correct and complete, so for this thing something along the lines:

Check code doesn't effectively change anything
Check that no other blacksmith has such an entry
Check that all templates that need a <RequiredTechnology>phase_town</RequiredTechnology> have one

However do code does change something: the visible classes of the two changes blacksmiths got changed, and are now better since they are the same as all other blacksmith (see where this relic can lead too), so that change is correct!

But it would be nice too fix some similar cases i.o there are some templates with <RequiredTechnology>phase_city</RequiredTechnology> that doesn't need it. These are mainly champion and hero units having it and the parent of them already has the tech specification. So if you want you can grep on <RequiredTechnology>phase_city</RequiredTechnology> and fix the cases popping up in that search. (one could argue that these changes are different enough so they can go in a new revision, so i propose doing that. smaller patch => easier review => faster commit :P)

However there are some changed that should be included similar to the maur fix: cart mercenaries, (all civ) healers have the same problem. (I can't think of more units that could have the problem and also actually have it, but its always nice to catch some more if there are)

Well, I'm willing to have a look at healers, mercenaries, champions, heroes, etc. later, but I believe I read somewhere it's important to keep patches small and do separate changes in separate diffs. Is there anything I have to do before this one (D887) could get reviewed?

Keeping patches small is indeed wanted but patches should be complete as well. So I suggest to change everything related to the phase_town in this patch (so add the healers and mercs). And then phase_city things can come in a later patch. (also a patch changing 5lines is not considered big at all, its more like we should avoid patches of like >2k lines when possible)

Given that this is my first time using phabricator and I'm still learning, how do I add more files to this diff?

Well download and apply the patch by typing arc patch D887 in the command line (when having a clean svn), then make all the further changes in the files (just like you made these). Then do arc diff --preview and attach the patch (using the webgui) to this revision.

Also before i forget please add yourself in the programming.json so you name will come into the credits

You don't have to use arcanist at all. You can just as well uploaded a new patch via the web interface (old patches are still visible in the History tab).
(https://trac.wildfiregames.com/wiki/SubmittingPatches)

OK, I figured out how to update a patch. However, I think it's better to postpone adding
<RequiredTechnology>phase_town</RequiredTechnology>
into the template_unit_support_healer.xml file, because another revision (D889) also changes this file, and I don't want to cause an edit conflict.

PS Where is that programming.json file located?

binaries/data/mods/public/gui/credits/programming.json (when i don't know file location i just use "files" to search for them, ctlr+F ftw)

Also that patch doesn't seem to conflict with these changes, they might change the same file but not the same lines, and even if something breaks it would be an easy rebase anyway.

In D887#34629, @bb wrote:

Then do arc diff --preview and attach the patch (using the webgui) to this revision.

Just do arc diff --update D887. By that you avoid using guis ;)

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/1994/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/1995/ for more details.

programming.json entry?
what about these:
./binaries/data/mods/public/simulation/templates/template_unit_hero_cavalry_javelinist.xml
./binaries/data/mods/public/simulation/templates/template_structure_defense_wall_gate.xml

further patch looks ok and commitable
(do notice that its not always true that completeness goes over patchsize, there needs to be some balance, however this patch size is quite ok).

aww not too happy with the cav change, certainly it is slightly out of scope (and this way the patch is growing biggish), so i suggest splitting of the cav change and push that into another revision. (probably just go to the previous diff and just change the 3 files my comment was about, the old this is found with using the "history tab" just below)

Actually that cavalry change is the logical result of your earlier suggestions, but if you insist, I'll revert it :)

well not entirely, its indeed related, but it is changing an entry around, not just removing useless ones or add missing (which is what the others are doing)

Effectively nothing is changed, so I fail to see this is different. Anyway, how do I load the older version via the command line (arc patch)?

arc help patch brings help :

arc patch --diff 3581

(3581 is the diff id found under the history thing)

is adding this really necessary?

Also added a “town phase” requirement to the healer.

they train from temples only which is built in town phase

Added a “town phase” requirement to Carthaginian mercenaries.

they train from embassies only which is built in town phase

it is since those buildings could be there set from atlas on the map, and a player shouldn't be able to train them until town phase

Thank you for your help. It was an useful exercise to learn phabricator etc.

Now, do you also want me to create a new patch for that cavalry change, or do you prefer to leave it unchanged as it currently is?

glad you have learned a good bit. regarding the cav change, i did leave it as is now, since i plan on changing th functionality of the RequiredTechnology (so multiple techs and stuff) and we can probably better do that change after that (so we nuke merge conflicts and so)

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/2000/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/2001/ for more details.