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

[m. allan noah]

Sane is an ancient project by FLOSS standards, and in those many
years, has developed some bad habits. Some of these are by necessity,
as it is difficult or impossible to properly review code for hardware
you don't have. Some of these habits are by choice- most of us would
rather use our limited hours to write code for hardware we do have,
than write docs or sometimes even update tickets. We get enough
'proper' procedure in our day jobs :)  We also have a massive number
of developers who have moved on to other interests, yet their backends
are still (marginally) supported by the sane project. This increases
the burden on the remaining devs somewhat.

All-in-all, sane is a bit of a mess. I'll be the first to take the
blame for that, as I have not done a good job recruiting new devs, and
I have not kept up with all the housekeeping duties. Releases are
sporadic, bugs are not assigned, known issues to core infrastructure
are not fixed, and sometimes moderated emails sit for a day or two
before I get a chance to review them. So, I am acutely aware of your
concerns, and agree with some of them.

However, I do NOT like your methods. It appears that you know very
little about sane, its protocol, or it's history. Yet you find it
necessary to ask bug-reporting end users for an education (when their
request is perfectly clear) [1], you give incorrect information in the
bug tracker [2][3], and you waste the bug reporters time asking them
to provide useless data like a description of the scanner or a
pointless downstream bug report [4].

So, how about you talk less, and code more. Get the patches you
submitted to completion (so otherwise busy folks like stef don't have
to do it for you). Find some more bugs in the tracker to produce
patches for. Be prepared to spend your own money to buy scanners just
so you can verify your code. Then, consider picking up one of the
unmaintained backends. That is how the rest of us got started. Once
you reach that point, you will have a much better understanding about
what the problems are with SANE, and you will be in a better position
to help correct them.

[1]https://alioth.debian.org/tracker/?func=detail&atid=410366&aid=313734&group_id=30186
[2]https://alioth.debian.org/tracker/index.php?func=detail&aid=314021&group_id=30186&atid=410366
[3]https://alioth.debian.org/tracker/index.php?func=detail&aid=314019&group_id=30186&atid=410366
[4]http://lists.alioth.debian.org/pipermail/sane-devel/2013-February/030902.html

allan

On Mon, Feb 11, 2013 at 5:15 AM, Paul Menzel
<paulepanter at users.sourceforge.net> wrote:
> 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/
>
> --
> sane-devel mailing list: sane-devel at lists.alioth.debian.org
> http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/sane-devel
> Unsubscribe: Send mail with subject "unsubscribe your_password"
>              to sane-devel-request at lists.alioth.debian.org



-- 
"The truth is an offense, but not a sin"