+ Mailman Developers, since this seems like a general discussion topic. On Thu, Aug 8, 2019, at 2:01 AM, Mike Gabriel wrote: > Hi Abhilash, > > I just looked into the two recent merges from Daniel on mailman's > master branch. > > I noticed that you squashed the MR branch commits into one commit > before the merge and that this one commit uses the latest commit > messages of the MR branch to describe what the squashed commits do. > Unfortunately, that commit messages does not appropriately describe > what the commit in fact does. > > The resulting commit on master for the merge of MR !543 is this: > > ``` > commit baf7fbca96cf77ae028740b668d79a906eb600ef > Author: Daniel Teichmann <dan...@teichm-sh.de> > Date: Thu Aug 8 01:25:05 2019 +0000 > > commands/tests/test_cli_members.py: Add testcase for del_infp > files containing commented and blank lines > > ```
This looks surprising to me, I was hoping that it would use the title of the MR as the commit message and description as the description of the commit. Looking a bit more closely at the documentation[1], it looks like it will pickup: > The squashed commit’s commit message will be either: > - Taken from the first multi-line commit message in the merge. > - The merge request’s title if no multi-line commit message is found. I am going to keep an eye out for the commit message in future before merging, thanks for pointing this out. [1]: https://docs.gitlab.com/ee/user/project/merge_requests/squash_and_merge.html > It contains the complete "mailman members --delete" code (all commits > from MR !543), but its commit message is the message of the top commit > that was on the MR branch. Commit content and commit message deviate. > > > As this merging strategy seems to be a sub-optimal strategy, I'd love > to discuss it for mailman3, if that's ok. > > So... > > IMHO, it would be much better (for the sake of transparency and code > change documentation) to either avoid commit squashing on merges > (iirc, it can be disabled in GitLab on a pre project level) or > explicitly demand from contributors to stuff all their code changes > into one commit and ship an appropriate commit msg. This could have happened a few times in the past because I didn't know Gitlab would pick a commit message with the multi-line comment. However, this is not a general pattern that I am trying to enforce here. It would be great to have single commit MRs, however, after the review, I'd prefer to have additional commits added to the MR as compared to sqashing + force push. Additional commits makes 2nd and 3rd reviews much more easy, since I can just look at the comments I made and the changes made in the newer commits. Finally, we want the MR to be merged as a single commit, so we use the Gitlab's sqash-merge feature. It works okay most of the times, like I mentioned above. > For big MRs the second option is really sub-optimal (esp. from a > (Debian) downstream maintainer's point of view): if I needed to > cherry-pick tiny changes on the code into a stable mailman3 release in > Debian, for example, then I'd love to have atomic changes to > cherry-pick from in the upstream Git. However, if all merges' commits > get squashed before being applied on master, then the changesets on > master become quite bulky and un-cherry-pickable. Ideally, MRs are a complete unit and it doesn't make sense IMO to cherry-pick only parts of the MR. MRs should be atomic. If there are two independent things being done in a single MR, it should ideally be split into separate MRs and hence get merged as separate commits. > > For future merge requests, would you prefer us to stuff all changes of > one MR into one commit? Or is it ok, to have several commits in one > MR? This would IMHO require from you to not squash the commits on > those MRs. I hope I answered this above, but in summary, please squash commits when your Open the MR, but please don't force-push when you make requested changed from the review. Also, we maintainers should be aware and just keep an eye out for the commit message. It is totally possible in Gitlab to change the Squash'd commit's message and in case Gitlab doesn't pick up the MR's title and desc, I'll try to pick it up manually. Does that sound reasonable? > Please let me know what your position is on this. Thanks! > > Mike > -- > > DAS-NETZWERKTEAM > c\o Technik- und Ökologiezentrum Eckernförde > Mike Gabriel, Marienthaler str. 17, 24340 Eckernförde > mobile: +49 (1520) 1976 148 > landline: +49 (4351) 850 8940 > > GnuPG Fingerprint: 9BFB AEE8 6C0A A5FF BF22 0782 9AF4 6B30 2577 1B31 > mail: mike.gabr...@das-netzwerkteam.de, http://das-netzwerkteam.de > > -- thanks, Abhilash Raj (maxking) _______________________________________________ Mailman-Developers mailing list -- mailman-developers@python.org To unsubscribe send an email to mailman-developers-le...@python.org https://mail.python.org/mailman3/lists/mailman-developers.python.org/ Mailman FAQ: https://wiki.list.org/x/AgA3 Security Policy: https://wiki.list.org/x/QIA9
