Q. about usage of notes

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

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, [off-list ref]
www.herland.net

On 8/31/2010 11:07 AM, Johan Herland wrote:

On Tuesday 31 August 2010, Stefan Naewe wrote:

quoted

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.

quoted

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.

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.

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

On Tuesday 31 August 2010, Stefan Naewe wrote:

On 8/31/2010 11:07 AM, Johan Herland wrote:

quoted

On Tuesday 31 August 2010, Stefan Naewe wrote:

quoted

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, [off-list ref]
www.herland.net

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

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

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

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

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

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

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

-- 
Johan Herland, [off-list ref]
www.herland.net

Jeff King venit, vidit, dixit 31.08.2010 18:01:

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

quoted

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.

quoted

+	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