Q. about usage of notes

Q. about usage of notes

[Stefan Naewe]

Hi,

I was playing around with 'git notes' these days (after reading S.Chacons
post to this list and hist blog post at progit.org).

Some things came to my mind when doing some 'git notes add' and
'git notes remove':

How do I really get rid of git notes ? 'git notes remove' doesn't really
remove the notes but creates a new commit (like 'git rm file ; git commit..' does).

And why does 'git remove' do that repetetively (is that even a word...?), i.e. 
'git add -m"Note" ; git remove; git remove; git remove; git remove' creates 5
commit objects under 'refs/notes/commits' Is that the intended behaviour ?

I'm a little bit puzzled....

Regards,

Stefan
-- 
----------------------------------------------------------------
/dev/random says: It's not the principle of the thing, it's the money

Re: Q. about usage of notes

[Johan Herland]

On Tuesday 31 August 2010, Stefan Naewe wrote:
> Hi,
> 
> I was playing around with 'git notes' these days (after reading S.Chacons
> post to this list and hist blog post at progit.org).
> 
> Some things came to my mind when doing some 'git notes add' and
> 'git notes remove':
> 
> How do I really get rid of git notes ? 'git notes remove' doesn't really
> remove the notes but creates a new commit (like 'git rm file ; git
> commit..' does).

Well, how do you "really" get rid of a file on a "regular" Git branch? The 
answer is that Git doesn't let you do this without rewriting history (e.g. 
using 'git filter-branch' to create a new history where that file never 
existed). The same argument goes for notes: If you want to remove all traces 
of notes for a given object, you must rewrite the notes history so that 
those notes never existed.

At least that's how the 'git notes' porcelain behaves. At the plumbing 
level, it's possible to create notes commits that don't point to the 
preceding notes history (every notes commit is a root commit), and thus end 
up with "history-less" notes. An example of this is the notes-cache code.

> And why does 'git remove' do that repetetively (is that even a word...?),
> i.e. 'git add -m"Note" ; git remove; git remove; git remove; git remove'
> creates 5 commit objects under 'refs/notes/commits' Is that the intended
> behaviour ?

(I assume your meant to put "notes" in all those commands: git notes add, 
git notes remove, etc.)

Yes, this is the intended behaviour. Otherwise you would need a separate 
notes index where you could stage notes changes (with git notes add/remove), 
and then later commit those notes changes with (the imaginary) git notes 
commit. This was deemed too cumbersome/complicated, and we settled for the 
current approach instead.

If you want to make several adds/removes per notes commit, you could:

1. Use git-fast-import and its 'N' command (search the manual page for 
"notemodify").

2. Check out the notes ref into your working tree ("git checkout 
refs/notes/commits"). You can now edit notes directly, like you would edit 
your "regular" files. (Ideally, you should have some knowledge about the 
format of notes trees before you add/rename files). When you're done, you 
stage and commit as you would do in your regular checkout.

> I'm a little bit puzzled....

Hope this helps, :)

...Johan

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

Re: Q. about usage of notes

[Stefan Naewe]

On 8/31/2010 11:07 AM, Johan Herland wrote:
> On Tuesday 31 August 2010, Stefan Naewe wrote:
>> Hi,
>>
>> I was playing around with 'git notes' these days (after reading S.Chacons
>> post to this list and hist blog post at progit.org).
>>
>> Some things came to my mind when doing some 'git notes add' and
>> 'git notes remove':
>>
>> How do I really get rid of git notes ? 'git notes remove' doesn't really
>> remove the notes but creates a new commit (like 'git rm file ; git
>> commit..' does).
> 
> Well, how do you "really" get rid of a file on a "regular" Git branch? The 
> answer is that Git doesn't let you do this without rewriting history (e.g. 
> using 'git filter-branch' to create a new history where that file never 
> existed). The same argument goes for notes: If you want to remove all traces 
> of notes for a given object, you must rewrite the notes history so that 
> those notes never existed.

OK. Understood.
 
> At least that's how the 'git notes' porcelain behaves. At the plumbing 
> level, it's possible to create notes commits that don't point to the 
> preceding notes history (every notes commit is a root commit), and thus end 
> up with "history-less" notes. An example of this is the notes-cache code.
> 
>> And why does 'git remove' do that repetetively (is that even a word...?),
>> i.e. 'git add -m"Note" ; git remove; git remove; git remove; git remove'
>> creates 5 commit objects under 'refs/notes/commits' Is that the intended
>> behaviour ?
> 
> (I assume your meant to put "notes" in all those commands: git notes add, 
> git notes remove, etc.)

Yes, sorry.

> Yes, this is the intended behaviour. Otherwise you would need a separate 
> notes index where you could stage notes changes (with git notes add/remove), 
> and then later commit those notes changes with (the imaginary) git notes 
> commit. This was deemed too cumbersome/complicated, and we settled for the 
> current approach instead.

But if I do:

$ touch file ; git add file ; git commit -m"add file"

and then 

$ for n in 1 2 3; do git rm file; git commit -m"rm file"; done
 
I get:

rm 'file'
[master 5b24511] rm file
 0 files changed, 0 insertions(+), 0 deletions(-)
 delete mode 100644 file
fatal: pathspec 'file' did not match any files
# On branch master
nothing to commit (working directory clean)
fatal: pathspec 'file' did not match any files
# On branch master
nothing to commit (working directory clean)

I don't get 4 commits. That's the part I don't understand :-(

> If you want to make several adds/removes per notes commit, you could:
> 
> 1. Use git-fast-import and its 'N' command (search the manual page for 
> "notemodify").

OK. Thanks. Didn't know that. 

> 2. Check out the notes ref into your working tree ("git checkout 
> refs/notes/commits"). You can now edit notes directly, like you would edit 
> your "regular" files. (Ideally, you should have some knowledge about the 
> format of notes trees before you add/rename files). When you're done, you 
> stage and commit as you would do in your regular checkout.

OK. I already tried that.
 
And to delete the 'notes branch' I can only use 'git update-ref' ?!


Thanks for your answer

Stefan
-- 
----------------------------------------------------------------
/dev/random says: Nostalgia isn't what it used to be.

Re: Q. about usage of notes

[Stefan Naewe]

On 8/31/2010 12:13 PM, Stefan Naewe wrote:
> 
> But if I do:
> 
> $ touch file ; git add file ; git commit -m"add file"
> 
> and then 
> 
> $ for n in 1 2 3; do git rm file; git commit -m"rm file"; done
>  
> I get:
> 
> rm 'file'
> [master 5b24511] rm file
>  0 files changed, 0 insertions(+), 0 deletions(-)
>  delete mode 100644 file
> fatal: pathspec 'file' did not match any files
> # On branch master
> nothing to commit (working directory clean)
> fatal: pathspec 'file' did not match any files
> # On branch master
> nothing to commit (working directory clean)
> 
> I don't get 4 commits. That's the part I don't understand :-(

Just to be clear:

Of course I do understand why I don't get 4 commits here - I just don't
understand why I get a commit for every 'git notes remove' even if there's
nothing to remove.

Stefan
-- 
----------------------------------------------------------------
/dev/random says: This tagl ineh asto oman yfou rlet terw ords.

[RFC] notes: avoid recommitting identical trees

[Michael J Gruber]

Currently, "git notes" behaves like "git commit --allow-empty" when
committing notes trees. In particular, removing nonexisting notes leads
to empty commits "commits with no diff".

Change this to avoid unnecessary notes commits.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
I can't believe there's no easier way to lookup the sha1 of a tree of a commit
but I didn't find any, and I did not want to employ the diff machinery for diffing
the trees when their sha1 is (should be) known.

 builtin/notes.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index fbc347c..48da228 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -303,11 +303,15 @@ int commit_notes(struct notes_tree *t, const char *msg)
 		hashclr(prev_commit);
 		parent = NULL;
 	}
-	if (commit_tree(buf.buf + 7, tree_sha1, parent, new_commit, NULL))
-		die("Failed to commit notes tree to database");
-
-	/* Update notes ref with new commit */
-	update_ref(buf.buf, t->ref, new_commit, prev_commit, 0, DIE_ON_ERR);
+	if (!parent || parse_commit(parent->item) || parse_tree(parent->item->tree) ||
+		hashcmp(parent->item->tree->object.sha1, tree_sha1)) {
+		/* avoid recommitting the same tree */
+		if (commit_tree(buf.buf + 7, tree_sha1, parent, new_commit, NULL))
+			die("Failed to commit notes tree to database");
+
+		/* Update notes ref with new commit */
+		update_ref(buf.buf, t->ref, new_commit, prev_commit, 0, DIE_ON_ERR);
+	}
 
 	strbuf_release(&buf);
 	return 0;
-- 
1.7.2.2.398.g5af70

Re: [RFC] notes: avoid recommitting identical trees

[Jeff King]

On Tue, Aug 31, 2010 at 05:16:17PM +0200, Michael J Gruber wrote:

> Currently, "git notes" behaves like "git commit --allow-empty" when
> committing notes trees. In particular, removing nonexisting notes leads
> to empty commits "commits with no diff".
> 
> Change this to avoid unnecessary notes commits.

Is this a sufficient check in the case of notes? Is it possible that we
re-balanced the fanout of the notes tree and got a different tree sha1,
even though there is nothing interesting to commit?

> +	if (!parent || parse_commit(parent->item) || parse_tree(parent->item->tree) ||
> +		hashcmp(parent->item->tree->object.sha1, tree_sha1)) {

I didn't check, but I can imagine you can drop the parse_tree here. We
should know the object sha1 once the commit is parsed.

-Peff

Re: [RFC] notes: avoid recommitting identical trees

[Michael J Gruber]

Jeff King venit, vidit, dixit 31.08.2010 18:01:
> On Tue, Aug 31, 2010 at 05:16:17PM +0200, Michael J Gruber wrote:
> 
>> Currently, "git notes" behaves like "git commit --allow-empty" when
>> committing notes trees. In particular, removing nonexisting notes leads
>> to empty commits "commits with no diff".
>>
>> Change this to avoid unnecessary notes commits.
> 
> Is this a sufficient check in the case of notes? Is it possible that we
> re-balanced the fanout of the notes tree and got a different tree sha1,
> even though there is nothing interesting to commit?

Yes, but I don't think this hurts. The main thrust here is to catch the
case of repeated "git notes remove". Also, we might even want to record
the history when there is rebalancing since this is indeed a tree change.

Johan's (later ;) ) approach, while being more intrusive, catches this
at the point of removal - if there's nothing to remove, nothing gets
rewritten.

> 
>> +	if (!parent || parse_commit(parent->item) || parse_tree(parent->item->tree) ||
>> +		hashcmp(parent->item->tree->object.sha1, tree_sha1)) {
> 
> I didn't check, but I can imagine you can drop the parse_tree here. We
> should know the object sha1 once the commit is parsed.

parse_commit() does a lookup_tree() but I don't think that it parses the
tree, i.e. I don't hink it fills in tree->object.sha1. At least it
segfaulted without that ;)

Michael

Re: [RFC] notes: avoid recommitting identical trees

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

>>> +	if (!parent || parse_commit(parent->item) || parse_tree(parent->item->tree) ||
>>> +		hashcmp(parent->item->tree->object.sha1, tree_sha1)) {
>> 
>> I didn't check, but I can imagine you can drop the parse_tree here. We
>> should know the object sha1 once the commit is parsed.
>
> parse_commit() does a lookup_tree() but I don't think that it parses the
> tree, i.e. I don't hink it fills in tree->object.sha1.

Huh?  parse_tree(tree) calls read_sha1_file(tree->object.sha1) to parse
the tree.  How can it do without filling tree->object.sha1?

Re: [RFC] notes: avoid recommitting identical trees

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 31.08.2010 18:44:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>>>> +	if (!parent || parse_commit(parent->item) || parse_tree(parent->item->tree) ||
>>>> +		hashcmp(parent->item->tree->object.sha1, tree_sha1)) {
>>>
>>> I didn't check, but I can imagine you can drop the parse_tree here. We
>>> should know the object sha1 once the commit is parsed.
>>
>> parse_commit() does a lookup_tree() but I don't think that it parses the
>> tree, i.e. I don't hink it fills in tree->object.sha1.
> 
> Huh?  parse_tree(tree) calls read_sha1_file(tree->object.sha1) to parse
> the tree.  How can it do without filling tree->object.sha1?
> 

Sure parse_tree() does that. That's why I call it. I never claimed it
doesn't.

Michael

Re: [RFC] notes: avoid recommitting identical trees

[Jeff King]

On Tue, Aug 31, 2010 at 08:26:34PM +0200, Michael J Gruber wrote:

> Junio C Hamano venit, vidit, dixit 31.08.2010 18:44:
> > Michael J Gruber <git@drmicha.warpmail.net> writes:
> > 
> >>>> +	if (!parent || parse_commit(parent->item) || parse_tree(parent->item->tree) ||
> >>>> +		hashcmp(parent->item->tree->object.sha1, tree_sha1)) {
> >>>
> >>> I didn't check, but I can imagine you can drop the parse_tree here. We
> >>> should know the object sha1 once the commit is parsed.
> >>
> >> parse_commit() does a lookup_tree() but I don't think that it parses the
> >> tree, i.e. I don't hink it fills in tree->object.sha1.
> > 
> > Huh?  parse_tree(tree) calls read_sha1_file(tree->object.sha1) to parse
> > the tree.  How can it do without filling tree->object.sha1?
> > 
> 
> Sure parse_tree() does that. That's why I call it. I never claimed it
> doesn't.

I think the claim is that it is already parsed. Look at parse_tree. The
first non-declaration lines are:

  if (item->object.parsed)
          return 0;
  buffer = read_sha1_file(item->object.sha1, &type, &size);

So _somebody_ has already filled in item->object.sha1.

-Peff

Re: [RFC] notes: avoid recommitting identical trees

[Michael J Gruber]

Jeff King venit, vidit, dixit 31.08.2010 20:29:
> On Tue, Aug 31, 2010 at 08:26:34PM +0200, Michael J Gruber wrote:
> 
>> Junio C Hamano venit, vidit, dixit 31.08.2010 18:44:
>>> Michael J Gruber <git@drmicha.warpmail.net> writes:
>>>
>>>>>> +	if (!parent || parse_commit(parent->item) || parse_tree(parent->item->tree) ||
>>>>>> +		hashcmp(parent->item->tree->object.sha1, tree_sha1)) {
>>>>>
>>>>> I didn't check, but I can imagine you can drop the parse_tree here. We
>>>>> should know the object sha1 once the commit is parsed.
>>>>
>>>> parse_commit() does a lookup_tree() but I don't think that it parses the
>>>> tree, i.e. I don't hink it fills in tree->object.sha1.
>>>
>>> Huh?  parse_tree(tree) calls read_sha1_file(tree->object.sha1) to parse
>>> the tree.  How can it do without filling tree->object.sha1?
>>>
>>
>> Sure parse_tree() does that. That's why I call it. I never claimed it
>> doesn't.
> 
> I think the claim is that it is already parsed. Look at parse_tree. The
> first non-declaration lines are:
> 
>   if (item->object.parsed)
>           return 0;
>   buffer = read_sha1_file(item->object.sha1, &type, &size);
> 
> So _somebody_ has already filled in item->object.sha1.
> 
> -Peff

OK, now I understand Junio's answer...
...and I also see where lookup_tree() fills the sha1, which I failed to
see before.

I guess what happened was that I had the parse_tree without
parse_commit, which segfaulted, and then inserted parse_commit.

In any case, it seems Johan is going with his approach, so you can
forget about the RFC, be it with or without parse_tree.

Michael

Re: [RFC] notes: avoid recommitting identical trees

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Junio C Hamano venit, vidit, dixit 31.08.2010 18:44:
>> Michael J Gruber <git@drmicha.warpmail.net> writes:
>> 
>>>>> +	if (!parent || parse_commit(parent->item) || parse_tree(parent->item->tree) ||
>>>>> +		hashcmp(parent->item->tree->object.sha1, tree_sha1)) {
>>>>
>>>> I didn't check, but I can imagine you can drop the parse_tree here. We
>>>> should know the object sha1 once the commit is parsed.
>>>
>>> parse_commit() does a lookup_tree() but I don't think that it parses the
>>> tree, i.e. I don't hink it fills in tree->object.sha1.
>> 
>> Huh?  parse_tree(tree) calls read_sha1_file(tree->object.sha1) to parse
>> the tree.  How can it do without filling tree->object.sha1?
>
> Sure parse_tree() does that. That's why I call it. I never claimed it
> doesn't.

Sorry, My second sentence was incorrect.

       How can it do so without getting a tree with tree->object.sha1
       already filled?

Re: [RFC] notes: avoid recommitting identical trees

[Johan Herland]

On Tuesday 31 August 2010, Michael J Gruber wrote:
> Currently, "git notes" behaves like "git commit --allow-empty" when
> committing notes trees. In particular, removing nonexisting notes
> leads to empty commits "commits with no diff".
>
> Change this to avoid unnecessary notes commits.
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>

I just posted a patch with the same objective, but with a different 
approach. Instead of parsing the previous commit and comparing tree 
object SHA1s, I add a few lines of notes code to let remove_note() 
report whether it removed a note or not (thus determining whether a 
commit is necessary or not).

In general, the notes_tree.dirty flag should be sufficient to determine 
whether a commit is needed or not (remove_note()'s unconditional 
setting of this flag is also fixed in my patch).


...Johan

> ---
> I can't believe there's no easier way to lookup the sha1 of a tree of
> a commit but I didn't find any, and I did not want to employ the diff
> machinery for diffing the trees when their sha1 is (should be) known.
>
>  builtin/notes.c |   14 +++++++++-----
>  1 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/notes.c b/builtin/notes.c
> index fbc347c..48da228 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -303,11 +303,15 @@ int commit_notes(struct notes_tree *t, const
> char *msg) hashclr(prev_commit);
>  		parent = NULL;
>  	}
> -	if (commit_tree(buf.buf + 7, tree_sha1, parent, new_commit, NULL))
> -		die("Failed to commit notes tree to database");
> -
> -	/* Update notes ref with new commit */
> -	update_ref(buf.buf, t->ref, new_commit, prev_commit, 0,
> DIE_ON_ERR); +	if (!parent || parse_commit(parent->item) ||
> parse_tree(parent->item->tree) ||
> +		hashcmp(parent->item->tree->object.sha1, tree_sha1)) {
> +		/* avoid recommitting the same tree */
> +		if (commit_tree(buf.buf + 7, tree_sha1, parent, new_commit, NULL))
> +			die("Failed to commit notes tree to database");
> +
> +		/* Update notes ref with new commit */
> +		update_ref(buf.buf, t->ref, new_commit, prev_commit, 0,
> DIE_ON_ERR); +	}
>
>  	strbuf_release(&buf);
>  	return 0;



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

Re: Q. about usage of notes

[Johan Herland]

On Tuesday 31 August 2010, Stefan Naewe wrote:
> On 8/31/2010 11:07 AM, Johan Herland wrote:
> > On Tuesday 31 August 2010, Stefan Naewe wrote:
> >> And why does 'git remove' do that repetetively (is that even a
> >> word...?), i.e. 'git add -m"Note" ; git remove; git remove; git
> >> remove; git remove' creates 5 commit objects under
> >> 'refs/notes/commits' Is that the intended behaviour ?
> >
> > Yes, this is the intended behaviour. Otherwise you would need a
> > separate notes index where you could stage notes changes (with git
> > notes add/remove), and then later commit those notes changes with
> > (the imaginary) git notes commit. This was deemed too
> > cumbersome/complicated, and we settled for the current approach
> > instead.
>
> But if I do:
>
> $ touch file ; git add file ; git commit -m"add file"
>
> and then
>
> $ for n in 1 2 3; do git rm file; git commit -m"rm file"; done
>
> I get:
>
> rm 'file'
> [master 5b24511] rm file
>  0 files changed, 0 insertions(+), 0 deletions(-)
>  delete mode 100644 file
> fatal: pathspec 'file' did not match any files
> # On branch master
> nothing to commit (working directory clean)
> fatal: pathspec 'file' did not match any files
> # On branch master
> nothing to commit (working directory clean)
>
> I don't get 4 commits. That's the part I don't understand :-(
> 
> Just to be clear:
> 
> Of course I do understand why I don't get 4 commits here - I just
> don't understand why I get a commit for every 'git notes remove' even
> if there's nothing to remove.

Ah, that would be a bug in 'git notes remove'. Patch coming soon.

> And to delete the 'notes branch' I can only use 'git update-ref' ?!

Yes, 'git update-ref -d refs/notes/foo' will delete the "foo" notes 
branch.


...Johan

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

[PATCH 1/2] notes: Don't create (empty) commit when removing non-existing notes

[Johan Herland]

Extend remove_note() in the notes API to return whether or not a note was
actually removed. Use this in 'git notes remove' to skip the creation of
a notes commit when no notes were actually removed.

Also add a test illustrating the change in behavior.

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

This patch is against master.

 builtin/notes.c  |   14 ++++++++++----
 notes.c          |   14 ++++++++++----
 notes.h          |    4 +++-
 t/t3301-notes.sh |    7 +++++++
 4 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index fbc347c..6d07aac 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -769,6 +769,7 @@ static int remove_cmd(int argc, const char **argv, const char *prefix)
 	const char *object_ref;
 	struct notes_tree *t;
 	unsigned char object[20];
+	int retval;
 
 	argc = parse_options(argc, argv, prefix, options,
 			     git_notes_remove_usage, 0);
@@ -785,12 +786,17 @@ static int remove_cmd(int argc, const char **argv, const char *prefix)
 
 	t = init_notes_check("remove");
 
-	fprintf(stderr, "Removing note for object %s\n", sha1_to_hex(object));
-	remove_note(t, object);
+	retval = remove_note(t, object);
+	if (retval)
+		fprintf(stderr, "Object %s has no note\n", sha1_to_hex(object));
+	else {
+		fprintf(stderr, "Removing note for object %s\n",
+			sha1_to_hex(object));
 
-	commit_notes(t, "Notes removed by 'git notes remove'");
+		commit_notes(t, "Notes removed by 'git notes remove'");
+	}
 	free_notes(t);
-	return 0;
+	return retval;
 }
 
 static int prune(int argc, const char **argv, const char *prefix)
diff --git a/notes.c b/notes.c
index 7fd2035..70d0013 100644
--- a/notes.c
+++ b/notes.c
@@ -263,11 +263,13 @@ static int note_tree_consolidate(struct int_node *tree,
  * To remove a leaf_node:
  * Search to the tree location appropriate for the given leaf_node's key:
  * - If location does not hold a matching entry, abort and do nothing.
+ * - Copy the matching entry's value into the given entry.
  * - Replace the matching leaf_node with a NULL entry (and free the leaf_node).
  * - Consolidate int_nodes repeatedly, while walking up the tree towards root.
  */
-static void note_tree_remove(struct notes_tree *t, struct int_node *tree,
-		unsigned char n, struct leaf_node *entry)
+static void note_tree_remove(struct notes_tree *t,
+		struct int_node *tree, unsigned char n,
+		struct leaf_node *entry)
 {
 	struct leaf_node *l;
 	struct int_node *parent_stack[20];
@@ -282,6 +284,7 @@ static void note_tree_remove(struct notes_tree *t, struct int_node *tree,
 		return; /* key mismatch, nothing to remove */
 
 	/* we have found a matching entry */
+	hashcpy(entry->val_sha1, l->val_sha1);
 	free(l);
 	*p = SET_PTR_TYPE(NULL, PTR_TYPE_NULL);
 
@@ -1003,17 +1006,20 @@ void add_note(struct notes_tree *t, const unsigned char *object_sha1,
 	note_tree_insert(t, t->root, 0, l, PTR_TYPE_NOTE, combine_notes);
 }
 
-void remove_note(struct notes_tree *t, const unsigned char *object_sha1)
+int remove_note(struct notes_tree *t, const unsigned char *object_sha1)
 {
 	struct leaf_node l;
 
 	if (!t)
 		t = &default_notes_tree;
 	assert(t->initialized);
-	t->dirty = 1;
 	hashcpy(l.key_sha1, object_sha1);
 	hashclr(l.val_sha1);
 	note_tree_remove(t, t->root, 0, &l);
+	if (is_null_sha1(l.val_sha1)) // no note was removed
+		return 1;
+	t->dirty = 1;
+	return 0;
 }
 
 const unsigned char *get_note(struct notes_tree *t,
diff --git a/notes.h b/notes.h
index 65fc3a6..5106761 100644
--- a/notes.h
+++ b/notes.h
@@ -89,8 +89,10 @@ void add_note(struct notes_tree *t, const unsigned char *object_sha1,
  * IMPORTANT: The changes made by remove_note() to the given notes_tree
  * structure are not persistent until a subsequent call to write_notes_tree()
  * returns zero.
+ *
+ * Return 0 if a note was removed; 1 if there was no note to remove.
  */
-void remove_note(struct notes_tree *t, const unsigned char *object_sha1);
+int remove_note(struct notes_tree *t, const unsigned char *object_sha1);
 
 /*
  * Get the note object SHA1 containing the note data for the given object
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 96b7581..a2b79a0 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -365,6 +365,13 @@ c18dc024e14f08d18d14eea0d747ff692d66d6a3 1584215f1d29c65e99c6c6848626553fdd07fd7
 c9c6af7f78bc47490dbf3e822cf2f3c24d4b9061 268048bfb8a1fb38e703baceb8ab235421bf80c5
 EOF
 
+test_expect_success 'removing non-existing note should not create new commit' '
+	git rev-parse --verify refs/notes/commits > before_commit &&
+	test_must_fail git notes remove HEAD^ &&
+	git rev-parse --verify refs/notes/commits > after_commit &&
+	test_cmp before_commit after_commit
+'
+
 test_expect_success 'list notes with "git notes list"' '
 	git notes list > output &&
 	test_cmp expect output
-- 
1.7.2.220.gea1d3

[PATCH 2/2] notes.h: Extend remove_note to return the SHA1 of the removed note, if any

[Johan Herland]

It might be useful to identify which note was removed by a call to
remove_note(). Add an optional parameter to remove_note() for recording
the SHA1 of the removed note object. If no note is removed by remove_note(),
null_sha1 is stored in this parameter.

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

This patch is very much optional. It adds a small API extension that
_might_ be useful in the future.


 builtin/notes.c |    6 +++---
 notes.c         |    7 +++++--
 notes.h         |    6 +++++-
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 6d07aac..e5c8208 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -572,7 +572,7 @@ static int add(int argc, const char **argv, const char *prefix)
 	create_note(object, &msg, 0, note, new_note);
 
 	if (is_null_sha1(new_note))
-		remove_note(t, object);
+		remove_note(t, object, NULL);
 	else
 		add_note(t, object, new_note, combine_notes_overwrite);
 
@@ -711,7 +711,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)
 	create_note(object, &msg, !edit, note, new_note);
 
 	if (is_null_sha1(new_note))
-		remove_note(t, object);
+		remove_note(t, object, NULL);
 	else
 		add_note(t, object, new_note, combine_notes_overwrite);
 
@@ -786,7 +786,7 @@ static int remove_cmd(int argc, const char **argv, const char *prefix)
 
 	t = init_notes_check("remove");
 
-	retval = remove_note(t, object);
+	retval = remove_note(t, object, NULL);
 	if (retval)
 		fprintf(stderr, "Object %s has no note\n", sha1_to_hex(object));
 	else {
diff --git a/notes.c b/notes.c
index 70d0013..9fe857b 100644
--- a/notes.c
+++ b/notes.c
@@ -1006,7 +1006,8 @@ void add_note(struct notes_tree *t, const unsigned char *object_sha1,
 	note_tree_insert(t, t->root, 0, l, PTR_TYPE_NOTE, combine_notes);
 }
 
-int remove_note(struct notes_tree *t, const unsigned char *object_sha1)
+int remove_note(struct notes_tree *t, const unsigned char *object_sha1,
+		unsigned char *removed_note)
 {
 	struct leaf_node l;
 
@@ -1016,6 +1017,8 @@ int remove_note(struct notes_tree *t, const unsigned char *object_sha1)
 	hashcpy(l.key_sha1, object_sha1);
 	hashclr(l.val_sha1);
 	note_tree_remove(t, t->root, 0, &l);
+	if (removed_note)
+		hashcpy(removed_note, l.val_sha1);
 	if (is_null_sha1(l.val_sha1)) // no note was removed
 		return 1;
 	t->dirty = 1;
@@ -1085,7 +1088,7 @@ void prune_notes(struct notes_tree *t, int flags)
 		if (flags & NOTES_PRUNE_VERBOSE)
 			printf("%s\n", sha1_to_hex(l->sha1));
 		if (!(flags & NOTES_PRUNE_DRYRUN))
-			remove_note(t, l->sha1);
+			remove_note(t, l->sha1, NULL);
 		l = l->next;
 	}
 }
diff --git a/notes.h b/notes.h
index 5106761..c61e962 100644
--- a/notes.h
+++ b/notes.h
@@ -90,9 +90,13 @@ void add_note(struct notes_tree *t, const unsigned char *object_sha1,
  * structure are not persistent until a subsequent call to write_notes_tree()
  * returns zero.
  *
+ * If removed_note is non-NULL, the SHA1 of the removed note will be written
+ * there. If there was no note to remove, then null_sha1 will be written there.
+ *
  * Return 0 if a note was removed; 1 if there was no note to remove.
  */
-int remove_note(struct notes_tree *t, const unsigned char *object_sha1);
+int remove_note(struct notes_tree *t, const unsigned char *object_sha1,
+		unsigned char *removed_note);
 
 /*
  * Get the note object SHA1 containing the note data for the given object
-- 
1.7.2.220.gea1d3