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

[stef]

Le 11/02/2013 11:15, Paul Menzel a écrit :
> 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/
>
>

Hello,

if you are upset because attribution for the trailing whitespace 
removal, I can consider that.
But #314012, is incomplete, I should have rejected as such (and not 
marked it fixed). Since cppcheck seems to bail out on the first 
occurence of the same memleak in a function, you have to rerun it after 
fixing each error to get sure everything is fine like I did. So it is 
clearly my work. Like it was to set up a test script, take reference 
data and test, to ensure there is no regression in sane-desc.

By
- asking to rename the list because you didn't pay attention to its notice
- thinking I am the project leader while it is Allan (who is doing a 
great job BTW)
- telling bug tracker is not used
- going ballistic on such mundane thing than a few extra spaces
- teaching lesson on bug reporting to someone that has already went 
through the hassle of registering to the bug tracker and posting to the list
you're neither building cooperation nor helping us.

I didn't expect to have burning so much of my free time on such 
activities tonight, when I planned to work on a LiDE60 bug. I still 
think you are welcome provide you stay constructive. Did you get notice 
of yesterday's comments on #314014 and #314015 ? There is real work to 
do on these.


Regards,
Stef