Code review comment for ~michal-maloszewski99/ubuntu/+source/amavisd-new:merge-1.2.13.0-3_mantic

Revision history for this message
Bryce Harrington (bryce) wrote :

Leaving the formal review to Christian since he owns the review slot, but since he's on PTO today I can give some drive-by review comments:

- In the debian/changelog the first line should include the merge bug number, LP: #2018057

- 0c5c7cf: In changelog entries, I strongly recommend stating *what* was changed and/or *why* it was changed. In this case, "Add information to the README.Debian about Ubuntu specific changes" is too vague. I know you didn't write this and are simply carrying forward the delta (correctly), but especially for longstanding delta it is worth adding some clarity. Sometimes in doing that it gives yourself or future mergers a clue about how the delta can eventually be retired.

  So in this case, look at the change applied to the README.Debian, and update the changelog entry to mention that the Ubuntu specific change was something to do with DKIM. In doing this, you may also find this change goes along with one of the other commits that actually implements the described change, and can be squashed to that one.

- f2f64b5: The changelog entry for this can also be made less vague by mentioning the packages by name (this can help future mergers who may be grepping through the changelog), and by stating the reason for *why* they are missing. In this case because the packages in question aren't provided by Debian or were removed in Debian. It would also be useful to annotate this commit (using the "--CL--" marker trick) and mention this delta is waiting on Debian to resolve, with a link to the Debian bug in question.

- cbb4bb1: This changelog entry is also a bit generic, and would be better to state that it's enabling dkim verification and reducing verbosity about sending mail. There's a couple other settings to undef but I can't tell if those are actually needed or if they actually cause a behavioral change (if they don't, why are they there?) You might also investigate if the dkim verification change should go with the other dkim commit(s).

  Oh, also small misspelling, "insure" should be "ensure".

- 630f928: The commit message here is great! It explains what was changed and why, and identifies actions for future mergers. Two minor suggestions, one would be to clarify the changelog entry to "d/rules: fix build with (empty) amavisd-new-postfix", second would be that this would be another good place to use the "--CL--" trick to separate out the annotation from the changelog entry so you don't have to hand-edit the changelog after the reconstruct-changelog step.

- 2405aa1: Instead of saying "after discussion with the server team" state what the rationale actually was. No one will remember that conversation, and it doesn't really matter where something is discussed, what matters is the decision and rationale. In this case I'm guessing the rationale was that altermime and ripole are universe packages?

I verified the package debuild's successfully locally. There are no DEP8 tests so I did no testing beyond that.

I didn't review the apport hook nor the postinst/postrm changes since while those are part of the Ubuntu delta, those aren't listed as part of the MP's Unmerged commits. (Actually I did quickly skim the diff but didn't spot anything other than English wording nitpicks not important enough to mention).

review: Needs Fixing

« Back to merge proposal