Procedure for code review

[THLeaderH]

TL;DR
Why don't we use GitHub pull request for code review?


Let me have a discussion about code reivew with you. Currently, to implement a new feature to simutrans, the proposer without the nightly trunk committing right submits a patch file that contains implementation. The person with the commiting right reviews the submitted patch, fixes the code if neccesary, and commits into the svn repository. I think there are some problems in this procedure.

First, the person who commits the patch fixes the code without doing a feedback to the submittor. Due to lack of proper feedback, the submittor may make the same mistakes, which is a great loss for our community.

Second, this forum is not so friendly to handle the code. There is no syntax highlight and the system to pointout the specific line of code. This especially makes it harder to review the code of larger projects.

Third, the patch file submittor has to submit the patch file everytime he/she makes an update. This is not just simply boring, but brings version management problems. We frequently encounter the situation that it is not clear which patch file is the latest.

Thus, I think we should use some service to do a code review more efficiently. Since we are using GitHub for making executable binaries, the pull request feature of GitHub would be proper place for us to do a code review. If there is a nicer service to do that, please suggest it here

I am submitting a pull request on GitHub as a trial. This PR is only for the code review and is not intended to be merged.
If we use GitHub pull requset for the code review process, we should define a pull request template for the code review.
https://github.com/aburch/simutrans/pull/16

[prissi]

Nobody has access to aburch but Ansgar Buchard, who is not very active in Simutrans. He has just set up a server to push the SVN to Github.

I am not using GitHub, but for macintosh and Android builds. I am one of the very few developers who is unhappy with Github, since it requires a totally different workflow (for instance just trying something on my fork and revert to aburch is very hard. One has to rather make a branch and on success merge it back or discard it. Feels like the messy room of my daughter, full on half finished work ...). I can use it as a user, but I am not confident to really maintain a repo.

svn up
becomes according to google:

Code Select Expand

git remote add upstream https://github.com/aburch/simutrans.git
git fetch upstream
git rebase upstream/master
git push origin master --force
Only this does not work properly. So I usually delete the prissi/simutrans and fork aburch/simutrans whenever needed again.

And svn revert -R . becomes "git reset --hard 0ad5a7a6" with the right commit number.Who designed that commands? Sorry, the chances of me breaking the master repo with git is way too high. I started looking deeper into into git, but the weird unintuitive syntax drove me off each time (and I am not a proper developer. I mean, I can click in github and the windows client, but that 's it. Svn is my own server, that I am fairly confident with..

[THLeaderH]

I am not suggesting to use git as a version control suite, but to use GitHub pull request for code review. When the pull request is approved, the PR submitter makes the patch file and you or other commiter will commit the patch to the nightly trunk.

Since we have almost no privileges for aburch/simutrans, I agree that we should use simutrans-organization(?)/simutrans repository for code review.

[Mariculous]

svn up
becomes according to google:

Code Select Expand


git remote add upstream https://github.com/aburch/simutrans.git
git fetch upstream
git rebase upstream/master
git push origin master --force


Ouch.
Yes, the git workflow is a little different and the svn bridge part makes it more difficult.
If you simply push your changes to  the git repository, then apply those changes via patch in svn and finally try to update your repository from git again, git will see two different commits. It cannot know it's actually the same, just applied in different ways.

The usual workflow on git is not to work on master at all. Instead create a feature branch, work on it, and then incorporate it using one of the merge variants.
Once incorporated, you can delete the feature branch.

This workflow can simply be applied to the svn/git bridge setup: Instead of merging into master using git, create a patch and apply it in svn.
Then, you can still delete the feature branch and update your master as simple as git pull

You do not want to apply destructive operations (you know that you have done such, when you have to force push afterwards) on a public repository.
That means, whilst rebasing and stuff like that is absolutely fine and super useful locally, you should never do this to repositories stored on a public repository.

When the pull request is approved, the PR submitter makes the patch file and you or other commiter will commit the patch to the nightly trunk.

Yes, that might work. The PR history will look a bt weird (all closed, none merged) but that's still better than forum discussions imho.
The downside is breaking with simutrans tradition and the requirement of a github account.
Thus, I'd not enforce all commits to use github PRs, but instead offer it as a tool and strongly suggesting or even enforcing it on larger commits.

About the patch, github will do it for you!
Just append.diff or .patch to the url:
https://patch-diff.githubusercontent.com/raw/aburch/simutrans/pull/16.diff
https://patch-diff.githubusercontent.com/raw/aburch/simutrans/pull/16.patch

[prissi]

To review and test a patch, I have to download it (if was aware of adding diff, that feature I used a long time), patch my svn repo. Then I find errors, fix them and if this is a large fix, create a patch do "svn revert -R ." and post the patch on the forum. Otherwise, if there is little change I incorporate it and maybe update the translator, the base.tab (which is not on github, as well as some other files below trunk), and add German and English translations.

If possible, I try to incorpoarte patches as soon as I review and test them, because I have not the time or energy for two reviews and tests most of the time.

Anyway what is your suggestesd workflow now: I have a changed repo with a branch due to patching (not sure how to do this, never worked with branches on git.) Apparently then I have to generate a pull request with the revised code? So please explain me your envisioned workflow for the above example to somebody who never ever really have used git or github for colaboration.

[THLeaderH]

Freahk wrote an awesome guide for git beginners. This guide is exactly same for standard, except for the fact that the original repository is not jamespetts/simutrans-extended.
https://forum.simutrans.com/index.php/topic,21197.msg197581.html#msg197581

[Mariculous]

Indeed, but it is written from a contributors perspective, not from a repository maintainsers perspective.

From a repository maintainers perspective, one possible workflow is as follows.
Situation: A contrubutor has created a PR.
There are now two things that might need to be done in no particular order:
- review view the code changes
- test the resulting program

The former is the point where github code reviews might be extremely useful.
Open the PR on github, and click the "Files changed button". You will see the changes that PR will introduce to your repository in a well visualised way.
Whenever you notice something that needs to be changed or is unclear, you can click and drag on the line numbers to select a block of code and attach a review comment to it.
Once you have reviewed a file, check the "viewed" checkbox.
Once finished with reviewing, click the "Review changes" in the top right corner. A window will show up where you can type a review conclusion. Furthermore you can set the review to be an approval (you consider the code in this exact way to be ready for incorporation), a change request (you identified something that has to be change before it can be incorporated) or a comment (you have some questions about it before you can make a conclusion)

Anyone who has write access to the origin branch of the PR can then make changes and push these. The PR will automatically update to include these.
Depending on how the PR was opened, the creator of the PR might have given access permissions for that branch to you, in which case you could git checkout <full_url_to_the_repository> <branch_name>, do changes on your own and git push <full_url_to_the_repository> <branch_name> the cahnges back into that branch, thus including these changes in the PR.
If you don't want to use the full URL each single time, you can create an alias for that remote: git remote add <name> <full_url_to_the_repository>

To test the resulting program, you can always do git fetch origin pull/<PRid>/head followed by git checkout FETCH_HEAD
This will get your local repository to the same state as the PR.

Up to here, the workflow is the same as for maintaining a github repository directly.
Finally, once you conside rthe changes to be ready for incorporation, visit <full_url_of_the_pull_request>.patch and appy that patch in svn.
As the changes are incorporated now, you should close the PR on github.

[prissi]

Maybe there was a misunderstanding. If the code review is moved to github instead of forum, then of course the main repo would be on github and the svn will be retired. There are good enough clients do not bother with gits stupid hard and inconsistent commandline syntax (and the authentication problems with github). I just wish there is an easy way to do "svn revert -R .", i.e. discarding all local changes without stashing or so, just getting again a clean copy. So far, deleting everything and fresh checkout seems to work best for that.

On code review: Who on earth can do code review by just looking at a diff unless it is almost trivial. I usually need to build it and test it, then there are of course some minor changes needed as well. SO what would be the workflow then, as I am apparently not able to edit a pull request but just commend. If I push to the master, then of course the pull request updates, but that is not the intention. I just would like to update a pull request. (Or rather that was the original question). Beyond just writing a comment, which takes more time than correction a few mistakes. Or did I overlook something?

[Andarix]

Maybe there was a misunderstanding. If the code review is moved to github instead of forum, then of course the main repo would be on github and the svn will be retired. There are good enough clients do not bother with gits stupid hard and inconsistent commandline syntax (and the authentication problems with github). I just wish there is an easy way to do "svn revert -R .", i.e. discarding all local changes without stashing or so, just getting again a clean copy. So far, deleting everything and fresh checkout seems to work best for that.
...

I'm using TortoiseGit on Windows

there is also an undo
https://tortoisegit.org/docs/tortoisegit/git-command.html#git-reset(1)

[THLeaderH]

Maybe there was a misunderstanding. If the code review is moved to github instead of forum, then of course the main repo would be on github and the svn will be retired. There are good enough clients do not bother with gits stupid hard and inconsistent commandline syntax (and the authentication problems with github). I just wish there is an easy way to do "svn revert -R .", i.e. discarding all local changes without stashing or so, just getting again a clean copy. So far, deleting everything and fresh checkout seems to work best for that.

If you want to discard any local changes that are not commited yet, please use `git checkout .`. If you want to discard commits that are not published (pushed), one of simplest ways is deleting the local working branch, and fetching the remote branch again.
Until you get familiar with git, GitHub Desktop ( https://desktop.github.com/ ) can be a good client for you. GitHub desktop enables you to discard uncommited changes and undo the unpublished commit with a few clicks. I'm using GitHub Desktop although now I got familiar with git since I can do various git operations without typing commands. TortoiseGit can be also good.

On code review: Who on earth can do code review by just looking at a diff unless it is almost trivial. I usually need to build it and test it, then there are of course some minor changes needed as well. SO what would be the workflow then, as I am apparently not able to edit a pull request but just commend. If I push to the master, then of course the pull request updates, but that is not the intention. I just would like to update a pull request. (Or rather that was the original question). Beyond just writing a comment, which takes more time than correction a few mistakes. Or did I overlook something?

Code review with GitHub pull request itself does not reduce the steps of code review. To do the full procedure, you should fetch the branch of the pull request, then do a test run. If there are anything wrong, write a comment on GitHub to ask the author to fix it.

As a principle, in GitHub pull request workflow, the author is responsible in fixing the code, and others only suggest how to fix the problems. However, I understand that you want to fix the submitted pull request by your hand and there are several ways to do so.
The first way is merging the pull request into another newly created working branch. The merging target branch can be changed on the pull request page, and you can fix the code by your hand on that working branch. Then, you can merge the working branch into the master branch.
The other way is asking PR submitters to use the branch that belongs to simutrans organization. If the branch for the PR belongs to simutrans organization, you can commit directly to the PR branch since you have the write privileges to simutrans organization repositories. However, I do not recommend this approach since this can easily cause code conflict.

Although the first way is preferred to fix the code by your own hand, I recommend to make PR the submitter fix all problems as long as you have time. The submitter learns how the code should be through this fix process and this brings less erros in his/her next PR.

[Mariculous]

"svn revert -R ."

About "svn revert -R ." in the first place, a proper git workflow should not involve a simmilar git command.
If you want to test a branch without the intention of making changes to it, you just want to checkout a so-called detached head.
To checkout a PR in detached head mode, just run git fetch origin pull/<PRid>/head followed by git checkout FETCH_HEAD
Then test it. You might even make changes although you are in detached head state. If you finally decide that yo uwant to throw away eventual changes, simply check out a different branch. If you want to keep eventual changes, run git checkout -b <new_feature_branch> to create a new branch from your (detached) head.

If you somehow messed things up and you really need something like "snv revert -R .", you can simply run git reset --hard, which will reset all tracked files to the same state when you have checked the branch out. You can further run git clean -rf, which will remove all new untracked files since your last checkout.

About reviews, sometimes you might immediately notice a big issue with a commit just from the code, so you can request changes in the code of that PR.
No need to test the feaure as it might behave differently after these changes have been made anyways. Same goes for the other way round.
Surely, you don't want to confirm PRs without doing both, testing it and looking at the code.

[prissi]

I am mostly using github desktop, and with it is is impossbile to discard all changes. With this delteion and checkout again is the only way.

If the submitter fixes the code, it has been to be reviewed again. Also if I have to repair it first (like the multitile buildings) to know what is actually broken. Making this again into comments is extremely time consuming, especially if I have to review it again. I have simply no time for this. I could start a branch on the main repo with these changes and my edits but then they would not be tested in the nightly.

I would like to avoid things like the overtaking patch, which added more on each interation and thus became too large to fully review at that time for inclusion (apart from disagreement on how to indicate that roads are two lane).

Standard aims as a rock solid base with easy user experience. This requires a lot of review and editing.

[Andarix]

I would tend to suggest a different route.

I don't know now whether Git supports different access rights for code branches.

If someone submits a larger patch, a new branch should be created for the patch creator with access rights. All main developers can check the code changes there and make changes if necessary.

I think it should also be possible to use Git Actions to create program files for testing from these code branches.

If the patch then meets the requirements, the code branch can be merged into the main branch.

[Mariculous]

Yes, you can set up branch protection rules control per branch permissions to specific users.
With git ci, an automated process can be set this up when assigning a specific label to a PR (e.g. create new branch from the PRed one, grant permissions, create a PR of that branch, leave a comment on the original PR and close the original PR.

[Andarix]

In this way, even asleep patches can be picked up and finished by others.

[THLeaderH]

I agree to use "feature branch" for larger projects. So I suggest a new workflow for a code change proposal.

~For a small change~
1. The proposer makes a pull request (PR).
2. (Optional) Coders on our forum review and comment on the PR. The proposer fixes code as suggested.
3. The code owner (typically prissi) merges the PR to a working branch, makes some fixes on the working branch, and merges it to the master branch.

~For a larger project~
1. The code owner makes a feature branch for the project.
2. The proposer makes a PR for a part of the project.
3. (Optional) Coders on our forum review and comment on the PR. The proposer fixes code as suggested.
4. The code owner (typically prissi) merges the PR to the feature branch and makes some fixes on that branch.
5. Repeat step 2 to 4.
6. After the all PRs are merged to the feature branch and the code owner approves all changes, the code owner merges the feature branch to the master branch.

I think this workflow does not break prissi's code review style and other coders can easily review and comment on pull requests. If coders comment on PRs and the proposer fixes for them before prissi starts the review, this can reduce prissi's effort to fix the bug and improve the code.

[prissi]

branches, I can understand that one sees the changed code. But no one but the other contributor will test it, since very little feedback comes even from the nightlies.

I played around more using a fork repo on github. I found it very hard and unintuitive. A simple diff to upstream seems is not easy in github. When there is more than one commit, github desktop does not show a linear diff. I have to use the command line "git diff master..upstream/master". Insofar it follows the git experience I had in the past, of introducing new words for old stuff and making things inconsistent just to make the scripting of the underlying tools easier (showing gits origin as a rather quick hack.) Unfortunately all more user-friendly command-line tools (like gitless) did not survive.

I will need a lot of time to do things properly, which I would have rather avoided.

Also there are other comitters, who could (and should) also commit stuff. I did not intend to be the only one (that was the reason to go open source). Just due to fragmentation almost nothing is left with the original.

[Mariculous]

Simply append /compare to the url: https://github.com/prissi/simutrans/compare very handy and indeed unintuitive. I have really no idea why there doesn't exist a button for that purpose...

[Dwachs]

Personally, I use git for development and git-svn to commit to the svn repository. I find this very comfortable. I do not have much time and energy to do proper code review.