From: Johan Herland <hidden> Date: 2016-06-15 22:47:19
Another iteration of the 'git notes' feature. Rebased on top of 'next':
- Patches 1-7 are unchanged from (patches 1-5 + 7-8 of) the last iteration.
- Patch 8 introduces the new notes lookup code that offers both handling of
fanout subtrees, and other performance improvements.
- Patch 9 adds a selftest that verifies correct parsing of notes trees with
various fanouts.
- Patch 10 adds simple memory pooling to parts of the data structure from
patch 8. This improves performance slightly.
- Patches 11-12 adds the '%N' format specifier for pretty-printing commit
notes (as suggested by Dscho in the previous notes thread).
Some performance numbers from the notes lookup code:
(these numbers are from my Core 2 Quad, 4 GB RAM)
Test scenario I: Running t3302-notes-index-expensive
Timing numbers from the 'time_notes 100' following 'create_repo 10000'
no-notes notes
before 16.22s 23.74s
after 16.31s 22.16s
after+mempool 16.24s 22.03s
Comments: This is a worst case scenario for pretty much any notes lookup
algorithm: Looking up all 10000 notes with no fanout (fanout level 0).
The new implementation does marginally better than the old.
Test scenario II: Repo with 100,000 commits, 1 note per commit.
Timing 100 repetitions of 'git log -n 10 refs/heads/master >/dev/null'
without notes fanout level 0 fanout level 1 fanout level 2
before 0.20s 32.44s N/A N/A
after 0.19s 16.66s 0.85s 0.61s
after+mempool 0.19s 16.20s 0.83s 0.57s
Comments: This hopefully gives a better simulation of a common use case
(displaying only a handful of commits, and their notes). In the (relative)
worst case (fanout level 0), the new code almost twice as fast as the old one.
As we add fanout, the runtime plummets (since we only need to unpack a handful
of subtrees).
In practice, this means that with even a modest 2/38 or 2/2/36 fanout in the
notes tree (fanout level 1 and 2, respectively), the 'git log' user experience
goes from unbearable to barely noticeable in a repo with hundreds of thousands
of notes.
Have fun! :)
...Johan
Johan Herland (7):
Teach "-m <msg>" and "-F <file>" to "git notes edit"
fast-import: Add support for importing commit notes
t3302-notes-index-expensive: Speed up create_repo()
Teach the notes lookup code to parse notes trees with various fanout schemes
Selftests verifying semantics when loading notes trees with various fanouts
notes.c: Implement simple memory pooling of leaf nodes
Add flags to get_commit_notes() to control the format of the note string
Johannes Schindelin (5):
Introduce commit notes
Add a script to edit/inspect notes
Speed up git notes lookup
Add an expensive test for git-notes
Add '%N'-format for pretty-printing commit notes
.gitignore | 1 +
Documentation/config.txt | 13 ++
Documentation/git-fast-import.txt | 45 +++++-
Documentation/git-notes.txt | 60 +++++++
Documentation/pretty-formats.txt | 1 +
Makefile | 3 +
cache.h | 4 +
command-list.txt | 1 +
commit.c | 1 +
config.c | 5 +
environment.c | 1 +
fast-import.c | 88 +++++++++-
git-notes.sh | 121 +++++++++++++
notes.c | 338 +++++++++++++++++++++++++++++++++++++
notes.h | 10 +
pretty.c | 10 +
t/t3301-notes.sh | 150 ++++++++++++++++
t/t3302-notes-index-expensive.sh | 118 +++++++++++++
t/t3303-notes-subtrees.sh | 206 ++++++++++++++++++++++
t/t9300-fast-import.sh | 166 ++++++++++++++++++
20 files changed, 1332 insertions(+), 10 deletions(-)
create mode 100644 Documentation/git-notes.txt
create mode 100755 git-notes.sh
create mode 100644 notes.c
create mode 100644 notes.h
create mode 100755 t/t3301-notes.sh
create mode 100755 t/t3302-notes-index-expensive.sh
create mode 100755 t/t3303-notes-subtrees.sh
From: Johan Herland <hidden> Date: 2016-06-15 22:47:19
From: Johannes Schindelin <redacted>
Commit notes are blobs which are shown together with the commit
message. These blobs are taken from the notes ref, which you can
configure by the config variable core.notesRef, which in turn can
be overridden by the environment variable GIT_NOTES_REF.
The notes ref is a branch which contains "files" whose names are
the names of the corresponding commits (i.e. the SHA-1).
The rationale for putting this information into a ref is this: we
want to be able to fetch and possibly union-merge the notes,
maybe even look at the date when a note was introduced, and we
want to store them efficiently together with the other objects.
This patch has been improved by the following contributions:
- Thomas Rast: fix core.notesRef documentation
- Tor Arne Vestbø: fix printing of multi-line notes
- Alex Riesen: Using char array instead of char pointer costs less BSS
Signed-off-by: Johannes Schindelin <redacted>
Signed-off-by: Thomas Rast <redacted>
Signed-off-by: Tor Arne Vestbø <redacted>
Signed-off-by: Johan Herland <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
Documentation/config.txt | 13 +++++++++
Makefile | 2 +
cache.h | 4 +++
commit.c | 1 +
config.c | 5 +++
environment.c | 1 +
notes.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++
notes.h | 7 +++++
pretty.c | 5 +++
9 files changed, 106 insertions(+), 0 deletions(-)
create mode 100644 notes.c
create mode 100644 notes.h
@@ -439,6 +439,19 @@ On some file system/operating system combinations, this is unreliable. Set this config setting to 'rename' there; However, This will remove the check that makes sure that existing object files will not get overwritten.+core.notesRef::+ When showing commit messages, also show notes which are stored in+ the given ref. This ref is expected to contain files named+ after the full SHA-1 of the commit they annotate.+++If such a file exists in the given ref, the referenced blob is read, and+appended to the commit message, separated by a "Notes:" line. If the+given ref itself does not exist, it is not an error, but means that no+notes should be printed.+++This setting defaults to "refs/notes/commits", and can be overridden by+the `GIT_NOTES_REF` environment variable.+ add.ignore-errors:: Tells 'git-add' to continue adding files when some files cannot be added due to indexing errors. Equivalent to the '--ignore-errors'
@@ -0,0 +1,68 @@+#include"cache.h"+#include"commit.h"+#include"notes.h"+#include"refs.h"+#include"utf8.h"+#include"strbuf.h"++staticintinitialized;++voidget_commit_notes(conststructcommit*commit,structstrbuf*sb,+constchar*output_encoding)+{+staticconstcharutf8[]="utf-8";+structstrbufname=STRBUF_INIT;+unsignedcharsha1[20];+char*msg,*msg_p;+unsignedlonglinelen,msglen;+enumobject_typetype;++if(!initialized){+constchar*env=getenv(GIT_NOTES_REF_ENVIRONMENT);+if(env)+notes_ref_name=getenv(GIT_NOTES_REF_ENVIRONMENT);+elseif(!notes_ref_name)+notes_ref_name=GIT_NOTES_DEFAULT_REF;+if(notes_ref_name&&read_ref(notes_ref_name,sha1))+notes_ref_name=NULL;+initialized=1;+}++if(!notes_ref_name)+return;++strbuf_addf(&name,"%s:%s",notes_ref_name,+sha1_to_hex(commit->object.sha1));+if(get_sha1(name.buf,sha1))+return;++if(!(msg=read_sha1_file(sha1,&type,&msglen))||!msglen||+type!=OBJ_BLOB)+return;++if(output_encoding&&*output_encoding&&+strcmp(utf8,output_encoding)){+char*reencoded=reencode_string(msg,output_encoding,utf8);+if(reencoded){+free(msg);+msg=reencoded;+msglen=strlen(msg);+}+}++/* we will end the annotation by a newline anyway */+if(msglen&&msg[msglen-1]=='\n')+msglen--;++strbuf_addstr(sb,"\nNotes:\n");++for(msg_p=msg;msg_p<msg+msglen;msg_p+=linelen+1){+linelen=strchrnul(msg_p,'\n')-msg_p;++strbuf_addstr(sb," ");+strbuf_add(sb,msg_p,linelen);+strbuf_addch(sb,'\n');+}++free(msg);+}
From: Johan Herland <hidden> Date: 2016-06-15 22:47:19
From: Johannes Schindelin <redacted>
The script 'git notes' allows you to edit and show commit notes, by
calling either
git notes show <commit>
or
git notes edit <commit>
This patch has been improved by the following contributions:
- Tor Arne Vestbø: fix printing of multi-line notes
- Michael J Gruber: test and handle empty notes gracefully
- Thomas Rast:
- only clean up message file when editing
- use GIT_EDITOR and core.editor over VISUAL/EDITOR
- t3301: fix confusing quoting in test for valid notes ref
- t3301: use test_must_fail instead of !
- refuse to edit notes outside refs/notes/
- Junio C Hamano: tests: fix "export var=val"
- Christian Couder: documentation: fix 'linkgit' macro in "git-notes.txt"
- Johan Herland: minor cleanup and bugfixing in git-notes.sh (v2)
Signed-off-by: Johannes Schindelin <redacted>
Signed-off-by: Tor Arne Vestbø <redacted>
Signed-off-by: Michael J Gruber <redacted>
Signed-off-by: Thomas Rast <redacted>
Signed-off-by: Christian Couder <redacted>
Signed-off-by: Johan Herland <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
.gitignore | 1 +
Documentation/git-notes.txt | 46 +++++++++++++++++
Makefile | 1 +
command-list.txt | 1 +
git-notes.sh | 73 +++++++++++++++++++++++++++
t/t3301-notes.sh | 114 +++++++++++++++++++++++++++++++++++++++++++
6 files changed, 236 insertions(+), 0 deletions(-)
create mode 100644 Documentation/git-notes.txt
create mode 100755 git-notes.sh
create mode 100755 t/t3301-notes.sh
@@ -0,0 +1,46 @@+git-notes(1)+============++NAME+----+git-notes - Add/inspect commit notes++SYNOPSIS+--------+[verse]+'git-notes' (edit | show) [commit]++DESCRIPTION+-----------+This command allows you to add notes to commit messages, without+changing the commit. To discern these notes from the message stored+in the commit object, the notes are indented like the message, after+an unindented line saying "Notes:".++To disable commit notes, you have to set the config variable+core.notesRef to the empty string. Alternatively, you can set it+to a different ref, something like "refs/notes/bugzilla". This setting+can be overridden by the environment variable "GIT_NOTES_REF".+++SUBCOMMANDS+-----------++edit::+ Edit the notes for a given commit (defaults to HEAD).++show::+ Show the notes for a given commit (defaults to HEAD).+++Author+------+Written by Johannes Schindelin <johannes.schindelin@gmx.de>++Documentation+-------------+Documentation by Johannes Schindelin++GIT+---+Part of the linkgit:git[7] suite
@@ -0,0 +1,73 @@+#!/bin/sh++USAGE="(edit | show) [commit]"+.git-sh-setup++test-n"$3"&&usage++test-z"$1"&&usage+ACTION="$1";shift++test-z"$GIT_NOTES_REF"&&GIT_NOTES_REF="$(gitconfigcore.notesref)"+test-z"$GIT_NOTES_REF"&&GIT_NOTES_REF="refs/notes/commits"++COMMIT=$(gitrev-parse--verify--defaultHEAD"$@")||+die"Invalid commit: $@"++case"$ACTION"in+edit)+if["${GIT_NOTES_REF#refs/notes/}"="$GIT_NOTES_REF"];then+die"Refusing to edit notes in $GIT_NOTES_REF (outside of refs/notes/)"+fi++MSG_FILE="$GIT_DIR/new-notes-$COMMIT"+GIT_INDEX_FILE="$MSG_FILE.idx"+exportGIT_INDEX_FILE++trap'+test-f"$MSG_FILE"&&rm"$MSG_FILE"+test-f"$GIT_INDEX_FILE"&&rm"$GIT_INDEX_FILE"+'0++GIT_NOTES_REF=gitlog-1$COMMIT|sed"s/^/#/">"$MSG_FILE"++CURRENT_HEAD=$(gitshow-ref"$GIT_NOTES_REF"|cut-f1-d' ')+if[-z"$CURRENT_HEAD"];then+PARENT=+else+PARENT="-p $CURRENT_HEAD"+gitread-tree"$GIT_NOTES_REF"||die"Could not read index"+gitcat-fileblob:$COMMIT>>"$MSG_FILE"2>/dev/null+fi++core_editor="$(gitconfigcore.editor)"+${GIT_EDITOR:-${core_editor:-${VISUAL:-${EDITOR:-vi}}}}"$MSG_FILE"++grep-v^#<"$MSG_FILE"|gitstripspace>"$MSG_FILE".processed+mv"$MSG_FILE".processed"$MSG_FILE"+if[-s"$MSG_FILE"];then+BLOB=$(githash-object-w"$MSG_FILE")||+die"Could not write into object database"+gitupdate-index--add--cacheinfo0644$BLOB$COMMIT||+die"Could not write index"+else+test-z"$CURRENT_HEAD"&&+die"Will not initialise with empty tree"+gitupdate-index--force-remove$COMMIT||+die"Could not update index"+fi++TREE=$(gitwrite-tree)||die"Could not write tree"+NEW_HEAD=$(echoAnnotate$COMMIT|gitcommit-tree$TREE$PARENT)||+die"Could not annotate"+gitupdate-ref-m"Annotate $COMMIT"\+"$GIT_NOTES_REF"$NEW_HEAD$CURRENT_HEAD+;;+show)+gitrev-parse-q--verify"$GIT_NOTES_REF":$COMMIT>/dev/null||+die"No note for commit $COMMIT."+gitshow"$GIT_NOTES_REF":$COMMIT+;;+*)+usage+esac
From: Johan Herland <hidden> Date: 2016-06-15 22:47:19
From: Johannes Schindelin <redacted>
To avoid looking up each and every commit in the notes ref's tree
object, which is very expensive, speed things up by slurping the tree
object's contents into a hash_map.
The idea for the hashmap singleton is from David Reiss, initial
benchmarking by Jeff King.
Note: the implementation allows for arbitrary entries in the notes
tree object, ignoring those that do not reference a valid object. This
allows you to annotate arbitrary branches, or objects.
This patch has been improved by the following contributions:
- Junio C Hamano: fixed an obvious error in initialize_hash_map()
Signed-off-by: Johannes Schindelin <redacted>
Signed-off-by: Johan Herland <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
notes.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
1 files changed, 102 insertions(+), 10 deletions(-)
@@ -4,15 +4,112 @@#include"refs.h"#include"utf8.h"#include"strbuf.h"+#include"tree-walk.h"++structentry{+unsignedcharcommit_sha1[20];+unsignedcharnotes_sha1[20];+};++structhash_map{+structentry*entries;+off_tcount,size;+};staticintinitialized;+staticstructhash_maphash_map;++staticinthash_index(structhash_map*map,constunsignedchar*sha1)+{+inti=((*(unsignedint*)sha1)%map->size);++for(;;){+unsignedchar*current=map->entries[i].commit_sha1;++if(!hashcmp(sha1,current))+returni;++if(is_null_sha1(current))+return-1-i;++if(++i==map->size)+i=0;+}+}++staticvoidadd_entry(constunsignedchar*commit_sha1,+constunsignedchar*notes_sha1)+{+intindex;++if(hash_map.count+1>hash_map.size>>1){+inti,old_size=hash_map.size;+structentry*old=hash_map.entries;++hash_map.size=old_size?old_size<<1:64;+hash_map.entries=(structentry*)+xcalloc(sizeof(structentry),hash_map.size);++for(i=0;i<old_size;i++)+if(!is_null_sha1(old[i].commit_sha1)){+index=-1-hash_index(&hash_map,+old[i].commit_sha1);+memcpy(hash_map.entries+index,old+i,+sizeof(structentry));+}+free(old);+}++index=hash_index(&hash_map,commit_sha1);+if(index<0){+index=-1-index;+hash_map.count++;+}++hashcpy(hash_map.entries[index].commit_sha1,commit_sha1);+hashcpy(hash_map.entries[index].notes_sha1,notes_sha1);+}++staticvoidinitialize_hash_map(constchar*notes_ref_name)+{+unsignedcharsha1[20],commit_sha1[20];+unsignedmode;+structtree_descdesc;+structname_entryentry;+void*buf;++if(!notes_ref_name||read_ref(notes_ref_name,commit_sha1)||+get_tree_entry(commit_sha1,"",sha1,&mode))+return;++buf=fill_tree_descriptor(&desc,sha1);+if(!buf)+die("Could not read %s for notes-index",sha1_to_hex(sha1));++while(tree_entry(&desc,&entry))+if(!get_sha1(entry.path,commit_sha1))+add_entry(commit_sha1,entry.sha1);+free(buf);+}++staticunsignedchar*lookup_notes(constunsignedchar*commit_sha1)+{+intindex;++if(!hash_map.size)+returnNULL;++index=hash_index(&hash_map,commit_sha1);+if(index<0)+returnNULL;+returnhash_map.entries[index].notes_sha1;+}voidget_commit_notes(conststructcommit*commit,structstrbuf*sb,constchar*output_encoding){staticconstcharutf8[]="utf-8";-structstrbufname=STRBUF_INIT;-unsignedcharsha1[20];+unsignedchar*sha1;char*msg,*msg_p;unsignedlonglinelen,msglen;enumobject_typetype;
From: Johan Herland <hidden> Date: 2016-06-15 22:47:19
From: Johannes Schindelin <redacted>
git-notes have the potential of being pretty expensive, so test with
a lot of commits. A lot. So to make things cheaper, you have to
opt-in explicitely, by setting the environment variable
GIT_NOTES_TIMING_TESTS.
This patch has been improved by the following contributions:
- Junio C Hamano: tests: fix "export var=val"
Signed-off-by: Johannes Schindelin <redacted>
Signed-off-by: Johan Herland <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
t/t3302-notes-index-expensive.sh | 98 ++++++++++++++++++++++++++++++++++++++
1 files changed, 98 insertions(+), 0 deletions(-)
create mode 100755 t/t3302-notes-index-expensive.sh
From: Johan Herland <hidden> Date: 2016-06-15 22:47:19
The "-m" and "-F" options are already the established method
(in both git-commit and git-tag) to specify a commit/tag message
without invoking the editor. This patch teaches "git notes edit"
to respect the same options for specifying a notes message without
invoking the editor.
Multiple "-m" and/or "-F" options are concatenated as separate
paragraphs.
The patch also updates the "git notes" documentation and adds
selftests for the new functionality. Unfortunately, the added
selftests include a couple of lines with trailing whitespace
(without these the test will fail). This may cause git to warn
about "whitespace errors".
This patch has been improved by the following contributions:
- Thomas Rast: fix trailing whitespace in t3301
Signed-off-by: Johan Herland <redacted>
---
Documentation/git-notes.txt | 16 ++++++++++-
git-notes.sh | 64 +++++++++++++++++++++++++++++++++++++-----
t/t3301-notes.sh | 36 ++++++++++++++++++++++++
3 files changed, 107 insertions(+), 9 deletions(-)
@@ -33,6 +33,20 @@ show:: Show the notes for a given commit (defaults to HEAD).+OPTIONS+-------+-m <msg>::+ Use the given note message (instead of prompting).+ If multiple `-m` (or `-F`) options are given, their+ values are concatenated as separate paragraphs.++-F <file>::+ Take the note message from the given file. Use '-' to+ read the note message from the standard input.+ If multiple `-F` (or `-m`) options are given, their+ values are concatenated as separate paragraphs.++ Author ------ Written by Johannes Schindelin <johannes.schindelin@gmx.de>
From: Johan Herland <hidden> Date: 2016-06-15 22:47:19
Creating repos with 10/100/1000/10000 commits and notes takes a lot of time.
However, using git-fast-import to do the job is a lot more efficient than
using plumbing commands to do the same.
This patch decreases the overall run-time of this test on my machine from
~3 to ~1 minutes.
Signed-off-by: Johan Herland <redacted>
Acked-by: Johannes Schindelin <redacted>
---
t/t3302-notes-index-expensive.sh | 74 ++++++++++++++++++++++++--------------
1 files changed, 47 insertions(+), 27 deletions(-)
From: Johan Herland <hidden> Date: 2016-06-15 22:47:19
Introduce a 'notemodify' subcommand of the 'commit' command. This subcommand
is similar to 'filemodify', except that no mode is supplied (all notes have
mode 0644), and the path is set to the hex SHA1 of the given "comittish".
This enables fast import of note objects along with their associated commits,
since the notes can now be named using the mark references of their
corresponding commits.
The patch also includes a test case of the added functionality.
Signed-off-by: Johan Herland <redacted>
Acked-by: Shawn O. Pearce <redacted>
---
Documentation/git-fast-import.txt | 45 +++++++++--
fast-import.c | 88 +++++++++++++++++++-
t/t9300-fast-import.sh | 166 +++++++++++++++++++++++++++++++++++++
3 files changed, 289 insertions(+), 10 deletions(-)
@@ -339,14 +339,13 @@ commit message use a 0 length data. Commit messages are free-form and are not interpreted by Git. Currently they must be encoded in UTF-8, as fast-import does not permit other encodings to be specified.-Zero or more `filemodify`, `filedelete`, `filecopy`, `filerename`-and `filedeleteall` commands+Zero or more `filemodify`, `filedelete`, `filecopy`, `filerename`,+`filedeleteall` and `notemodify` commands may be included to update the contents of the branch prior to creating the commit. These commands may be supplied in any order. However it is recommended that a `filedeleteall` command precede-all `filemodify`, `filecopy` and `filerename` commands in the same-commit, as `filedeleteall`-wipes the branch clean (see below).+all `filemodify`, `filecopy`, `filerename` and `notemodify` commands in+the same commit, as `filedeleteall` wipes the branch clean (see below). The `LF` after the command is optional (it used to be required).
@@ -595,6 +594,40 @@ more memory per active branch (less than 1 MiB for even most large projects); so frontends that can easily obtain only the affected paths for a commit are encouraged to do so.+`notemodify`+^^^^^^^^^^^^+Included in a `commit` command to add a new note (annotating a given+commit) or change the content of an existing note. This command has+two different means of specifying the content of the note.++External data format::+ The data content for the note was already supplied by a prior+ `blob` command. The frontend just needs to connect it to the+ commit that is to be annotated.+++....+ 'N' SP <dataref> SP <committish> LF+....+++Here `<dataref>` can be either a mark reference (`:<idnum>`)+set by a prior `blob` command, or a full 40-byte SHA-1 of an+existing Git blob object.++Inline data format::+ The data content for the note has not been supplied yet.+ The frontend wants to supply it as part of this modify+ command.+++....+ 'N' SP 'inline' SP <committish> LF+ data+....+++See below for a detailed description of the `data` command.++In both formats `<committish>` is any of the commit specification+expressions also accepted by `from` (see above).+ `mark` ~~~~~~ Arranges for fast-import to save a reference to the current object, allowing
@@ -22,8 +22,8 @@ Format of STDIN stream:('author'spnamesp'<'email'>'spwhenlf)?'committer'spnamesp'<'email'>'spwhenlfcommit_msg-('from'sp(ref_str|hexsha1|sha1exp_str|idnum)lf)?-('merge'sp(ref_str|hexsha1|sha1exp_str|idnum)lf)*+('from'spcommittishlf)?+('merge'spcommittishlf)*file_change*lf?;commit_msg::=data;
@@ -41,15 +41,18 @@ Format of STDIN stream:file_obm::='M'spmodesp(hexsha1|idnum)sppath_strlf;file_inm::='M'spmodesp'inline'sppath_strlfdata;+note_obm::='N'sp(hexsha1|idnum)spcommittishlf;+note_inm::='N'sp'inline'spcommittishlf+data;new_tag::='tag'sptag_strlf-'from'sp(ref_str|hexsha1|sha1exp_str|idnum)lf+'from'spcommittishlf('tagger'spnamesp'<'email'>'spwhenlf)?tag_msg;tag_msg::=data;reset_branch::='reset'spref_strlf-('from'sp(ref_str|hexsha1|sha1exp_str|idnum)lf)?+('from'spcommittishlf)?lf?;checkpoint::='checkpoint'lf
@@ -88,6 +91,7 @@ Format of STDIN stream:# stream formatting is: \, " and LF. Otherwise these values# are UTF8.#+committish::=(ref_str|hexsha1|sha1exp_str|idnum);ref_str::=ref;sha1exp_str::=sha1exp;tag_str::=tag;
@@ -2003,6 +2007,80 @@ static void file_change_cr(struct branch *b, int rename)leaf.tree);}+staticvoidnote_change_n(structbranch*b)+{+constchar*p=command_buf.buf+2;+staticstructstrbufuq=STRBUF_INIT;+structobject_entry*oe=oe;+structbranch*s;+unsignedcharsha1[20],commit_sha1[20];+uint16_tinline_data=0;++/* <dataref> or 'inline' */+if(*p==':'){+char*x;+oe=find_mark(strtoumax(p+1,&x,10));+hashcpy(sha1,oe->sha1);+p=x;+}elseif(!prefixcmp(p,"inline")){+inline_data=1;+p+=6;+}else{+if(get_sha1_hex(p,sha1))+die("Invalid SHA1: %s",command_buf.buf);+oe=find_object(sha1);+p+=40;+}+if(*p++!=' ')+die("Missing space after SHA1: %s",command_buf.buf);++/* <committish> */+s=lookup_branch(p);+if(s){+hashcpy(commit_sha1,s->sha1);+}elseif(*p==':'){+uintmax_tcommit_mark=strtoumax(p+1,NULL,10);+structobject_entry*commit_oe=find_mark(commit_mark);+if(commit_oe->type!=OBJ_COMMIT)+die("Mark :%"PRIuMAX" not a commit",commit_mark);+hashcpy(commit_sha1,commit_oe->sha1);+}elseif(!get_sha1(p,commit_sha1)){+unsignedlongsize;+char*buf=read_object_with_reference(commit_sha1,+commit_type,&size,commit_sha1);+if(!buf||size<46)+die("Not a valid commit: %s",p);+free(buf);+}else+die("Invalid ref name or SHA1 expression: %s",p);++if(inline_data){+staticstructstrbufbuf=STRBUF_INIT;++if(p!=uq.buf){+strbuf_addstr(&uq,p);+p=uq.buf;+}+read_next_command();+parse_data(&buf);+store_object(OBJ_BLOB,&buf,&last_blob,sha1,0);+}elseif(oe){+if(oe->type!=OBJ_BLOB)+die("Not a blob (actually a %s): %s",+typename(oe->type),command_buf.buf);+}else{+enumobject_typetype=sha1_object_info(sha1,NULL);+if(type<0)+die("Blob not found: %s",command_buf.buf);+if(type!=OBJ_BLOB)+die("Not a blob (actually a %s): %s",+typename(type),command_buf.buf);+}++tree_content_set(&b->branch_tree,sha1_to_hex(commit_sha1),sha1,+S_IFREG|0644,NULL);+}+staticvoidfile_change_deleteall(structbranch*b){release_tree_content_recursive(b->branch_tree.tree);
@@ -1088,4 +1088,170 @@ INPUT_END test_expect_success'P: fail on blob mark in gitlink''test_must_failgitfast-import<input'+###+### series Q (notes)+###++note1_data="Note for the first commit"+note2_data="Note for the second commit"+note3_data="Note for the third commit"++test_tick+cat>input<<INPUT_END+blob+mark:2+data<<EOF+$file2_data+EOF++commitrefs/heads/notes-test+mark:3+committer$GIT_COMMITTER_NAME<$GIT_COMMITTER_EMAIL>$GIT_COMMITTER_DATE+data<<COMMIT+first(:3)+COMMIT++M644:2file2++blob+mark:4+data$file4_len+$file4_data+commitrefs/heads/notes-test+mark:5+committer$GIT_COMMITTER_NAME<$GIT_COMMITTER_EMAIL>$GIT_COMMITTER_DATE+data<<COMMIT+second(:5)+COMMIT++M644:4file4++commitrefs/heads/notes-test+mark:6+committer$GIT_COMMITTER_NAME<$GIT_COMMITTER_EMAIL>$GIT_COMMITTER_DATE+data<<COMMIT+third(:6)+COMMIT++M644inlinefile5+data<<EOF+$file5_data+EOF++M755inlinefile6+data<<EOF+$file6_data+EOF++blob+mark:7+data<<EOF+$note1_data+EOF++blob+mark:8+data<<EOF+$note2_data+EOF++commitrefs/notes/foobar+mark:9+committer$GIT_COMMITTER_NAME<$GIT_COMMITTER_EMAIL>$GIT_COMMITTER_DATE+data<<COMMIT+notes(:9)+COMMIT++N:7:3+N:8:5+Ninline:6+data<<EOF+$note3_data+EOF++INPUT_END+test_expect_success\+'Q: commit notes'\+'gitfast-import<input&&+gitwhatchangednotes-test'+test_expect_success\+'Q: verify pack'\+'for p in .git/objects/pack/*.pack;do git verify-pack $p||exit;done'++commit1=$(gitrev-parsenotes-test~2)+commit2=$(gitrev-parsenotes-test^)+commit3=$(gitrev-parsenotes-test)++cat>expect<<EOF+author$GIT_COMMITTER_NAME<$GIT_COMMITTER_EMAIL>$GIT_COMMITTER_DATE+committer$GIT_COMMITTER_NAME<$GIT_COMMITTER_EMAIL>$GIT_COMMITTER_DATE++first(:3)+EOF+test_expect_success\+'Q: verify first commit'\+'gitcat-filecommitnotes-test~2|sed1d>actual&&+test_cmpexpectactual'++cat>expect<<EOF+parent$commit1+author$GIT_COMMITTER_NAME<$GIT_COMMITTER_EMAIL>$GIT_COMMITTER_DATE+committer$GIT_COMMITTER_NAME<$GIT_COMMITTER_EMAIL>$GIT_COMMITTER_DATE++second(:5)+EOF+test_expect_success\+'Q: verify second commit'\+'gitcat-filecommitnotes-test^|sed1d>actual&&+test_cmpexpectactual'++cat>expect<<EOF+parent$commit2+author$GIT_COMMITTER_NAME<$GIT_COMMITTER_EMAIL>$GIT_COMMITTER_DATE+committer$GIT_COMMITTER_NAME<$GIT_COMMITTER_EMAIL>$GIT_COMMITTER_DATE++third(:6)+EOF+test_expect_success\+'Q: verify third commit'\+'gitcat-filecommitnotes-test|sed1d>actual&&+test_cmpexpectactual'++cat>expect<<EOF+author$GIT_COMMITTER_NAME<$GIT_COMMITTER_EMAIL>$GIT_COMMITTER_DATE+committer$GIT_COMMITTER_NAME<$GIT_COMMITTER_EMAIL>$GIT_COMMITTER_DATE++notes(:9)+EOF+test_expect_success\+'Q: verify notes commit'\+'gitcat-filecommitrefs/notes/foobar|sed1d>actual&&+test_cmpexpectactual'++cat>expect.unsorted<<EOF+100644blob$commit1+100644blob$commit2+100644blob$commit3+EOF+catexpect.unsorted|sort>expect+test_expect_success\+'Q: verify notes tree'\+'gitcat-file-prefs/notes/foobar^{tree}|sed"s/ [0-9a-f]* / /">actual&&+test_cmpexpectactual'++echo"$note1_data">expect+test_expect_success\+'Q: verify note for first commit'\+'git cat-file blob refs/notes/foobar:$commit1 >actual && test_cmp expect actual'++echo"$note2_data">expect+test_expect_success\+'Q: verify note for second commit'\+'git cat-file blob refs/notes/foobar:$commit2 >actual && test_cmp expect actual'++echo"$note3_data">expect+test_expect_success\+'Q: verify note for third commit'\+'git cat-file blob refs/notes/foobar:$commit3 >actual && test_cmp expect actual'+ test_done
From: Johan Herland <hidden> Date: 2016-06-15 22:47:19
The semantics used when parsing notes trees (with regards to fanout subtrees)
follow Dscho's proposal fairly closely:
- No concatenation/merging of notes is performed. If there are several notes
objects referencing a given commit, only one of those objects are used.
- If a notes object for a given commit is present in the "root" notes tree,
no subtrees are consulted; the object in the root tree is used directly.
- If there are more than one subtree that prefix-matches the given commit,
only the subtree with the longest matching prefix is consulted. This
means that if the given commit is e.g. "deadbeef", and the notes tree have
subtrees "de" and "dead", then the following paths in the notes tree are
searched: "deadbeef", "dead/beef". Note that "de/adbeef" is NOT searched.
- Fanout directories (subtrees) must references a whole number of bytes
from the SHA1 sum they subdivide. E.g. subtrees "dead" and "de" are
acceptable; "d" and "dea" are not.
- Multiple levels of fanout are allowed. All the above rules apply
recursively. E.g. "de/adbeef" is preferred over "de/adbe/ef", etc.
This patch changes the in-memory datastructure for holding parsed notes:
Instead of holding all note (and subtree) entries in a hash table, a
simple 16-tree structure is used instead. The tree structure consists of
16-arrays as internal nodes, and note/subtree entries as leaf nodes. The
tree is traversed by indexing subsequent nibbles of the search key until
a leaf node is encountered. If a subtree entry is encountered while
searching for a note, the subtree is unpacked into the 16-tree structure,
and the search continues into that subtree.
The new algorithm performs significantly better in the cases where only
a fraction of the notes need to be looked up (this is assumed to be the
common case for notes lookup). The new code even performs marginally
better in the worst case (where _all_ the notes are looked up).
In addition to this, comes the massive performance win associated with
organizing the notes tree according to some fanout scheme. Even a simple
2/38 fanout scheme is dramatically quicker to traverse (going from tens of
seconds to sub-second runtimes).
As for memory usage, the new code is marginally better than the old code in
the worst case, but in the case of looking up only some notes from a notes
tree with proper fanout, the new code uses only a small fraction of the
memory needed to hold the entire notes tree.
However, there is one casualty of this patch. The old notes lookup code was
able to parse notes that were associated with non-SHA1s (e.g. refs). The new
code requires the referenced object to be named by a SHA1 sum. Still, this
is not considered a major setback, since the notes infrastructure was not
originally intended to annotate objects outside the Git object database.
Cc: Johannes Schindelin <redacted>
Signed-off-by: Johan Herland <redacted>
---
notes.c | 294 +++++++++++++++++++++++++++++++++++++++++++++++++--------------
1 files changed, 228 insertions(+), 66 deletions(-)
From: Johan Herland <hidden> Date: 2016-06-15 22:47:19
Allocate page-sized chunks for holding struct leaf_node objects.
This slightly (but consistently) improves runtime performance of notes
lookup, at a very slight increase (~2K on average) in memory usage.
When allocating a new memory pool, the older pool is leaked, but this is
no worse than the current situation, where (pretty much) all leaf_nodes
are leaked anyway.
Signed-off-by: Johan Herland <redacted>
---
notes.c | 22 ++++++++++++++++++----
1 files changed, 18 insertions(+), 4 deletions(-)
From: Johan Herland <hidden> Date: 2016-06-15 22:47:19
This patch adds the following flags to get_commit_notes() for adjusting the
format of the produced note string:
- NOTES_SHOW_HEADER: Print "Notes:" line before the notes contents
- NOTES_INDENT: Indent notes contents by 4 spaces
Suggested-by: Johannes Schindelin <redacted>
Signed-off-by: Johan Herland <redacted>
---
notes.c | 8 +++++---
notes.h | 5 ++++-
pretty.c | 3 ++-
3 files changed, 11 insertions(+), 5 deletions(-)
@@ -123,6 +123,7 @@ The placeholders are: - '%s': subject - '%f': sanitized subject line, suitable for a filename - '%b': body+- '%N': commit notes - '%Cred': switch color to red - '%Cgreen': switch color to green - '%Cblue': switch color to blue
@@ -702,6 +702,10 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,case'd':format_decoration(sb,commit);return1;+case'N':+get_commit_notes(commit,sb,git_log_output_encoding?+git_log_output_encoding:git_commit_encoding,0);+return1;}/* For the rest we have to parse the commit header. */
From: Johan Herland <hidden> Date: 2016-06-15 22:47:19
Add selftests verifying:
- that we are able to parse notes trees with various fanout schemes
- that notes trees with conflicting fanout schemes are parsed as expected
Signed-off-by: Johan Herland <redacted>
---
t/t3303-notes-subtrees.sh | 206 +++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 206 insertions(+), 0 deletions(-)
create mode 100755 t/t3303-notes-subtrees.sh
@@ -0,0 +1,206 @@+#!/bin/sh++test_description='Test commit notes organized in subtrees'++../test-lib.sh++number_of_commits=100++start_note_commit(){+test_tick&&+cat<<INPUT_END+commitrefs/notes/commits+committer$GIT_COMMITTER_NAME<$GIT_COMMITTER_EMAIL>$GIT_COMMITTER_DATE+data<<COMMIT+notes+COMMIT++fromrefs/notes/commits^0+deleteall+INPUT_END++}++verify_notes(){+gitlog|grep"^ ">output&&+i=$number_of_commits&&+while[$i-gt0];do+echo" commit #$i"&&+echo" note for commit #$i"&&+i=$(($i-1));+done>expect&&+test_cmpexpectoutput+}++test_expect_success'setup: create $number_of_commits commits''++(+nr=0&&+while[$nr-lt$number_of_commits];do+nr=$(($nr+1))&&+test_tick&&+cat<<INPUT_END+commitrefs/heads/master+committer$GIT_COMMITTER_NAME<$GIT_COMMITTER_EMAIL>$GIT_COMMITTER_DATE+data<<COMMIT+commit#$nr+COMMIT++M644inlinefile+data<<EOF+fileincommit#$nr+EOF++INPUT_END++done&&+test_tick&&+cat<<INPUT_END+commitrefs/notes/commits+committer$GIT_COMMITTER_NAME<$GIT_COMMITTER_EMAIL>$GIT_COMMITTER_DATE+data<<COMMIT+nonotes+COMMIT++deleteall++INPUT_END++)|+gitfast-import--quiet&&+gitconfigcore.notesRefrefs/notes/commits+'++test_expect_success'test notes in 2/38-fanout''++(+start_note_commit&&+nr=$number_of_commits&&+gitrev-listrefs/heads/master|+whilereadsha1;do+note_path=$(echo"$sha1"|sed"s|^..|&/|")+cat<<INPUT_END&&+M100644inline$note_path+data<<EOF+noteforcommit#$nr+EOF++INPUT_END++nr=$(($nr-1))+done+)|+gitfast-import--quiet+'++test_expect_success'verify notes in 2/38-fanout''verify_notes'++test_expect_success'test notes in 4/36-fanout''++(+start_note_commit&&+nr=$number_of_commits&&+gitrev-listrefs/heads/master|+whilereadsha1;do+note_path=$(echo"$sha1"|sed"s|^....|&/|")+cat<<INPUT_END&&+M100644inline$note_path+data<<EOF+noteforcommit#$nr+EOF++INPUT_END++nr=$(($nr-1))+done+)|+gitfast-import--quiet+'++test_expect_success'verify notes in 4/36-fanout''verify_notes'++test_expect_success'test notes in 4/36-fanout overriding 2/38-fanout''++(+start_note_commit&&+nr=$number_of_commits&&+gitrev-listrefs/heads/master|+whilereadsha1;do+ignored_note_path=$(echo"$sha1"|sed"s|^..|&/|")+preferred_note_path=$(echo"$sha1"|sed"s|^....|&/|")+cat<<INPUT_END&&+M100644inline$ignored_note_path+data<<EOF+IGNOREDnoteforcommit#$nr+EOF++M100644inline$preferred_note_path+data<<EOF+noteforcommit#$nr+EOF++INPUT_END++nr=$(($nr-1))+done+)|+gitfast-import--quiet+'++test_expect_success'verify notes in 4/36-fanout overriding 2/38-fanout''verify_notes'++test_expect_success'test notes in 2/2/36-fanout''++(+start_note_commit&&+nr=$number_of_commits&&+gitrev-listrefs/heads/master|+whilereadsha1;do+note_path=$(echo"$sha1"|sed"s|^\(..\)\(..\)|\1/\2/|")+cat<<INPUT_END&&+M100644inline$note_path+data<<EOF+noteforcommit#$nr+EOF++INPUT_END++nr=$(($nr-1))+done+)|+gitfast-import--quiet+'++test_expect_success'verify notes in 2/2/36-fanout''verify_notes'++test_expect_success'test notes in 2/38-fanout overriding 2/2/36-fanout''++(+start_note_commit&&+nr=$number_of_commits&&+gitrev-listrefs/heads/master|+whilereadsha1;do+ignored_note_path=$(echo"$sha1"|sed"s|^\(..\)\(..\)|\1/\2/|")+preferred_note_path=$(echo"$sha1"|sed"s|^..|&/|")+cat<<INPUT_END&&+M100644inline$ignored_note_path+data<<EOF+IGNOREDnoteforcommit#$nr+EOF++M100644inline$preferred_note_path+data<<EOF+noteforcommit#$nr+EOF++INPUT_END++nr=$(($nr-1))+done+)|+gitfast-import--quiet+'++test_expect_success'verify notes in 2/38-fanout overriding 2/2/36-fanout''verify_notes'++test_done
From: Junio C Hamano <hidden> Date: 2016-06-15 22:47:19
Johan Herland [off-list ref] writes:
The semantics used when parsing notes trees (with regards to fanout subtrees)
follow Dscho's proposal fairly closely:
- No concatenation/merging of notes is performed. If there are several notes
objects referencing a given commit, only one of those objects are used.
- If a notes object for a given commit is present in the "root" notes tree,
no subtrees are consulted; the object in the root tree is used directly.
- If there are more than one subtree that prefix-matches the given commit,
only the subtree with the longest matching prefix is consulted. This
means that if the given commit is e.g. "deadbeef", and the notes tree have
subtrees "de" and "dead", then the following paths in the notes tree are
searched: "deadbeef", "dead/beef". Note that "de/adbeef" is NOT searched.
- Fanout directories (subtrees) must references a whole number of bytes
from the SHA1 sum they subdivide. E.g. subtrees "dead" and "de" are
acceptable; "d" and "dea" are not.
- Multiple levels of fanout are allowed. All the above rules apply
recursively. E.g. "de/adbeef" is preferred over "de/adbe/ef", etc.
If I am reading this correctly, the earlier parts of the series were
aiming to let multiple people to add notes to the same commit more or less
uncordinated while still allowing to merge them sensibly, but now such a
workflow becomes impossible with this change.
The above claims notes trees with different levels of fan-out are allowed,
but what it really means is that merging notes trees with different levels
of fan-out will produce a useless result that records notes for the same
commit in different blobs all over the notes tree, and asking the notes
mechanism to give the notes for one commit will give only one piece that
originates in the tree whose creator happened to have used the longest
prefix while ignoring all others. It may _allow_ such a layout, but how
would such semantics be useful in the first place?
I suspect that I am missing something but my gut feeling is that this
change turns an interesting hack (even though it might be expensive) into
a hack that is not useful at all in the real world, without some order,
discipline, or guideline is applied.
What's the recommended way to work with this system from the end user's
point of view in a distirbuted environment? Somebody up in the project is
supposed to decide what fan-out is to be used for the whole project and
everybody should follow that structure? If a participant in the project
forgets that rule (or makes a mistake), a notes tree that mistakenly
merges his notes tree becomes practically useless? If so, perhaps we
would need a mechanism to avoid such a mistake from happening?
From: Alex Riesen <hidden> Date: 2016-06-15 22:47:19
On Thu, Aug 27, 2009 at 03:43, Johan Herland[off-list ref] wrote:
When allocating a new memory pool, the older pool is leaked, but this is
no worse than the current situation, where (pretty much) all leaf_nodes
are leaked anyway.
Could you return the unused nodes back into tghe mempool?
By making the pool a preallocated list, perhaps?
And then it is trivial to provide a deallocation function for the mempool,
which something really concerned about the memleak can call (like when
or if libgit get more usable in an application context).
From: Johan Herland <hidden> Date: 2016-06-15 22:47:19
On Thursday 27 August 2009, Junio C Hamano wrote:
Johan Herland [off-list ref] writes:
quoted
The semantics used when parsing notes trees (with regards to fanout
subtrees) follow Dscho's proposal fairly closely:
- No concatenation/merging of notes is performed. If there are several
notes objects referencing a given commit, only one of those objects are
used. - If a notes object for a given commit is present in the "root"
notes tree, no subtrees are consulted; the object in the root tree is
used directly. - If there are more than one subtree that prefix-matches
the given commit, only the subtree with the longest matching prefix is
consulted. This means that if the given commit is e.g. "deadbeef", and
the notes tree have subtrees "de" and "dead", then the following paths
in the notes tree are searched: "deadbeef", "dead/beef". Note that
"de/adbeef" is NOT searched. - Fanout directories (subtrees) must
references a whole number of bytes from the SHA1 sum they subdivide.
E.g. subtrees "dead" and "de" are acceptable; "d" and "dea" are not.
- Multiple levels of fanout are allowed. All the above rules apply
recursively. E.g. "de/adbeef" is preferred over "de/adbe/ef", etc.
If I am reading this correctly, the earlier parts of the series were
aiming to let multiple people to add notes to the same commit more or
less uncordinated while still allowing to merge them sensibly, but now
such a workflow becomes impossible with this change.
The above claims notes trees with different levels of fan-out are
allowed, but what it really means is that merging notes trees with
different levels of fan-out will produce a useless result that records
notes for the same commit in different blobs all over the notes tree, and
asking the notes mechanism to give the notes for one commit will give
only one piece that originates in the tree whose creator happened to have
used the longest prefix while ignoring all others. It may _allow_ such a
layout, but how would such semantics be useful in the first place?
I suspect that I am missing something but my gut feeling is that this
change turns an interesting hack (even though it might be expensive) into
a hack that is not useful at all in the real world, without some order,
discipline, or guideline is applied.
As you may remember, the major, remaining problem with the jh/notes patch
series was that as the number of notes in a repo grew, the runtime cost of
displaying (even a single one of) them became prohibitive. This was the
major reason why Dscho and me did NOT want you to merge this series earlier.
The solution to this performance problem (which has been discussed since
almost a year ago), is to use some fanout scheme in the notes tree, so that
we can load individual notes without necessarily parsing the entire notes
tree. This patch implements the _reading_ of a notes trees with fanout.
However, as you correctly identify, allowing fanout makes it possible to add
multiple notes for the same commit in the same notes tree.
Therefore, we must now create the order/discipline/guideline you request by
taking away this extra freedom.
This will take the form of enforcing a chosen fanout scheme when _writing_
notes. This code (yet to be written) will include:
- refactoring the notes tree when the number of notes call for a different
fanout scheme (e.g. create a 2/38 fanout scheme when the number of notes in
a (sub)tree exceed some threshold).
- adding and editing notes while following the current fanout scheme.
- adding a custom merge strategy for note trees, which reads notes trees
Currently I have a two different ideas on a suitable fanout scheme:
1. Define a threshold where the number of notes in a (sub)tree becomes large
enough warrant a fanout. For now, let's assume that threshold is 1024. Start
with en empty notes tree with no fanout. When we reach 1024 notes, split the
notes tree into 256 subdirs (2/38 fanout). As each of the subdirs reach 1024
notes, split those subdirs further (2/2/36 fanout), and so on. In practice
(since SHA1 gives us a uniform distribution), notes trees with up to:
- < 1024 notes will have no fanout
- < ~256K notes will have 2/38 fanout
- < ~64M notes will have 2/2/36 fanout
- < ~16G notes will have 2/2/2/34 fanout
2. Simply decide on a constant 2/2/36 fanout. For the case with < 256K
notes, this is somewhat wasteful, but not prohibitively expensive. For the
case with > 64M notes, performance will start to degrade. The big advantage
with this approach is that when this is hardcoded into the notes code, we
have regained the property that notes for a given commit have exactly _one_
unique position in the notes tree across all installations (enabling us to
fall back on the regular merge strategy).
What's the recommended way to work with this system from the end user's
point of view in a distirbuted environment?
The end user should not know or care about what fanout scheme is used.
Everything should be handled seamlessly by the code.
Somebody up in the project is supposed to decide what fan-out is to be
used for the whole project and everybody should follow that structure?
Nope, the code should decide which fanout scheme to use.
If a participant in the project forgets that rule (or makes a mistake), a
notes tree that mistakenly merges his notes tree becomes practically
useless?
No. Again this should be invisible to the user.
If so, perhaps we would need a mechanism to avoid such a mistake from
happening?
The notes code should prevent notes that violate the fanout scheme from
being created.
The fanout scheme is not something the user should have to worry about at
all.
I totally understand that you don't want to merge the notes feature before
the "writing" side is taken care of. As such, this iteration is simply yet
another iteration towards that final goal.
...Johan
--
Johan Herland, [off-list ref]
www.herland.net
From: Johan Herland <hidden> Date: 2016-06-15 22:47:19
On Thursday 27 August 2009, Alex Riesen wrote:
On Thu, Aug 27, 2009 at 03:43, Johan Herland[off-list ref] wrote:
quoted
When allocating a new memory pool, the older pool is leaked, but this
is no worse than the current situation, where (pretty much) all
leaf_nodes are leaked anyway.
Could you return the unused nodes back into the mempool?
By making the pool a preallocated list, perhaps?
Yes, maintaining a free-list is certainly possible. However, the number of
free()d leaf_nodes is relatively small (only subtree entries are free()d
after unpacking them into the tree structure), so I'm not sure it pays off,
runtime-wise.
And then it is trivial to provide a deallocation function for the
mempool, which something really concerned about the memleak can call
(like when or if libgit get more usable in an application context).
Yes, I plan to provide a free_notes() function that free()s all the memory
associated with the notes data structure. This would of course keep
references to all the mempools, and deallocate them (along with all the
int_nodes).
...Johan
--
Johan Herland, [off-list ref]
www.herland.net
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:47:19
Hi,
On Wed, 26 Aug 2009, Junio C Hamano wrote:
Johan Herland [off-list ref] writes:
quoted
The semantics used when parsing notes trees (with regards to fanout subtrees)
follow Dscho's proposal fairly closely:
- No concatenation/merging of notes is performed. If there are several notes
objects referencing a given commit, only one of those objects are used.
- If a notes object for a given commit is present in the "root" notes tree,
no subtrees are consulted; the object in the root tree is used directly.
- If there are more than one subtree that prefix-matches the given commit,
only the subtree with the longest matching prefix is consulted. This
means that if the given commit is e.g. "deadbeef", and the notes tree have
subtrees "de" and "dead", then the following paths in the notes tree are
searched: "deadbeef", "dead/beef". Note that "de/adbeef" is NOT searched.
- Fanout directories (subtrees) must references a whole number of bytes
from the SHA1 sum they subdivide. E.g. subtrees "dead" and "de" are
acceptable; "d" and "dea" are not.
- Multiple levels of fanout are allowed. All the above rules apply
recursively. E.g. "de/adbeef" is preferred over "de/adbe/ef", etc.
If I am reading this correctly, the earlier parts of the series were
aiming to let multiple people to add notes to the same commit more or less
uncordinated while still allowing to merge them sensibly, but now such a
workflow becomes impossible with this change.
The above claims notes trees with different levels of fan-out are allowed,
but what it really means is that merging notes trees with different levels
of fan-out will produce a useless result that records notes for the same
commit in different blobs all over the notes tree, and asking the notes
mechanism to give the notes for one commit will give only one piece that
originates in the tree whose creator happened to have used the longest
prefix while ignoring all others. It may _allow_ such a layout, but how
would such semantics be useful in the first place?
I suspect that I am missing something but my gut feeling is that this
change turns an interesting hack (even though it might be expensive) into
a hack that is not useful at all in the real world, without some order,
discipline, or guideline is applied.
What's the recommended way to work with this system from the end user's
point of view in a distirbuted environment? Somebody up in the project is
supposed to decide what fan-out is to be used for the whole project and
everybody should follow that structure? If a participant in the project
forgets that rule (or makes a mistake), a notes tree that mistakenly
merges his notes tree becomes practically useless? If so, perhaps we
would need a mechanism to avoid such a mistake from happening?
Hmm...
Maybe you're right (and mugwump was right all along) that _all_ matching
notes should be shown...
Ciao,
Dscho
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:47:19
Hi,
On Thu, 27 Aug 2009, Johan Herland wrote:
On Thursday 27 August 2009, Junio C Hamano wrote:
quoted
Somebody up in the project is supposed to decide what fan-out is to be
used for the whole project and everybody should follow that structure?
Nope, the code should decide which fanout scheme to use.
I half-agree, the code should decide which fanout scheme to use, but
_only_ when producing new notes.
I imagine that it could merge the existing notes, and try to make sure
that there are no more blobs in a given subtree than a certain threshold;
if that threshold is reached, it could fan-out using 2-digit subtrees,
merging what needs merging (by concatenation) along the way.
The natural precedence of shallower paths/longer basenames should cope
well with that (i.e. prefer to show abcd/... over ab/cd/...).
Ciao,
Dscho
From: Johan Herland <hidden> Date: 2016-06-15 22:47:19
On Thursday 27 August 2009, Johan Herland wrote:
On Thursday 27 August 2009, Alex Riesen wrote:
quoted
On Thu, Aug 27, 2009 at 03:43, Johan Herland[off-list ref] wrote:
quoted
When allocating a new memory pool, the older pool is leaked, but this
is no worse than the current situation, where (pretty much) all
leaf_nodes are leaked anyway.
Could you return the unused nodes back into the mempool?
By making the pool a preallocated list, perhaps?
Yes, maintaining a free-list is certainly possible. However, the number
of free()d leaf_nodes is relatively small (only subtree entries are
free()d after unpacking them into the tree structure), so I'm not sure it
pays off, runtime-wise.
I played around with the free-list idea, but it cost more than the memory
pooling code saved in the first place. I'm leaning towards dropping the
whole memory pooling idea, as the small run-time improvement is probably not
worth the added complexity. We'll see. I'll re-evaluate once I've refactored
the code according to the other threads of this discussion.
quoted
And then it is trivial to provide a deallocation function for the
mempool, which something really concerned about the memleak can call
(like when or if libgit get more usable in an application context).
Yes, I plan to provide a free_notes() function that free()s all the
memory associated with the notes data structure. This would of course
keep references to all the mempools, and deallocate them (along with all
the int_nodes).
This still stands, of course. Should be part of the next iteration.
...Johan
--
Johan Herland, [off-list ref]
www.herland.net