[sane-devel] Git best practices for proper application of patches

[Paul Menzel]

Dear SANE developers,


sorry for complaining again, but I am quite upset.

I was happy to see, that two of my bug reports I submitted to the Alioth
bug tracker were marked as »Fixed«.

1. [sane-Bugs][314012] Cppcheck: [tools/sane-desc.c:619]: (error) Memory leak: word [1]
2. [sane-Bugs][314013] tools/sane-desc.c: Trailing whitespace [2]

Running `git remote update` in my sane-backends Git clone, I was became
upset seeing just the following commit.

        commit e7804700f64660aedaff2aa5a429c4ed05031d78
        Author: Stéphane Voltz <stef.dev at free.fr>
        Date:   Mon Feb 11 07:55:43 2013 +0100
        
            minor fixes to sane-desc.c
            - memleaks on error paths
            - whitspaces at end of line

And I want to explain why, so you can hopefully understand me.

First, I spent several minutes to compose a proper commit messages and
attached them to the reports [1][2]. So afterward this time is now
*wasted*! We all have the same problem, that time is the most precious
thing Free Software developers/activists have as we are mostly involved
in several projects.

Second, commits like that – and looking through the Git commit history
of SANE unfortunately all commits are similar to that as already stated
in my message »[sane-devel] Current project status« [3] – violate basic
software development practices which popular projects like the Linux
kernel follow.

a) One commit for one change.

Why? α) Because distributions can then backport patches more easily with
`git cherry-pick`.

β) Besides that whitespace changes are always a nuisance, please look at
the commit and find out what the memory leak is! (Right, ignoring
whitespace changes with some git switch would work, but people do not
always have a checkout handy, so please find it in the Gitweb overview
or in the message to the SANE commit list! So people *waste* time to
understand a commit which has to be avoided.

b) Proper commit messages, which means a commit summary and a detailed
and elaborate description/explanation of the commit in the commit
message body. The OpenStack Git commit message page [5] sums the points
up quite nicely in my opinion. See also the pages [6] and [7].

   It is important to know how a bug was found, if it caused any issues
and how it is fixed. Most of the time this is present in the bug report,
so it needs to be referenced in the commit message!

   Referencing the bug report is also crucial as you cannot – well you
could, but it is bad practice for public branches – change commit
messages in public branches. So people needing to comment on the change
should easily find the bug report or mailing list thread.

   And no, it is not enough to just reference the bug report by saying:
“It contains all information.”. The developer preparing the commit now
knows what the problem is and what the fix does. Some days later (or a
year later) even the developer will not remember what happened. So
reading the code and the *whole* bug report would be needed to find out
that information. This takes people not involved with that change
longer! Again, please save everyone some time, and write a proper commit
message with all necessary information to understand the commit!

   Lastly, this is necessary for proper review. If I am not mistaken,
currently SANE does not have a proper review process and each backend
maintainer can commit whatever she or he wants. Good commit messages
would help the reviewer to check the correctness of the patch. If the
author writes something in the commit messages she or he thinks what
should have been accomplished, the reviewer can check against that and
find out if the code lives up to that requirement. Otherwise the
reviewer cannot verify that.


Therefore, for one please download attached patches for example created
with `git format-patch -1` and apply them with

    git am path/to/downloaded/file

which will keep the author information and set up the committer
information. Then maybe fix up some stuff with `git commit --amend` in
the commit message and then push it.

And *enforce* these rules in the SANE project by rejecting patches not
following these guidelines. SANE is big enough that proper patches are
crucial to not introduce regressions and to make it easy for sporadic
developers to get an overview over latest developments.

I hope I do not step on anyone toes, but in my opinion such things are
*very* important for FLOSS projects or any software project!


Thanks and sorry for another long post,

Paul


[1] https://alioth.debian.org/tracker/?func=detail&atid=410366&aid=314012&group_id=30186
[2] https://alioth.debian.org/tracker/?func=detail&atid=410366&aid=314013&group_id=30186
[3] http://lists.alioth.debian.org/pipermail/sane-devel/2013-February/030888.html
[4] http://anonscm.debian.org/gitweb/?p=sane/sane-backends.git;a=blobdiff;f=tools/sane-desc.c;h=5fb51bc6e21eaee5742d1ce0541216cdc19aeea9;hp=7bbd0124f7fe7777babe357b97be67059e9d2dc2;hb=e7804700f64660aedaff2aa5a429c4ed05031d78;hpb=8f3983ed2067f93aca81a2eae30838259b87404f
[5] http://wiki.openstack.org/GitCommitMessages
[6] http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
[7] http://ablogaboutcode.com/2011/03/23/proper-git-commit-messages-and-an-elegant-git-history/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.alioth.debian.org/pipermail/sane-devel/attachments/20130211/531c0904/attachment.pgp>