Re: [RFC/PATCH] commit notes workflow

[Jeff King <peff@peff.net>]

On Wed, Mar 02, 2011 at 01:21:54AM +0100, Johan Herland wrote:

> Just grepping through a "git log" from git.git master, I can find one 
> almost-false-positive in b6b84d1 ("---" appears slightly indented), and 
> grepping through linux-2.6 master, I find plenty potential for false 
> positives:
> 
>   a2d49358ba9bc93204dc001d5568c5bdb299b77d (almost false positive)
>   20cbd3e120a0c20bebe420e1fed0e816730bb988 (almost false positive)
>   68845cb2c82275efd7390026bba70c320ca6ef86 (false positive)
>   5e553110f27ff77591ec7305c6216ad6949f7a95 (false positive)
>   9638d89a75776abc614c29cdeece0cc874ea2a4c (false positive)

There is actually one false positive in git.git (1dfcfbc), but it looks
like a broken commit message in the first place (IOW, "---" _was_
special here, and it got broken during application). It appears many
times in linux-2.6, but in most I examined it looks like a similar case:
it _should_ have been removed during git-am or equivalent, but for some
reason was not, and the result is "---" cruft at the bottom of the
message, or sometimes a bunch of irrelevant patch text stuck in the
message.

The ones you mentioned are indeed false positives. I wonder if linux-2.6
is really a good repo to look at, though. Screwups aside, many patch
applications are happening using "git am", so of course we wouldn't see
the true number of false positives, as they were already mangled before
they made it into the repo.

> Remember that developers sometimes cut-n-paste output from other programs 
> (debug sessions, performance benchmarks, etc.) into their commit message, 
> and that makes a false positive a lot more likely to slip through.

Yeah, that's my biggest concern. I just really foresee myself getting
annoyed by typing "--- nOtes ---", or "-- Notes ---". It's just a few
characters shorter, but "---" is really less error prone.

> > Or maybe the divider should be configurable and default to something
> > long. But clueful people can set it to "---". That kind of seems like
> > overkill, though.
> 
> Not sure that would help. I consider myself "clueful" enough that I'd likely 
> set it to "---", but I also know myself well enough that if I pasted some 
> debug/performance output into a commit message, and that output happened to 
> contain a "---", it would likely slip through...

I think you're arguing both sides here. Making it "---" is too
error-prone that we should make the decision on behalf of everyone to
choose something else. Yet if given the opportunity to make the
decision, you would choose "---"?  :)

I am really leaning towards configurability. Somebody else pointed out
that we would probably want it translatable anyway, so we will have to
deal with an arbitrary string anyway.

> I find myself using -v every now and then, to just have the diff handy while 
> I construct the commit message. Makes it easier to refer to function names, 
> etc. in the commit message.

My new tests cover this (and --cleanup=verbatim leaving both intact).

> Indeed, the notes rewrite does not depend on the post-rewrite hook at all.

Yeah, I was thinking of the config you have to setup, which I had not
done before. The original patch actually did OK with it, but we created
useless extra "notes copy" commits on the notes ref which got
superseded. The new version just avoids the rewrite if we are doing an
edit.

So here's my new version. Still some work to be done, as noted in the
cover letter for 2/2.

  [1/2]: notes: make expand_notes_ref globally accessible
  [2/2]: commit: allow editing notes in commit message editor

-Peff