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@xxxxxxxxxxx>
www.herland.net
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html