Re: [RFC/PATCH] commit notes workflow

From: Johan Herland <johan@herland.net>
Date: Feb 25, 2011, 15:58 UTC
Message-ID: <201102251658.22678.johan@herland.net>
In-Reply-To: <20110225133056.GA1026@sigill.intra.peff.net>

On Friday 25 February 2011, Jeff King wrote:

Show 24 quoted lines

> So my solution was that "git commit" should be able to embed and
> extract notes from the commit message itself. The patch below
> implements "git commit --notes", which does two things:
>
>   1. If we are amending, it populates the commit message not just
> with the existing message, but also with a "---" divider and any
> notes on the commit.
>
>   2. After editing the commit message, it looks for the "---" divider
>      and puts everything after it into a commit note (whether or not
> it put in a divider in step (1), so you can add new notes, too).
>
> So your commit template looks like:
>
>   subject
>
>   commit message body
>   ---
>   notes data
>
>   # usual template stuff
>
> I'm curious what people think. Do others find this useful? Does it
> seem harmful?

I _really_ like the idea. :)

Show 7 quoted lines

> It's yet another magic format to worry about when writing a commit
> message. But you don't need to care unless you use "--notes" (and I
> would probably add a config option, since I would always want this on
> personally). And "---" is already something to be aware of, since
> "am" treats it specially (technically, I could just drop "notes"
> entirely and use "---" in my commit message; so perhaps this is just
> overengineering).

Maybe we should use a slightly more verbose separator (i.e. more unlikely to trigger false positives). As you say, we already have to watch out for "---" because of "am", but that only applies to projects that _use_ "am" (i.e. mailing-list-centric projects like git.git and the Linux kernel). Other projects (e.g. github-centric projects or most centralized "$dayjob-style" projects) seldom or never use "am" at all, so I wouldn't expect those developers think of "---" as "special" in any way.

What about using something like "--- Notes ---" instead?

Show 8 quoted lines

> How should this interact with --cleanup? Right now it splits
> everything after the "---" into the notes part, including any "#"
> lines. Which should be fine, I think, because they get pulled out by
> stripspace in either case. If you were using --cleanup=verbatim, then
> you'd have gotten rid of them manually anyway. And if you really want
> a literal "---", you would use "git commit" (or "git commit
> --no-notes" once there is a config option). So I think the behavior
> in this patch is sane.

What if you combine --notes with --verbose (i.e. including the diff-to-be-committed in the commit message template)?

AFAICS, stripspace() doesn't know how to remove the diff (there's a separate section in cmd_commit() discarding everything following "\ndiff --git ").

Show 6 quoted lines

> I only turn on --edit when we launch an editor. It seems somehow more
> confusing to me that "git commit -F file" should split notes out (or
> worse, "git commit -m"). If you are doing things non-interactively,
> it's probably not a big deal to just call "git notes add" separately.
> And I expect "-F" is used by porcelains, or people wanting to do
> verbatim stuff.

Agreed.

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

Show 6 quoted lines

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

Show 6 quoted lines

> @@ -730,6 +780,9 @@ static int prepare_to_commit(const char
> *index_file, const char *prefix, strbuf_release(&sob);
>  	}
>
> +	if (edit_notes && amend)
> +		add_notes_from_commit(&sb, "HEAD");

I haven't read the sources closely enough to figure out when/where the commit diff is added to the commit message (in case of --verbose), but I trust that it happens _after_ the above lines (so that the notes part doesn't end up after the diff)

Otherwise, this looks good to me from a precursory review.

...Johan

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