• 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 <daniel@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 documentation1, 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.

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.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de

-- thanks, Abhilash Raj (maxking)