How to add credit to non-committers authors #109438

New Issue

Bastien Montagne · 2023-06-28T12:31:53+02:00

Bastien Montagne commented

2023-06-28 12:31:53 +02:00

Current Issue

The current automated script used to generate credits for Blender only takes into account authors of commits as stored in the GIT repository.

This means that we have no way to add co-authors, people who submitted a patch but could not commit it (although this case is less of an issue with PRs), etc.

E.g. @super_jo_nathan is not the only author of the new C++ PLY I/O code added to Blender 3.6 (43e9c90061), the whole team of students should be credited.

Ideas

This can be solved in many ways, depending on how automated, future-proof and simple-to-use we want to keep this credit-generation process. here are some ideas:

Manually edit the credit file after automated generation from the script. This would have to done every time the script is executed.
Parse the commit messages for some specifically formatted lines identifying co-authors.
Have a text file in the Blender git repositories, listing extra authors for the commits that need it.

Solution 1. is mainly listed here for sake of exhaustivity, it does not seem like a good way to handle the problem, as it would add a noticeable overhead to release managers, and would be extremely error-prone.

Solution 2. is fairly simple to implement, and is the less intrusive of all. Its down-sides are that it requires a relatively strict formatting of the commit messages, and, the biggest drawback, that commit messages are not editable. In other words, it is practically impossible to fix a mistake once the commit is pushed, or the PR merged.

Solution 3. is a bit more intrusive and less straight-forward as solution 2., since it requires editing of an external file. Further more, it needs to be edited and committed after the actual feature, to have a valid commit hash to associate with the co-authors. However, a big plus is that it can be edited afterwards, to add missing people, fix mistakes, etc.

Proposal:

The main way to support listing co-authors will be to parse the Co-authored-by: formatted lines in commit messages (see also e.g. this page). #109468, 56e9abf133
In addition, an internal mapping between commits and authors will be added to the tool, for the rare cases where author-related information extracted from the git commits needs to be fixed. #109468, 56e9abf133
The documentation should be updated.

# Current Issue The [current automated script](https://projects.blender.org/blender/blender/src/branch/main/tools/utils/credits_git_gen.py) used to generate credits for Blender only takes into account authors of commits as stored in the GIT repository. This means that we have no way to add co-authors, people who submitted a patch but could not commit it (although this case is less of an issue with PRs), etc. E.g. @super_jo_nathan is not the only author of the new C++ PLY I/O code added to Blender 3.6 (43e9c90061fc1ebae), the whole team of students should be credited. # Ideas This can be solved in many ways, depending on how automated, future-proof and simple-to-use we want to keep this credit-generation process. here are some ideas: 1. Manually edit the credit file after automated generation from the script. This would have to done every time the script is executed. 2. Parse the commit messages for some specifically formatted lines identifying co-authors. 3. Have a text file in the Blender git repositories, listing extra authors for the commits that need it. Solution `1.` is mainly listed here for sake of exhaustivity, it does not seem like a good way to handle the problem, as it would add a noticeable overhead to release managers, and would be extremely error-prone. Solution `2.` is fairly simple to implement, and is the less intrusive of all. Its down-sides are that it requires a relatively strict formatting of the commit messages, and, the biggest drawback, that commit messages are not editable. In other words, it is practically impossible to fix a mistake once the commit is pushed, or the PR merged. Solution `3.` is a bit more intrusive and less straight-forward as solution `2.`, since it requires editing of an external file. Further more, it needs to be edited and committed _after_ the actual feature, to have a valid commit hash to associate with the co-authors. However, a big plus is that it can be edited afterwards, to add missing people, fix mistakes, etc. # Proposal: * [x] The main way to support listing co-authors will be to parse the `Co-authored-by:` formatted lines in commit messages (see also e.g. [this page](https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors)). #109468, 56e9abf133 * [x] In addition, an internal mapping between commits and authors will be added to the tool, for the rare cases where author-related information extracted from the git commits needs to be fixed. #109468, 56e9abf133 * [x] The [documentation](https://wiki.blender.org/wiki/Style_Guide/Commit_Messages#Author.28s.29_.2F_Committer) should be updated.

Bastien Montagne added the

Module

Platforms, Builds & Tests

Type

Design

labels 2023-06-28 12:31:54 +02:00

Bastien Montagne added this to the Platforms, Builds Tests & Devices project 2023-06-28 12:31:55 +02:00

Bastien Montagne commented

2023-06-28 12:32:59 +02:00

Pokes to @dfelinto @brecht @ideasman42 @Sergey @ThomasDinges

Thomas Dinges commented

2023-06-28 12:50:33 +02:00

Thanks for the design task Bastien!

Nothing to add for solution 1., this is not an option for me.

Both 2. and 3. can work though. How about a mix of these two?

The script checks the commit message for co-authors (needs standardized formatting of course)
Additionally it checks if there is a reference to that commit in the extra file. If there is, it takes the info from the extra file instead.

This way there would be no additional work (maintaining an extra file), unless we want to fix a mistake or have co-authors for already existing commits.

Thanks for the design task Bastien! Nothing to add for solution `1.`, this is not an option for me. Both `2.` and `3.` can work though. How about a mix of these two? * The script checks the commit message for co-authors (needs standardized formatting of course) * Additionally it checks if there is a reference to that commit in the extra file. If there is, it takes the info from the extra file instead. This way there would be no additional work (maintaining an extra file), unless we want to fix a mistake or have co-authors for already existing commits.

👍 1

Campbell Barton commented

2023-06-28 13:35:52 +02:00

Some notes:

1. not an option, but there are some possible low-overhead alternatives such as adding users,year-ranges & commits without the complications of mapping contributors to every SHA1.
2. may be reasonable, {blank-line}Includes contributions by {comma separated usernames}., shouldn't be so difficult to parse (maybe some variations on this wording, but keep it limited). EDIT (I wasn't aware of Co-authored-by: convention, better use that)
3. would prefer 2, 3 could be added too but more as a fallback.

Note that when we used GForge + SVN it was possible to export a CSV of contributors (which the credits generator supported at the time), if there is a way of manually assigning developers - we could extract the authors for patches and set them as author overrides. I recall some developers weren't happy to be removed from the credits when moving to Phabricator.

Some notes: * `1.` not an option, but there are some possible low-overhead alternatives such as adding users,year-ranges & commits without the complications of mapping contributors to every SHA1. * `2.` may be reasonable, `{blank-line}Includes contributions by {comma separated usernames}.`, shouldn't be so difficult to parse (maybe some variations on this wording, but keep it limited). **EDIT** _(I wasn't aware of `Co-authored-by:` convention, better use that)_ * `3.` would prefer 2, 3 could be added too but more as a fallback. ---- Note that when we used GForge + SVN it was possible to export a CSV of contributors (which the credits generator supported at the time), if there is a way of manually assigning developers - we could extract the authors for patches and set them as author overrides. I recall some developers weren't happy to be removed from the credits when moving to Phabricator.

Brecht Van Lommel commented

2023-06-28 13:43:10 +02:00

I think we should do 2) with the standard Co-authored-by: syntax various tools and platforms already understand. Gitea will automatically add this when squashing commits from multiple authors.

We can add a list of authors that were forgotten in commits in credits_git_gen.py, for the case where it was forgotten. But I don't think we have to associate specific commits there.

I think we should do 2) with the standard `Co-authored-by:` syntax various tools and platforms already understand. Gitea will automatically add this when squashing commits from multiple authors. We can add a list of authors that were forgotten in commits in `credits_git_gen.py`, for the case where it was forgotten. But I don't think we have to associate specific commits there.

Thomas Dinges commented

2023-06-28 14:34:53 +02:00

@brecht Strictly speaking, the output of the script depends on the range it's given. If I for example ask the script to give me all commits between 3.5 and 3.6, it should only give me these and not always append an extra list of names.

Brecht Van Lommel commented

2023-06-28 14:42:11 +02:00

Ok, adding a commit hash for that purpose seems fine.

Bastien Montagne referenced this issue

2023-06-28 20:24:49 +02:00

Update `credits_git_gen` tool to also parse co-authors in commit messages, #109468

Bastien Montagne commented

2023-06-28 20:28:51 +02:00

Thanks for the feedback, updated the proposal and added a PR with proposed implementation: #109468

Bastien Montagne commented

2023-06-29 11:28:48 +02:00

FYI also updated to documentation to reflect changes discussed here.

FYI also updated to [documentation](https://wiki.blender.org/wiki/Style_Guide/Commit_Messages#Author.28s.29_.2F_Committer) to reflect changes discussed here.

Bastien Montagne referenced this issue from a commit

2023-06-30 14:53:00 +02:00

Update `credits_git_gen` tool to also parse co-authors in commit messages,

Bastien Montagne commented

2023-06-30 14:54:35 +02:00

Committed the changes to main, will add it to LTSs backports lists as well. Thanks.

Bastien Montagne closed this issue

2023-06-30 14:54:37 +02:00

Blender Bot added the

Status

Archived

label 2023-06-30 14:54:38 +02:00