Re: Another misleading commit log (was Re: A "cosmetic changes" commit that removes security fixes)

[Ludovic Courtès]

Hi Guix!

Thanks Mark for raising these issues.  I definitely share your concerns,
specifically regarding the two commits you mentioned and how they
misleadingly have undesirable consequences:

  
https://git.savannah.gnu.org/cgit/guix.git/commit/?h=wip-gnome&id=d975ed975456a2c8e855eb024b5487c4c460684a
  
https://git.savannah.gnu.org/cgit/guix.git/commit/?h=wip-gnome&id=f5fc3c609e2f38ca1c0523deadb9f77d838fbf32

I believe all the parties involved behaved in good faith and I’m more
interested in (1) fixing these two mistakes, and (2) making sure this
doesn’t happen again in the future.


Regarding #1, I see 宋文武 (iyzsong) proposed a patch that reinstates
the Cairo security fixes, so it seems we’re on the right track.

Raghav, could you post a patch reinstating the gdk-pixbuf security fix
removed in f5fc3c609e2f38ca1c0523deadb9f77d838fbf32?  If that commit is
only on ‘wip-gnome’, can you simply drop it?  (The convention is that a
‘wip-’ branch may be rebased at will by the person responsible for it.)

Could you also check ‘wip-gnome’ and ‘core-updates’ for similar
“cosmetic changes” commits likely to be controversial?


Regarding #2, everyone please keep in mind the commit rules¹.  We’re all
pouring hours of our time into this.  It’s a social project before being
a technical one, so it’s crucial to not step on each other’s toes.

As per these rules, ‘wip-gnome’ will have to go through review as usual.
As always, it’s best if you can submit it for review in small chunks.
Note that review has to happen via guix-patches@gnu.org so everyone in
the project can see it and has a chance to chime in.  You can have
pre-review/mentoring elsewhere if you want (it’s great if you can have
that), but it “doesn’t count” from the project’s viewpoint.

I think committers and particularly vouchers should spend more time
reviewing and mentoring newcomers.

Regarding commit logs, we only add a ‘Signed-off-by’ line when applying
someone else’s patches; let’s keep following that rule, for clarity.

For the subject line: as 宋文武 wrote, as a rule of thumb, if you cannot
summarize the change in the subject line, that probably means the patch
should be split into smaller logical units.  More generally, never
bundle together unrelated changes in the same commit, as per:

  https://guix.gnu.org/manual/en/html_node/Submitting-Patches.html

Here are additional rules I try to follow for the commit message and
that I’d recommend following:

  1. Never write “cosmetic changes” as the summary: it’s too vague.
     Instead, be more specific: “reindent foo.scm”, “remove unused
     module imports”, whatever.

  2. Never write “Fix X”.  Instead describe the fix: “Avoid
     non-top-level 'use-modules'”, “Remove file that no longer exists”,
     “Honor proxy settings”, “Disable tests when building on i586-gnu”,
     etc.  In all these examples I could have written “Fix …”, but
     fellow hackers would have had to look at the diff to understand
     what’s going on.


A long message!

Let’s calm down and focus the way forward: fixing the security issues
that were reported, checking whether similar ones are lurking, and
improving our practices.  If you feel the need for it, feel free to
propose improvements to the “Commit Access” and “Submitting Patches”
sections, too!

Thanks in advance.  :-)

Ludo’.

¹ https://guix.gnu.org/manual/en/html_node/Commit-Access.html