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

From: Johan Herland <johan@herland.net>
Date: Mar 8, 2011, 09:15 UTC
Message-ID: <201103081015.24474.johan@herland.net>
In-Reply-To: <20110307234138.GB20912@sigill.intra.peff.net>

On Tuesday 08 March 2011, Jeff King wrote:

Show 7 quoted lines

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

Show 10 quoted lines

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

Show 12 quoted lines

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

[...]

Show 5 quoted lines

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

[...]

Show 17 quoted lines

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