threads / discuss / 24920

Q. about usage of notes

Subject: Q. about usage of notes

## tl;dr

16 messages between Aug 31, 2010 and Aug 31, 2010.

replies: 15people: 5as markdown or json

Stefan Naewe· Aug 31, 2010, 08:09 UTC · lore
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
Johan Herland· Aug 31, 2010, 09:07 UTC · re: Stefan Naewe · lore

Re: Q. about usage of notes

On Tuesday 31 August 2010, Stefan Naewe wrote:
Show 11 quoted lines
> 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
Stefan Naewe· Aug 31, 2010, 10:13 UTC · re: Johan Herland · lore

Re: Q. about usage of notes

On 8/31/2010 11:07 AM, Johan Herland wrote:
Show 19 quoted lines
> 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.
Show 12 quoted lines
> 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.
Show 5 quoted lines
> 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.
Show 5 quoted lines
> 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.
Stefan Naewe· Aug 31, 2010, 10:15 UTC · re: Stefan Naewe · lore

Re: Q. about usage of notes

On 8/31/2010 12:13 PM, Stefan Naewe wrote:
Show 23 quoted lines
> 
> 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.
Michael J Gruber· Aug 31, 2010, 15:16 UTC · re: Stefan Naewe · lore

[RFC] notes: avoid recommitting identical trees

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
Jeff King· Aug 31, 2010, 16:01 UTC · re: Michael J Gruber · lore

Re: [RFC] notes: avoid recommitting identical trees

On Tue, Aug 31, 2010 at 05:16:17PM +0200, Michael J Gruber wrote:
Show 5 quoted lines
> 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
Michael J Gruber· Aug 31, 2010, 16:15 UTC · re: Jeff King · lore

Re: [RFC] notes: avoid recommitting identical trees

Jeff King venit, vidit, dixit 31.08.2010 18:01:
Show 11 quoted lines
> 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.

Show 6 quoted lines
> 
>> +	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
Junio C Hamano· Aug 31, 2010, 16:44 UTC · re: Michael J Gruber · lore

Re: [RFC] notes: avoid recommitting identical trees

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 8 quoted lines
>>> +	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?

Michael J Gruber· Aug 31, 2010, 18:26 UTC · re: Junio C Hamano · lore

Re: [RFC] notes: avoid recommitting identical trees

Junio C Hamano venit, vidit, dixit 31.08.2010 18:44:
Show 14 quoted lines
> 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
Jeff King· Aug 31, 2010, 18:29 UTC · re: Michael J Gruber · lore

Re: [RFC] notes: avoid recommitting identical trees

On Tue, Aug 31, 2010 at 08:26:34PM +0200, Michael J Gruber wrote:
Show 18 quoted lines
> 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
Michael J Gruber· Aug 31, 2010, 18:45 UTC · re: Jeff King · lore

Re: [RFC] notes: avoid recommitting identical trees

Jeff King venit, vidit, dixit 31.08.2010 20:29:
Show 31 quoted lines
> 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
Junio C Hamano· Aug 31, 2010, 18:43 UTC · re: Michael J Gruber · lore

Re: [RFC] notes: avoid recommitting identical trees

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 17 quoted lines
> 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?
Johan Herland· Aug 31, 2010, 16:08 UTC · re: Michael J Gruber · lore

Re: [RFC] notes: avoid recommitting identical trees

On Tuesday 31 August 2010, Michael J Gruber wrote:
Show 7 quoted lines
> 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
Show 34 quoted lines
> ---
> 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
Johan Herland· Aug 31, 2010, 15:53 UTC · re: Stefan Naewe · lore

Re: Q. about usage of notes

On Tuesday 31 August 2010, Stefan Naewe wrote:
Show 42 quoted lines
> 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
Johan Herland· Aug 31, 2010, 15:56 UTC · re: Johan Herland · lore

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

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
Johan Herland· Aug 31, 2010, 15:59 UTC · re: Johan Herland · lore

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

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

← back to recent threads