Re: [RFC/PATCH] commit notes workflow

[Johan Herland <johan@herland.net>]

On Tuesday 01 March 2011, Jeff King wrote:
> On Fri, Feb 25, 2011 at 04:58:22PM +0100, Johan Herland wrote:
> > What about using something like "--- Notes ---" instead?
> 
> Yeah, it is true that many git users will never care about the
> patch-through-mail workflow. And I think these days that is OK, because
> rebase will take care to keep their commit message intact even if it
> doesn't format well in a "format-patch | am" pipeline.
> 
> I really wanted to keep it short and natural, though. Because eventually
> I'd like to have this on all the time via a config option, and I don't
> want to see "--- Notes ---" in every commit that doesn't have notes. But
> I _do_ want to be able to quickly say "oh, let me make a note on this"
> and just add a quick separator.
> 
> It wouldn't be a regression if people had to opt into the feature using
> the command-line or config option. So in theory they could learn about
> "---" then, unless we turn it on by default (but why would we? A user
> has to know about this feature to use it, so they can easily turn on the
> option).

I'm not so sure. Requiring users to opt in might be enough protection 
against false "---" positives. I'm just paranoid about someone turning this 
on in their global config, and then forgetting all about it when they format 
a commit message like this:

   Subject line of the commit message

   What
   ----
   Lorem ipsum dolor sit amet.

   Why
   ---
   eggs spam bacon spam spam spam.

(which will end the commit message after "Why", and add the last line as a 
note)

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)

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.

> 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...

> > > How should this interact with the commit-msg hook? In my
> > > implementation, it sees the whole thing, message and notes. Should we
> > > be picking apart the two bits after the editor and rewriting the
> > > COMMIT_EDITMSG before the hook sees it?
> > 
> > I'm not sure about this, but I suspect we should follow the same
> > behaviour as --verbose (i.e. does the commit-msg hook see the entire
> > diff inline in the commit message?).
> > 
> > A short look at builtin/commit.c indicates that we should leave
> > everything in there for the commit-msg hook (AFAICS, the commit-msg
> > hook is invoked from prepare_to_commit(), which is invoked from
> > cmd_commit() _before_ the verbose diff part is removed.)
> 
> Yeah, I think the commit-msg hook sees everything. Which is arguably not
> the most convenient behavior, but it is the most flexible. Sort of. The
> hook doesn't actually know whether "-v" was supplied, so it has to guess
> at what is "-v" junk and what is not.

Yeah, it might be messy today, but I don't think you can clean it up without 
changing the commit-msg hook interface, which to me means that the cleanup 
should probably happen in a separate series.

> I wonder if anyone actually uses
> "-v" these days. It seems like "git add -p" would have superseded it in
> most workflows.

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.

> > > How should this interact with the post-rewrite hook? I obviously need
> > > to set that up for my workflow, too, but I haven't yet. This patch
> > > does nothing, but I'm pretty sure it should turn of "git commit
> > > --amend" calling the rewrite hook if we are using --notes (since the
> > > user has already seen and edited the notes, and we've written them
> > > out).
> > 
> > I don't see what this has to do with the post-rewrite hook. Currently,
> > the post-rewrite documentation ("git help hooks") states that it is run
> > _after_ the automatic notes copying. AFAICS, your --notes simply
> > replaces the usual automatic notes copying with a
> > semi-automatic "edit-and-copy" instead. But this all happens before the
> > port-rewrite hook is called, and thus shouldn't affect it.
> 
> I think this was just me showing my cluelessness about how the notes
> rewriting code worked. I was thinking you needed to have a post-rewrite
> hook to make it work at all, but it looks like it does the rewrite and
> then lets you tweak it.

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

> So my code doesn't turn off the existing copy, but it probably should.

Yeah, if the user edits the note, you don't want the notes rewriting code 
clobbering the edited note by copying the original note on top of it.

> Should the post-rewrite hook run after this? I'm not really sure what
> people use post-rewrite hooks for, to be honest.

Me neither, but from its name I gather that it should be run whenever a 
commit is "rewritten" (amend, rebase, etc.). As such, it binds closer to the 
commit rewrite itself, rather than to the accompanying notes copy (or "edit-
and-copy" in your case). That's why I argue that --notes should not affect 
the invocation of the post-rewrite hook.

> > Otherwise, this looks good to me from a precursory review.
> 
> Thanks. I'll work on some tests for the --cleanup and -v cases so we can
> be sure that it's behaving as we want, and then hopefully submit a nicer
> version.

Looking forward to it. :)


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net