[RFC/PATCH] commit notes workflow

[RFC/PATCH] commit notes workflow

[Jeff King]

I was revising a long-ish series today, and I have been wanting to start
using "git notes" to store information on what's changed between
versions (which will eventually go after the "---" in format-patch).

So my workflow was something like:

  1. git rebase -i, mark one or more commits for edit

  2. For each commit we stop at:

     a. Tweak the tree either with enhancements, or to resolve
        conflicts from tweaks to earlier patches.

     b. commit --amend, tweak commit message is needed

     c. git notes add, mention changes

     d. git rebase --continue

Two things annoyed me:

  1. Editing the commit message and notes separately felt awkward. They
     are conceptually part of the same task to me.

  2. In the conflict case, there is no opportunity to run "git notes
     add" because you fix up commits and directly run "rebase
     --continue".

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?

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

My initial attempt was to implement in terms of prepare-commit-msg,
commit-msg, and post-commit hooks. And it worked OK, but was foiled by
rebase using "git commit --no-verify".

There are still a few questions to address.

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.

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.

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?

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

Anyway, here is the patch.

---
 builtin/commit.c |   65 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 64 insertions(+), 1 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 5295032..3e21e33 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -26,6 +26,7 @@
 #include "unpack-trees.h"
 #include "quote.h"
 #include "submodule.h"
+#include "blob.h"
 
 static const char * const builtin_commit_usage[] = {
 	"git commit [options] [--] <filepattern>...",
@@ -104,6 +105,7 @@ static char *cleanup_arg;
 static enum commit_whence whence;
 static int use_editor = 1, initial_commit, include_status = 1;
 static int show_ignored_in_status;
+static int edit_notes;
 static const char *only_include_assumed;
 static struct strbuf message;
 
@@ -146,6 +148,7 @@ static struct option builtin_commit_options[] = {
 	OPT_BOOLEAN('e', "edit", &edit_flag, "force edit of commit"),
 	OPT_STRING(0, "cleanup", &cleanup_arg, "default", "how to strip spaces and #comments from message"),
 	OPT_BOOLEAN(0, "status", &include_status, "include status in commit message template"),
+	OPT_BOOLEAN(0, "notes", &edit_notes, "edit notes interactively"),
 	/* end commit message options */
 
 	OPT_GROUP("Commit contents options"),
@@ -603,6 +606,53 @@ static char *cut_ident_timestamp_part(char *string)
 	return ket;
 }
 
+static void add_notes_from_commit(struct strbuf *out, const char *name)
+{
+	struct commit *commit;
+	struct strbuf note = STRBUF_INIT;
+
+	commit = lookup_commit_reference_by_name(name);
+	if (!commit)
+		die("could not lookup commit %s", name);
+	format_note(NULL, commit->object.sha1, &note,
+		    get_commit_output_encoding(), 0);
+
+	if (note.len) {
+		strbuf_addstr(out, "\n---\n");
+		strbuf_addbuf(out, &note);
+	}
+	strbuf_release(&note);
+}
+
+static void extract_notes_from_message(struct strbuf *msg, struct strbuf *notes)
+{
+	const char *separator = strstr(msg->buf, "\n---\n");
+
+	if (!separator)
+		return;
+
+	strbuf_addstr(notes, separator + 5);
+	strbuf_setlen(msg, separator - msg->buf + 1);
+}
+
+static void update_notes_for_commit(struct strbuf *notes,
+				    unsigned char *commit_sha1)
+{
+	stripspace(notes, cleanup_mode == CLEANUP_ALL);
+
+	if (!notes->len)
+		remove_note(NULL, commit_sha1);
+	else {
+		unsigned char blob_sha1[20];
+		if (write_sha1_file(notes->buf, notes->len,
+				    blob_type, blob_sha1) < 0)
+			die("unable to write note blob");
+		add_note(NULL, commit_sha1, blob_sha1,
+			 combine_notes_overwrite);
+	}
+	commit_notes(NULL, "updated by commit --notes");
+}
+
 static int prepare_to_commit(const char *index_file, const char *prefix,
 			     struct wt_status *s,
 			     struct strbuf *author_ident)
@@ -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");
+
 	if (fwrite(sb.buf, 1, sb.len, fp) < sb.len)
 		die_errno("could not write commit template");
 
@@ -997,8 +1050,10 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		use_editor = 0;
 	if (edit_flag)
 		use_editor = 1;
-	if (!use_editor)
+	if (!use_editor) {
 		xsetenv("GIT_EDITOR", ":", 1);
+		edit_notes = 0;
+	}
 
 	if (get_sha1("HEAD", head_sha1))
 		initial_commit = 1;
@@ -1357,6 +1412,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf author_ident = STRBUF_INIT;
+	struct strbuf notes = STRBUF_INIT;
 	const char *index_file, *reflog_msg;
 	char *nl, *p;
 	unsigned char commit_sha1[20];
@@ -1458,6 +1514,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 			strbuf_setlen(&sb, p - sb.buf + 1);
 	}
 
+	if (edit_notes)
+		extract_notes_from_message(&sb, &notes);
+
 	if (cleanup_mode != CLEANUP_NONE)
 		stripspace(&sb, cleanup_mode == CLEANUP_ALL);
 	if (message_is_empty(&sb) && !allow_empty_message) {
@@ -1473,6 +1532,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	}
 	strbuf_release(&author_ident);
 
+	if (edit_notes)
+		update_notes_for_commit(&notes, commit_sha1);
+	strbuf_release(&notes);
+
 	ref_lock = lock_any_ref_for_update("HEAD",
 					   initial_commit ? NULL : head_sha1,
 					   0);
-- 
1.7.2.5.20.g7ba93.dirty

Re: [RFC/PATCH] commit notes workflow

[Johan Herland]

On Friday 25 February 2011, Jeff King wrote:
> 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. :)

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

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

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

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

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

Re: [RFC/PATCH] commit notes workflow

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

As long as this is done only under "commit --notes", I don't think it
should hurt innocent bystanders.

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

Then --no-notes would also be necessary, but I think you would get it for
free these days ;-).

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

So if you see -F -m and there is no --edit, you don't split out notes at
the divider?  That sounds like a sensible thing to me.

Re: [RFC/PATCH] commit notes workflow

[Drew Northup]

On Fri, 2011-02-25 at 08:30 -0500, Jeff King wrote:
> I was revising a long-ish series today, and I have been wanting to start
> using "git notes" to store information on what's changed between
> versions (which will eventually go after the "---" in format-patch).


>   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'm in agreement with the others that it doesn't seem like a bad idea,
and likely a good one. Just one thing, can you add an end-of-note
delimiter (the same thing perhaps)? I didn't spend a long time looking
at the code, but I can imagine more than a few ways for this to go wrong
without one.

-- 
-Drew Northup
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

Re: [RFC/PATCH] commit notes workflow

[Michael J Gruber]

Jeff King venit, vidit, dixit 25.02.2011 14:30:
> I was revising a long-ish series today, and I have been wanting to start
> using "git notes" to store information on what's changed between
> versions (which will eventually go after the "---" in format-patch).
> 
> So my workflow was something like:
> 
>   1. git rebase -i, mark one or more commits for edit
> 
>   2. For each commit we stop at:
> 
>      a. Tweak the tree either with enhancements, or to resolve
>         conflicts from tweaks to earlier patches.
> 
>      b. commit --amend, tweak commit message is needed
> 
>      c. git notes add, mention changes
> 
>      d. git rebase --continue
> 
> Two things annoyed me:
> 
>   1. Editing the commit message and notes separately felt awkward. They
>      are conceptually part of the same task to me.
> 
>   2. In the conflict case, there is no opportunity to run "git notes
>      add" because you fix up commits and directly run "rebase
>      --continue".
> 
> 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 can haz tis wiz "format-patch --notes-behind-triple-dash"?

Michael

Re: [RFC/PATCH] commit notes workflow

[Jeff King]

On Fri, Feb 25, 2011 at 04:58:22PM +0100, Johan Herland wrote:

> > I'm curious what people think. Do others find this useful? Does it
> > seem harmful?
> 
> I _really_ like the idea. :)

Thanks. Everybody seems to like it, so I'm going to polish it up and
submit a nicer version.

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

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

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.

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

Ugh. Yeah, I looked at that in an earlier iteration but then forgot
about it in the final. We will end up with "-v" crap in the notes. I'll
fix that in the next revision.

I also think it will be worth making a nice test script of all of these
different cases.

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

> > 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. So my code doesn't turn off the existing copy,
but it probably should. Should the post-rewrite hook run after this? I'm
not really sure what people use post-rewrite hooks for, to be honest.

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

I think so, but I'll double check. I agree that it's important for it to
go right after the commit message (and before the "#" comment lines, I
think).

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

-Peff

Re: [RFC/PATCH] commit notes workflow

[Jeff King]

On Fri, Feb 25, 2011 at 03:30:54PM -0500, Drew Northup wrote:

> > 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'm in agreement with the others that it doesn't seem like a bad idea,
> and likely a good one. Just one thing, can you add an end-of-note
> delimiter (the same thing perhaps)? I didn't spend a long time looking
> at the code, but I can imagine more than a few ways for this to go wrong
> without one.

We could add one pretty easily, but I'm not sure what you would be
delimiting it from. Can you describe a case where it would be useful?

-Peff

Re: [RFC/PATCH] commit notes workflow

[Jeff King]

On Sun, Feb 27, 2011 at 03:31:50PM +0100, Michael J Gruber wrote:

> > So your commit template looks like:
> > 
> >   subject
> > 
> >   commit message body
> >   ---
> >   notes data
> > 
> >   # usual template stuff
> > 
> I can haz tis wiz "format-patch --notes-behind-triple-dash"?

Yeah, I think that would be a nice 2/2 to this series. From past
threads, it seems that it is not as trivial as one would like, but I can
take a look.

-Peff

Re: [RFC/PATCH] commit notes workflow

[Drew Northup]

On Tue, 2011-03-01 at 17:00 -0500, Jeff King wrote:
> On Fri, Feb 25, 2011 at 03:30:54PM -0500, Drew Northup wrote:
> 
> > > 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'm in agreement with the others that it doesn't seem like a bad idea,
> > and likely a good one. Just one thing, can you add an end-of-note
> > delimiter (the same thing perhaps)? I didn't spend a long time looking
> > at the code, but I can imagine more than a few ways for this to go wrong
> > without one.
> 
> We could add one pretty easily, but I'm not sure what you would be
> delimiting it from. Can you describe a case where it would be useful?
> 
> -Peff

A notes message which contains "the usual template stuff" as means of
describing a change to it, for starters...

There is likely good reason why the commit message already has an end
mark, I suspect that also applies here. (Unless you count "---" between
the commit message and the patch as "the usual template stuff"--which
wasn't clear at this keyboard anyway.) If that's already in there then
please forgive the noise--it didn't jump out at me, but I also spend way
too much time programming in too many languages for that to be very
likely.

-- 
-Drew Northup
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

Re: [RFC/PATCH] commit notes workflow

[Jeff King]

On Tue, Mar 01, 2011 at 05:18:33PM -0500, Drew Northup wrote:

> A notes message which contains "the usual template stuff" as means of
> describing a change to it, for starters...

But we strip that from the notes, unless you use --cleanup. But in that
case, you would have deleted the template cruft, since it pollutes your
message.

> There is likely good reason why the commit message already has an end
> mark, I suspect that also applies here.

It doesn't have an end mark. The "usual template stuff" just happens to
be at the end. But any line starting with "#" will be removed unless you
use --cleanup, whether you use --notes or no. Similarly, unadorned lines
after the "#" lines will be counted as part of the message.

> (Unless you count "---" between the commit message and the patch as
> "the usual template stuff"--which wasn't clear at this keyboard
> anyway.)

No, I meant the "#" lines. The "---" of format-patch isn't relevant
here, since we're just talking about commit messages inside the editor
during git-commit.

The really evil bit is "-v" which appends a giant diff with no real
indication that it isn't part of the commit message. We already get rid
of it with some heuristics (which I remember improving a while back).
I don't think my RFC patch handles it very well, but that is something I
will be looking at for the next revision.

-Peff

Re: [RFC/PATCH] commit notes workflow

[Drew Northup]

On Tue, 2011-03-01 at 17:23 -0500, Jeff King wrote:
> On Tue, Mar 01, 2011 at 05:18:33PM -0500, Drew Northup wrote:

> 
> No, I meant the "#" lines. The "---" of format-patch isn't relevant
> here, since we're just talking about commit messages inside the editor
> during git-commit.

That's the part I missed. I'll crawl back under my rock now...

-- 
-Drew Northup
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

Re: [RFC/PATCH] commit notes workflow

[Johan Herland]

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

Re: [RFC/PATCH] commit notes workflow

[Chris Packham]

On 02/03/11 10:59, Jeff King wrote:
> On Fri, Feb 25, 2011 at 04:58:22PM +0100, Johan Herland wrote:
>> 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?
> 
> 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.

<bikesheding>
What about "#---"? Satisfies the quick to type and is a lot less likely
to appear in commit messages. Not sure about the implications of finding
that string before the commit message is stripped.
</bikesheding>

Re: [RFC/PATCH] commit notes workflow

[Drew Northup]

On Wed, 2011-03-02 at 20:01 +1300, Chris Packham wrote:
> On 02/03/11 10:59, Jeff King wrote:
> > On Fri, Feb 25, 2011 at 04:58:22PM +0100, Johan Herland wrote:
> >> 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?
> > 
> > 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.
> 
> <bikesheding>
> What about "#---"? Satisfies the quick to type and is a lot less likely
> to appear in commit messages. Not sure about the implications of finding
> that string before the commit message is stripped.
> </bikesheding>


True enough, but that would be seen as a comment and dropped outright
the way things are currently standing. If you want short, definitely
rare, and most likely intentional you'd need something harder to
remember like "-!N" as the tag. I don't know how well that'd go over
with people--it definitely isn't natural.

-- 
-Drew Northup
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

Re: [RFC/PATCH] commit notes workflow

[Piotr Krukowiecki]

Hi,

On Wed, Mar 2, 2011 at 8:01 AM, Chris Packham <judge.packham@gmail.com> wrote:
> On 02/03/11 10:59, Jeff King wrote:
>> On Fri, Feb 25, 2011 at 04:58:22PM +0100, Johan Herland wrote:
>>> 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?
>>
>> 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.

IMO typing "--- Notes ---" is quite fast. I suspect that most commits won't
have any notes, so you'll have to type it rarely.

Also, what should be the template with notes enabled? Should there
be the separator by default or not? Assuming notes are entered rarely,
I think template should not have the separator. But if you use "--notes"
command line option (i.e. interactive use), it should be there.


> <bikesheding>
> What about "#---"? Satisfies the quick to type and is a lot less likely
> to appear in commit messages. Not sure about the implications of finding
> that string before the commit message is stripped.
> </bikesheding>

I think the separator should be:
1. unique enough so people won't enter it by accident
2. easy to remember, easy to type
3. descriptive so you won't have to look into documentation to see what "#---"
    means.

I think "--- Notes ---" fulfills all requirements.

Also, in case of separator with some text, like "--- Notes ---", we would
like to be able to translate it probably, so I will see "--- Notatki
---" in Polish.


-- 
Piotrek

Re: [RFC/PATCH] commit notes workflow

[Sverre Rabbelier]

Heya,

On Wed, Mar 2, 2011 at 01:21, Johan Herland <johan@herland.net> wrote:
>> 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.

Can someone explain why -v does not output it's data prefixed by a
'#'? If someone really wanted to include it in their commit message
they can column-select-delete it, and if they don't, it just gets
deleted by the cleanup code?

-- 
Cheers,

Sverre Rabbelier

Re: [RFC/PATCH] commit notes workflow

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> Can someone explain why -v does not output it's data prefixed by a
> '#'? If someone really wanted to include it in their commit message
> they can column-select-delete it, and if they don't, it just gets
> deleted by the cleanup code?

Good question.  There was no reason other than "that is just a historical
accident".

The intention has always been "allow people to review the change for the
last time while writing a log message"; there was never a feature request
to allow the diff to be included---it would always have been an unwelcome
accident it that ever happened.

Re: [RFC/PATCH] commit notes workflow

[Sverre Rabbelier]

Heya,

On Thu, Mar 3, 2011 at 04:50, Junio C Hamano <gitster@pobox.com> wrote:
> Good question.  There was no reason other than "that is just a historical
> accident".
>
> The intention has always been "allow people to review the change for the
> last time while writing a log message"; there was never a feature request
> to allow the diff to be included---it would always have been an unwelcome
> accident it that ever happened.

In that case, can we change it to be that way now if it makes this case easier?

-- 
Cheers,

Sverre Rabbelier

[PATCH] commit, status: #comment diff output in verbose mode

[Ian Ward Comfort]

On 3 Mar 2011, at 3:12 AM, Sverre Rabbelier wrote:
> On Thu, Mar 3, 2011 at 04:50, Junio C Hamano <gitster@pobox.com> wrote:
>> Good question.  There was no reason other than "that is just a historical
>> accident".
>>
>> The intention has always been "allow people to review the change for the
>> last time while writing a log message"; there was never a feature request
>> to allow the diff to be included---it would always have been an unwelcome
>> accident it that ever happened.
>
> In that case, can we change it to be that way now if it makes this case
> easier?

How about something like this?

--8<--

By historical accident, diffs included in commit templates and status
output when the "-v" option is given are not prefixed with the # comment
character, as other advice and status information is. Stripping these
lines is thus a best-effort operation, as it is not always possible to
tell which lines were generated by "-v" and which were inserted by the
user.

Improve this situation by adding the # prefix to diff output along with
all other status output in these cases. The change is simply made thanks
to a3c158d (Add a prefix output callback to diff output, 2010-05-26). The
prefixed diff can be stripped (or not, as configured) by the standard
cleanup code, so our special verbose-mode heuristic can be removed.

Documentation and a few tests which rely on the old "-v" format are
updated to match. One known breakage is fixed in t7507.

Signed-off-by: Ian Ward Comfort <icomfort@stanford.edu>
---
 Documentation/git-commit.txt |    3 +--
 builtin/commit.c             |    7 -------
 t/t4030-diff-textconv.sh     |    2 +-
 t/t7502-commit.sh            |    4 ++--
 t/t7507-commit-verbose.sh    |    4 ++--
 wt-status.c                  |   12 ++++++++++++
 6 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 8f89f6f..792f993 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -233,8 +233,7 @@ configuration variable documented in linkgit:git-config[1].
 --verbose::
 	Show unified diff between the HEAD commit and what
 	would be committed at the bottom of the commit message
-	template.  Note that this diff output doesn't have its
-	lines prefixed with '#'.
+	template.
 
 -q::
 --quiet::
diff --git a/builtin/commit.c b/builtin/commit.c
index 355b2cb..efecac3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1381,13 +1381,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		die("could not read commit message: %s", strerror(saved_errno));
 	}
 
-	/* Truncate the message just before the diff, if any. */
-	if (verbose) {
-		p = strstr(sb.buf, "\ndiff --git ");
-		if (p != NULL)
-			strbuf_setlen(&sb, p - sb.buf + 1);
-	}
-
 	if (cleanup_mode != CLEANUP_NONE)
 		stripspace(&sb, cleanup_mode == CLEANUP_ALL);
 	if (message_is_empty(&sb) && !allow_empty_message) {
diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh
index 88c5619..b00999e 100755
--- a/t/t4030-diff-textconv.sh
+++ b/t/t4030-diff-textconv.sh
@@ -79,7 +79,7 @@ test_expect_success 'format-patch produces binary' '
 test_expect_success 'status -v produces text' '
 	git reset --soft HEAD^ &&
 	git status -v >diff &&
-	find_diff <diff >actual &&
+	sed -e "s/^# //" <diff | find_diff >actual &&
 	test_cmp expect.text actual &&
 	git reset --soft HEAD@{1}
 '
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 50da034..a916001 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -151,8 +151,8 @@ test_expect_success 'verbose' '
 
 	echo minus >negative &&
 	git add negative &&
-	git status -v | sed -ne "/^diff --git /p" >actual &&
-	echo "diff --git a/negative b/negative" >expect &&
+	git status -v | sed -ne "/^# diff --git /p" >actual &&
+	echo "# diff --git a/negative b/negative" >expect &&
 	test_cmp expect actual
 
 '
diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index da5bd3b..5b21bbb 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -5,7 +5,7 @@ test_description='verbose commit template'
 
 cat >check-for-diff <<EOF
 #!$SHELL_PATH
-exec grep '^diff --git' "\$1"
+exec grep '^# diff --git' "\$1"
 EOF
 chmod +x check-for-diff
 test_set_editor "$PWD/check-for-diff"
@@ -65,7 +65,7 @@ test_expect_success 'diff in message is retained without -v' '
 	check_message diff
 '
 
-test_expect_failure 'diff in message is retained with -v' '
+test_expect_success 'diff in message is retained with -v' '
 	git commit --amend -F diff -v &&
 	check_message diff
 '
diff --git a/wt-status.c b/wt-status.c
index a82b11d..fc0063e 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -32,6 +32,12 @@ static const char *color(int slot, struct wt_status *s)
 	return c;
 }
 
+static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void *data)
+{
+	assert(data);
+	return (struct strbuf *)data;
+}
+
 void wt_status_prepare(struct wt_status *s)
 {
 	unsigned char sha1[20];
@@ -588,6 +594,7 @@ static void wt_status_print_verbose(struct wt_status *s)
 {
 	struct rev_info rev;
 	struct setup_revision_opt opt;
+	struct strbuf diff_output_prefix = STRBUF_INIT;
 
 	init_revisions(&rev, NULL);
 	DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);
@@ -596,10 +603,14 @@ static void wt_status_print_verbose(struct wt_status *s)
 	opt.def = s->is_initial ? EMPTY_TREE_SHA1_HEX : s->reference;
 	setup_revisions(0, NULL, &rev, &opt);
 
+	strbuf_addstr(&diff_output_prefix, "# ");
+
 	rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
 	rev.diffopt.detect_rename = 1;
 	rev.diffopt.file = s->fp;
 	rev.diffopt.close_file = 0;
+	rev.diffopt.output_prefix = diff_output_prefix_callback;
+	rev.diffopt.output_prefix_data = &diff_output_prefix;
 	/*
 	 * If we're not going to stdout, then we definitely don't
 	 * want color, since we are going to the commit message
@@ -609,6 +620,7 @@ static void wt_status_print_verbose(struct wt_status *s)
 	if (s->fp != stdout)
 		DIFF_OPT_CLR(&rev.diffopt, COLOR_DIFF);
 	run_diff_index(&rev, 1);
+	strbuf_release(&diff_output_prefix);
 }
 
 static void wt_status_print_tracking(struct wt_status *s)
-- 
1.7.4.1.177.g1c06f

Re: [PATCH] commit, status: #comment diff output in verbose mode

[Sverre Rabbelier]

Heya,

On Thu, Mar 3, 2011 at 12:23, Ian Ward Comfort <icomfort@stanford.edu> wrote:
> How about something like this?

Yup, that's what I meant :)

-- 
Cheers,

Sverre Rabbelier

Re: [RFC/PATCH] commit notes workflow

[Jeff King]

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

[PATCH 1/2] notes: make expand_notes_ref globally accessible

[Jeff King]

This function is useful for other commands besides "git
notes" which want to let users refer to notes by their
shorthand name.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/notes.c |   10 ----------
 notes.c         |   10 ++++++++++
 notes.h         |    3 +++
 3 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 0aab150..f2ccb75 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -100,16 +100,6 @@ struct msg_arg {
 	struct strbuf buf;
 };
 
-static void expand_notes_ref(struct strbuf *sb)
-{
-	if (!prefixcmp(sb->buf, "refs/notes/"))
-		return; /* we're happy */
-	else if (!prefixcmp(sb->buf, "notes/"))
-		strbuf_insert(sb, 0, "refs/", 5);
-	else
-		strbuf_insert(sb, 0, "refs/notes/", 11);
-}
-
 static int list_each_note(const unsigned char *object_sha1,
 		const unsigned char *note_sha1, char *note_path,
 		void *cb_data)
diff --git a/notes.c b/notes.c
index a013c1b..f6b9b6a 100644
--- a/notes.c
+++ b/notes.c
@@ -1285,3 +1285,13 @@ int copy_note(struct notes_tree *t,
 
 	return 0;
 }
+
+void expand_notes_ref(struct strbuf *sb)
+{
+	if (!prefixcmp(sb->buf, "refs/notes/"))
+		return; /* we're happy */
+	else if (!prefixcmp(sb->buf, "notes/"))
+		strbuf_insert(sb, 0, "refs/", 5);
+	else
+		strbuf_insert(sb, 0, "refs/notes/", 11);
+}
diff --git a/notes.h b/notes.h
index 83bd6e0..60bdf28 100644
--- a/notes.h
+++ b/notes.h
@@ -307,4 +307,7 @@ void string_list_add_refs_by_glob(struct string_list *list, const char *glob);
 void string_list_add_refs_from_colon_sep(struct string_list *list,
 					 const char *globs);
 
+/* Expand inplace a note ref like "foo" or "notes/foo" into "refs/notes/foo" */
+void expand_notes_ref(struct strbuf *sb);
+
 #endif
-- 
1.7.4.31.g76e18

[PATCH 2/2] commit: allow editing notes in commit message editor

[Jeff King]

One workflow for git-notes is to informally keep a list of
changes from one version of a patch to another as the patch
is modified via "commit --amend" and "git rebase". Often the
most convenient time for this is while editing the commit
message, since you see it during amend, during "rebase -i"
edit stops, and when "rebase" finds a conflict.

This patch adds a "--notes" option which displays existing
notes in the commit editor (in the case of --amend), and
extracts new notes from the editor message to add to the
newly created commit.

The feature is activated only for interactive edits, so "-F"
and "-m" messages are safe from munging.

Signed-off-by: Jeff King <peff@peff.net>
---
Changes from v1:

  - fix bug with adding notes to new commit (we failed to
    initialize the notes tree properly in this case)

  - you can now do "commit --notes=foo" to view/edit
    refs/notes/foo

  - added tests for basic operations, plus interaction with
    --cleanup and -v

  - turn off commit rewriting when we edit

Todo:

  - commit.notes config variable to have this on all the time

  - I punted on the separator decision here.

  - probably still some magic needed for rebase conflict
    case; we will be making a new commit, so we don't know
    to pull the notes in from the old commit as we do with
    --amend.

  - still needs the format-patch component to make the
    workflow complete :)

 builtin/commit.c        |   87 ++++++++++++++++++++++-
 t/t7510-commit-notes.sh |  183 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 268 insertions(+), 2 deletions(-)
 create mode 100755 t/t7510-commit-notes.sh

diff --git a/builtin/commit.c b/builtin/commit.c
index d71e1e0..f84ca23 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -26,6 +26,7 @@
 #include "unpack-trees.h"
 #include "quote.h"
 #include "submodule.h"
+#include "blob.h"
 
 static const char * const builtin_commit_usage[] = {
 	"git commit [options] [--] <filepattern>...",
@@ -90,6 +91,8 @@ static char *cleanup_arg;
 
 static int use_editor = 1, initial_commit, in_merge, include_status = 1;
 static int show_ignored_in_status;
+static const char *edit_notes;
+static struct notes_tree edit_notes_tree;
 static const char *only_include_assumed;
 static struct strbuf message;
 
@@ -132,6 +135,8 @@ static struct option builtin_commit_options[] = {
 	OPT_BOOLEAN('e', "edit", &edit_flag, "force edit of commit"),
 	OPT_STRING(0, "cleanup", &cleanup_arg, "default", "how to strip spaces and #comments from message"),
 	OPT_BOOLEAN(0, "status", &include_status, "include status in commit message template"),
+	{ OPTION_STRING, 0, "notes", &edit_notes, "ref", "edit notes interactively",
+		PARSE_OPT_OPTARG, NULL, 1 },
 	/* end commit message options */
 
 	OPT_GROUP("Commit contents options"),
@@ -559,6 +564,68 @@ static char *cut_ident_timestamp_part(char *string)
 	return ket;
 }
 
+static void init_edit_notes() {
+	struct strbuf ref = STRBUF_INIT;
+	if (edit_notes_tree.initialized)
+		return;
+	strbuf_addstr(&ref, edit_notes);
+	expand_notes_ref(&ref);
+	init_notes(&edit_notes_tree, ref.buf,
+		   combine_notes_overwrite, 0);
+}
+
+static void add_notes_from_commit(struct strbuf *out, const char *name)
+{
+	struct commit *commit;
+	struct strbuf note = STRBUF_INIT;
+
+	init_edit_notes();
+
+	commit = lookup_commit_reference_by_name(name);
+	if (!commit)
+		die("could not lookup commit %s", name);
+	format_note(&edit_notes_tree, commit->object.sha1, &note,
+		    get_commit_output_encoding(), 0);
+
+	if (note.len) {
+		strbuf_addstr(out, "\n---\n");
+		strbuf_addbuf(out, &note);
+	}
+	strbuf_release(&note);
+}
+
+static void extract_notes_from_message(struct strbuf *msg, struct strbuf *notes)
+{
+	const char *separator = strstr(msg->buf, "\n---\n");
+
+	if (!separator)
+		return;
+
+	strbuf_addstr(notes, separator + 5);
+	strbuf_setlen(msg, separator - msg->buf + 1);
+}
+
+static void update_notes_for_commit(struct strbuf *notes,
+				    unsigned char *commit_sha1)
+{
+	init_edit_notes();
+
+	if (cleanup_mode != CLEANUP_NONE)
+		stripspace(notes, cleanup_mode == CLEANUP_ALL);
+
+	if (!notes->len)
+		remove_note(&edit_notes_tree, commit_sha1);
+	else {
+		unsigned char blob_sha1[20];
+		if (write_sha1_file(notes->buf, notes->len,
+				    blob_type, blob_sha1) < 0)
+			die("unable to write note blob");
+		add_note(&edit_notes_tree, commit_sha1, blob_sha1,
+			 combine_notes_overwrite);
+	}
+	commit_notes(&edit_notes_tree, "updated by commit --notes");
+}
+
 static int prepare_to_commit(const char *index_file, const char *prefix,
 			     struct wt_status *s,
 			     struct strbuf *author_ident)
@@ -682,6 +749,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");
+
 	if (fwrite(sb.buf, 1, sb.len, fp) < sb.len)
 		die_errno("could not write commit template");
 
@@ -918,8 +988,13 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		use_editor = 0;
 	if (edit_flag)
 		use_editor = 1;
-	if (!use_editor)
+	if (!use_editor) {
 		setenv("GIT_EDITOR", ":", 1);
+		edit_notes = NULL;
+	}
+	/* Magic value for "no ref passed" */
+	if (edit_notes == (void *)1)
+		edit_notes = default_notes_ref();
 
 	if (get_sha1("HEAD", head_sha1))
 		initial_commit = 1;
@@ -1288,6 +1363,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf author_ident = STRBUF_INIT;
+	struct strbuf notes = STRBUF_INIT;
 	const char *index_file, *reflog_msg;
 	char *nl, *p;
 	unsigned char commit_sha1[20];
@@ -1388,6 +1464,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 			strbuf_setlen(&sb, p - sb.buf + 1);
 	}
 
+	if (edit_notes)
+		extract_notes_from_message(&sb, &notes);
+
 	if (cleanup_mode != CLEANUP_NONE)
 		stripspace(&sb, cleanup_mode == CLEANUP_ALL);
 	if (message_is_empty(&sb) && !allow_empty_message) {
@@ -1403,6 +1482,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	}
 	strbuf_release(&author_ident);
 
+	if (edit_notes)
+		update_notes_for_commit(&notes, commit_sha1);
+	strbuf_release(&notes);
+
 	ref_lock = lock_any_ref_for_update("HEAD",
 					   initial_commit ? NULL : head_sha1,
 					   0);
@@ -1436,7 +1519,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 	rerere(0);
 	run_hook(get_index_file(), "post-commit", NULL);
-	if (amend && !no_post_rewrite) {
+	if (!edit_notes && amend && !no_post_rewrite) {
 		struct notes_rewrite_cfg *cfg;
 		cfg = init_copy_notes_for_rewrite("amend");
 		if (cfg) {
diff --git a/t/t7510-commit-notes.sh b/t/t7510-commit-notes.sh
new file mode 100755
index 0000000..ffc0781
--- /dev/null
+++ b/t/t7510-commit-notes.sh
@@ -0,0 +1,183 @@
+#!/bin/sh
+
+test_description='commit w/ --notes'
+. ./test-lib.sh
+
+# Fake editor to simulate user adding a note.
+cat >add.sh <<'EOF'
+perl -i -pe '
+  BEGIN { $n = shift }
+  # insert at $n-th blank line
+  if (/^$/ && ++$count == $n) {
+	  print "---\n";
+	  print "added note\n";
+	  print "with multiple lines\n";
+  }
+' "$@"
+EOF
+cat >expect-add <<'EOF'
+added note
+with multiple lines
+EOF
+
+# Fake editor to simulate user deleting a note.
+cat >del.sh <<'EOF'
+perl -i -ne '
+  if (/^---$/) {
+	  while (<>) {
+		  last if /^$/;
+	  }
+	  next;
+  }
+  print;
+' "$1"
+EOF
+
+# Fake editor to simulate user modifying a note.
+cat >mod.sh <<'EOF'
+perl -i -pe '
+  s/added note/modified note/
+' "$1"
+EOF
+cat >expect-mod <<'EOF'
+modified note
+with multiple lines
+EOF
+
+# Fake editor for leaving notes untouched.
+cat >nil.sh <<'EOF'
+EOF
+
+# Convenience function for setting up editor.
+set_editor() {
+	git config core.editor "\"$SHELL_PATH\" $1.sh $2"
+}
+
+cat >expect-msg <<'EOF'
+commit one
+
+this is the body of commit one
+EOF
+
+test_expect_success 'setup' '
+	test_commit one &&
+	git commit --amend -F expect-msg
+'
+
+test_expect_success 'add a note' '
+	set_editor add 2 &&
+	git commit --notes --amend &&
+	git notes show >actual &&
+	test_cmp expect-add actual &&
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect-msg actual
+'
+
+test_expect_success 'notes are preserved' '
+	set_editor nil &&
+	git commit --notes --amend &&
+	git notes show >actual &&
+	test_cmp expect-add actual &&
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect-msg actual
+'
+
+test_expect_success 'modify a note' '
+	set_editor mod &&
+	git commit --notes --amend &&
+	git notes show >actual &&
+	test_cmp expect-mod actual &&
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect-msg actual
+'
+
+test_expect_success 'delete a note' '
+	set_editor del &&
+	git commit --notes --amend &&
+	test_must_fail git notes show &&
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect-msg actual
+'
+
+test_expect_success 'add to commit without body' '
+	test_commit two &&
+	git log -1 --pretty=format:%B >expect-msg &&
+	set_editor add 1 &&
+	git commit --notes --amend &&
+	git notes show >actual &&
+	test_cmp expect-add actual &&
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect-msg actual
+'
+
+cat >expect-verbatim-msg <<'EOF'
+verbatim subject
+
+verbatim body
+# embedded comment
+
+EOF
+cat >expect-verbatim-note <<'EOF'
+
+verbatim note
+with leading and trailing whitespace
+# and embedded comments
+
+EOF
+cat >verbatim.sh <<'EOF'
+{
+	cat expect-verbatim-msg &&
+	echo --- &&
+	cat expect-verbatim-note
+} >"$1"
+EOF
+
+test_expect_success 'commit --cleanup=verbatim preserves message and notes' '
+	test_tick &&
+	echo content >>file && git add file &&
+	set_editor verbatim &&
+	git commit --notes --cleanup=verbatim &&
+	git cat-file commit HEAD >msg.raw &&
+	sed "1,/^\$/d" <msg.raw >actual &&
+	test_cmp expect-verbatim-msg actual &&
+	git notes show >actual &&
+	test_cmp expect-verbatim-note actual
+'
+
+test_expect_success 'commit -v does not interfere with notes' '
+	test_commit three &&
+	git log -1 --pretty=format:%B >expect-msg
+	set_editor add 1 &&
+	git commit -v --notes --amend &&
+	git notes show >actual &&
+	test_cmp actual expect-add &&
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect-msg actual
+'
+
+test_expect_success 'edit notes on alternate ref' '
+	test_commit four &&
+	set_editor add 1 &&
+	git commit --notes=foo --amend &&
+	test_must_fail git notes show &&
+	git notes --ref refs/notes/foo show >actual &&
+	test_cmp expect-add actual
+'
+
+test_expect_success 'ref rewriting does not overwrite our edits' '
+	git config notes.rewriteRef refs/notes/commits &&
+	test_commit five &&
+	set_editor add 1 &&
+	git commit --notes --amend &&
+	git notes show >actual &&
+	test_cmp expect-add actual &&
+	set_editor mod &&
+	git commit --notes --amend &&
+	git notes show >actual &&
+	test_cmp expect-mod actual &&
+	set_editor del &&
+	git commit --notes --amend &&
+	test_must_fail git notes show
+'
+
+test_done
-- 
1.7.4.31.g76e18

Re: [PATCH 1/2] notes: make expand_notes_ref globally accessible

[Johan Herland]

On Tuesday 08 March 2011, Jeff King wrote:
> This function is useful for other commands besides "git
> notes" which want to let users refer to notes by their
> shorthand name.
> 
> Signed-off-by: Jeff King <peff@peff.net>

Acked-by: Johan Herland <johan@herland.net>


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

Re: [PATCH 2/2] commit: allow editing notes in commit message editor

[Johan Herland]

On Tuesday 08 March 2011, Jeff King wrote:
> Changes from v1:
> 
>   - fix bug with adding notes to new commit (we failed to
>     initialize the notes tree properly in this case)
> 
>   - you can now do "commit --notes=foo" to view/edit
>     refs/notes/foo

Nice. :)

>   - added tests for basic operations, plus interaction with
>     --cleanup and -v
> 
>   - turn off commit rewriting when we edit
> 
> Todo:
> 
>   - commit.notes config variable to have this on all the time
> 
>   - I punted on the separator decision here.

We probably want to make it configurable, as mentioned earlier in the 
thread. Still, making it configurable gives me the somewhat uneasy feeling 
that we're "blaming" the user for any false positives ("It's your fault for 
not choosing a more unique separator...")...

What if we start the separator with a comment character (e.g. "# ---"). That 
way, the user could not expect a false positive to make it into the commit 
message in the first place (since it'd be stripped along with other 
comments). Of course, we'd have to make sure that the notes separator was 
parsed before removing the comments, but I think that's already taken care 
of in the patch below.

>   - probably still some magic needed for rebase conflict
>     case; we will be making a new commit, so we don't know
>     to pull the notes in from the old commit as we do with
>     --amend.

Maybe add a "--notes-copy=<commit>" argument to "git commit" that causes 
"<commit>" to be passed to add_notes_from_commit(). Of course, in the case 
of --amend, the default is "--notes-copy=HEAD".

>   - still needs the format-patch component to make the
>     workflow complete :)
> 
>  builtin/commit.c        |   87 ++++++++++++++++++++++-
>  t/t7510-commit-notes.sh |  183 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 268 insertions(+), 2 deletions(-)
>  create mode 100755 t/t7510-commit-notes.sh
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index d71e1e0..f84ca23 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c

[...]

> @@ -559,6 +564,68 @@ static char *cut_ident_timestamp_part(char *string)
>  	return ket;
>  }
> 
> +static void init_edit_notes() {

style nit: move "{" to next line.

[...]

> +static void update_notes_for_commit(struct strbuf *notes,
> +				    unsigned char *commit_sha1)
> +{
> +	init_edit_notes();
> +
> +	if (cleanup_mode != CLEANUP_NONE)
> +		stripspace(notes, cleanup_mode == CLEANUP_ALL);
> +
> +	if (!notes->len)
> +		remove_note(&edit_notes_tree, commit_sha1);
> +	else {
> +		unsigned char blob_sha1[20];
> +		if (write_sha1_file(notes->buf, notes->len,
> +				    blob_type, blob_sha1) < 0)
> +			die("unable to write note blob");
> +		add_note(&edit_notes_tree, commit_sha1, blob_sha1,
> +			 combine_notes_overwrite);

We may want to consider adding a small convenience function to the notes API 
for turning a strbuf into a notes blob. (Maybe s/strbuf/char* + len/ to 
cater for binary notes blobs as well.) This would move some low-level 
details (#include "blob.h", and write_sha1_file(...)) out of the notes API 
users' code.


Otherwise, this looks really good.


Have fun! :)

...Johan

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

Re: [RFC/PATCH] commit notes workflow

[Michel Lespinasse]

On Mon, Mar 7, 2011 at 3:39 PM, Jeff King <peff@peff.net> wrote:
> 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.

git often puts comment lines lines (starting with '#') around the
commit message. How about just adding one more such comment:

# Lines after this one will be added as git notes

and make it so that any non-empty line entered afterwards does get
added as notes ?


Also, I would love to see this functionality in other places such as
rebasing (if the original commit has notes attached, I would like
these to show up so I can attach these to the rebased commit as well).
I realize not everybody would want that, but this could be easily
controlled with message hooks...

-- 
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.

Re: [RFC/PATCH] commit notes workflow

[Yann Dirson]

That's a nice feature.

It may be good to extend the idea to support editing non-default notes
refs too.  Maybe something like:

<commit msg>
--- Notes ---
<info for GIT_NOTES_REF>
--- Notes <whatever> ---
<info for notes/whatever>

In this case, if we want to allow the user to customize the mark, we
will want to allow formatting like "--- Notes %N ---", but then the
defaulting for GIT_NOTES_REF would not fit - would we want to force the
use of "--- Notes commits ---" or similar ?  Maybe this would warrant a
separate mark for this default case:

	--default-note-mark="---"
	--note-mark="--- %N ---"

OTOH, using a single --note-mark and no special case for the default
notes ref seems more sane to me, since that shows the user when a
non-default GIT_NOTES_REF is in effect.

We may also want it to behave in a way similar to git-log, including
--show-notes[=<ref>] support to override the list of notes ref
to be considered.

-- 
Yann Dirson - Bertin Technologies