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

[Johan Herland <johan@herland.net>]

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