What's cooking in git.git (Jan 2009, #05; Wed, 21)

What's cooking in git.git (Jan 2009, #05; Wed, 21)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with '-' are
only in 'pu' while commits prefixed with '+' are in 'next'.  The ones
marked with '.' do not appear in any of the branches, but I am still
holding onto them.

The topics list the commits in reverse chronological order.  The topics
meant to be merged to the maintenance series have "maint-" in their names.

----------------------------------------------------------------
[New Topics]

* js/valgrind (Wed Jan 21 02:36:40 2009 +0100) 2 commits
 - valgrind: ignore ldso errors
 - Add valgrind support in test scripts

Dscho seems to have some updates out of discussion with Peff.

----------------------------------------------------------------
[Stalled and may need help and prodding to go forward]

* jc/blame (Wed Jun 4 22:58:40 2008 -0700) 2 commits
 + blame: show "previous" information in --porcelain/--incremental
   format
 + git-blame: refactor code to emit "porcelain format" output

This gives Porcelains (like gitweb) the information on the commit _before_
the one that the final blame is laid on, which should save them one
rev-parse to dig further.  The line number in the "previous" information
may need refining, and sanity checking code for reference counting may
need to be resurrected before this can move forward.

* db/foreign-scm (Sun Jan 11 15:12:10 2009 -0500) 3 commits
 - Support fetching from foreign VCSes
 - Add specification of git-vcs helpers
 - Add "vcs" config option in remotes

The "spec" did not seem quite well cooked yet, but in the longer term I
think something like this to allow interoperating with other SCMs as if
the other end is a native git repository is a very worthy goal.

----------------------------------------------------------------
[Actively cooking]

* sp/runtime-prefix (Sun Jan 18 13:00:15 2009 +0100) 7 commits
 - Windows: Revert to default paths and convert them by
   RUNTIME_PREFIX
 - Compute prefix at runtime if RUNTIME_PREFIX is set
 - Modify setup_path() to only add git_exec_path() to PATH
 - Add calls to git_extract_argv0_path() in programs that call
   git_config_*
 - git_extract_argv0_path(): Move check for valid argv0 from caller
   to callee
 - Refactor git_set_argv0_path() to git_extract_argv0_path()
 - Move computation of absolute paths from Makefile to runtime (in
   preparation for RUNTIME_PREFIX)

We should move this to 'next' soon with J6t's blessing.

* lh/submodule-tree-traversal (Mon Jan 12 00:45:55 2009 +0100) 3 commits
 + builtin-ls-tree: enable traversal of submodules
 + archive.c: enable traversal of submodules
 + tree.c: add support for traversal of submodules

I think choosing the submodules to descend into by seeing if the commit
happens to be available is a horribly broken semantics; it needs to be
fixed before this can move to 'master'.

* jk/signal-cleanup (Sun Jan 11 06:36:49 2009 -0500) 3 commits
 - pager: do wait_for_pager on signal death
 - refactor signal handling for cleanup functions
 - chain kill signals for cleanup functions

I think this can move to 'next', as Peff and J6t agreed on how to fix
things up as needed for Windows.

* ks/maint-mailinfo-folded (Tue Jan 13 01:21:04 2009 +0300) 5 commits
 - mailinfo: tests for RFC2047 examples
 - mailinfo: add explicit test for mails like '<a.u.thor@example.com>
   (A U Thor)'
 - mailinfo: more smarter removal of rfc822 comments from 'From'
 + mailinfo: 'From:' header should be unfold as well
 + mailinfo: correctly handle multiline 'Subject:' header

As far as I can see, the only remaining thing is a minor fix-up in the
"comment removal" one before we can move this fully to 'next'.

* js/notes (Tue Jan 13 20:57:16 2009 +0100) 6 commits
 + git-notes: fix printing of multi-line notes
 + notes: fix core.notesRef documentation
 + Add an expensive test for git-notes
 + Speed up git notes lookup
 + Add a script to edit/inspect notes
 + Introduce commit notes

It would be nice to hear a real world success story using the notes
mechanism before casting this design in stone.

* sc/gitweb-category (Fri Dec 12 00:45:12 2008 +0100) 3 commits
 - gitweb: Optional grouping of projects by category
 - gitweb: Split git_project_list_body in two functions
 - gitweb: Modularized git_get_project_description to be more generic

Design discussion between Jakub and Sebastien continues.

----------------------------------------------------------------
[Graduated to "master"]

* jk/color-parse (Sat Jan 17 10:38:46 2009 -0500) 2 commits
 + expand --pretty=format color options
 + color: make it easier for non-config to parse color specs

* sb/hook-cleanup (Sat Jan 17 04:02:55 2009 +0100) 5 commits
 + run_hook(): allow more than 9 hook arguments
 + run_hook(): check the executability of the hook before filling
   argv
 + api-run-command.txt: talk about run_hook()
 + Move run_hook() from builtin-commit.c into run-command.c (libgit)
 + checkout: don't crash on file checkout before running post-
   checkout hook

* rs/ctype (Sat Jan 17 16:50:37 2009 +0100) 4 commits
 + Add is_regex_special()
 + Change NUL char handling of isspecial()
 + Reformat ctype.c
 + Add ctype test

* jf/am-failure-report (Sun Jan 18 19:34:31 2009 -0800) 2 commits
 + git-am: re-fix the diag message printing
 + git-am: Make it easier to see which patch failed

* sg/maint-gitdir-in-subdir (Fri Jan 16 16:37:33 2009 +0100) 1 commit
 + Fix gitdir detection when in subdir of gitdir

This has my "don't do the fullpath if you are directly inside .git"
squashed in, so it should be much safer.

* am/maint-push-doc (Sun Jan 18 15:36:58 2009 +0100) 4 commits
 + Documentation: avoid using undefined parameters
 + Documentation: mention branches rather than heads
 + Documentation: remove a redundant elaboration
 + Documentation: git push repository can also be a remote

* lt/maint-wrap-zlib (Wed Jan 7 19:54:47 2009 -0800) 1 commit
 + Wrap inflate and other zlib routines for better error reporting

Needs the "free our memory upon seeing Z_MEM_ERROR and try again" bits
extracted from Shawn's patch on top of this one.

* kb/am-directory (Wed Jan 14 16:29:59 2009 -0800) 2 commits
 + git-am: fix shell quoting
 + git-am: add --directory=<dir> option

This is "third-time-lucky, perhaps?" resurrection.  I do not think I'd be
using this very often, but it originated from a real user request.

* jc/maint-format-patch-o-relative (Mon Jan 12 15:18:02 2009 -0800) 1 commit
 + Teach format-patch to handle output directory relative to cwd

----------------------------------------------------------------
[Will merge to "master" soon]

* kb/lstat-cache (Sun Jan 18 16:14:54 2009 +0100) 5 commits
 + lstat_cache(): introduce clear_lstat_cache() function
 + lstat_cache(): introduce invalidate_lstat_cache() function
 + lstat_cache(): introduce has_dirs_only_path() function
 + lstat_cache(): introduce has_symlink_or_noent_leading_path()
   function
 + lstat_cache(): more cache effective symlink/directory detection

* tr/previous-branch (Wed Jan 21 00:37:38 2009 -0800) 10 commits
 + Simplify parsing branch switching events in reflog
 + Introduce for_each_recent_reflog_ent().
 + interpret_nth_last_branch(): plug small memleak
 + Fix reflog parsing for a malformed branch switching entry
 + Fix parsing of @{-1}@{1}
 + interpret_nth_last_branch(): avoid traversing the reflog twice
 + checkout: implement "-" abbreviation, add docs and tests
 + sha1_name: support @{-N} syntax in get_sha1()
 + sha1_name: tweak @{-N} lookup
 + checkout: implement "@{-N}" shortcut name for N-th last branch

* js/maint-all-implies-HEAD (Sat Jan 17 22:27:08 2009 -0800) 2 commits
 + bundle: allow the same ref to be given more than once
 + revision walker: include a detached HEAD in --all

* mh/unify-color (Sun Jan 18 21:39:12 2009 +0100) 2 commits
 + move the color variables to color.c
 + handle color.ui at a central place

* cb/add-pathspec (Wed Jan 14 15:54:35 2009 +0100) 2 commits
 + remove pathspec_match, use match_pathspec instead
 + clean up pathspec matching

* js/diff-color-words (Tue Jan 20 21:46:57 2009 -0600) 8 commits
 + color-words: Support diff.wordregex config option
 + color-words: make regex configurable via attributes
 + color-words: expand docs with precise semantics
 + color-words: enable REG_NEWLINE to help user
 + color-words: take an optional regular expression describing words
 + color-words: change algorithm to allow for 0-character word
   boundaries
 + color-words: refactor word splitting and use ALLOC_GROW()
 + Add color_fwrite_lines(), a function coloring each line
   individually

* js/patience-diff (Thu Jan 1 17:39:37 2009 +0100) 3 commits
 + bash completions: Add the --patience option
 + Introduce the diff option '--patience'
 + Implement the patience diff algorithm

----------------------------------------------------------------
[On Hold]

* jk/renamelimit (Sat May 3 13:58:42 2008 -0700) 1 commit
 . diff: enable "too large a rename" warning when -M/-C is explicitly
   asked for

* jc/stripspace (Sun Mar 9 00:30:35 2008 -0800) 6 commits
 . git-am --forge: add Signed-off-by: line for the author
 . git-am: clean-up Signed-off-by: lines
 . stripspace: add --log-clean option to clean up signed-off-by:
   lines
 . stripspace: use parse_options()
 . Add "git am -s" test
 . git-am: refactor code to add signed-off-by line for the committer

* jc/post-simplify (Fri Aug 15 01:34:51 2008 -0700) 2 commits
 . revision --simplify-merges: incremental simplification
 . revision --simplify-merges: prepare for incremental simplification

* wp/add-patch-find (Thu Nov 27 04:08:03 2008 +0000) 3 commits
 . In add --patch, Handle K,k,J,j slightly more gracefully.
 . Add / command in add --patch
 . git-add -i/-p: Change prompt separater from slash to comma

* jc/grafts (Wed Jul 2 17:14:12 2008 -0700) 1 commit
 . [BROKEN wrt shallow clones] Ignore graft during object transfer

* jc/replace (Fri Oct 31 09:21:39 2008 -0700) 1 commit
 . WIP

Re: What's cooking in git.git (Jan 2009, #05; Wed, 21)

[Jeff King]

On Wed, Jan 21, 2009 at 07:55:36PM -0800, Junio C Hamano wrote:

> * jk/signal-cleanup (Sun Jan 11 06:36:49 2009 -0500) 3 commits
>  - pager: do wait_for_pager on signal death
>  - refactor signal handling for cleanup functions
>  - chain kill signals for cleanup functions
> 
> I think this can move to 'next', as Peff and J6t agreed on how to fix
> things up as needed for Windows.

Please wait for my re-roll, which I'll send in a few minutes.

-Peff

Re: What's cooking in git.git (Jan 2009, #05; Wed, 21)

[Johannes Schindelin]

Hi,

On Wed, 21 Jan 2009, Junio C Hamano wrote:

> * js/notes (Tue Jan 13 20:57:16 2009 +0100) 6 commits
>  + git-notes: fix printing of multi-line notes
>  + notes: fix core.notesRef documentation
>  + Add an expensive test for git-notes
>  + Speed up git notes lookup
>  + Add a script to edit/inspect notes
>  + Introduce commit notes
> 
> It would be nice to hear a real world success story using the notes
> mechanism before casting this design in stone.

I'd like to have some profiling done before that.  For example, I am still 
a bit unsure how the things would perform with a 50-deep delta chain for 
a notes tree having 50,000+ notes in it (which I think will not be all 
that unreasonable for a medium-sized project that stores bug-tracking 
information in the notes).

I have a gut feeling that the performance dip I saw is a direct result of 
doing away with the fan-out "subdirectories": remember, originally, I had 
a tree structure much like the loose objects in .git/objects/??/, while 
Peff convinced me that a flat tree object should be enough.

I could be wrong on that, though.

> * js/patience-diff (Thu Jan 1 17:39:37 2009 +0100) 3 commits
>  + bash completions: Add the --patience option
>  + Introduce the diff option '--patience'
>  + Implement the patience diff algorithm

There is this one issue that my patience's output differs from bzr's.  
Since the patience diff algorithm is so lousily documented, I do not know 
if it is due to my misunderstanding the algorithm, or due to bzr doing 
something clever in addition.

I'd be thankful if somebody could clarify that issue.

Ciao,
Dscho

Re: What's cooking in git.git (Jan 2009, #05; Wed, 21)

[Boyd Stephen Smith Jr.]

[-- Attachment #1: Type: text/plain, Size: 2008 bytes --]

On Wednesday 21 January 2009, Junio C Hamano <gitster@pobox.com> wrote 
about 'What's cooking in git.git (Jan 2009, #05; Wed, 21)':
>* js/notes (Tue Jan 13 20:57:16 2009 +0100) 6 commits
> + git-notes: fix printing of multi-line notes
> + notes: fix core.notesRef documentation
> + Add an expensive test for git-notes
> + Speed up git notes lookup
> + Add a script to edit/inspect notes
> + Introduce commit notes
>
>It would be nice to hear a real world success story using the notes
>mechanism before casting this design in stone.

I'll see if I can't try to put this through some paces over the week.  
Also, I'd like to see some support for notes in push/fetch, but it could 
certainly be added afterwards.

>* js/diff-color-words (Tue Jan 20 21:46:57 2009 -0600) 8 commits
> + color-words: Support diff.wordregex config option
> + color-words: make regex configurable via attributes
> + color-words: expand docs with precise semantics
> + color-words: enable REG_NEWLINE to help user
> + color-words: take an optional regular expression describing words
> + color-words: change algorithm to allow for 0-character word
>   boundaries
> + color-words: refactor word splitting and use ALLOC_GROW()
> + Add color_fwrite_lines(), a function coloring each line
>   individually

I think my patch in 
http://thread.gmane.org/gmane.comp.version-control.git/106567 should be 
applied to the top of this.  It respells "wordregex" to match existing 
uses throughout the repo.  Dscho had issues with one hunk, but I think I 
addressed them in my follow-up.  It looks like 98a4d87b (color-words: 
Support diff.wordregex config option) still has the internally-consistent 
runtogether spelling that doesn't match other configuration variables etc.
-- 
Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
bss@iguanasuicide.net                     ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.net/                      \_/     

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH v2 1/5] Windows: Fix signal numbers

[Jeff King]

From: Johannes Sixt <j6t@kdbg.org>

We had defined some SIG_FOO macros that appear in the code, but that are
not supported on Windows, in order to make the code compile.  But a
subsequent change will assert that a signal number is non-zero.  We now
use the signal numbers that are commonly used on POSIX systems.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Jeff King <peff@peff.net>
---
This is necessary to avoid violating sigchain assertions in the next
patch.

 compat/mingw.h |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/compat/mingw.h b/compat/mingw.h
index 4f275cb..a255898 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -21,12 +21,12 @@ typedef int pid_t;
 #define WEXITSTATUS(x) ((x) & 0xff)
 #define WIFSIGNALED(x) ((unsigned)(x) > 259)
 
-#define SIGKILL 0
-#define SIGCHLD 0
-#define SIGPIPE 0
-#define SIGHUP 0
-#define SIGQUIT 0
-#define SIGALRM 100
+#define SIGHUP 1
+#define SIGQUIT 3
+#define SIGKILL 9
+#define SIGPIPE 13
+#define SIGALRM 14
+#define SIGCHLD 17
 
 #define F_GETFD 1
 #define F_SETFD 2
-- 
1.6.1.403.g6c435

[PATCH v2 2/5] diff: refactor tempfile cleanup handling

[Jeff King]

There are two pieces of code that create tempfiles for diff:
run_external_diff and run_textconv. The former cleans up its
tempfiles in the face of premature death (i.e., by die() or
by signal), but the latter does not. After this patch, they
will both use the same cleanup routines.

To make clear what the change is, let me first explain what
happens now:

  - run_external_diff uses a static global array of 2
    diff_tempfile structs (since it knows it will always
    need exactly 2 tempfiles). It calls prepare_temp_file
    (which doesn't know anything about the global array) on
    each of the structs, creating the tempfiles that need to
    be cleaned up. It then registers atexit and signal
    handlers to look through the global array and remove the
    tempfiles. If it succeeds, it calls the handler manually
    (which marks the tempfile structs as unused).

  - textconv has its own tempfile struct, which it allocates
    using prepare_temp_file and cleans up manually. No
    signal or atexit handlers.

The new code moves the installation of cleanup handlers into
the prepare_temp_file function. Which means that that
function now has to understand that there is static tempfile
storage. So what happens now is:

  - run_external_diff calls prepare_temp_file
  - prepare_temp_file calls claim_diff_tempfile, which
    allocates an unused slot from our global array
  - prepare_temp_file installs (if they have not already
    been installed) atexit and signal handlers for cleanup
  - prepare_temp_file sets up the tempfile as usual
  - prepare_temp_file returns a pointer to the allocated
    tempfile

The advantage being that run_external_diff no longer has to
care about setting up cleanup handlers. Now by virtue of
calling prepare_temp_file, run_textconv gets the same
benefit, as will any future users of prepare_temp_file.

There are also a few side benefits to the specific
implementation:

  - we now install cleanup handlers _before_ allocating the
    tempfile, closing a race which could leave temp cruft

  - when allocating a slot in the global array, we will now
    detect a situation where the old slots were not properly
    vacated (i.e., somebody forgot to call remove upon
    leaving the function). In the old code, such a situation
    would silently overwrite the tempfile names, meaning we
    would forget to clean them up. The new code dies with a
    bug warning.

  - we make sure only to install the signal handler once.
    This isn't a big deal, since we are just overwriting the
    old handler, but will become an issue when a later patch
    converts the code to use sigchain

Signed-off-by: Jeff King <peff@peff.net>
---
This patch is new since v1. I started with just looking at whether it
was safe to move the signal() call, but there were really several things
to be cleaned up, and it just made sense to do them all together.

 diff.c |  107 +++++++++++++++++++++++++++++++++-------------------------------
 1 files changed, 55 insertions(+), 52 deletions(-)

diff --git a/diff.c b/diff.c
index 0731313..ae6d552 100644
--- a/diff.c
+++ b/diff.c
@@ -167,6 +167,33 @@ static struct diff_tempfile {
 	char tmp_path[PATH_MAX];
 } diff_temp[2];
 
+static struct diff_tempfile *claim_diff_tempfile(void) {
+	int i;
+	for (i = 0; i < ARRAY_SIZE(diff_temp); i++)
+		if (!diff_temp[i].name)
+			return diff_temp + i;
+	die("BUG: diff is failing to clean up its tempfiles");
+}
+
+static int remove_tempfile_installed;
+
+static void remove_tempfile(void)
+{
+	int i;
+	for (i = 0; i < ARRAY_SIZE(diff_temp); i++)
+		if (diff_temp[i].name == diff_temp[i].tmp_path) {
+			unlink(diff_temp[i].name);
+			diff_temp[i].name = NULL;
+		}
+}
+
+static void remove_tempfile_on_signal(int signo)
+{
+	remove_tempfile();
+	signal(SIGINT, SIG_DFL);
+	raise(signo);
+}
+
 static int count_lines(const char *data, int size)
 {
 	int count, ch, completely_empty = 1, nl_just_seen = 0;
@@ -1859,10 +1886,11 @@ static void prep_temp_blob(struct diff_tempfile *temp,
 	sprintf(temp->mode, "%06o", mode);
 }
 
-static void prepare_temp_file(const char *name,
-			      struct diff_tempfile *temp,
-			      struct diff_filespec *one)
+static struct diff_tempfile *prepare_temp_file(const char *name,
+		struct diff_filespec *one)
 {
+	struct diff_tempfile *temp = claim_diff_tempfile();
+
 	if (!DIFF_FILE_VALID(one)) {
 	not_a_valid_file:
 		/* A '-' entry produces this for file-2, and
@@ -1871,7 +1899,13 @@ static void prepare_temp_file(const char *name,
 		temp->name = "/dev/null";
 		strcpy(temp->hex, ".");
 		strcpy(temp->mode, ".");
-		return;
+		return temp;
+	}
+
+	if (!remove_tempfile_installed) {
+		atexit(remove_tempfile);
+		signal(SIGINT, remove_tempfile_on_signal);
+		remove_tempfile_installed = 1;
 	}
 
 	if (!one->sha1_valid ||
@@ -1911,7 +1945,7 @@ static void prepare_temp_file(const char *name,
 			 */
 			sprintf(temp->mode, "%06o", one->mode);
 		}
-		return;
+		return temp;
 	}
 	else {
 		if (diff_populate_filespec(one, 0))
@@ -1919,24 +1953,7 @@ static void prepare_temp_file(const char *name,
 		prep_temp_blob(temp, one->data, one->size,
 			       one->sha1, one->mode);
 	}
-}
-
-static void remove_tempfile(void)
-{
-	int i;
-
-	for (i = 0; i < 2; i++)
-		if (diff_temp[i].name == diff_temp[i].tmp_path) {
-			unlink(diff_temp[i].name);
-			diff_temp[i].name = NULL;
-		}
-}
-
-static void remove_tempfile_on_signal(int signo)
-{
-	remove_tempfile();
-	signal(SIGINT, SIG_DFL);
-	raise(signo);
+	return temp;
 }
 
 /* An external diff command takes:
@@ -1954,34 +1971,22 @@ static void run_external_diff(const char *pgm,
 			      int complete_rewrite)
 {
 	const char *spawn_arg[10];
-	struct diff_tempfile *temp = diff_temp;
 	int retval;
-	static int atexit_asked = 0;
-	const char *othername;
 	const char **arg = &spawn_arg[0];
 
-	othername = (other? other : name);
-	if (one && two) {
-		prepare_temp_file(name, &temp[0], one);
-		prepare_temp_file(othername, &temp[1], two);
-		if (! atexit_asked &&
-		    (temp[0].name == temp[0].tmp_path ||
-		     temp[1].name == temp[1].tmp_path)) {
-			atexit_asked = 1;
-			atexit(remove_tempfile);
-		}
-		signal(SIGINT, remove_tempfile_on_signal);
-	}
-
 	if (one && two) {
+		struct diff_tempfile *temp_one, *temp_two;
+		const char *othername = (other ? other : name);
+		temp_one = prepare_temp_file(name, one);
+		temp_two = prepare_temp_file(othername, two);
 		*arg++ = pgm;
 		*arg++ = name;
-		*arg++ = temp[0].name;
-		*arg++ = temp[0].hex;
-		*arg++ = temp[0].mode;
-		*arg++ = temp[1].name;
-		*arg++ = temp[1].hex;
-		*arg++ = temp[1].mode;
+		*arg++ = temp_one->name;
+		*arg++ = temp_one->hex;
+		*arg++ = temp_one->mode;
+		*arg++ = temp_two->name;
+		*arg++ = temp_two->hex;
+		*arg++ = temp_two->mode;
 		if (other) {
 			*arg++ = other;
 			*arg++ = xfrm_msg;
@@ -3450,15 +3455,15 @@ void diff_unmerge(struct diff_options *options,
 static char *run_textconv(const char *pgm, struct diff_filespec *spec,
 		size_t *outsize)
 {
-	struct diff_tempfile temp;
+	struct diff_tempfile *temp;
 	const char *argv[3];
 	const char **arg = argv;
 	struct child_process child;
 	struct strbuf buf = STRBUF_INIT;
 
-	prepare_temp_file(spec->path, &temp, spec);
+	temp = prepare_temp_file(spec->path, spec);
 	*arg++ = pgm;
-	*arg++ = temp.name;
+	*arg++ = temp->name;
 	*arg = NULL;
 
 	memset(&child, 0, sizeof(child));
@@ -3467,13 +3472,11 @@ static char *run_textconv(const char *pgm, struct diff_filespec *spec,
 	if (start_command(&child) != 0 ||
 	    strbuf_read(&buf, child.out, 0) < 0 ||
 	    finish_command(&child) != 0) {
-		if (temp.name == temp.tmp_path)
-			unlink(temp.name);
+		remove_tempfile();
 		error("error running textconv command '%s'", pgm);
 		return NULL;
 	}
-	if (temp.name == temp.tmp_path)
-		unlink(temp.name);
+	remove_tempfile();
 
 	return strbuf_detach(&buf, outsize);
 }
-- 
1.6.1.403.g6c435

[PATCH v2 3/5] chain kill signals for cleanup functions

[Jeff King]

If a piece of code wanted to do some cleanup before exiting
(e.g., cleaning up a lockfile or a tempfile), our usual
strategy was to install a signal handler that did something
like this:

  do_cleanup(); /* actual work */
  signal(signo, SIG_DFL); /* restore previous behavior */
  raise(signo); /* deliver signal, killing ourselves */

For a single handler, this works fine. However, if we want
to clean up two _different_ things, we run into a problem.
The most recently installed handler will run, but when it
removes itself as a handler, it doesn't put back the first
handler.

This patch introduces sigchain, a tiny library for handling
a stack of signal handlers. You sigchain_push each handler,
and use sigchain_pop to restore whoever was before you in
the stack.

Signed-off-by: Jeff King <peff@peff.net>
---
Two changes since last time:

 - rebased on new 2/5, which fixes a problem the original had in calling
   sigchain_push every time you did an external diff

 - I tried to handle all systems in the test script, which should
   hopefully now pass on Windows.

 .gitignore            |    1 +
 Makefile              |    3 +++
 builtin-clone.c       |    5 +++--
 builtin-fetch--tool.c |    5 +++--
 builtin-fetch.c       |    5 +++--
 diff.c                |    5 +++--
 http-push.c           |   11 ++++++-----
 lockfile.c            |   13 +++++++------
 sigchain.c            |   43 +++++++++++++++++++++++++++++++++++++++++++
 sigchain.h            |    9 +++++++++
 t/t0005-signals.sh    |   22 ++++++++++++++++++++++
 test-sigchain.c       |   22 ++++++++++++++++++++++
 12 files changed, 125 insertions(+), 19 deletions(-)
 create mode 100644 sigchain.c
 create mode 100644 sigchain.h
 create mode 100755 t/t0005-signals.sh
 create mode 100644 test-sigchain.c

diff --git a/.gitignore b/.gitignore
index d9adce5..f28a54d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -152,6 +152,7 @@ test-match-trees
 test-parse-options
 test-path-utils
 test-sha1
+test-sigchain
 common-cmds.h
 *.tar.gz
 *.dsc
diff --git a/Makefile b/Makefile
index 270b223..30371d1 100644
--- a/Makefile
+++ b/Makefile
@@ -388,6 +388,7 @@ LIB_H += revision.h
 LIB_H += run-command.h
 LIB_H += sha1-lookup.h
 LIB_H += sideband.h
+LIB_H += sigchain.h
 LIB_H += strbuf.h
 LIB_H += tag.h
 LIB_H += transport.h
@@ -481,6 +482,7 @@ LIB_OBJS += sha1-lookup.o
 LIB_OBJS += sha1_name.o
 LIB_OBJS += shallow.o
 LIB_OBJS += sideband.o
+LIB_OBJS += sigchain.o
 LIB_OBJS += strbuf.o
 LIB_OBJS += symlinks.o
 LIB_OBJS += tag.o
@@ -1365,6 +1367,7 @@ TEST_PROGRAMS += test-match-trees$X
 TEST_PROGRAMS += test-parse-options$X
 TEST_PROGRAMS += test-path-utils$X
 TEST_PROGRAMS += test-sha1$X
+TEST_PROGRAMS += test-sigchain$X
 
 all:: $(TEST_PROGRAMS)
 
diff --git a/builtin-clone.c b/builtin-clone.c
index f7e5a7b..849cefc 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -19,6 +19,7 @@
 #include "strbuf.h"
 #include "dir.h"
 #include "pack-refs.h"
+#include "sigchain.h"
 
 /*
  * Overall FIXMEs:
@@ -288,7 +289,7 @@ static void remove_junk(void)
 static void remove_junk_on_signal(int signo)
 {
 	remove_junk();
-	signal(SIGINT, SIG_DFL);
+	sigchain_pop(signo);
 	raise(signo);
 }
 
@@ -441,7 +442,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	}
 	junk_git_dir = git_dir;
 	atexit(remove_junk);
-	signal(SIGINT, remove_junk_on_signal);
+	sigchain_push(SIGINT, remove_junk_on_signal);
 
 	setenv(CONFIG_ENVIRONMENT, xstrdup(mkpath("%s/config", git_dir)), 1);
 
diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index 469b07e..b1d7f8f 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -2,6 +2,7 @@
 #include "cache.h"
 #include "refs.h"
 #include "commit.h"
+#include "sigchain.h"
 
 static char *get_stdin(void)
 {
@@ -186,7 +187,7 @@ static void remove_keep(void)
 static void remove_keep_on_signal(int signo)
 {
 	remove_keep();
-	signal(SIGINT, SIG_DFL);
+	sigchain_pop(signo);
 	raise(signo);
 }
 
@@ -245,7 +246,7 @@ static int fetch_native_store(FILE *fp,
 	char buffer[1024];
 	int err = 0;
 
-	signal(SIGINT, remove_keep_on_signal);
+	sigchain_push(SIGINT, remove_keep_on_signal);
 	atexit(remove_keep);
 
 	while (fgets(buffer, sizeof(buffer), stdin)) {
diff --git a/builtin-fetch.c b/builtin-fetch.c
index de6f307..8c86974 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -10,6 +10,7 @@
 #include "transport.h"
 #include "run-command.h"
 #include "parse-options.h"
+#include "sigchain.h"
 
 static const char * const builtin_fetch_usage[] = {
 	"git fetch [options] [<repository> <refspec>...]",
@@ -58,7 +59,7 @@ static void unlock_pack(void)
 static void unlock_pack_on_signal(int signo)
 {
 	unlock_pack();
-	signal(SIGINT, SIG_DFL);
+	sigchain_pop(signo);
 	raise(signo);
 }
 
@@ -672,7 +673,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		ref_nr = j;
 	}
 
-	signal(SIGINT, unlock_pack_on_signal);
+	sigchain_push(SIGINT, unlock_pack_on_signal);
 	atexit(unlock_pack);
 	exit_code = do_fetch(transport,
 			parse_fetch_refspec(ref_nr, refs), ref_nr);
diff --git a/diff.c b/diff.c
index ae6d552..dacd5d2 100644
--- a/diff.c
+++ b/diff.c
@@ -12,6 +12,7 @@
 #include "run-command.h"
 #include "utf8.h"
 #include "userdiff.h"
+#include "sigchain.h"
 
 #ifdef NO_FAST_WORKING_DIRECTORY
 #define FAST_WORKING_DIRECTORY 0
@@ -190,7 +191,7 @@ static void remove_tempfile(void)
 static void remove_tempfile_on_signal(int signo)
 {
 	remove_tempfile();
-	signal(SIGINT, SIG_DFL);
+	sigchain_pop(signo);
 	raise(signo);
 }
 
@@ -1904,7 +1905,7 @@ static struct diff_tempfile *prepare_temp_file(const char *name,
 
 	if (!remove_tempfile_installed) {
 		atexit(remove_tempfile);
-		signal(SIGINT, remove_tempfile_on_signal);
+		sigchain_push(SIGINT, remove_tempfile_on_signal);
 		remove_tempfile_installed = 1;
 	}
 
diff --git a/http-push.c b/http-push.c
index cb5bf95..4c92f80 100644
--- a/http-push.c
+++ b/http-push.c
@@ -10,6 +10,7 @@
 #include "exec_cmd.h"
 #include "remote.h"
 #include "list-objects.h"
+#include "sigchain.h"
 
 #include <expat.h>
 
@@ -1364,7 +1365,7 @@ static void remove_locks(void)
 static void remove_locks_on_signal(int signo)
 {
 	remove_locks();
-	signal(signo, SIG_DFL);
+	sigchain_pop(signo);
 	raise(signo);
 }
 
@@ -2266,10 +2267,10 @@ int main(int argc, char **argv)
 		goto cleanup;
 	}
 
-	signal(SIGINT, remove_locks_on_signal);
-	signal(SIGHUP, remove_locks_on_signal);
-	signal(SIGQUIT, remove_locks_on_signal);
-	signal(SIGTERM, remove_locks_on_signal);
+	sigchain_push(SIGINT, remove_locks_on_signal);
+	sigchain_push(SIGHUP, remove_locks_on_signal);
+	sigchain_push(SIGQUIT, remove_locks_on_signal);
+	sigchain_push(SIGTERM, remove_locks_on_signal);
 
 	/* Check whether the remote has server info files */
 	remote->can_update_info_refs = 0;
diff --git a/lockfile.c b/lockfile.c
index 8589155..3cd57dc 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -2,6 +2,7 @@
  * Copyright (c) 2005, Junio C Hamano
  */
 #include "cache.h"
+#include "sigchain.h"
 
 static struct lock_file *lock_file_list;
 static const char *alternate_index_output;
@@ -24,7 +25,7 @@ static void remove_lock_file(void)
 static void remove_lock_file_on_signal(int signo)
 {
 	remove_lock_file();
-	signal(signo, SIG_DFL);
+	sigchain_pop(signo);
 	raise(signo);
 }
 
@@ -136,11 +137,11 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 	lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (0 <= lk->fd) {
 		if (!lock_file_list) {
-			signal(SIGINT, remove_lock_file_on_signal);
-			signal(SIGHUP, remove_lock_file_on_signal);
-			signal(SIGTERM, remove_lock_file_on_signal);
-			signal(SIGQUIT, remove_lock_file_on_signal);
-			signal(SIGPIPE, remove_lock_file_on_signal);
+			sigchain_push(SIGINT, remove_lock_file_on_signal);
+			sigchain_push(SIGHUP, remove_lock_file_on_signal);
+			sigchain_push(SIGTERM, remove_lock_file_on_signal);
+			sigchain_push(SIGQUIT, remove_lock_file_on_signal);
+			sigchain_push(SIGPIPE, remove_lock_file_on_signal);
 			atexit(remove_lock_file);
 		}
 		lk->owner = getpid();
diff --git a/sigchain.c b/sigchain.c
new file mode 100644
index 0000000..a18d505
--- /dev/null
+++ b/sigchain.c
@@ -0,0 +1,43 @@
+#include "sigchain.h"
+#include "cache.h"
+
+#define SIGCHAIN_MAX_SIGNALS 32
+
+struct sigchain_signal {
+	sigchain_fun *old;
+	int n;
+	int alloc;
+};
+static struct sigchain_signal signals[SIGCHAIN_MAX_SIGNALS];
+
+static void check_signum(int sig)
+{
+	if (sig < 1 || sig >= SIGCHAIN_MAX_SIGNALS)
+		die("BUG: signal out of range: %d", sig);
+}
+
+int sigchain_push(int sig, sigchain_fun f)
+{
+	struct sigchain_signal *s = signals + sig;
+	check_signum(sig);
+
+	ALLOC_GROW(s->old, s->n + 1, s->alloc);
+	s->old[s->n] = signal(sig, f);
+	if (s->old[s->n] == SIG_ERR)
+		return -1;
+	s->n++;
+	return 0;
+}
+
+int sigchain_pop(int sig)
+{
+	struct sigchain_signal *s = signals + sig;
+	check_signum(sig);
+	if (s->n < 1)
+		return 0;
+
+	if (signal(sig, s->old[s->n - 1]) == SIG_ERR)
+		return -1;
+	s->n--;
+	return 0;
+}
diff --git a/sigchain.h b/sigchain.h
new file mode 100644
index 0000000..254ebb0
--- /dev/null
+++ b/sigchain.h
@@ -0,0 +1,9 @@
+#ifndef SIGCHAIN_H
+#define SIGCHAIN_H
+
+typedef void (*sigchain_fun)(int);
+
+int sigchain_push(int sig, sigchain_fun f);
+int sigchain_pop(int sig);
+
+#endif /* SIGCHAIN_H */
diff --git a/t/t0005-signals.sh b/t/t0005-signals.sh
new file mode 100755
index 0000000..9707af7
--- /dev/null
+++ b/t/t0005-signals.sh
@@ -0,0 +1,22 @@
+#!/bin/sh
+
+test_description='signals work as we expect'
+. ./test-lib.sh
+
+cat >expect <<EOF
+three
+two
+one
+EOF
+
+test_expect_success 'sigchain works' '
+	test-sigchain >actual
+	case "$?" in
+	130) true ;; # POSIX w/ SIGINT=2
+	  3) true ;; # Windows
+	  *) false ;;
+	esac &&
+	test_cmp expect actual
+'
+
+test_done
diff --git a/test-sigchain.c b/test-sigchain.c
new file mode 100644
index 0000000..8747dea
--- /dev/null
+++ b/test-sigchain.c
@@ -0,0 +1,22 @@
+#include "sigchain.h"
+#include "cache.h"
+
+#define X(f) \
+static void f(int sig) { \
+	puts(#f); \
+	fflush(stdout); \
+	sigchain_pop(sig); \
+	raise(sig); \
+}
+X(one)
+X(two)
+X(three)
+#undef X
+
+int main(int argc, char **argv) {
+	sigchain_push(SIGINT, one);
+	sigchain_push(SIGINT, two);
+	sigchain_push(SIGINT, three);
+	raise(SIGINT);
+	return 0;
+}
-- 
1.6.1.403.g6c435

[PATCH v2 4/5] refactor signal handling for cleanup functions

[Jeff King]

The current code is very inconsistent about which signals
are caught for doing cleanup of temporary files and lock
files. Some callsites checked only SIGINT, while others
checked a variety of death-dealing signals.

This patch factors out those signals to a single function,
and then calls it everywhere. For some sites, that means
this is a simple clean up. For others, it is an improvement
in that they will now properly clean themselves up after a
larger variety of signals.

Signed-off-by: Jeff King <peff@peff.net>
---
Same as before, but needed rebasing due to previous diff.c changes.

 builtin-clone.c       |    2 +-
 builtin-fetch--tool.c |    2 +-
 builtin-fetch.c       |    2 +-
 diff.c                |    2 +-
 http-push.c           |    5 +----
 lockfile.c            |    6 +-----
 sigchain.c            |    9 +++++++++
 sigchain.h            |    2 ++
 8 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 849cefc..313df6a 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -442,7 +442,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	}
 	junk_git_dir = git_dir;
 	atexit(remove_junk);
-	sigchain_push(SIGINT, remove_junk_on_signal);
+	sigchain_push_common(remove_junk_on_signal);
 
 	setenv(CONFIG_ENVIRONMENT, xstrdup(mkpath("%s/config", git_dir)), 1);
 
diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index b1d7f8f..29356d2 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -246,7 +246,7 @@ static int fetch_native_store(FILE *fp,
 	char buffer[1024];
 	int err = 0;
 
-	sigchain_push(SIGINT, remove_keep_on_signal);
+	sigchain_push_common(remove_keep_on_signal);
 	atexit(remove_keep);
 
 	while (fgets(buffer, sizeof(buffer), stdin)) {
diff --git a/builtin-fetch.c b/builtin-fetch.c
index 8c86974..1e4a3d9 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -673,7 +673,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		ref_nr = j;
 	}
 
-	sigchain_push(SIGINT, unlock_pack_on_signal);
+	sigchain_push_common(unlock_pack_on_signal);
 	atexit(unlock_pack);
 	exit_code = do_fetch(transport,
 			parse_fetch_refspec(ref_nr, refs), ref_nr);
diff --git a/diff.c b/diff.c
index dacd5d2..715709b 100644
--- a/diff.c
+++ b/diff.c
@@ -1905,7 +1905,7 @@ static struct diff_tempfile *prepare_temp_file(const char *name,
 
 	if (!remove_tempfile_installed) {
 		atexit(remove_tempfile);
-		sigchain_push(SIGINT, remove_tempfile_on_signal);
+		sigchain_push_common(remove_tempfile_on_signal);
 		remove_tempfile_installed = 1;
 	}
 
diff --git a/http-push.c b/http-push.c
index 4c92f80..178af75 100644
--- a/http-push.c
+++ b/http-push.c
@@ -2267,10 +2267,7 @@ int main(int argc, char **argv)
 		goto cleanup;
 	}
 
-	sigchain_push(SIGINT, remove_locks_on_signal);
-	sigchain_push(SIGHUP, remove_locks_on_signal);
-	sigchain_push(SIGQUIT, remove_locks_on_signal);
-	sigchain_push(SIGTERM, remove_locks_on_signal);
+	sigchain_push_common(remove_locks_on_signal);
 
 	/* Check whether the remote has server info files */
 	remote->can_update_info_refs = 0;
diff --git a/lockfile.c b/lockfile.c
index 3cd57dc..021c337 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -137,11 +137,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 	lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (0 <= lk->fd) {
 		if (!lock_file_list) {
-			sigchain_push(SIGINT, remove_lock_file_on_signal);
-			sigchain_push(SIGHUP, remove_lock_file_on_signal);
-			sigchain_push(SIGTERM, remove_lock_file_on_signal);
-			sigchain_push(SIGQUIT, remove_lock_file_on_signal);
-			sigchain_push(SIGPIPE, remove_lock_file_on_signal);
+			sigchain_push_common(remove_lock_file_on_signal);
 			atexit(remove_lock_file);
 		}
 		lk->owner = getpid();
diff --git a/sigchain.c b/sigchain.c
index a18d505..1118b99 100644
--- a/sigchain.c
+++ b/sigchain.c
@@ -41,3 +41,12 @@ int sigchain_pop(int sig)
 	s->n--;
 	return 0;
 }
+
+void sigchain_push_common(sigchain_fun f)
+{
+	sigchain_push(SIGINT, f);
+	sigchain_push(SIGHUP, f);
+	sigchain_push(SIGTERM, f);
+	sigchain_push(SIGQUIT, f);
+	sigchain_push(SIGPIPE, f);
+}
diff --git a/sigchain.h b/sigchain.h
index 254ebb0..618083b 100644
--- a/sigchain.h
+++ b/sigchain.h
@@ -6,4 +6,6 @@ typedef void (*sigchain_fun)(int);
 int sigchain_push(int sig, sigchain_fun f);
 int sigchain_pop(int sig);
 
+void sigchain_push_common(sigchain_fun f);
+
 #endif /* SIGCHAIN_H */
-- 
1.6.1.403.g6c435

[PATCH v2 5/5] pager: do wait_for_pager on signal death

[Jeff King]

Since ea27a18 (spawn pager via run_command interface), the
original git process actually does git work, and the pager
is a child process (actually, on Windows it has always been
that way, since Windows lacks fork). After spawning the
pager, we register an atexit() handler that waits for the
pager to finish.

Unfortunately, that handler does not always run. In
particular, if git is killed by a signal, then we exit
immediately. The calling shell then thinks that git is done;
however, the pager is still trying to run and impact the
terminal. The result can be seen by running a long git
process with a pager (e.g., "git log -p") and hitting ^C.
Depending on your config, you should see the shell prompt,
but pressing a key causes the pager to do any terminal
de-initialization sequence.

This patch just intercepts any death-dealing signals and
waits for the pager before dying. Under typical less
configuration, that means hitting ^C will cause git to stop
generating output, but the pager will keep running.

Signed-off-by: Jeff King <peff@peff.net>
---
Same as before.

 pager.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/pager.c b/pager.c
index f19ddbc..4921843 100644
--- a/pager.c
+++ b/pager.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "run-command.h"
+#include "sigchain.h"
 
 /*
  * This is split up from the rest of git so that we can do
@@ -38,6 +39,13 @@ static void wait_for_pager(void)
 	finish_command(&pager_process);
 }
 
+static void wait_for_pager_signal(int signo)
+{
+	wait_for_pager();
+	sigchain_pop(signo);
+	raise(signo);
+}
+
 void setup_pager(void)
 {
 	const char *pager = getenv("GIT_PAGER");
@@ -75,6 +83,7 @@ void setup_pager(void)
 	close(pager_process.in);
 
 	/* this makes sure that the parent terminates after the pager */
+	sigchain_push_common(wait_for_pager_signal);
 	atexit(wait_for_pager);
 }
 
-- 
1.6.1.403.g6c435

Re: What's cooking in git.git (Jan 2009, #05; Wed, 21)

[Junio C Hamano]

"Boyd Stephen Smith Jr." <bss@iguanasuicide.net> writes:

>>* js/diff-color-words (Tue Jan 20 21:46:57 2009 -0600) 8 commits
>> + color-words: Support diff.wordregex config option
>> + color-words: make regex configurable via attributes
>> + color-words: expand docs with precise semantics
>> + color-words: enable REG_NEWLINE to help user
>> + color-words: take an optional regular expression describing words
>> + color-words: change algorithm to allow for 0-character word
>>   boundaries
>> + color-words: refactor word splitting and use ALLOC_GROW()
>> + Add color_fwrite_lines(), a function coloring each line
>>   individually
>
> I think my patch in 
> http://thread.gmane.org/gmane.comp.version-control.git/106567 should be 
> applied to the top of this.

Thanks.

Re: What's cooking in git.git (Jan 2009, #05; Wed, 21)

[Boyd Stephen Smith Jr.]

[-- Attachment #1: Type: text/plain, Size: 951 bytes --]

On Wednesday 21 January 2009, "Boyd Stephen Smith Jr." 
<bss@iguanasuicide.net> wrote about 'Re: What's cooking in git.git (Jan 
2009, #05; Wed, 21)':
>On Wednesday 21 January 2009, Junio C Hamano <gitster@pobox.com> wrote
>about 'What's cooking in git.git (Jan 2009, #05; Wed, 21)':
>>* js/notes (Tue Jan 13 20:57:16 2009 +0100) 6 commits
>>
>>It would be nice to hear a real world success story using the notes
>>mechanism before casting this design in stone.
>
>I'll see if I can't try to put this through some paces over the week.

Yeah, that's not gonna happen.  I still want to play with this some, but 
that's being pushed to the background, so I can't say when I'll really get 
time to test it.
-- 
Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
bss@iguanasuicide.net                     ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.net/                      \_/     

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH v2 3/5] chain kill signals for cleanup functions

[Jeff King]

On Thu, Jan 22, 2009 at 01:02:35AM -0500, Jeff King wrote:

> diff --git a/test-sigchain.c b/test-sigchain.c
> new file mode 100644
> index 0000000..8747dea
> --- /dev/null
> +++ b/test-sigchain.c
> [...]
> +int main(int argc, char **argv) {
> +	sigchain_push(SIGINT, one);
> +	sigchain_push(SIGINT, two);
> +	sigchain_push(SIGINT, three);
> +	raise(SIGINT);
> +	return 0;
> +}

The signal-handling test was failing on my Solaris auto-build. After
much painful debugging, it seems that when running without a controlling
terminal (such as under cron), the signal handler for terminal related
signals (including SIGINT) is initialized to SIG_IGN. Thus after popping
all of our signal handlers, we restore the SIG_IGN, the program is _not_
killed by the signal, and the test fails.

One fix would be to just "signal(SIGINT, SIG_DFL)" at the top. But I
think it makes the test cleaner to just switch to a more reliable
signal. The patch would look something like what is below. But I need to
know what exit code Windows generates for SIGTERM. Johannes?

---
diff --git a/t/t0005-signals.sh b/t/t0005-signals.sh
index 9707af7..09f855a 100755
--- a/t/t0005-signals.sh
+++ b/t/t0005-signals.sh
@@ -12,7 +12,7 @@ EOF
 test_expect_success 'sigchain works' '
 	test-sigchain >actual
 	case "$?" in
-	130) true ;; # POSIX w/ SIGINT=2
+	143) true ;; # POSIX w/ SIGTERM=15
 	  3) true ;; # Windows
 	  *) false ;;
 	esac &&
diff --git a/test-sigchain.c b/test-sigchain.c
index 8747dea..42db234 100644
--- a/test-sigchain.c
+++ b/test-sigchain.c
@@ -14,9 +14,9 @@ X(three)
 #undef X
 
 int main(int argc, char **argv) {
-	sigchain_push(SIGINT, one);
-	sigchain_push(SIGINT, two);
-	sigchain_push(SIGINT, three);
-	raise(SIGINT);
+	sigchain_push(SIGTERM, one);
+	sigchain_push(SIGTERM, two);
+	sigchain_push(SIGTERM, three);
+	raise(SIGTERM);
 	return 0;
 }

Re: [PATCH v2 3/5] chain kill signals for cleanup functions

[Johannes Sixt]

Jeff King schrieb:
> One fix would be to just "signal(SIGINT, SIG_DFL)" at the top. But I
> think it makes the test cleaner to just switch to a more reliable
> signal. The patch would look something like what is below. But I need to
> know what exit code Windows generates for SIGTERM. Johannes?

The same as with SIGINT: 3.

-- Hannes

Re: [PATCH v2 3/5] chain kill signals for cleanup functions

[Jeff King]

On Fri, Jan 30, 2009 at 09:13:00AM +0100, Johannes Sixt wrote:

> Jeff King schrieb:
> > One fix would be to just "signal(SIGINT, SIG_DFL)" at the top. But I
> > think it makes the test cleaner to just switch to a more reliable
> > signal. The patch would look something like what is below. But I need to
> > know what exit code Windows generates for SIGTERM. Johannes?
> 
> The same as with SIGINT: 3.

Hmm. Clever.

Junio, can you apply this on top of the jk/signal-cleanup topic?

-- >8 --
Subject: [PATCH] t0005: use SIGTERM for sigchain test

The signal tests consists of checking that each of our
handlers is executed, and that the test program was killed
by the final signal. We arbitrarily used SIGINT as the kill
signal.

However, some platforms (notably Solaris) will default
SIGINT to SIG_IGN if there is no controlling terminal. In
that case, we don't end up killing the program with the
final signal and the test fails.

This is a problem since the test script should not depend
on outside factors; let's use SIGTERM instead, which should
behave consistently.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t0005-signals.sh |    2 +-
 test-sigchain.c    |    8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t0005-signals.sh b/t/t0005-signals.sh
index 9707af7..09f855a 100755
--- a/t/t0005-signals.sh
+++ b/t/t0005-signals.sh
@@ -12,7 +12,7 @@ EOF
 test_expect_success 'sigchain works' '
 	test-sigchain >actual
 	case "$?" in
-	130) true ;; # POSIX w/ SIGINT=2
+	143) true ;; # POSIX w/ SIGTERM=15
 	  3) true ;; # Windows
 	  *) false ;;
 	esac &&
diff --git a/test-sigchain.c b/test-sigchain.c
index 8747dea..42db234 100644
--- a/test-sigchain.c
+++ b/test-sigchain.c
@@ -14,9 +14,9 @@ X(three)
 #undef X
 
 int main(int argc, char **argv) {
-	sigchain_push(SIGINT, one);
-	sigchain_push(SIGINT, two);
-	sigchain_push(SIGINT, three);
-	raise(SIGINT);
+	sigchain_push(SIGTERM, one);
+	sigchain_push(SIGTERM, two);
+	sigchain_push(SIGTERM, three);
+	raise(SIGTERM);
 	return 0;
 }
-- 
1.6.1.2.420.ga6a64.dirty

Re: [PATCH v2 3/5] chain kill signals for cleanup functions

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jan 30, 2009 at 09:13:00AM +0100, Johannes Sixt wrote:
>
>> Jeff King schrieb:
>> > One fix would be to just "signal(SIGINT, SIG_DFL)" at the top. But I
>> > think it makes the test cleaner to just switch to a more reliable
>> > signal. The patch would look something like what is below. But I need to
>> > know what exit code Windows generates for SIGTERM. Johannes?
>> 
>> The same as with SIGINT: 3.
>
> Hmm. Clever.
>
> Junio, can you apply this on top of the jk/signal-cleanup topic?

Will do, but I've been sick today, haven't caught up with the list
traffic, and I do not think I'll be reading my mails for the rest of the
day either.  It may take some time for it to appear in the public
repositories.

Thanks.

Re: [PATCH v2 3/5] chain kill signals for cleanup functions

[Jeff King]

On Fri, Jan 30, 2009 at 04:28:39PM -0800, Junio C Hamano wrote:

> > Hmm. Clever.
> >
> > Junio, can you apply this on top of the jk/signal-cleanup topic?
> 
> Will do, but I've been sick today, haven't caught up with the list
> traffic, and I do not think I'll be reading my mails for the rest of the
> day either.  It may take some time for it to appear in the public
> repositories.

No problem. It really is a fix for a false negative in the test, not any
actual git bug, so no rush.

Now I'm off to go make fun of you, buried deep in a thread where you
won't see it.

-Peff

Re: What's cooking in git.git (Jan 2009, #05; Wed, 21)

[Sam Vilain]

On Thu, 2009-01-22 at 06:13 +0100, Johannes Schindelin wrote:
> > It would be nice to hear a real world success story using the notes
> > mechanism before casting this design in stone.
> 
> I'd like to have some profiling done before that.  For example, I am still 
> a bit unsure how the things would perform with a 50-deep delta chain for 
> a notes tree having 50,000+ notes in it (which I think will not be all 
> that unreasonable for a medium-sized project that stores bug-tracking 
> information in the notes).

Is there any reason why the split has to be cast in stone at all?

ie, the code could just scan the root tree of the branch, and
progressively descend into sub-trees based on a partial match of the
object for which the note is to be found.  If you find a partial name
then you expect that it is a tree and descend into it and scan for the
rest.  If you find a complete name then you expect that it is a blob and
open it.  If it turns out to be a tree then there are multiple notes for
that commit.  Then I think you get the best of both worlds; you can
start with a simple flat structure and then later someone can come along
and make it split it when there are more than N entries in the root tree
(where N is determined from profiling etc).

There are two practical applications I could use this for straight away
for perl.git, and I think that they would be important use cases.

One would be to allow grafts to be noted.  These might want to live in a
different place to refs/notes/commits, like refs/notes/grafts, to avoid
performance issues and to recognise they are a different type of data.
A second would be for commit header information - particularly the
author field and commit description - to be amended.  I think this all
belongs under refs/notes/commits.  These are in essence, historical
corrections that don't need to alter the tree.

The idea of making it allow a union merge seems relatively workable, I
think for simplicity and flexibility that the contents of the note
should be considered to be format-patch output (except without the diff
of course).  So union-ish, more like a RFC822-aware merge of mail
messages.

eg, say the contents of the note are:

  Some text

=> appends "Some text" to the note as currently implemented

  Subject: Blah blah

  Blah blah blah

=> _replaces_ commit message and body, as if it had been committed
   with the above message

  From: Sam Vilain <sam@vilain.net>
  Date: Thu, 22 Jan 2009 06:13:01 +1300

  Blah blah blah

=> replaces 'author' line in commit.  "Blah blah blah" appended to
   commit body.

Sound sane?

Sam.

Re: [PATCH v2 3/5] chain kill signals for cleanup functions

[Jeff King]

On Fri, Jan 30, 2009 at 08:44:20PM -0500, Jeff King wrote:

> > Will do, but I've been sick today, haven't caught up with the list
> > traffic, and I do not think I'll be reading my mails for the rest of the
> > day either.  It may take some time for it to appear in the public
> > repositories.
> [...]
> Now I'm off to go make fun of you, buried deep in a thread where you
> won't see it.

Hmm, reading that again, it sounds mean, and I didn't mean it that way.
I should have put a ";P" at the end. What I meant was "while the cat is
away, the mice will play" (i.e., make mischief while our benevolent
dictator is out of commission).

Hope you feel better soon.

-Peff

Re: What's cooking in git.git (Jan 2009, #05; Wed, 21)

[Jeff King]

On Sat, Jan 31, 2009 at 07:45:54PM +1300, Sam Vilain wrote:

> Is there any reason why the split has to be cast in stone at all?
> 
> ie, the code could just scan the root tree of the branch, and
> progressively descend into sub-trees based on a partial match of the
> object for which the note is to be found.  If you find a partial name
> then you expect that it is a tree and descend into it and scan for the
> rest.  If you find a complete name then you expect that it is a blob and
> open it.  If it turns out to be a tree then there are multiple notes for
> that commit.  Then I think you get the best of both worlds; you can
> start with a simple flat structure and then later someone can come along
> and make it split it when there are more than N entries in the root tree
> (where N is determined from profiling etc).

Actually, lookup is even easier than that: we iterate through the entire
tree recursively and add everything to a flat hash. So we really don't
care there what the layout is like (just take the first 40 characters of
any directory name as a hash).

But it violates the usual git principle of "content has a unique name".
What happens when I add "a/b" and you add "ab"? A dumb merge will let
both co-exist, but which one do you return for lookup?

> One would be to allow grafts to be noted.  These might want to live in a
> different place to refs/notes/commits, like refs/notes/grafts, to avoid
> performance issues and to recognise they are a different type of data.
> A second would be for commit header information - particularly the
> author field and commit description - to be amended.  I think this all
> belongs under refs/notes/commits.  These are in essence, historical
> corrections that don't need to alter the tree.

I agree that there should be multiple note hierarchies, and multiple
keys within each hierarchy. I have posted some thoughts on that before
(and you should be able to find them searching for "notes" in the list
archive), but unfortunately I have not had time to sit down and really
work out a notes implementation that matches what I posted (which I
don't think is that far from Dscho's work in next).

And I think what you are proposing (here and in the rest of your
message) is that certain notes hierarchies may have particular formats
and semantics. And that sounds reasonable to me.

-Peff

Re: [PATCH v2 3/5] chain kill signals for cleanup functions

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jan 30, 2009 at 08:44:20PM -0500, Jeff King wrote:
>
>> > Will do, but I've been sick today, haven't caught up with the list
>> > traffic, and I do not think I'll be reading my mails for the rest of the
>> > day either.  It may take some time for it to appear in the public
>> > repositories.
>> [...]
>> Now I'm off to go make fun of you, buried deep in a thread where you
>> won't see it.
>
> Hmm, reading that again, it sounds mean, and I didn't mean it that way.
> I should have put a ";P" at the end. What I meant was "while the cat is
> away, the mice will play" (i.e., make mischief while our benevolent
> dictator is out of commission).
>
> Hope you feel better soon.

Thanks.  I guess not much happened on the list while I was sick in bed.

I'll try to take a bit more rest and catch up tomorrow.

[PATCH] split notes [was: Re: What's cooking in git.git (Jan 2009, #05; Wed, 21)]

[Sam Vilain]

On Sat, 2009-01-31 at 02:36 -0500, Jeff King wrote:
> Actually, lookup is even easier than that: we iterate through the entire
> tree recursively and add everything to a flat hash. So we really don't
> care there what the layout is like (just take the first 40 characters of
> any directory name as a hash).

Sure, but if you do want to scale to a hundred thousand notes, then I
think it would pay to have a plan for making it lazy as required.  ie,
if a run just wants notes for 20 commits and there are 256 sub-trees
then only read 20 of them.  Doesn't matter if it's not implemented
initially of course, so long as the on-disk format is supported by the
tools in the first release they will be backward compatible.  And it's
not that complicated; see attached.

> But it violates the usual git principle of "content has a unique name".
> What happens when I add "a/b" and you add "ab"? A dumb merge will let
> both co-exist, but which one do you return for lookup?

It should only be tools adding to it, the trees shouldn't be modified
directly by users.  In the below patch I make these all get deleted on
'git notes edit'.

Depending on the semantics of the notes, it might not be an error to
have multiple notes for a commit.  In my patch this is "tolerated" to
some extent but not supported by git-notes.sh porcelain yet.

> I agree that there should be multiple note hierarchies, and multiple
> keys within each hierarchy. I have posted some thoughts on that before
> (and you should be able to find them searching for "notes" in the list
> archive), but unfortunately I have not had time to sit down and really
> work out a notes implementation that matches what I posted (which I
> don't think is that far from Dscho's work in next).

I had a brief look and couldn't find it, this was about the best one I
found from you in terms of links to previous discussions;
http://kerneltrap.org/mailarchive/git/2008/12/16/4427794 If there's
another thread you'd like me to read please fish it out and respond!
The more messages we have linking to the previous discussions the
better :).

> And I think what you are proposing (here and in the rest of your
> message) is that certain notes hierarchies may have particular formats
> and semantics. And that sounds reasonable to me.

Yes that was one part of it.  But also make a convention that the
'commits' notes, ie the default ones, an RFC822 message if they begin
with "known" headers.  Then porcelain such as log can inject them into
the fields at the appropriate point.

Anyway, without further ado here's the XX/XXXX split patch.

Subject: [PATCH] git-notes: allow for arbitrary split of entries into sub-trees

It might later turn out for performance reasons that a single tree for
notes will not be sufficient.  While this does not solve the
performance problem as it still loads the entire lot of notes into a
hash at start-up, it does mean that such a change does not have to
worry about backward compatibility with git versions that don't yet
support it.

Signed-off-by: Sam Vilain <sam@vilain.net>
---
 git-notes.sh     |   45 ++++++++++++++++++++++++++++++++++++++-------
 notes.c          |   39 ++++++++++++++++++++++++++++++++-------
 t/t3301-notes.sh |   11 +++++++++++
 3 files changed, 81 insertions(+), 14 deletions(-)

diff --git a/git-notes.sh b/git-notes.sh
index bfdbaa8..e07499f 100755
--- a/git-notes.sh
+++ b/git-notes.sh
@@ -10,15 +10,35 @@ ACTION="$1"; shift
 
 test -z "$GIT_NOTES_REF" && GIT_NOTES_REF="$(git config core.notesref)"
 test -z "$GIT_NOTES_REF" && GIT_NOTES_REF="refs/notes/commits"
+export GIT_NOTES_REF
 
 COMMIT=$(git rev-parse --verify --default HEAD "$@") ||
 die "Invalid commit: $@"
+NOTES_PATH=$COMMIT
+case "$GIT_NOTES_SPLIT" in
+	[1-9]|[1-4][0-9])
+		NOTES_PATH=$( echo $COMMIT | perl -pe 's{^(.{'$GIT_NOTES_SPLIT'})}{$1/}' )
+		;;
+esac
 
 MESSAGE="$GIT_DIR"/new-notes-$COMMIT
 trap '
 	test -f "$MESSAGE" && rm "$MESSAGE"
 ' 0
 
+show_note() {
+	COMMIT=$1
+	NOTE_PATH=$( git ls-tree --name-only -r $GIT_NOTES_REF | perl -nle '
+		$x = $_; s{/}{}g;
+		if (m{'$COMMIT'}) {
+			print $x;
+			exit;
+		}
+	' )
+	[ -n "$NOTE_PATH" ] &&
+		git cat-file blob $GIT_NOTES_REF:$NOTE_PATH
+}
+
 case "$ACTION" in
 edit)
 	GIT_NOTES_REF= git log -1 $COMMIT | sed "s/^/#/" > "$MESSAGE"
@@ -32,7 +52,7 @@ edit)
 	else
 		PARENT="-p $CURRENT_HEAD"
 		git read-tree "$GIT_NOTES_REF" || die "Could not read index"
-		git cat-file blob :$COMMIT >> "$MESSAGE" 2> /dev/null
+		show_note $COMMIT >> "$MESSAGE"
 	fi
 
 	${VISUAL:-${EDITOR:-vi}} "$MESSAGE"
@@ -42,15 +62,26 @@ edit)
 	if [ -s "$MESSAGE" ]; then
 		BLOB=$(git hash-object -w "$MESSAGE") ||
 			die "Could not write into object database"
-		git update-index --add --cacheinfo 0644 $BLOB $COMMIT ||
+		git update-index --add --cacheinfo 0644 $BLOB $NOTES_PATH ||
 			die "Could not write index"
 	else
-		test -z "$CURRENT_HEAD" &&
-			die "Will not initialise with empty tree"
-		git update-index --force-remove $COMMIT ||
-			die "Could not update index"
+		NOTES_PATH=dummy
 	fi
 
+	git ls-files | perl -nle '
+		$x = $_; s{/}{}g;
+		if (m{'$COMMIT'} and $x ne q{'$NOTES_PATH'}) {
+			print $x;
+		}' |
+		while read path
+			do
+				git update-index --force-remove $path ||
+			    		die "Could not update index"
+			done
+	
+	[ -z "$(git ls-files)" -a -z "$CURRENT_HEAD" ] &&
+		die "Will not initialise with empty tree"
+
 	TREE=$(git write-tree) || die "Could not write tree"
 	NEW_HEAD=$(echo Annotate $COMMIT | git commit-tree $TREE $PARENT) ||
 		die "Could not annotate"
@@ -58,7 +89,7 @@ edit)
 		"$GIT_NOTES_REF" $NEW_HEAD $CURRENT_HEAD
 ;;
 show)
-	git show "$GIT_NOTES_REF":$COMMIT
+	show_note $COMMIT
 ;;
 *)
 	usage
diff --git a/notes.c b/notes.c
index bd73784..d763b50 100644
--- a/notes.c
+++ b/notes.c
@@ -70,28 +70,53 @@ static void add_entry(const unsigned char *commit_sha1,
 	hashcpy(hash_map.entries[index].notes_sha1, notes_sha1);
 }
 
-static void initialize_hash_map(const char *notes_ref_name)
+static void initialize_hash_map_recursive(const char *tree_sha1, const char *path)
 {
 	unsigned char sha1[20], commit_sha1[20];
+	unsigned char path_combined[40];
 	unsigned mode;
 	struct tree_desc desc;
 	struct name_entry entry;
+	int length = strlen(path);
+	int length_combined;
 	void *buf;
 
-	if (!notes_ref_name || read_ref(notes_ref_name, commit_sha1) ||
-	    get_tree_entry(commit_sha1, "", sha1, &mode))
+	strcpy(path_combined, path);
+
+	if (get_tree_entry(tree_sha1, "", sha1, &mode))
 		return;
 
 	buf = fill_tree_descriptor(&desc, sha1);
 	if (!buf)
-		die("Could not read %s for notes-index", sha1_to_hex(sha1));
+		die("Could not read %s for notes-index", sha1_to_hex(tree_sha1));
+
+	while (tree_entry(&desc, &entry)) {
+		length_combined = length + strlen(entry.path);
+		if (length_combined >= 40) {
+			strncpy(path_combined + length, entry.path,
+				41 - length);
+			if (!get_sha1(path_combined, commit_sha1))
+				add_entry(commit_sha1, entry.sha1);
+		}
+		else {
+			strcpy(path_combined + length, entry.path);
+			initialize_hash_map_recursive(entry.sha1, path_combined);
+		}
+	}
 
-	while (tree_entry(&desc, &entry))
-		if (!get_sha1(entry.path, commit_sha1))
-			add_entry(commit_sha1, entry.sha1);
 	free(buf);
 }
 
+static void initialize_hash_map(const char *notes_ref_name)
+{
+	unsigned char commit_sha1[20];
+
+	if (!notes_ref_name || read_ref(notes_ref_name, commit_sha1))
+		return;
+
+	initialize_hash_map_recursive( commit_sha1, "" );
+}
+
 static unsigned char *lookup_notes(const unsigned char *commit_sha1)
 {
 	int index;
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 9393a25..3734b55 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -92,4 +92,15 @@ test_expect_success 'show multi-line notes' '
 	test_cmp expect-multiline output
 '
 
+test_expect_success 'create split notes tree' '
+	: > a4 &&
+	git add a4 &&
+	test_tick &&
+	git commit -m 4th &&
+	MSG="b4" GIT_NOTES_SPLIT=2 git notes edit &&
+	[ "$(git notes show)" = "b4" ] &&
+	[ -n "$(git ls-tree --name-only -r refs/notes/commits | grep /)" ] &&
+	[ -n "$(git log -1 | grep Notes:)" ]
+'
+
 test_done
-- 
debian.1.5.6.1

[PATCH] split notes [was: Re: What's cooking in git.git (Jan 2009, #05; Wed, 21)]

[Sam Vilain]

[re-sent with correct envelope from so hopefully it will get to the
list this time]

On Sat, 2009-01-31 at 02:36 -0500, Jeff King wrote:
> Actually, lookup is even easier than that: we iterate through the entire
> tree recursively and add everything to a flat hash. So we really don't
> care there what the layout is like (just take the first 40 characters of
> any directory name as a hash).

Sure, but if you do want to scale to a hundred thousand notes, then I
think it would pay to have a plan for making it lazy as required.  ie,
if a run just wants notes for 20 commits and there are 256 sub-trees
then only read 20 of them.  Doesn't matter if it's not implemented
initially of course, so long as the on-disk format is supported by the
tools in the first release they will be backward compatible.  And it's
not that complicated; see attached.

> But it violates the usual git principle of "content has a unique name".
> What happens when I add "a/b" and you add "ab"? A dumb merge will let
> both co-exist, but which one do you return for lookup?

It should only be tools adding to it, the trees shouldn't be modified
directly by users.  In the below patch I make these all get deleted on
'git notes edit'.

Depending on the semantics of the notes, it might not be an error to
have multiple notes for a commit.  In my patch this is "tolerated" to
some extent but not supported by git-notes.sh porcelain yet.

> I agree that there should be multiple note hierarchies, and multiple
> keys within each hierarchy. I have posted some thoughts on that before
> (and you should be able to find them searching for "notes" in the list
> archive), but unfortunately I have not had time to sit down and really
> work out a notes implementation that matches what I posted (which I
> don't think is that far from Dscho's work in next).

I had a brief look and couldn't find it, this was about the best one I
found from you in terms of links to previous discussions;
http://kerneltrap.org/mailarchive/git/2008/12/16/4427794 If there's
another thread you'd like me to read please fish it out and respond!
The more messages we have linking to the previous discussions the
better :).

> And I think what you are proposing (here and in the rest of your
> message) is that certain notes hierarchies may have particular formats
> and semantics. And that sounds reasonable to me.

Yes that was one part of it.  But also make a convention that the
'commits' notes, ie the default ones, an RFC822 message if they begin
with "known" headers.  Then porcelain such as log can inject them into
the fields at the appropriate point.

Anyway, without further ado here's the XX/XXXX split patch.

Subject: [PATCH] git-notes: allow for arbitrary split of entries into sub-trees

It might later turn out for performance reasons that a single tree for
notes will not be sufficient.  While this does not solve the
performance problem as it still loads the entire lot of notes into a
hash at start-up, it does mean that such a change does not have to
worry about backward compatibility with git versions that don't yet
support it.

Signed-off-by: Sam Vilain <sam@vilain.net>
---
 git-notes.sh     |   45 ++++++++++++++++++++++++++++++++++++++-------
 notes.c          |   39 ++++++++++++++++++++++++++++++++-------
 t/t3301-notes.sh |   11 +++++++++++
 3 files changed, 81 insertions(+), 14 deletions(-)

diff --git a/git-notes.sh b/git-notes.sh
index bfdbaa8..e07499f 100755
--- a/git-notes.sh
+++ b/git-notes.sh
@@ -10,15 +10,35 @@ ACTION="$1"; shift
 
 test -z "$GIT_NOTES_REF" && GIT_NOTES_REF="$(git config core.notesref)"
 test -z "$GIT_NOTES_REF" && GIT_NOTES_REF="refs/notes/commits"
+export GIT_NOTES_REF
 
 COMMIT=$(git rev-parse --verify --default HEAD "$@") ||
 die "Invalid commit: $@"
+NOTES_PATH=$COMMIT
+case "$GIT_NOTES_SPLIT" in
+	[1-9]|[1-4][0-9])
+		NOTES_PATH=$( echo $COMMIT | perl -pe 's{^(.{'$GIT_NOTES_SPLIT'})}{$1/}' )
+		;;
+esac
 
 MESSAGE="$GIT_DIR"/new-notes-$COMMIT
 trap '
 	test -f "$MESSAGE" && rm "$MESSAGE"
 ' 0
 
+show_note() {
+	COMMIT=$1
+	NOTE_PATH=$( git ls-tree --name-only -r $GIT_NOTES_REF | perl -nle '
+		$x = $_; s{/}{}g;
+		if (m{'$COMMIT'}) {
+			print $x;
+			exit;
+		}
+	' )
+	[ -n "$NOTE_PATH" ] &&
+		git cat-file blob $GIT_NOTES_REF:$NOTE_PATH
+}
+
 case "$ACTION" in
 edit)
 	GIT_NOTES_REF= git log -1 $COMMIT | sed "s/^/#/" > "$MESSAGE"
@@ -32,7 +52,7 @@ edit)
 	else
 		PARENT="-p $CURRENT_HEAD"
 		git read-tree "$GIT_NOTES_REF" || die "Could not read index"
-		git cat-file blob :$COMMIT >> "$MESSAGE" 2> /dev/null
+		show_note $COMMIT >> "$MESSAGE"
 	fi
 
 	${VISUAL:-${EDITOR:-vi}} "$MESSAGE"
@@ -42,15 +62,26 @@ edit)
 	if [ -s "$MESSAGE" ]; then
 		BLOB=$(git hash-object -w "$MESSAGE") ||
 			die "Could not write into object database"
-		git update-index --add --cacheinfo 0644 $BLOB $COMMIT ||
+		git update-index --add --cacheinfo 0644 $BLOB $NOTES_PATH ||
 			die "Could not write index"
 	else
-		test -z "$CURRENT_HEAD" &&
-			die "Will not initialise with empty tree"
-		git update-index --force-remove $COMMIT ||
-			die "Could not update index"
+		NOTES_PATH=dummy
 	fi
 
+	git ls-files | perl -nle '
+		$x = $_; s{/}{}g;
+		if (m{'$COMMIT'} and $x ne q{'$NOTES_PATH'}) {
+			print $x;
+		}' |
+		while read path
+			do
+				git update-index --force-remove $path ||
+			    		die "Could not update index"
+			done
+	
+	[ -z "$(git ls-files)" -a -z "$CURRENT_HEAD" ] &&
+		die "Will not initialise with empty tree"
+
 	TREE=$(git write-tree) || die "Could not write tree"
 	NEW_HEAD=$(echo Annotate $COMMIT | git commit-tree $TREE $PARENT) ||
 		die "Could not annotate"
@@ -58,7 +89,7 @@ edit)
 		"$GIT_NOTES_REF" $NEW_HEAD $CURRENT_HEAD
 ;;
 show)
-	git show "$GIT_NOTES_REF":$COMMIT
+	show_note $COMMIT
 ;;
 *)
 	usage
diff --git a/notes.c b/notes.c
index bd73784..d763b50 100644
--- a/notes.c
+++ b/notes.c
@@ -70,28 +70,53 @@ static void add_entry(const unsigned char *commit_sha1,
 	hashcpy(hash_map.entries[index].notes_sha1, notes_sha1);
 }
 
-static void initialize_hash_map(const char *notes_ref_name)
+static void initialize_hash_map_recursive(const char *tree_sha1, const char *path)
 {
 	unsigned char sha1[20], commit_sha1[20];
+	unsigned char path_combined[40];
 	unsigned mode;
 	struct tree_desc desc;
 	struct name_entry entry;
+	int length = strlen(path);
+	int length_combined;
 	void *buf;
 
-	if (!notes_ref_name || read_ref(notes_ref_name, commit_sha1) ||
-	    get_tree_entry(commit_sha1, "", sha1, &mode))
+	strcpy(path_combined, path);
+
+	if (get_tree_entry(tree_sha1, "", sha1, &mode))
 		return;
 
 	buf = fill_tree_descriptor(&desc, sha1);
 	if (!buf)
-		die("Could not read %s for notes-index", sha1_to_hex(sha1));
+		die("Could not read %s for notes-index", sha1_to_hex(tree_sha1));
+
+	while (tree_entry(&desc, &entry)) {
+		length_combined = length + strlen(entry.path);
+		if (length_combined >= 40) {
+			strncpy(path_combined + length, entry.path,
+				41 - length);
+			if (!get_sha1(path_combined, commit_sha1))
+				add_entry(commit_sha1, entry.sha1);
+		}
+		else {
+			strcpy(path_combined + length, entry.path);
+			initialize_hash_map_recursive(entry.sha1, path_combined);
+		}
+	}
 
-	while (tree_entry(&desc, &entry))
-		if (!get_sha1(entry.path, commit_sha1))
-			add_entry(commit_sha1, entry.sha1);
 	free(buf);
 }
 
+static void initialize_hash_map(const char *notes_ref_name)
+{
+	unsigned char commit_sha1[20];
+
+	if (!notes_ref_name || read_ref(notes_ref_name, commit_sha1))
+		return;
+
+	initialize_hash_map_recursive( commit_sha1, "" );
+}
+
 static unsigned char *lookup_notes(const unsigned char *commit_sha1)
 {
 	int index;
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 9393a25..3734b55 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -92,4 +92,15 @@ test_expect_success 'show multi-line notes' '
 	test_cmp expect-multiline output
 '
 
+test_expect_success 'create split notes tree' '
+	: > a4 &&
+	git add a4 &&
+	test_tick &&
+	git commit -m 4th &&
+	MSG="b4" GIT_NOTES_SPLIT=2 git notes edit &&
+	[ "$(git notes show)" = "b4" ] &&
+	[ -n "$(git ls-tree --name-only -r refs/notes/commits | grep /)" ] &&
+	[ -n "$(git log -1 | grep Notes:)" ]
+'
+
 test_done
-- 
debian.1.5.6.1

Re: [PATCH] split notes [was: Re: What's cooking in git.git (Jan 2009, #05; Wed, 21)]

[Jakub Narebski]

Sam Vilain wrote:

>  test -z "$GIT_NOTES_REF" && GIT_NOTES_REF="$(git config core.notesref)"
>  test -z "$GIT_NOTES_REF" && GIT_NOTES_REF="refs/notes/commits"

GIT_NOTES_REF=$($(GIT_NOTES_REF:-$(git config core.notesref):-refs/notes/commits)

Or something like that.
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git