{"thread":{"id":"24920","subject":"Q. about usage of notes","startedAt":"2010-08-31T08:09:19Z","lastAt":"2010-08-31T18:45:22Z","messageCount":16,"participants":["Stefan Naewe","Johan Herland","Michael J Gruber","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"149402","messageId":"4C7CB8AF.5080008@atlas-elektronik.com","threadId":"24920","inReplyTo":null,"subject":"Q. about usage of notes","fromName":"Stefan Naewe","fromEmail":"stefan.naewe@atlas-elektronik.com","sentAt":"2010-08-31T08:09:19Z","receivedAt":"2010-08-31T08:09:19Z","isPatch":false,"sender":{"key":"stefan.naewe@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4468?v=4"},"body":"Hi,\n\nI was playing around with 'git notes' these days (after reading S.Chacons\npost to this list and hist blog post at progit.org).\n\nSome things came to my mind when doing some 'git notes add' and\n'git notes remove':\n\nHow do I really get rid of git notes ? 'git notes remove' doesn't really\nremove the notes but creates a new commit (like 'git rm file ; git commit..' does).\n\nAnd why does 'git remove' do that repetetively (is that even a word...?), i.e. \n'git add -m\"Note\" ; git remove; git remove; git remove; git remove' creates 5\ncommit objects under 'refs/notes/commits' Is that the intended behaviour ?\n\nI'm a little bit puzzled....\n\nRegards,\n\nStefan\n-- \n----------------------------------------------------------------\n/dev/random says: It's not the principle of the thing, it's the money\n"},{"id":"149404","messageId":"201008311107.32278.johan@herland.net","threadId":"24920","inReplyTo":"4C7CB8AF.5080008@atlas-elektronik.com","subject":"Re: Q. about usage of notes","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2010-08-31T09:07:31Z","receivedAt":"2010-08-31T09:07:31Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Tuesday 31 August 2010, Stefan Naewe wrote:\n> Hi,\n> \n> I was playing around with 'git notes' these days (after reading S.Chacons\n> post to this list and hist blog post at progit.org).\n> \n> Some things came to my mind when doing some 'git notes add' and\n> 'git notes remove':\n> \n> How do I really get rid of git notes ? 'git notes remove' doesn't really\n> remove the notes but creates a new commit (like 'git rm file ; git\n> commit..' does).\n\nWell, how do you \"really\" get rid of a file on a \"regular\" Git branch? The \nanswer is that Git doesn't let you do this without rewriting history (e.g. \nusing 'git filter-branch' to create a new history where that file never \nexisted). The same argument goes for notes: If you want to remove all traces \nof notes for a given object, you must rewrite the notes history so that \nthose notes never existed.\n\nAt least that's how the 'git notes' porcelain behaves. At the plumbing \nlevel, it's possible to create notes commits that don't point to the \npreceding notes history (every notes commit is a root commit), and thus end \nup with \"history-less\" notes. An example of this is the notes-cache code.\n\n> And why does 'git remove' do that repetetively (is that even a word...?),\n> i.e. 'git add -m\"Note\" ; git remove; git remove; git remove; git remove'\n> creates 5 commit objects under 'refs/notes/commits' Is that the intended\n> behaviour ?\n\n(I assume your meant to put \"notes\" in all those commands: git notes add, \ngit notes remove, etc.)\n\nYes, this is the intended behaviour. Otherwise you would need a separate \nnotes index where you could stage notes changes (with git notes add/remove), \nand then later commit those notes changes with (the imaginary) git notes \ncommit. This was deemed too cumbersome/complicated, and we settled for the \ncurrent approach instead.\n\nIf you want to make several adds/removes per notes commit, you could:\n\n1. Use git-fast-import and its 'N' command (search the manual page for \n\"notemodify\").\n\n2. Check out the notes ref into your working tree (\"git checkout \nrefs/notes/commits\"). You can now edit notes directly, like you would edit \nyour \"regular\" files. (Ideally, you should have some knowledge about the \nformat of notes trees before you add/rename files). When you're done, you \nstage and commit as you would do in your regular checkout.\n\n> I'm a little bit puzzled....\n\nHope this helps, :)\n\n...Johan\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"149405","messageId":"4C7CD5B8.20602@atlas-elektronik.com","threadId":"24920","inReplyTo":"201008311107.32278.johan@herland.net","subject":"Re: Q. about usage of notes","fromName":"Stefan Naewe","fromEmail":"stefan.naewe@atlas-elektronik.com","sentAt":"2010-08-31T10:13:12Z","receivedAt":"2010-08-31T10:13:12Z","isPatch":false,"sender":{"key":"stefan.naewe@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4468?v=4"},"body":"On 8/31/2010 11:07 AM, Johan Herland wrote:\n> On Tuesday 31 August 2010, Stefan Naewe wrote:\n>> Hi,\n>>\n>> I was playing around with 'git notes' these days (after reading S.Chacons\n>> post to this list and hist blog post at progit.org).\n>>\n>> Some things came to my mind when doing some 'git notes add' and\n>> 'git notes remove':\n>>\n>> How do I really get rid of git notes ? 'git notes remove' doesn't really\n>> remove the notes but creates a new commit (like 'git rm file ; git\n>> commit..' does).\n> \n> Well, how do you \"really\" get rid of a file on a \"regular\" Git branch? The \n> answer is that Git doesn't let you do this without rewriting history (e.g. \n> using 'git filter-branch' to create a new history where that file never \n> existed). The same argument goes for notes: If you want to remove all traces \n> of notes for a given object, you must rewrite the notes history so that \n> those notes never existed.\n\nOK. Understood.\n \n> At least that's how the 'git notes' porcelain behaves. At the plumbing \n> level, it's possible to create notes commits that don't point to the \n> preceding notes history (every notes commit is a root commit), and thus end \n> up with \"history-less\" notes. An example of this is the notes-cache code.\n> \n>> And why does 'git remove' do that repetetively (is that even a word...?),\n>> i.e. 'git add -m\"Note\" ; git remove; git remove; git remove; git remove'\n>> creates 5 commit objects under 'refs/notes/commits' Is that the intended\n>> behaviour ?\n> \n> (I assume your meant to put \"notes\" in all those commands: git notes add, \n> git notes remove, etc.)\n\nYes, sorry.\n\n> Yes, this is the intended behaviour. Otherwise you would need a separate \n> notes index where you could stage notes changes (with git notes add/remove), \n> and then later commit those notes changes with (the imaginary) git notes \n> commit. This was deemed too cumbersome/complicated, and we settled for the \n> current approach instead.\n\nBut if I do:\n\n$ touch file ; git add file ; git commit -m\"add file\"\n\nand then \n\n$ for n in 1 2 3; do git rm file; git commit -m\"rm file\"; done\n \nI get:\n\nrm 'file'\n[master 5b24511] rm file\n 0 files changed, 0 insertions(+), 0 deletions(-)\n delete mode 100644 file\nfatal: pathspec 'file' did not match any files\n# On branch master\nnothing to commit (working directory clean)\nfatal: pathspec 'file' did not match any files\n# On branch master\nnothing to commit (working directory clean)\n\nI don't get 4 commits. That's the part I don't understand :-(\n\n> If you want to make several adds/removes per notes commit, you could:\n> \n> 1. Use git-fast-import and its 'N' command (search the manual page for \n> \"notemodify\").\n\nOK. Thanks. Didn't know that. \n\n> 2. Check out the notes ref into your working tree (\"git checkout \n> refs/notes/commits\"). You can now edit notes directly, like you would edit \n> your \"regular\" files. (Ideally, you should have some knowledge about the \n> format of notes trees before you add/rename files). When you're done, you \n> stage and commit as you would do in your regular checkout.\n\nOK. I already tried that.\n \nAnd to delete the 'notes branch' I can only use 'git update-ref' ?!\n\n\nThanks for your answer\n\nStefan\n-- \n----------------------------------------------------------------\n/dev/random says: Nostalgia isn't what it used to be.\n"},{"id":"149406","messageId":"4C7CD65F.10509@atlas-elektronik.com","threadId":"24920","inReplyTo":"4C7CD5B8.20602@atlas-elektronik.com","subject":"Re: Q. about usage of notes","fromName":"Stefan Naewe","fromEmail":"stefan.naewe@atlas-elektronik.com","sentAt":"2010-08-31T10:15:59Z","receivedAt":"2010-08-31T10:15:59Z","isPatch":false,"sender":{"key":"stefan.naewe@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4468?v=4"},"body":"On 8/31/2010 12:13 PM, Stefan Naewe wrote:\n> \n> But if I do:\n> \n> $ touch file ; git add file ; git commit -m\"add file\"\n> \n> and then \n> \n> $ for n in 1 2 3; do git rm file; git commit -m\"rm file\"; done\n>  \n> I get:\n> \n> rm 'file'\n> [master 5b24511] rm file\n>  0 files changed, 0 insertions(+), 0 deletions(-)\n>  delete mode 100644 file\n> fatal: pathspec 'file' did not match any files\n> # On branch master\n> nothing to commit (working directory clean)\n> fatal: pathspec 'file' did not match any files\n> # On branch master\n> nothing to commit (working directory clean)\n> \n> I don't get 4 commits. That's the part I don't understand :-(\n\nJust to be clear:\n\nOf course I do understand why I don't get 4 commits here - I just don't\nunderstand why I get a commit for every 'git notes remove' even if there's\nnothing to remove.\n\nStefan\n-- \n----------------------------------------------------------------\n/dev/random says: This tagl ineh asto oman yfou rlet terw ords.\n"},{"id":"149425","messageId":"bc44b3393db4018487bb956d00a12fa73f04ca9e.1283267564.git.git@drmicha.warpmail.net","threadId":"24920","inReplyTo":"4C7CD65F.10509@atlas-elektronik.com","subject":"[RFC] notes: avoid recommitting identical trees","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2010-08-31T15:16:17Z","receivedAt":"2010-08-31T15:16:17Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Currently, \"git notes\" behaves like \"git commit --allow-empty\" when\ncommitting notes trees. In particular, removing nonexisting notes leads\nto empty commits \"commits with no diff\".\n\nChange this to avoid unnecessary notes commits.\n\nSigned-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n---\nI can't believe there's no easier way to lookup the sha1 of a tree of a commit\nbut I didn't find any, and I did not want to employ the diff machinery for diffing\nthe trees when their sha1 is (should be) known.\n\n builtin/notes.c |   14 +++++++++-----\n 1 files changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex fbc347c..48da228 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -303,11 +303,15 @@ int commit_notes(struct notes_tree *t, const char *msg)\n \t\thashclr(prev_commit);\n \t\tparent = NULL;\n \t}\n-\tif (commit_tree(buf.buf + 7, tree_sha1, parent, new_commit, NULL))\n-\t\tdie(\"Failed to commit notes tree to database\");\n-\n-\t/* Update notes ref with new commit */\n-\tupdate_ref(buf.buf, t->ref, new_commit, prev_commit, 0, DIE_ON_ERR);\n+\tif (!parent || parse_commit(parent->item) || parse_tree(parent->item->tree) ||\n+\t\thashcmp(parent->item->tree->object.sha1, tree_sha1)) {\n+\t\t/* avoid recommitting the same tree */\n+\t\tif (commit_tree(buf.buf + 7, tree_sha1, parent, new_commit, NULL))\n+\t\t\tdie(\"Failed to commit notes tree to database\");\n+\n+\t\t/* Update notes ref with new commit */\n+\t\tupdate_ref(buf.buf, t->ref, new_commit, prev_commit, 0, DIE_ON_ERR);\n+\t}\n \n \tstrbuf_release(&buf);\n \treturn 0;\n-- \n1.7.2.2.398.g5af70\n"},{"id":"149436","messageId":"201008311753.37433.johan@herland.net","threadId":"24920","inReplyTo":"4C7CD5B8.20602@atlas-elektronik.com","subject":"Re: Q. about usage of notes","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2010-08-31T15:53:37Z","receivedAt":"2010-08-31T15:53:37Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Tuesday 31 August 2010, Stefan Naewe wrote:\n> On 8/31/2010 11:07 AM, Johan Herland wrote:\n> > On Tuesday 31 August 2010, Stefan Naewe wrote:\n> >> And why does 'git remove' do that repetetively (is that even a\n> >> word...?), i.e. 'git add -m\"Note\" ; git remove; git remove; git\n> >> remove; git remove' creates 5 commit objects under\n> >> 'refs/notes/commits' Is that the intended behaviour ?\n> >\n> > Yes, this is the intended behaviour. Otherwise you would need a\n> > separate notes index where you could stage notes changes (with git\n> > notes add/remove), and then later commit those notes changes with\n> > (the imaginary) git notes commit. This was deemed too\n> > cumbersome/complicated, and we settled for the current approach\n> > instead.\n>\n> But if I do:\n>\n> $ touch file ; git add file ; git commit -m\"add file\"\n>\n> and then\n>\n> $ for n in 1 2 3; do git rm file; git commit -m\"rm file\"; done\n>\n> I get:\n>\n> rm 'file'\n> [master 5b24511] rm file\n>  0 files changed, 0 insertions(+), 0 deletions(-)\n>  delete mode 100644 file\n> fatal: pathspec 'file' did not match any files\n> # On branch master\n> nothing to commit (working directory clean)\n> fatal: pathspec 'file' did not match any files\n> # On branch master\n> nothing to commit (working directory clean)\n>\n> I don't get 4 commits. That's the part I don't understand :-(\n> \n> Just to be clear:\n> \n> Of course I do understand why I don't get 4 commits here - I just\n> don't understand why I get a commit for every 'git notes remove' even\n> if there's nothing to remove.\n\nAh, that would be a bug in 'git notes remove'. Patch coming soon.\n\n> And to delete the 'notes branch' I can only use 'git update-ref' ?!\n\nYes, 'git update-ref -d refs/notes/foo' will delete the \"foo\" notes \nbranch.\n\n\n...Johan\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"149440","messageId":"201008311756.50509.johan@herland.net","threadId":"24920","inReplyTo":"201008311753.37433.johan@herland.net","subject":"[PATCH 1/2] notes: Don't create (empty) commit when removing non-existing notes","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2010-08-31T15:56:50Z","receivedAt":"2010-08-31T15:56:50Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Extend remove_note() in the notes API to return whether or not a note was\nactually removed. Use this in 'git notes remove' to skip the creation of\na notes commit when no notes were actually removed.\n\nAlso add a test illustrating the change in behavior.\n\nSigned-off-by: Johan Herland <johan@herland.net>\n---\n\nThis patch is against master.\n\n builtin/notes.c  |   14 ++++++++++----\n notes.c          |   14 ++++++++++----\n notes.h          |    4 +++-\n t/t3301-notes.sh |    7 +++++++\n 4 files changed, 30 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex fbc347c..6d07aac 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -769,6 +769,7 @@ static int remove_cmd(int argc, const char **argv, const char *prefix)\n \tconst char *object_ref;\n \tstruct notes_tree *t;\n \tunsigned char object[20];\n+\tint retval;\n \n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t     git_notes_remove_usage, 0);\n@@ -785,12 +786,17 @@ static int remove_cmd(int argc, const char **argv, const char *prefix)\n \n \tt = init_notes_check(\"remove\");\n \n-\tfprintf(stderr, \"Removing note for object %s\\n\", sha1_to_hex(object));\n-\tremove_note(t, object);\n+\tretval = remove_note(t, object);\n+\tif (retval)\n+\t\tfprintf(stderr, \"Object %s has no note\\n\", sha1_to_hex(object));\n+\telse {\n+\t\tfprintf(stderr, \"Removing note for object %s\\n\",\n+\t\t\tsha1_to_hex(object));\n \n-\tcommit_notes(t, \"Notes removed by 'git notes remove'\");\n+\t\tcommit_notes(t, \"Notes removed by 'git notes remove'\");\n+\t}\n \tfree_notes(t);\n-\treturn 0;\n+\treturn retval;\n }\n \n static int prune(int argc, const char **argv, const char *prefix)\ndiff --git a/notes.c b/notes.c\nindex 7fd2035..70d0013 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -263,11 +263,13 @@ static int note_tree_consolidate(struct int_node *tree,\n  * To remove a leaf_node:\n  * Search to the tree location appropriate for the given leaf_node's key:\n  * - If location does not hold a matching entry, abort and do nothing.\n+ * - Copy the matching entry's value into the given entry.\n  * - Replace the matching leaf_node with a NULL entry (and free the leaf_node).\n  * - Consolidate int_nodes repeatedly, while walking up the tree towards root.\n  */\n-static void note_tree_remove(struct notes_tree *t, struct int_node *tree,\n-\t\tunsigned char n, struct leaf_node *entry)\n+static void note_tree_remove(struct notes_tree *t,\n+\t\tstruct int_node *tree, unsigned char n,\n+\t\tstruct leaf_node *entry)\n {\n \tstruct leaf_node *l;\n \tstruct int_node *parent_stack[20];\n@@ -282,6 +284,7 @@ static void note_tree_remove(struct notes_tree *t, struct int_node *tree,\n \t\treturn; /* key mismatch, nothing to remove */\n \n \t/* we have found a matching entry */\n+\thashcpy(entry->val_sha1, l->val_sha1);\n \tfree(l);\n \t*p = SET_PTR_TYPE(NULL, PTR_TYPE_NULL);\n \n@@ -1003,17 +1006,20 @@ void add_note(struct notes_tree *t, const unsigned char *object_sha1,\n \tnote_tree_insert(t, t->root, 0, l, PTR_TYPE_NOTE, combine_notes);\n }\n \n-void remove_note(struct notes_tree *t, const unsigned char *object_sha1)\n+int remove_note(struct notes_tree *t, const unsigned char *object_sha1)\n {\n \tstruct leaf_node l;\n \n \tif (!t)\n \t\tt = &default_notes_tree;\n \tassert(t->initialized);\n-\tt->dirty = 1;\n \thashcpy(l.key_sha1, object_sha1);\n \thashclr(l.val_sha1);\n \tnote_tree_remove(t, t->root, 0, &l);\n+\tif (is_null_sha1(l.val_sha1)) // no note was removed\n+\t\treturn 1;\n+\tt->dirty = 1;\n+\treturn 0;\n }\n \n const unsigned char *get_note(struct notes_tree *t,\ndiff --git a/notes.h b/notes.h\nindex 65fc3a6..5106761 100644\n--- a/notes.h\n+++ b/notes.h\n@@ -89,8 +89,10 @@ void add_note(struct notes_tree *t, const unsigned char *object_sha1,\n  * IMPORTANT: The changes made by remove_note() to the given notes_tree\n  * structure are not persistent until a subsequent call to write_notes_tree()\n  * returns zero.\n+ *\n+ * Return 0 if a note was removed; 1 if there was no note to remove.\n  */\n-void remove_note(struct notes_tree *t, const unsigned char *object_sha1);\n+int remove_note(struct notes_tree *t, const unsigned char *object_sha1);\n \n /*\n  * Get the note object SHA1 containing the note data for the given object\ndiff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\nindex 96b7581..a2b79a0 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -365,6 +365,13 @@ c18dc024e14f08d18d14eea0d747ff692d66d6a3 1584215f1d29c65e99c6c6848626553fdd07fd7\n c9c6af7f78bc47490dbf3e822cf2f3c24d4b9061 268048bfb8a1fb38e703baceb8ab235421bf80c5\n EOF\n \n+test_expect_success 'removing non-existing note should not create new commit' '\n+\tgit rev-parse --verify refs/notes/commits > before_commit &&\n+\ttest_must_fail git notes remove HEAD^ &&\n+\tgit rev-parse --verify refs/notes/commits > after_commit &&\n+\ttest_cmp before_commit after_commit\n+'\n+\n test_expect_success 'list notes with \"git notes list\"' '\n \tgit notes list > output &&\n \ttest_cmp expect output\n-- \n1.7.2.220.gea1d3\n"},{"id":"149441","messageId":"201008311759.05908.johan@herland.net","threadId":"24920","inReplyTo":"201008311753.37433.johan@herland.net","subject":"[PATCH 2/2] notes.h: Extend remove_note to return the SHA1 of the removed note, if any","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2010-08-31T15:59:05Z","receivedAt":"2010-08-31T15:59:05Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"It might be useful to identify which note was removed by a call to\nremove_note(). Add an optional parameter to remove_note() for recording\nthe SHA1 of the removed note object. If no note is removed by remove_note(),\nnull_sha1 is stored in this parameter.\n\nSigned-off-by: Johan Herland <johan@herland.net>\n---\n\nThis patch is very much optional. It adds a small API extension that\n_might_ be useful in the future.\n\n\n builtin/notes.c |    6 +++---\n notes.c         |    7 +++++--\n notes.h         |    6 +++++-\n 3 files changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 6d07aac..e5c8208 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -572,7 +572,7 @@ static int add(int argc, const char **argv, const char *prefix)\n \tcreate_note(object, &msg, 0, note, new_note);\n \n \tif (is_null_sha1(new_note))\n-\t\tremove_note(t, object);\n+\t\tremove_note(t, object, NULL);\n \telse\n \t\tadd_note(t, object, new_note, combine_notes_overwrite);\n \n@@ -711,7 +711,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \tcreate_note(object, &msg, !edit, note, new_note);\n \n \tif (is_null_sha1(new_note))\n-\t\tremove_note(t, object);\n+\t\tremove_note(t, object, NULL);\n \telse\n \t\tadd_note(t, object, new_note, combine_notes_overwrite);\n \n@@ -786,7 +786,7 @@ static int remove_cmd(int argc, const char **argv, const char *prefix)\n \n \tt = init_notes_check(\"remove\");\n \n-\tretval = remove_note(t, object);\n+\tretval = remove_note(t, object, NULL);\n \tif (retval)\n \t\tfprintf(stderr, \"Object %s has no note\\n\", sha1_to_hex(object));\n \telse {\ndiff --git a/notes.c b/notes.c\nindex 70d0013..9fe857b 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1006,7 +1006,8 @@ void add_note(struct notes_tree *t, const unsigned char *object_sha1,\n \tnote_tree_insert(t, t->root, 0, l, PTR_TYPE_NOTE, combine_notes);\n }\n \n-int remove_note(struct notes_tree *t, const unsigned char *object_sha1)\n+int remove_note(struct notes_tree *t, const unsigned char *object_sha1,\n+\t\tunsigned char *removed_note)\n {\n \tstruct leaf_node l;\n \n@@ -1016,6 +1017,8 @@ int remove_note(struct notes_tree *t, const unsigned char *object_sha1)\n \thashcpy(l.key_sha1, object_sha1);\n \thashclr(l.val_sha1);\n \tnote_tree_remove(t, t->root, 0, &l);\n+\tif (removed_note)\n+\t\thashcpy(removed_note, l.val_sha1);\n \tif (is_null_sha1(l.val_sha1)) // no note was removed\n \t\treturn 1;\n \tt->dirty = 1;\n@@ -1085,7 +1088,7 @@ void prune_notes(struct notes_tree *t, int flags)\n \t\tif (flags & NOTES_PRUNE_VERBOSE)\n \t\t\tprintf(\"%s\\n\", sha1_to_hex(l->sha1));\n \t\tif (!(flags & NOTES_PRUNE_DRYRUN))\n-\t\t\tremove_note(t, l->sha1);\n+\t\t\tremove_note(t, l->sha1, NULL);\n \t\tl = l->next;\n \t}\n }\ndiff --git a/notes.h b/notes.h\nindex 5106761..c61e962 100644\n--- a/notes.h\n+++ b/notes.h\n@@ -90,9 +90,13 @@ void add_note(struct notes_tree *t, const unsigned char *object_sha1,\n  * structure are not persistent until a subsequent call to write_notes_tree()\n  * returns zero.\n  *\n+ * If removed_note is non-NULL, the SHA1 of the removed note will be written\n+ * there. If there was no note to remove, then null_sha1 will be written there.\n+ *\n  * Return 0 if a note was removed; 1 if there was no note to remove.\n  */\n-int remove_note(struct notes_tree *t, const unsigned char *object_sha1);\n+int remove_note(struct notes_tree *t, const unsigned char *object_sha1,\n+\t\tunsigned char *removed_note);\n \n /*\n  * Get the note object SHA1 containing the note data for the given object\n-- \n1.7.2.220.gea1d3\n"},{"id":"149442","messageId":"20100831160145.GC11014@sigill.intra.peff.net","threadId":"24920","inReplyTo":"bc44b3393db4018487bb956d00a12fa73f04ca9e.1283267564.git.git@drmicha.warpmail.net","subject":"Re: [RFC] notes: avoid recommitting identical trees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-08-31T16:01:45Z","receivedAt":"2010-08-31T16:01:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 31, 2010 at 05:16:17PM +0200, Michael J Gruber wrote:\n\n> Currently, \"git notes\" behaves like \"git commit --allow-empty\" when\n> committing notes trees. In particular, removing nonexisting notes leads\n> to empty commits \"commits with no diff\".\n> \n> Change this to avoid unnecessary notes commits.\n\nIs this a sufficient check in the case of notes? Is it possible that we\nre-balanced the fanout of the notes tree and got a different tree sha1,\neven though there is nothing interesting to commit?\n\n> +\tif (!parent || parse_commit(parent->item) || parse_tree(parent->item->tree) ||\n> +\t\thashcmp(parent->item->tree->object.sha1, tree_sha1)) {\n\nI didn't check, but I can imagine you can drop the parse_tree here. We\nshould know the object sha1 once the commit is parsed.\n\n-Peff\n"},{"id":"149445","messageId":"201008311808.11203.johan@herland.net","threadId":"24920","inReplyTo":"bc44b3393db4018487bb956d00a12fa73f04ca9e.1283267564.git.git@drmicha.warpmail.net","subject":"Re: [RFC] notes: avoid recommitting identical trees","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2010-08-31T16:08:11Z","receivedAt":"2010-08-31T16:08:11Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Tuesday 31 August 2010, Michael J Gruber wrote:\n> Currently, \"git notes\" behaves like \"git commit --allow-empty\" when\n> committing notes trees. In particular, removing nonexisting notes\n> leads to empty commits \"commits with no diff\".\n>\n> Change this to avoid unnecessary notes commits.\n>\n> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n\nI just posted a patch with the same objective, but with a different \napproach. Instead of parsing the previous commit and comparing tree \nobject SHA1s, I add a few lines of notes code to let remove_note() \nreport whether it removed a note or not (thus determining whether a \ncommit is necessary or not).\n\nIn general, the notes_tree.dirty flag should be sufficient to determine \nwhether a commit is needed or not (remove_note()'s unconditional \nsetting of this flag is also fixed in my patch).\n\n\n...Johan\n\n> ---\n> I can't believe there's no easier way to lookup the sha1 of a tree of\n> a commit but I didn't find any, and I did not want to employ the diff\n> machinery for diffing the trees when their sha1 is (should be) known.\n>\n>  builtin/notes.c |   14 +++++++++-----\n>  1 files changed, 9 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index fbc347c..48da228 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -303,11 +303,15 @@ int commit_notes(struct notes_tree *t, const\n> char *msg) hashclr(prev_commit);\n>  \t\tparent = NULL;\n>  \t}\n> -\tif (commit_tree(buf.buf + 7, tree_sha1, parent, new_commit, NULL))\n> -\t\tdie(\"Failed to commit notes tree to database\");\n> -\n> -\t/* Update notes ref with new commit */\n> -\tupdate_ref(buf.buf, t->ref, new_commit, prev_commit, 0,\n> DIE_ON_ERR); +\tif (!parent || parse_commit(parent->item) ||\n> parse_tree(parent->item->tree) ||\n> +\t\thashcmp(parent->item->tree->object.sha1, tree_sha1)) {\n> +\t\t/* avoid recommitting the same tree */\n> +\t\tif (commit_tree(buf.buf + 7, tree_sha1, parent, new_commit, NULL))\n> +\t\t\tdie(\"Failed to commit notes tree to database\");\n> +\n> +\t\t/* Update notes ref with new commit */\n> +\t\tupdate_ref(buf.buf, t->ref, new_commit, prev_commit, 0,\n> DIE_ON_ERR); +\t}\n>\n>  \tstrbuf_release(&buf);\n>  \treturn 0;\n\n\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"149447","messageId":"4C7D2AAE.9000701@drmicha.warpmail.net","threadId":"24920","inReplyTo":"20100831160145.GC11014@sigill.intra.peff.net","subject":"Re: [RFC] notes: avoid recommitting identical trees","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2010-08-31T16:15:42Z","receivedAt":"2010-08-31T16:15:42Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 31.08.2010 18:01:\n> On Tue, Aug 31, 2010 at 05:16:17PM +0200, Michael J Gruber wrote:\n> \n>> Currently, \"git notes\" behaves like \"git commit --allow-empty\" when\n>> committing notes trees. In particular, removing nonexisting notes leads\n>> to empty commits \"commits with no diff\".\n>>\n>> Change this to avoid unnecessary notes commits.\n> \n> Is this a sufficient check in the case of notes? Is it possible that we\n> re-balanced the fanout of the notes tree and got a different tree sha1,\n> even though there is nothing interesting to commit?\n\nYes, but I don't think this hurts. The main thrust here is to catch the\ncase of repeated \"git notes remove\". Also, we might even want to record\nthe history when there is rebalancing since this is indeed a tree change.\n\nJohan's (later ;) ) approach, while being more intrusive, catches this\nat the point of removal - if there's nothing to remove, nothing gets\nrewritten.\n\n> \n>> +\tif (!parent || parse_commit(parent->item) || parse_tree(parent->item->tree) ||\n>> +\t\thashcmp(parent->item->tree->object.sha1, tree_sha1)) {\n> \n> I didn't check, but I can imagine you can drop the parse_tree here. We\n> should know the object sha1 once the commit is parsed.\n\nparse_commit() does a lookup_tree() but I don't think that it parses the\ntree, i.e. I don't hink it fills in tree->object.sha1. At least it\nsegfaulted without that ;)\n\nMichael\n"},{"id":"149451","messageId":"7vfwxud94w.fsf@alter.siamese.dyndns.org","threadId":"24920","inReplyTo":"4C7D2AAE.9000701@drmicha.warpmail.net","subject":"Re: [RFC] notes: avoid recommitting identical trees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-31T16:44:47Z","receivedAt":"2010-08-31T16:44:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber <git@drmicha.warpmail.net> writes:\n\n>>> +\tif (!parent || parse_commit(parent->item) || parse_tree(parent->item->tree) ||\n>>> +\t\thashcmp(parent->item->tree->object.sha1, tree_sha1)) {\n>> \n>> I didn't check, but I can imagine you can drop the parse_tree here. We\n>> should know the object sha1 once the commit is parsed.\n>\n> parse_commit() does a lookup_tree() but I don't think that it parses the\n> tree, i.e. I don't hink it fills in tree->object.sha1.\n\nHuh?  parse_tree(tree) calls read_sha1_file(tree->object.sha1) to parse\nthe tree.  How can it do without filling tree->object.sha1?\n"},{"id":"149473","messageId":"4C7D495A.1080806@drmicha.warpmail.net","threadId":"24920","inReplyTo":"7vfwxud94w.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC] notes: avoid recommitting identical trees","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2010-08-31T18:26:34Z","receivedAt":"2010-08-31T18:26:34Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 31.08.2010 18:44:\n> Michael J Gruber <git@drmicha.warpmail.net> writes:\n> \n>>>> +\tif (!parent || parse_commit(parent->item) || parse_tree(parent->item->tree) ||\n>>>> +\t\thashcmp(parent->item->tree->object.sha1, tree_sha1)) {\n>>>\n>>> I didn't check, but I can imagine you can drop the parse_tree here. We\n>>> should know the object sha1 once the commit is parsed.\n>>\n>> parse_commit() does a lookup_tree() but I don't think that it parses the\n>> tree, i.e. I don't hink it fills in tree->object.sha1.\n> \n> Huh?  parse_tree(tree) calls read_sha1_file(tree->object.sha1) to parse\n> the tree.  How can it do without filling tree->object.sha1?\n> \n\nSure parse_tree() does that. That's why I call it. I never claimed it\ndoesn't.\n\nMichael\n"},{"id":"149474","messageId":"20100831182931.GA21489@sigill.intra.peff.net","threadId":"24920","inReplyTo":"4C7D495A.1080806@drmicha.warpmail.net","subject":"Re: [RFC] notes: avoid recommitting identical trees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-08-31T18:29:32Z","receivedAt":"2010-08-31T18:29:32Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 31, 2010 at 08:26:34PM +0200, Michael J Gruber wrote:\n\n> Junio C Hamano venit, vidit, dixit 31.08.2010 18:44:\n> > Michael J Gruber <git@drmicha.warpmail.net> writes:\n> > \n> >>>> +\tif (!parent || parse_commit(parent->item) || parse_tree(parent->item->tree) ||\n> >>>> +\t\thashcmp(parent->item->tree->object.sha1, tree_sha1)) {\n> >>>\n> >>> I didn't check, but I can imagine you can drop the parse_tree here. We\n> >>> should know the object sha1 once the commit is parsed.\n> >>\n> >> parse_commit() does a lookup_tree() but I don't think that it parses the\n> >> tree, i.e. I don't hink it fills in tree->object.sha1.\n> > \n> > Huh?  parse_tree(tree) calls read_sha1_file(tree->object.sha1) to parse\n> > the tree.  How can it do without filling tree->object.sha1?\n> > \n> \n> Sure parse_tree() does that. That's why I call it. I never claimed it\n> doesn't.\n\nI think the claim is that it is already parsed. Look at parse_tree. The\nfirst non-declaration lines are:\n\n  if (item->object.parsed)\n          return 0;\n  buffer = read_sha1_file(item->object.sha1, &type, &size);\n\nSo _somebody_ has already filled in item->object.sha1.\n\n-Peff\n"},{"id":"149475","messageId":"7vvd6qaaiu.fsf@alter.siamese.dyndns.org","threadId":"24920","inReplyTo":"4C7D495A.1080806@drmicha.warpmail.net","subject":"Re: [RFC] notes: avoid recommitting identical trees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-31T18:43:05Z","receivedAt":"2010-08-31T18:43:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber <git@drmicha.warpmail.net> writes:\n\n> Junio C Hamano venit, vidit, dixit 31.08.2010 18:44:\n>> Michael J Gruber <git@drmicha.warpmail.net> writes:\n>> \n>>>>> +\tif (!parent || parse_commit(parent->item) || parse_tree(parent->item->tree) ||\n>>>>> +\t\thashcmp(parent->item->tree->object.sha1, tree_sha1)) {\n>>>>\n>>>> I didn't check, but I can imagine you can drop the parse_tree here. We\n>>>> should know the object sha1 once the commit is parsed.\n>>>\n>>> parse_commit() does a lookup_tree() but I don't think that it parses the\n>>> tree, i.e. I don't hink it fills in tree->object.sha1.\n>> \n>> Huh?  parse_tree(tree) calls read_sha1_file(tree->object.sha1) to parse\n>> the tree.  How can it do without filling tree->object.sha1?\n>\n> Sure parse_tree() does that. That's why I call it. I never claimed it\n> doesn't.\n\nSorry, My second sentence was incorrect.\n\n       How can it do so without getting a tree with tree->object.sha1\n       already filled?\n"},{"id":"149477","messageId":"4C7D4DC2.4070008@drmicha.warpmail.net","threadId":"24920","inReplyTo":"20100831182931.GA21489@sigill.intra.peff.net","subject":"Re: [RFC] notes: avoid recommitting identical trees","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2010-08-31T18:45:22Z","receivedAt":"2010-08-31T18:45:22Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 31.08.2010 20:29:\n> On Tue, Aug 31, 2010 at 08:26:34PM +0200, Michael J Gruber wrote:\n> \n>> Junio C Hamano venit, vidit, dixit 31.08.2010 18:44:\n>>> Michael J Gruber <git@drmicha.warpmail.net> writes:\n>>>\n>>>>>> +\tif (!parent || parse_commit(parent->item) || parse_tree(parent->item->tree) ||\n>>>>>> +\t\thashcmp(parent->item->tree->object.sha1, tree_sha1)) {\n>>>>>\n>>>>> I didn't check, but I can imagine you can drop the parse_tree here. We\n>>>>> should know the object sha1 once the commit is parsed.\n>>>>\n>>>> parse_commit() does a lookup_tree() but I don't think that it parses the\n>>>> tree, i.e. I don't hink it fills in tree->object.sha1.\n>>>\n>>> Huh?  parse_tree(tree) calls read_sha1_file(tree->object.sha1) to parse\n>>> the tree.  How can it do without filling tree->object.sha1?\n>>>\n>>\n>> Sure parse_tree() does that. That's why I call it. I never claimed it\n>> doesn't.\n> \n> I think the claim is that it is already parsed. Look at parse_tree. The\n> first non-declaration lines are:\n> \n>   if (item->object.parsed)\n>           return 0;\n>   buffer = read_sha1_file(item->object.sha1, &type, &size);\n> \n> So _somebody_ has already filled in item->object.sha1.\n> \n> -Peff\n\nOK, now I understand Junio's answer...\n...and I also see where lookup_tree() fills the sha1, which I failed to\nsee before.\n\nI guess what happened was that I had the parse_tree without\nparse_commit, which segfaulted, and then inserted parse_commit.\n\nIn any case, it seems Johan is going with his approach, so you can\nforget about the RFC, be it with or without parse_tree.\n\nMichael\n"}]}