Re: [PATCH 10/10] add UNLEAK annotation for reducing leak false positives

From: Jeff King <peff@peff.net>
Date: Sep 12, 2017, 15:29 UTC
Message-ID: <20170912152935.iqmj6jpkvgifaju2@sigill.intra.peff.net>
In-Reply-To: <1505223698.27800.30.camel@gmail.com>

On Tue, Sep 12, 2017 at 07:11:38PM +0530, Kaartic Sivaraam wrote:

Show 22 quoted lines

> On Tue, 2017-09-05 at 09:05 -0400, Jeff King wrote:
> > This patch introduces an UNLEAK() macro that lets us do so.
> > To understand its design, let's first look at some of the
> > alternatives.
> > 
> 
> > This patch adds the UNLEAK() macro and enables it
> > automatically when Git is compiled with SANITIZE=leak.
> > It adds some UNLEAK() annotations to show off how the
> > feature works. On top of other recent leak fixes, these are
> > enough to get t0000 and t0001 to pass when compiled with
> > LSAN.
> 
> My nit of the day ;-)
> 
> The above paragraphs seems to going against the following guideline of
> Documentation/SubmittingPatches,
> 
>     Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
>     instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
>     to do frotz", as if you are giving orders to the codebase to change
>     its behavior.  Try to make sure your explanation can be understood

Like all good writing rules, I think it's important to know when to break them. :)

Writing in the imperative is _most_ important in the subject. You're likely to see a lot of subjects in a list, and it makes the list easier to read if they all match. It also tends to be shorter, which is good for subjects.

For short commit messages, I think the imperative also keeps things tight and to the point: describe the problem and then say how to fix it. The recent 0db3dc75f is a good example (which I picked by skimming recent "git log" output). But saying "this patch" is IMHO not that big a problem there, as long as it isn't done excessively.

When you the explanation is longer or more complicated, the imperative can actually be a bit _too_ terse. In longer text it helps to guide readers in the direction you want their thoughts to take. Having a three-paragraph explanation of the problem or current state of things and then jumping right into "Do this. Do that." lacks context. A marker like "this patch" helps the reader know that you're switching gears to talking about the solution.

I also think that "I" is useful in avoiding the passive voice. It can certainly be used gratuitously and make things less clear, but in most cases I'd rather see something like "I tested performance under these conditions" than "Performance was tested under these conditions". I also often use the "academic we" here even when I worked on something myself.

-Peff