Issue977

Title Enforce naming conventions for git repository
Priority feature Status resolved
Superseder Nosy List florian, jendrik, malte, patfer, silvan
Assigned To patfer Keywords
Optional summary

Created on 2020-07-09.14:19:53 by patfer, last changed by silvan.

Files
File name Uploaded Type Edit Remove
history_check.log patfer, 2020-07-09.14:46:18 text/x-log
Messages
msg10950 (view) Author: silvan Date: 2023-01-30.14:31:23
Since we changed our workflow to have a linear history by squashing all changes of an issue into a single commit, we don't need anything we discussed here.
msg10167 (view) Author: silvan Date: 2021-03-04.18:11:30
We discussed that we want to have the following conventions:

- commit on main: if it has two parents, then the first must be on main and the second not on main; if it has one parent, it obviously must be on main

- commit on issue branch: ideally, we would like the history to be linear. This would likely require using rebase a lot, which loses the experiment tags. Since we don't want to lose them entirely, we want to look into alternatives. One option would be to make sure that tags get rebased, too, and another would be to more generally re-think how and where we store the experiment scripts. If they can live outside the repo, possibly together with the archived code, then we wouldn't need to keep tags in the main branch/repo.

- we discussed if the wiki shouldn't "forbid" doing merges on Github but only describe how to do this locally. We should also see if "git push --follow-tags" is a better alternative than "git push" & "git push --tags".
msg9726 (view) Author: patfer Date: 2020-10-01.10:44:49
Any volunteer for reviewing?

Todos:
Patrick -> Verify that code does what msg9579
Patrick -> check if one pushes, if the commit is accepted and then the action is tested (and possibly fails) or if the commit is just accepted AFTER the action succeeded.
Patrick -> Output warnings (discourage rules) only for the look hook (add to local hook)
Reviewer -> Review Code
msg9580 (view) Author: patfer Date: 2020-07-10.12:53:52
sorry for the spam.
Another description for how to read:
M, X, Y are branch names. M := main branch, X & Y := issue branches with X != Y
msg9579 (view) Author: patfer Date: 2020-07-10.12:49:59
We decided which rules to enforce for now (and might change this, if it turns 
out to be problematic for our workflow)

(read as Parent + Parent + ... > Merge Commit)
ENFORCE:
no merge with more than 2 parents
add branch name into commit message
M + M > M
M + M > X
M + Y > X
Y + M > X

DISCOURAGE:
X + X > X
X + Y > X
X > Y
msg9578 (view) Author: silvan Date: 2020-07-10.09:30:49
Not necessarily, because Y could be merged into main before X is finished.
msg9567 (view) Author: patfer Date: 2020-07-09.17:43:42
The script is slightly updated. The biggest update is ignoring rule violations 
for commits older than a fix date (of course we can decide if we want to 
ignore only certain rule violations and adapt the other rules).

Looking into the kind of violations, we get the following patterns:
we have some commits that do not follow the new rules
X and Y are always different from 'main'

150x merge main + main into main
32x merge X + X into X
7x merge main + main into X
6x merge X/main + Y into X
1x X + main into Y
5x X is branched of Y
Looking at those violations, I would prohibit all except for the last one.
About the last kind of violations, I have no strong opinion. It seems to be a 
rare use case that we branch away from an issue branch. If we decide to allow 
this, then would a rule "if X branched of Y, then X has to be merged into Y" 
make sense?
msg9561 (view) Author: patfer Date: 2020-07-09.14:46:18
Here is the PR for a first version of the server side checks (but you can also 
run them locally):
https://github.com/PatrickFerber/downward/pull/1/files

It is partially tested (I continue testing after having eaten something), but 
because of the few days left, I thought you might already want to see it.

Our repository is not fully compliant with the rules Malte stated in msg9380. 
I have attached the output it produces on my machine. The issues are:
- merging main with main
- merging main with something not called issue\d+
msg9559 (view) Author: silvan Date: 2020-07-09.14:41:56
Mention issue968.
msg9557 (view) Author: patfer Date: 2020-07-09.14:19:53
This is a subtask of issue973.
We want some tool that enforces the commit message conventions and optional 
further conventions in the git repository.

We do not have the possibility to use pre_receive hooks with GitHub. This 
would be possible with GitHub enterprise and then we have to run the GitHub 
software on our own machines (GitHub Enterprise might be included in our 
education license, but still we need to host it ourself).

The route for the common people is to setup an GitHub Action, use PullRequests 
and allow merging only if the defined action succeeds. 

I tested the protection mechanism. We can quite easily configure a repository 
to allow merging a PR only if the GitHub action succeeded. This also caused 
that I could not merge locally and push the change. Thus, you cannot 
circumvent the protection easily by just doing your merge locally (of course, 
we can always disable the protection).

I decided to use tox to implement the checks. All tests shipped with the 
repository are currently defined for tox. Thus, I prefer that the new tests 
(for history compliance) use the same framework and are located around the 
same directories.
The GitHub Action simply calls tox on push and pull request. (once I have an 
issue number I create a PR for reviewing)
History
Date User Action Args
2023-01-30 14:31:23silvansetstatus: in-progress -> resolved
messages: + msg10950
2021-03-04 18:11:30silvansetmessages: + msg10167
2020-10-01 10:44:49patfersetmessages: + msg9726
2020-07-10 12:53:52patfersetmessages: + msg9580
2020-07-10 12:50:00patfersetmessages: + msg9579
2020-07-10 09:30:49silvansetmessages: + msg9578
2020-07-09 17:43:42patfersetmessages: + msg9567
2020-07-09 14:46:19patfersetfiles: + history_check.log
messages: + msg9561
summary: TODO: this should be consistent with the commit message hook on client side introduced in issue968. ->
2020-07-09 14:41:56silvansetmessages: + msg9559
summary: TODO: this should be consistent with the commit message hook on client side introduced in issue968.
2020-07-09 14:19:53patfercreate