Folks,
Here's a summary of the Gitlab experiment discussion, with some additional comments. Thanks to everybody who sent their feedback!
* Things that people like:
- Fetching commits directly with git instead of applying patches from emails (Huw, Jacek, Paul, Zeb). Indeed that's a major help for me as well.
- Better tracker, easy to see the list of pending reviews (Huw).
- Possibility to host more Wine projects, as well as private Wine trees to share WIP patches (Jacek). Indeed it would be nice to have all Wine projects in one place instead of the current mix of github/sourceforge/etc.
- Potential for automation (Jacek). Gitlab offers many services that we will be able to take advantage of, the most obvious being CI for the testbot. Having all the data in a proper database instead of free-form emails should make it possible to do other interesting things as well.
* Some other things I like:
- Updating status doesn't need to go through me, people can assign reviewers, supersede patches, etc. directly. That reduces my workload and improves the bus factor. Once we have figured out how to make testbot results reliable, we could also have maintainers merge commits directly.
- The full discussion thread for a given MR is readily accessible, it doesn't require hunting down the multiple revisions of a patch and associated threads in the mailman archive.
* Things that could be improved:
- Signoffs are a bit cumbersome (Rémi). We should change the requirements to something better adapted to Gitlab.
- It's only possible to approve the whole MR, not individual commits (Huw, Zeb). I think that's an acceptable trade-off, but we could imagine other approaches.
- The mailing list gateway creates too much noise; mixing comments from Gitlab and mailing list isn't very clean (Jacek, Rémi, Alex). We can make some tweaks, or use a separate list, or even rethink the approach of the mail gateway.
- Gitlab threading support is limited, nested comment threads are not supported (Zeb). That's true, but looking through the past few months of patch reviews, it seems that we almost never use nested threads, so I think we can live with that limitation.
- Reviewers can push fixups to commits, but that requires the author to grant explicit permission (Jacek). Hopefully we can tweak access rights to allow this by default.
* Conclusion
I think Gitlab is working well for us, and most people seem generally happy with it. So my plan is to go forward and make Gitlab the main development platform for Wine.
I'll start working on the transition, and on the improvements mentioned above. Any help will be welcome! I'll be posting a roadmap shortly.
Hi,
Il 14/06/22 12:55, Alexandre Julliard ha scritto:
- Signoffs are a bit cumbersome (Rémi). We should change the requirements to something better adapted to Gitlab.
I don't remember if this point has already been made, but we're using the Signed-off-by pseudo-header in a rather peculiar fashion. The standard meaning of Signed-off-by is something like "I have checked that I have the legal right to submit this patch for inclusion" (more specifically, see [1]), while we use it as "I have checked that this patch is technically sound". Other projects use other pseudo-headers for the latter, like "Reviewed-by" or "Acked-by".
[1] https://developercertificate.org/
Could we take the occasion to switch to something more standard?
Giovanni.
On 14/06/2022 17:03, Giovanni Mascellani wrote:
Hi,
Il 14/06/22 12:55, Alexandre Julliard ha scritto:
- Signoffs are a bit cumbersome (Rémi). We should change the
requirements to something better adapted to Gitlab.
I don't remember if this point has already been made, but we're using the Signed-off-by pseudo-header in a rather peculiar fashion. The standard meaning of Signed-off-by is something like "I have checked that I have the legal right to submit this patch for inclusion" (more specifically, see [1]), while we use it as "I have checked that this patch is technically sound". Other projects use other pseudo-headers for the latter, like "Reviewed-by" or "Acked-by".
[1] https://developercertificate.org/
Could we take the occasion to switch to something more standard?
Giovanni.
Well the winehq wiki says:
`Your patch must include a Signed-off-by line. Signed-off-by means "I think this code is good enough to go into Wine." It is an assertion that your work meets all legal and technical requirements of Wine development.`
Note the `*legal* and technical requirements` part (emphasis added).
Giovanni Mascellani gmascellani@codeweavers.com writes:
Hi,
Il 14/06/22 12:55, Alexandre Julliard ha scritto:
- Signoffs are a bit cumbersome (Rémi). We should change the requirements to something better adapted to Gitlab.
I don't remember if this point has already been made, but we're using the Signed-off-by pseudo-header in a rather peculiar fashion. The standard meaning of Signed-off-by is something like "I have checked that I have the legal right to submit this patch for inclusion" (more specifically, see [1]), while we use it as "I have checked that this patch is technically sound". Other projects use other pseudo-headers for the latter, like "Reviewed-by" or "Acked-by".
[1] https://developercertificate.org/
Could we take the occasion to switch to something more standard?
Signed-off-by is convenient because the git tools have direct support for it, but the real motivation for us was to keep track of who submitted the patch, to support sending a patch written by someone else. In that case the convention with email is to set From: to the original author, but the committer info is lost, hence the Signed-off-by.
With merge requests, the information of who submitted the merge request is preserved independently of the commit contents, so the easiest option is to simply get rid of signoffs completely.
Another option would be to require the submitter to explicitly approve their own MR if it contains a patch written by someone else; that would be similar to a reviewer pushing fixups and then approving the MR. It would require some mechanism to detect missing approvals though, so it may not be worth the trouble.
Thoughts?
Hi,
Il 14/06/22 22:01, Alexandre Julliard ha scritto:
Signed-off-by is convenient because the git tools have direct support for it, but the real motivation for us was to keep track of who submitted the patch, to support sending a patch written by someone else. In that case the convention with email is to set From: to the original author, but the committer info is lost, hence the Signed-off-by.
With merge requests, the information of who submitted the merge request is preserved independently of the commit contents, so the easiest option is to simply get rid of signoffs completely.
If that was the only intended meaning of the SOBs, then yes, I would say than now we have a better options and we could get rid of them.
Right now the SOBs also help keeping a trace of who had a look at the patch and said it was good, which might be an interesting information to keep tracking. My point is that we shouldn't be using SOB for that, because its standard meaning is another one.
In other words, there are a few different pieces of information that it can make sense to track for a certain commit: * who authored it; * who committed it; * who reviewed it from the technical point of view; * who checked that it's ok from the copyright point of view to have it in Wine.
For any of these we can (in principle) choose whether we want it or not, but each of them should be recorded in a way that is aligned with the standard best practices.
Giovanni.
Giovanni Mascellani gmascellani@codeweavers.com writes:
Hi,
Il 14/06/22 22:01, Alexandre Julliard ha scritto:
Signed-off-by is convenient because the git tools have direct support for it, but the real motivation for us was to keep track of who submitted the patch, to support sending a patch written by someone else. In that case the convention with email is to set From: to the original author, but the committer info is lost, hence the Signed-off-by. With merge requests, the information of who submitted the merge request is preserved independently of the commit contents, so the easiest option is to simply get rid of signoffs completely.
If that was the only intended meaning of the SOBs, then yes, I would say than now we have a better options and we could get rid of them.
Right now the SOBs also help keeping a trace of who had a look at the patch and said it was good, which might be an interesting information to keep tracking. My point is that we shouldn't be using SOB for that, because its standard meaning is another one.
With Gitlab, this is already replaced by approvals. They are stored in the merge request, and in the "Approved-by" field in the git notes.
On Tue, Jun 14, 2022 at 10:01:23PM +0200, Alexandre Julliard wrote:
Another option would be to require the submitter to explicitly approve their own MR if it contains a patch written by someone else; that would be similar to a reviewer pushing fixups and then approving the MR. It would require some mechanism to detect missing approvals though, so it may not be worth the trouble.
Could the (yet to be written) auto-add-reviewer script check whether the submitter and author are different and in that case auto add the submitter as a reviewer (as well as any other appropriate reviewers)? Then the approval check just becomes the same as for regular reviewers.
Huw.
Huw Davies huw@codeweavers.com writes:
On Tue, Jun 14, 2022 at 10:01:23PM +0200, Alexandre Julliard wrote:
Another option would be to require the submitter to explicitly approve their own MR if it contains a patch written by someone else; that would be similar to a reviewer pushing fixups and then approving the MR. It would require some mechanism to detect missing approvals though, so it may not be worth the trouble.
Could the (yet to be written) auto-add-reviewer script check whether the submitter and author are different and in that case auto add the submitter as a reviewer (as well as any other appropriate reviewers)? Then the approval check just becomes the same as for regular reviewers.
Yes, that would be a way of doing it, if we decide that we want that info in the git notes. Do you feel that it would be useful to have it there?
On Wed, Jun 15, 2022 at 04:06:03PM +0200, Alexandre Julliard wrote:
Huw Davies huw@codeweavers.com writes:
On Tue, Jun 14, 2022 at 10:01:23PM +0200, Alexandre Julliard wrote:
Another option would be to require the submitter to explicitly approve their own MR if it contains a patch written by someone else; that would be similar to a reviewer pushing fixups and then approving the MR. It would require some mechanism to detect missing approvals though, so it may not be worth the trouble.
Could the (yet to be written) auto-add-reviewer script check whether the submitter and author are different and in that case auto add the submitter as a reviewer (as well as any other appropriate reviewers)? Then the approval check just becomes the same as for regular reviewers.
Yes, that would be a way of doing it, if we decide that we want that info in the git notes. Do you feel that it would be useful to have it there?
If you're sending in someone else's work then you're essentially acting as a reviewer, so it seems logical to add an Approved-by tag in the notes. Of course I'm not the one writing the bot, so if it's a pain then perhaps it's not worth the hassle as this doesn't happen that often.
Huw.
Huw Davies huw@codeweavers.com writes:
On Wed, Jun 15, 2022 at 04:06:03PM +0200, Alexandre Julliard wrote:
Huw Davies huw@codeweavers.com writes:
On Tue, Jun 14, 2022 at 10:01:23PM +0200, Alexandre Julliard wrote:
Another option would be to require the submitter to explicitly approve their own MR if it contains a patch written by someone else; that would be similar to a reviewer pushing fixups and then approving the MR. It would require some mechanism to detect missing approvals though, so it may not be worth the trouble.
Could the (yet to be written) auto-add-reviewer script check whether the submitter and author are different and in that case auto add the submitter as a reviewer (as well as any other appropriate reviewers)? Then the approval check just becomes the same as for regular reviewers.
Yes, that would be a way of doing it, if we decide that we want that info in the git notes. Do you feel that it would be useful to have it there?
If you're sending in someone else's work then you're essentially acting as a reviewer, so it seems logical to add an Approved-by tag in the notes.
Makes sense.
Of course I'm not the one writing the bot, so if it's a pain then perhaps it's not worth the hassle as this doesn't happen that often.
Well, I volunteered Jer to write the bot, so I'm sure he will enjoy a little challenge ;-)
On 6/14/2022 5:55 AM, Alexandre Julliard wrote:
- The mailing list gateway creates too much noise; mixing comments from Gitlab and mailing list isn't very clean (Jacek, Rémi, Alex). We can make some tweaks, or use a separate list, or even rethink the approach of the mail gateway.
I'd be in favor of a new mailing list for this. I think all this noise on wine-devel has stopped all other technical discussion here. With the communication being done in Gitlab itself. I know some deplore web interfaces, so we will need need to keep some form of terminal based communication for them.
If we go with a new list, let me get mailman3 setup on WineHQ first. We need to migrate to it anyway, and it would be one less list to convert.
-N
On 6/14/22 09:34, Jeremy Newman wrote:
On 6/14/2022 5:55 AM, Alexandre Julliard wrote:
- The mailing list gateway creates too much noise; mixing comments from
Gitlab and mailing list isn't very clean (Jacek, Rémi, Alex). We can make some tweaks, or use a separate list, or even rethink the approach of the mail gateway.
I'd be in favor of a new mailing list for this. I think all this noise on wine-devel has stopped all other technical discussion here. With the communication being done in Gitlab itself. I know some deplore web interfaces, so we will need need to keep some form of terminal based communication for them.
For what it's worth, as someone who will not use the Gitlab UI, I don't "deplore web interfaces"; some work quite well for me. The problem is that Gitlab's web interface is terrible, borne out of mobile-first and "flat" UI—too much junk in the screen, too much padding, controls on every side of the screen without any pattern, incomprehensible symbols in place of words, infinite scrolling, sluggish and memory-heavy JavaScript...
If someone were to design a reasonable UI for Gitlab, I would probably use it. Since the switch is inevitable I may end up wasting some of my time doing so myself.
Jeremy Newman jnewman@codeweavers.com writes:
On 6/14/2022 5:55 AM, Alexandre Julliard wrote:
- The mailing list gateway creates too much noise; mixing comments from Gitlab and mailing list isn't very clean (Jacek, Rémi, Alex). We can make some tweaks, or use a separate list, or even rethink the approach of the mail gateway.
I'd be in favor of a new mailing list for this. I think all this noise on wine-devel has stopped all other technical discussion here. With the communication being done in Gitlab itself. I know some deplore web interfaces, so we will need need to keep some form of terminal based communication for them.
I've now created a wine-gitlab list for this purpose, we'll see if that works better. The list information is here:
https://www.winehq.org/mailman3/postorius/lists/wine-gitlab.winehq.org/
Please subscribe if you want to continue receiving emails from the Gitlab bridge. Currently mails are sent to both wine-gitlab and wine-devel, but once I've confirmed that everything is working I'll switch off the wine-devel side.
Alexandre Julliard julliard@winehq.org writes:
Jeremy Newman jnewman@codeweavers.com writes:
On 6/14/2022 5:55 AM, Alexandre Julliard wrote:
- The mailing list gateway creates too much noise; mixing comments from Gitlab and mailing list isn't very clean (Jacek, Rémi, Alex). We can make some tweaks, or use a separate list, or even rethink the approach of the mail gateway.
I'd be in favor of a new mailing list for this. I think all this noise on wine-devel has stopped all other technical discussion here. With the communication being done in Gitlab itself. I know some deplore web interfaces, so we will need need to keep some form of terminal based communication for them.
I've now created a wine-gitlab list for this purpose, we'll see if that works better. The list information is here:
https://www.winehq.org/mailman3/postorius/lists/wine-gitlab.winehq.org/
Please subscribe if you want to continue receiving emails from the Gitlab bridge. Currently mails are sent to both wine-gitlab and wine-devel, but once I've confirmed that everything is working I'll switch off the wine-devel side.
Mail delivery to wine-devel is now switched off. Make sure to subscribe to wine-gitlab if you want to receive patches through email.
1. Should the marvin's test reports also be moved to wine-gitlab instead of wine-devel? 2. Should this page (https://www.winehq.org/forums) have wine-gitlab listed?
On 7/12/22 10:59 AM, Biswapriyo Nath wrote:
- Should the marvin's test reports also be moved to wine-gitlab
instead of wine-devel?
I'm in favor of creating a new list for those. This way everyone can pick and choose what they want to subscribe to, and also reducing the amount of mail we send.
- Should this page (https://www.winehq.org/forums) have wine-gitlab
listed?
Yes, and I can do that.
-N
Jeremy Newman jnewman@codeweavers.com writes:
On 7/12/22 10:59 AM, Biswapriyo Nath wrote:
- Should the marvin's test reports also be moved to wine-gitlab
instead of wine-devel?
I'm in favor of creating a new list for those. This way everyone can pick and choose what they want to subscribe to, and also reducing the amount of mail we send.
Actually the testbot jobs should be run from a Gitlab CI pipeline, and the results reported there. Then people will get failure notifications from Gitlab according to their notification settings.
Hi,
On 14/06/2022 13:55, Alexandre Julliard wrote:
- Some other things I like:
- Updating status doesn't need to go through me, people can assign reviewers, supersede patches, etc. directly. That reduces my workload and improves the bus factor. Once we have figured out how to make testbot results reliable, we could also have maintainers merge commits directly.
I created my first MR since the switch seems inevitable. I completely fail at assigning a reviewer. I even tried the web UI and I can't find any setting.
I also tried the /assign_reviewer "quick action" on a comment and it said:
`Could not apply assign_reviewer command.`
Is this feature not working currently?
Thanks, Gabriel
Gabriel Ivăncescu gabrielopcode@gmail.com writes:
Hi,
On 14/06/2022 13:55, Alexandre Julliard wrote:
- Some other things I like:
- Updating status doesn't need to go through me, people can assign reviewers, supersede patches, etc. directly. That reduces my workload and improves the bus factor. Once we have figured out how to make testbot results reliable, we could also have maintainers merge commits directly.
I created my first MR since the switch seems inevitable. I completely fail at assigning a reviewer. I even tried the web UI and I can't find any setting.
I also tried the /assign_reviewer "quick action" on a comment and it said:
`Could not apply assign_reviewer command.`
Is this feature not working currently?
You need to be a project member to modify reviewers. There should be a link at the top of the project page to request access.
On 6/14/22 15:29, Gabriel Ivăncescu wrote:
Hi,
On 14/06/2022 13:55, Alexandre Julliard wrote:
- Some other things I like:
- Updating status doesn't need to go through me, people can assign
reviewers, supersede patches, etc. directly. That reduces my workload and improves the bus factor. Once we have figured out how to make testbot results reliable, we could also have maintainers merge commits directly.
I created my first MR since the switch seems inevitable. I completely fail at assigning a reviewer. I even tried the web UI and I can't find any setting.
I also tried the /assign_reviewer "quick action" on a comment and it said:
`Could not apply assign_reviewer command.`
Is this feature not working currently?
FWIW, there's a "lab" command line tool that lets you do
lab mr edit --assign <username>
and also
lab mr edit --review <username>
and I'm sure the "glab" has something with identical syntax.
Not sure what the difference between those are, though?
On 15/06/2022 01:16, Zebediah Figura wrote:
On 6/14/22 15:29, Gabriel Ivăncescu wrote:
Hi,
On 14/06/2022 13:55, Alexandre Julliard wrote:
- Some other things I like:
- Updating status doesn't need to go through me, people can assign
reviewers, supersede patches, etc. directly. That reduces my workload and improves the bus factor. Once we have figured out how to make testbot results reliable, we could also have maintainers merge commits directly.
I created my first MR since the switch seems inevitable. I completely fail at assigning a reviewer. I even tried the web UI and I can't find any setting.
I also tried the /assign_reviewer "quick action" on a comment and it said:
`Could not apply assign_reviewer command.`
Is this feature not working currently?
FWIW, there's a "lab" command line tool that lets you do
lab mr edit --assign <username>
and also
lab mr edit --review <username>
and I'm sure the "glab" has something with identical syntax.
Not sure what the difference between those are, though?
Thanks. These tools make it a lot more bearable.
Also I think "assign" is for someone who's working on the code (for example when a reviewer actually got to it, or when you're refactoring code based on reviews and taking a while). But can't say for sure.
There's one other (pretty big, for me) problem I can't seem to find how to replicate with MRs compared to sending patches: how do I add notes for each commit that shouldn't actually be committed? For patches I used to add below the --- line, and these are super useful when you just want to tell the information to the reviewer, which wouldn't make much sense to have in the codebase itself.
These seem to get lost when I push. I have to admit `git notes` seems pretty convoluted to me and I've no idea how it gets "stored" especially for merge requests. Patches were much easier to comprehend.
I mean, I guess I can add it to the MR description but that's not pointing out to a specific commit/patch... sigh.
On Thu, Jun 16, 2022 at 10:04 AM Gabriel Ivăncescu gabrielopcode@gmail.com wrote:
There's one other (pretty big, for me) problem I can't seem to find how to replicate with MRs compared to sending patches: how do I add notes for each commit that shouldn't actually be committed? For patches I used to add below the --- line, and these are super useful when you just want to tell the information to the reviewer, which wouldn't make much sense to have in the codebase itself.
These seem to get lost when I push. I have to admit `git notes` seems pretty convoluted to me and I've no idea how it gets "stored" especially for merge requests. Patches were much easier to comprehend.
I mean, I guess I can add it to the MR description but that's not pointing out to a specific commit/patch... sigh.
In a GitLab merge request, I can click on a specific commit to see its diff, then click on the commit hash again, and GitLab gives me a box to leave a comment either about a specific line of the commit or about the commit as a whole. Would that work for you?
-Alex
On 16/06/2022 19:33, Alex Henrie wrote:
On Thu, Jun 16, 2022 at 10:04 AM Gabriel Ivăncescu gabrielopcode@gmail.com wrote:
There's one other (pretty big, for me) problem I can't seem to find how to replicate with MRs compared to sending patches: how do I add notes for each commit that shouldn't actually be committed? For patches I used to add below the --- line, and these are super useful when you just want to tell the information to the reviewer, which wouldn't make much sense to have in the codebase itself.
These seem to get lost when I push. I have to admit `git notes` seems pretty convoluted to me and I've no idea how it gets "stored" especially for merge requests. Patches were much easier to comprehend.
I mean, I guess I can add it to the MR description but that's not pointing out to a specific commit/patch... sigh.
In a GitLab merge request, I can click on a specific commit to see its diff, then click on the commit hash again, and GitLab gives me a box to leave a comment either about a specific line of the commit or about the commit as a whole. Would that work for you?
-Alex
I see, the click on the hash again is definitely more convoluted than I intuitively expected, but at least there's a way to add such comment. It would be super if it could be automated (scripted) though. I hate clicking all over the place.
At least I found that a quick URL to get to such commit is:
https://gitlab.winehq.org/wine/wine/-/commit/<commit_sha>?merge_request_iid=<mr id>
That said I still have to copy and paste it manually to gitlab's web UI, which also requires logging in and loads slowly due to javascript bloat...
Is there a way to add this kind of comment with glab-cli?
Alex Henrie alexhenrie24@gmail.com writes:
On Thu, Jun 16, 2022 at 10:04 AM Gabriel Ivăncescu gabrielopcode@gmail.com wrote:
There's one other (pretty big, for me) problem I can't seem to find how to replicate with MRs compared to sending patches: how do I add notes for each commit that shouldn't actually be committed? For patches I used to add below the --- line, and these are super useful when you just want to tell the information to the reviewer, which wouldn't make much sense to have in the codebase itself.
These seem to get lost when I push. I have to admit `git notes` seems pretty convoluted to me and I've no idea how it gets "stored" especially for merge requests. Patches were much easier to comprehend.
I mean, I guess I can add it to the MR description but that's not pointing out to a specific commit/patch... sigh.
In a GitLab merge request, I can click on a specific commit to see its diff, then click on the commit hash again, and GitLab gives me a box to leave a comment either about a specific line of the commit or about the commit as a whole. Would that work for you?
Unfortunately comments tied to a specific commit are no longer visible when the branch is rebased, for instance when a reviewer pushes fixups. There's a issue filed with Gitlab about that, but until this is fixed it's better to avoid commit-specific comments.
On 16/06/2022 20:13, Alexandre Julliard wrote:
Alex Henrie alexhenrie24@gmail.com writes:
On Thu, Jun 16, 2022 at 10:04 AM Gabriel Ivăncescu gabrielopcode@gmail.com wrote:
There's one other (pretty big, for me) problem I can't seem to find how to replicate with MRs compared to sending patches: how do I add notes for each commit that shouldn't actually be committed? For patches I used to add below the --- line, and these are super useful when you just want to tell the information to the reviewer, which wouldn't make much sense to have in the codebase itself.
These seem to get lost when I push. I have to admit `git notes` seems pretty convoluted to me and I've no idea how it gets "stored" especially for merge requests. Patches were much easier to comprehend.
I mean, I guess I can add it to the MR description but that's not pointing out to a specific commit/patch... sigh.
In a GitLab merge request, I can click on a specific commit to see its diff, then click on the commit hash again, and GitLab gives me a box to leave a comment either about a specific line of the commit or about the commit as a whole. Would that work for you?
Unfortunately comments tied to a specific commit are no longer visible when the branch is rebased, for instance when a reviewer pushes fixups. There's a issue filed with Gitlab about that, but until this is fixed it's better to avoid commit-specific comments.
I might go with a normal comment (after a MR is created or after a force push for e.g. v2), with something like:
**Notes for `ntdll: Foobar`**
Notes go here. Multiple lines. etc.
**Notes for `server: Barfoo`**
v2: blah.
and so on. Which can also be scripted (and easily taken from own git notes). For example with something like:
git log --reverse --pretty='format:**Notes for `%s`**%n%n%N%n%n' origin..HEAD
Maybe we can standardize on a common method to encourage and put in the wiki? Well, or maybe it's just me.
On Thu, Jun 16, 2022 at 09:08:30PM +0300, Gabriel Ivăncescu wrote:
On 16/06/2022 20:13, Alexandre Julliard wrote:
Alex Henrie alexhenrie24@gmail.com writes:
On Thu, Jun 16, 2022 at 10:04 AM Gabriel Ivăncescu gabrielopcode@gmail.com wrote:
There's one other (pretty big, for me) problem I can't seem to find how to replicate with MRs compared to sending patches: how do I add notes for each commit that shouldn't actually be committed? For patches I used to add below the --- line, and these are super useful when you just want to tell the information to the reviewer, which wouldn't make much sense to have in the codebase itself.
These seem to get lost when I push. I have to admit `git notes` seems pretty convoluted to me and I've no idea how it gets "stored" especially for merge requests. Patches were much easier to comprehend.
I mean, I guess I can add it to the MR description but that's not pointing out to a specific commit/patch... sigh.
In a GitLab merge request, I can click on a specific commit to see its diff, then click on the commit hash again, and GitLab gives me a box to leave a comment either about a specific line of the commit or about the commit as a whole. Would that work for you?
Unfortunately comments tied to a specific commit are no longer visible when the branch is rebased, for instance when a reviewer pushes fixups. There's a issue filed with Gitlab about that, but until this is fixed it's better to avoid commit-specific comments.
I might go with a normal comment (after a MR is created or after a force push for e.g. v2), with something like:
**Notes for `ntdll: Foobar`**
Notes go here. Multiple lines. etc.
**Notes for `server: Barfoo`**
v2: blah.
and so on. Which can also be scripted (and easily taken from own git notes). For example with something like:
git log --reverse --pretty='format:**Notes for `%s`**%n%n%N%n%n' origin..HEAD
Maybe we can standardize on a common method to encourage and put in the wiki? Well, or maybe it's just me.
Feel free to do that, but it sounds over-engineered to me and I don't think we need to standardise this.
Personally, I've always found these sort of comments somewhat fragile. When I'm reviewing a series I'm reviewing it in-tree, at which point the comments have gone. Putting them in one place (in the equivalent of a cover-letter) means there's more of a chance that I'll read them.
Huw.
-
Alex Henrie -
Alexandre Julliard -
Biswapriyo Nath -
Gabriel Ivăncescu -
Giovanni Mascellani -
Huw Davies -
Jeremy Newman -
Zebediah Figura