Open kubark42 opened 3 years ago
kubark42
commented
3 years ago Branching and merging is also cleaner, as it allows code readers to understand what feature set a commit belongs to, which is sometimes crucial for placing changes in context:
Lastly, the GitHub process also allows all code to be automatically passed through continuous integration and build, before it is merged.
kalvdans
commented
3 years ago Taking 8cf082e as an example, subsequent discussion is impossible
Just pointing out that you can comment on a specific commit in gitlab, if we choose to do that way. I'm however all for forcing a PR even if they are merged without review.
kubark42
commented
3 years ago Indeed you can! But unless GitHub has changed since the last time I tried that, the ensuing conversation is somewhat unmoored and thus neither linkable nor searchable. I suppose an issue could be opened to call attention to a particular line, but it's somewhat awkward in my experience.
vedderb
commented
3 years ago This could be done at some point if the project grows more and there are more active developers, but at the moment it would be a bit much for me as there is almost no one else who can understand and test the effect of changes.
In general all my commits are only done after confirming that they work and make a measurable and worthwhile difference. https://github.com/vedderb/bldc/commit/8cf082e0e00b1936be3143011dc8823d59e62c75 is an example of that, as it significantly decreased the effect of dead-time distortion under different modulations and switching frequencies in the setup I was working on. Of course I can miss something in my testing, and the best way I have found for getting feedback from others is making regular test builds like this one for people to try out changes without having to set up their development environment: https://vesc-project.com/node/2859 Then, before a release, I will not make any changes for a week or so and just focus on testing to make sure that everything works as well as it can without any dangerous regressions.
The policy of testing and confirming that things are working and make worthwhile improvements is important for moving the project forward. For example, to merge PR https://github.com/vedderb/bldc/pull/370 I have to get a setup with an analog throttle and make sure that it still works as it even states that it is completely untested. Even if it works and saves the 6000 FPU operations per second, in the worst case with the highest loop rate, that is an improvement of (6000 / 168 000 000) * 100 = 0.0036 %. The extra work of testing this and risking to break things for super tiny performance improvements is not really worth it at the moment.
What I'm struggling with the most is that there is so much that has to be done and my time is not nearly enough, so I'm trying to get the most out of the time I put in.
kubark42
commented
3 years ago Gotcha. Let me propose this workflow: the commits you would normally make directly are instead added to a new branch, hypothetically called kitchen_sink_XXX, and then that is pushed, PR'ed, and merged all in the same minute?
It would have at most a marginal time cost, but it would give 80% of the benefits. In particular, it would give a good anchor to the code where discussions can occur, and context can be given.
For instance, the link between https://github.com/vedderb/bldc/commit/e9cf6eb4e8ed86b6dacba216604b2c205af177a0 and https://github.com/vedderb/bldc/pull/370 is not obvious, so to a third-party reading the code commit it would be hard to understand the "why" of the code change. If there had been a PR, then those could have been cross-linked afterwards.
The advantage of this workflow is that there is no time pressure on you, we other contributors can fill in the missing links via GitHub.
Currently, many code modifications are merged directly into the codebase without going through the public GitHub review process.
One of the consequences is that commits sometimes have a kitchen-sink feel, where they address many seemingly unrelated problems at once. Without the public review, there isn't much opportunity to discuss the changes and understand what's motivating them. Whereas with public discussion, better documentation for the various changes arises automatically.
Another advantage of the public review process is that comments can continue to be made even after the code is merged. Taking https://github.com/vedderb/bldc/commit/8cf082e0e00b1936be3143011dc8823d59e62c75 as an example, subsequent discussion is impossible so if a reader is interested in why those changes were made it becomes a dead end.
Why we care
This kind of visibility is essential to users who depend on the firmware for safety- or mission-critical applications. In those cases, an in-house an review of all modifications is a hard requirement before pushing the code to deployed hardware. The code reader is left to sparse git commit comments and code introspection. This is a lengthy process, and all too frequently the result is delaying rolling out upgrades.
If all code commits have a public-facing anchor, then this improves the efficiency of the req'd due diligence. The result benefits all users of the project.