[BUG] gitattribute macro expansion oddity

[BUG] gitattribute macro expansion oddity

[Jeff King]

I'm seeing some very odd behavior with git's attribute expansion for
diffs. You can see it with this repository:

  git clone git://github.com/libgit2/libgit2sharp.git

Try a diff of a non-binary file:

  $ git show 2a0f4bf7 LibGit2Sharp/Configuration.cs
  ...
  diff --git a/LibGit2Sharp/Configuration.cs b/LibGit2Sharp/Configuration.cs
  index 83cc9d6..9ab0b60 100644
  --- a/LibGit2Sharp/Configuration.cs
  +++ b/LibGit2Sharp/Configuration.cs

Looks OK. Now try a diff that also has a binary file (that is marked
such via gitattributes):

  $ git show 2a0f4bf7 Lib/NativeBinaries/x86/git2.dll \
                      LibGit2Sharp/Configuration.cs
  ...
  diff --git a/Lib/NativeBinaries/x86/git2.dll b/Lib/NativeBinaries/x86/git2.dll
  index dab0d04..8de18ab 100644
  Binary files a/Lib/NativeBinaries/x86/git2.dll and b/Lib/NativeBinaries/x86/git2.dll differ
  diff --git a/LibGit2Sharp/Configuration.cs b/LibGit2Sharp/Configuration.cs
  index 83cc9d6..9ab0b60 100644
  Binary files a/LibGit2Sharp/Configuration.cs and b/LibGit2Sharp/Configuration.cs differ

Now the Configuration.cs blobs appear binary!

It has nothing to do with pathspecs; if you do a non-limited diff of
2a0f4bf7, you'll see many of the files appear as binary. Running it
through the debugger, it looks like we are getting wrong diff attribute
values for later paths, as if the earlier lookups are somehow polluting
the attribute stack.

The gitattributes in this repository look reasonably sane, but even if
they were not, nothing should make a file have different attributes
based on other files that were diffed.

Bisection points to ec775c4 (attr: Expand macros immediately when
encountered., 2010-04-06), but it's too late for me to dig further
tonight. Cc'ing Junio as the author of the attr code and Henrik as the
author of ec775c4.

-Peff

Re: [BUG] gitattribute macro expansion oddity

[Michael Haggerty]

On 01/10/2012 08:03 AM, Jeff King wrote:
> I'm seeing some very odd behavior with git's attribute expansion for
> diffs. You can see it with this repository:
> 
>   git clone git://github.com/libgit2/libgit2sharp.git
> 
> Try a diff of a non-binary file: [...]

The problem has nothing with diffing; simply interrogating the attribute
values gives different results depending on the order of the files:

$ git check-attr --all Lib/NativeBinaries/x86/git2.dll
LibGit2Sharp/Configuration.cs
Lib/NativeBinaries/x86/git2.dll: binary: set
Lib/NativeBinaries/x86/git2.dll: diff: unset
Lib/NativeBinaries/x86/git2.dll: text: unset
LibGit2Sharp/Configuration.cs: binary: set
LibGit2Sharp/Configuration.cs: diff: unset
LibGit2Sharp/Configuration.cs: text: unset
LibGit2Sharp/Configuration.cs: crlf: set
$ git check-attr --all LibGit2Sharp/Configuration.cs
Lib/NativeBinaries/x86/git2.dll
LibGit2Sharp/Configuration.cs: diff: csharp
LibGit2Sharp/Configuration.cs: crlf: set
Lib/NativeBinaries/x86/git2.dll: binary: set
Lib/NativeBinaries/x86/git2.dll: diff: unset
Lib/NativeBinaries/x86/git2.dll: text: unset

It also doesn't depend on the fact that Lib/.gitattributes uses CRLF as
its EOL, nor does it depend on the use of the "binary" macro.  However,
it does depend on the fact that the directory name "Lib" matches the
first part of the directory name "LibGit2Sharp".  Here is a simplified
demonstration of the problem:

a=LibA/a.txt
b=Lib/b.bin
rm -rf foo
git init foo
cd foo
mkdir $(dirname $a) $(dirname $b)
touch $a $b
echo '*.txt foo' >.gitattributes
echo '* bar' >$(dirname $b)/.gitattributes
git add .
git commit -am 'Demonstrate problem'
echo '================================================='
git check-attr --all $b $a
echo '================================================='
git check-attr --all $a $b
echo '================================================='

The attributes of $a are different depending on what order $a and $b
appear in the "git check-attr" command line.

Changing the example to "a=foo/a.txt" makes the problem go away.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [BUG] gitattribute macro expansion oddity

[Jeff King]

On Tue, Jan 10, 2012 at 10:01:34AM +0100, Michael Haggerty wrote:

> On 01/10/2012 08:03 AM, Jeff King wrote:
> > I'm seeing some very odd behavior with git's attribute expansion for
> > diffs. You can see it with this repository:
> > 
> >   git clone git://github.com/libgit2/libgit2sharp.git
> > 
> > Try a diff of a non-binary file: [...]
> 
> The problem has nothing with diffing; simply interrogating the attribute
> values gives different results depending on the order of the files:
> 
> $ git check-attr --all Lib/NativeBinaries/x86/git2.dll
> LibGit2Sharp/Configuration.cs
> Lib/NativeBinaries/x86/git2.dll: binary: set
> Lib/NativeBinaries/x86/git2.dll: diff: unset
> Lib/NativeBinaries/x86/git2.dll: text: unset
> LibGit2Sharp/Configuration.cs: binary: set
> LibGit2Sharp/Configuration.cs: diff: unset
> LibGit2Sharp/Configuration.cs: text: unset
> LibGit2Sharp/Configuration.cs: crlf: set
> $ git check-attr --all LibGit2Sharp/Configuration.cs
> Lib/NativeBinaries/x86/git2.dll
> LibGit2Sharp/Configuration.cs: diff: csharp
> LibGit2Sharp/Configuration.cs: crlf: set
> Lib/NativeBinaries/x86/git2.dll: binary: set
> Lib/NativeBinaries/x86/git2.dll: diff: unset
> Lib/NativeBinaries/x86/git2.dll: text: unset

Thanks. I tried to test it with check-attr but for some reason wasn't
able to provoke the bug (I think I probably just screwed up the
invocation).

> It also doesn't depend on the fact that Lib/.gitattributes uses CRLF as
> its EOL, nor does it depend on the use of the "binary" macro.  However,
> it does depend on the fact that the directory name "Lib" matches the
> first part of the directory name "LibGit2Sharp".  Here is a simplified
> demonstration of the problem:

Ah, very helpful. That allowed me to find the problem very quickly by
grepping for "strncmp". :)

The patch below seem to fix it for me. I'll do a bit more testing before
posting it for real, though.

-Peff

diff --git a/attr.c b/attr.c
index 7467baf..f4beb62 100644
--- a/attr.c
+++ b/attr.c
@@ -528,7 +528,8 @@ static void prepare_attr_stack(const char *path, int dirlen)
 
 		elem = attr_stack;
 		if (namelen <= dirlen &&
-		    !strncmp(elem->origin, path, namelen))
+		    !strncmp(elem->origin, path, namelen) &&
+		    (!namelen || path[namelen] == '/' || path[namelen] == '\0'))
 			break;
 
 		debug_pop(elem);

[PATCH] attr: don't confuse prefixes with leading directories

[Jeff King]

When we prepare the attribute stack for a lookup on a path,
we start with the cached stack from the previous lookup
(because it is common to do several lookups in the same
directory hierarchy). So the first thing we must do in
preparing the stack is to pop any entries that point to
directories we are no longer interested in.

For example, if our stack contains gitattributes for:

  foo/bar/baz
  foo/bar
  foo

but we want to do a lookup in "foo/bar/bleep", then we want
to pop the top element, but retain the others.

To do this we walk down the stack from the top, popping
elements that do not match our lookup directory. However,
the test do this simply checked strncmp, meaning we would
mistake "foo/bar/baz" as a leading directory of
"foo/bar/baz_plus". We must also check that the character
after our match is '/', meaning we matched the whole path
component.

There are two special cases to consider:

  1. The top of our attr stack has the empty path. So we
     must not check for '/', but rather special-case the
     empty path, which always matches.

  2. Typically when matching paths in this way, you would
     also need to check for a full string match (i.e., the
     character after is '\0'). We don't need to do so in
     this case, though, because our path string is actually
     just the directory component of the path to a file
     (i.e., we know that it terminates with "/", because the
     filename comes after that).

Helped-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Jeff King <peff@peff.net>
---
I wrote it in the minimally-intrusive way, but you could also pull
the "!namelen" case before the strncmp (since a strncmp of size 0 is
always true, anyway). I don't know if it would be more obvious that way.

I prepared this on top of master, but the patch applies (with some
shifted line counts) on older releases, too.

 attr.c                |    3 ++-
 t/t0003-attributes.sh |   10 ++++++++++
 2 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/attr.c b/attr.c
index 76b079f..fa975da 100644
--- a/attr.c
+++ b/attr.c
@@ -582,7 +582,8 @@ static void prepare_attr_stack(const char *path)
 
 		elem = attr_stack;
 		if (namelen <= dirlen &&
-		    !strncmp(elem->origin, path, namelen))
+		    !strncmp(elem->origin, path, namelen) &&
+		    (!namelen || path[namelen] == '/'))
 			break;
 
 		debug_pop(elem);
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index dbb2623..51f3045 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -159,6 +159,16 @@ test_expect_success 'relative paths' '
 	(cd b && attr_check ../a/b/g a/b/g)
 '
 
+test_expect_success 'prefixes are not confused with leading directories' '
+	attr_check a_plus/g unspecified &&
+	cat >expect <<-\EOF &&
+	a/g: test: a/g
+	a_plus/g: test: unspecified
+	EOF
+	git check-attr test a/g a_plus/g >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'core.attributesfile' '
 	attr_check global unspecified &&
 	git config core.attributesfile "$HOME/global-gitattributes" &&
-- 
1.7.9.rc0.33.gd3c17

Re: [PATCH] attr: don't confuse prefixes with leading directories

[Jeff King]

On Tue, Jan 10, 2012 at 01:08:21PM -0500, Jeff King wrote:

> diff --git a/attr.c b/attr.c
> index 76b079f..fa975da 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -582,7 +582,8 @@ static void prepare_attr_stack(const char *path)
>  
>  		elem = attr_stack;
>  		if (namelen <= dirlen &&
> -		    !strncmp(elem->origin, path, namelen))
> +		    !strncmp(elem->origin, path, namelen) &&
> +		    (!namelen || path[namelen] == '/'))
>  			break;

Side note. One thing that confused me about this code is that
prepare_attr_stack does a popping loop like this:

  while (attr_stack && attr_stack->origin) {
          if (/* attr_stack->origin is a prefix */)
                  break;
          /* otherwise, pop */
          elem = attr_stack;
          attr_stack = elem->prev;
          free(elem);
  }

  /* now push our new ones */
  ...
  len = strlen(attr_stack->origin);

IOW, our loop breaks out when attr_stack is NULL, but then we go on to
assume that attr_stack is _not_ NULL. This isn't a bug, because it turns
out that we always leave something in the attr_stack: the root
gitattributes file (and the builtins).  But it is slightly confusing to
a reader because of the useless loop condition.

I'm not sure if the right solution is to change the popping loop to:

  /* we will never run out of stack, because we always have the root */
  while (attr_stack->origin) {
          ...

Or to be extra defensive and put:

  if (!attr_stack)
          die("BUG: we ran out of attr stack!?");

after the loop, or to somehow handle the case of an empty attr stack
below (which is hard to do, because it can't be triggered, so I have no
idea what it would mean).

-Peff

Re: [PATCH] attr: don't confuse prefixes with leading directories

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> IOW, our loop breaks out when attr_stack is NULL, but then we go on to
> assume that attr_stack is _not_ NULL. This isn't a bug, because it turns
> out that we always leave something in the attr_stack: the root
> gitattributes file (and the builtins).  But it is slightly confusing to
> a reader because of the useless loop condition.
> 
> I'm not sure if the right solution is to change the popping loop to:
>
>   /* we will never run out of stack, because we always have the root */
>   while (attr_stack->origin) {
>           ...

Yeah, that makes sense, as that existing check "attr_stack &&" was a
misguided defensive coding, that was _not_ defensive at all as we didn't
do anything after we stop iterating from that loop and without checking
dereferenced attr_stack->origin, which was a simple bogosity.

>
> Or to be extra defensive and put:
>
>   if (!attr_stack)
>           die("BUG: we ran out of attr stack!?");
>
> after the loop, or to somehow handle the case of an empty attr stack
> below (which is hard to do, because it can't be triggered, so I have no
> idea what it would mean).

And this is even more so.

Re: [PATCH] attr: don't confuse prefixes with leading directories

[Jeff King]

On Tue, Jan 10, 2012 at 11:23:01AM -0800, Junio C Hamano wrote:

> > I'm not sure if the right solution is to change the popping loop to:
> >
> >   /* we will never run out of stack, because we always have the root */
> >   while (attr_stack->origin) {
> >           ...
> 
> Yeah, that makes sense, as that existing check "attr_stack &&" was a
> misguided defensive coding, that was _not_ defensive at all as we didn't
> do anything after we stop iterating from that loop and without checking
> dereferenced attr_stack->origin, which was a simple bogosity.
> 
> >
> > Or to be extra defensive and put:
> >
> >   if (!attr_stack)
> >           die("BUG: we ran out of attr stack!?");
> >
> > after the loop, or to somehow handle the case of an empty attr stack
> > below (which is hard to do, because it can't be triggered, so I have no
> > idea what it would mean).
> 
> And this is even more so.

I wasn't clear: the second one is "even more so" making sense, or "even
more so" misguided defensive coding?

-Peff

Re: [PATCH] attr: don't confuse prefixes with leading directories

[Jeff King]

On Tue, Jan 10, 2012 at 02:28:10PM -0500, Jeff King wrote:

> On Tue, Jan 10, 2012 at 11:23:01AM -0800, Junio C Hamano wrote:
> 
> > > I'm not sure if the right solution is to change the popping loop to:
> > >
> > >   /* we will never run out of stack, because we always have the root */
> > >   while (attr_stack->origin) {
> > >           ...
> > 
> > Yeah, that makes sense, as that existing check "attr_stack &&" was a
> > misguided defensive coding, that was _not_ defensive at all as we didn't
> > do anything after we stop iterating from that loop and without checking
> > dereferenced attr_stack->origin, which was a simple bogosity.
> > 
> > >
> > > Or to be extra defensive and put:
> > >
> > >   if (!attr_stack)
> > >           die("BUG: we ran out of attr stack!?");
> > >
> > > after the loop, or to somehow handle the case of an empty attr stack
> > > below (which is hard to do, because it can't be triggered, so I have no
> > > idea what it would mean).
> > 
> > And this is even more so.
> 
> I wasn't clear: the second one is "even more so" making sense, or "even
> more so" misguided defensive coding?

If the latter, then I think we want this:

-- >8 --
Subject: [PATCH] attr: drop misguided defensive coding

In prepare_attr_stack, we pop the old elements of the stack
(which were left from a previous lookup and may or may not
be useful to us). Our loop to do so checks that we never
reach the top of the stack. However, the code immediately
afterwards will segfault if we did actually reach the top of
the stack.

Fortunately, this is not an actual bug, since we will never
pop all of the stack elements (we will always keep the root
gitattributes, as well as the builtin ones). So the extra
check in the loop condition simply clutters the code and
makes the intent less clear. Let's get rid of it.

Signed-off-by: Jeff King <peff@peff.net>
---
 attr.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/attr.c b/attr.c
index fa975da..cf8f2bc 100644
--- a/attr.c
+++ b/attr.c
@@ -577,7 +577,7 @@ static void prepare_attr_stack(const char *path)
 	 * Pop the ones from directories that are not the prefix of
 	 * the path we are checking.
 	 */
-	while (attr_stack && attr_stack->origin) {
+	while (attr_stack->origin) {
 		int namelen = strlen(attr_stack->origin);
 
 		elem = attr_stack;
-- 
1.7.9.rc0.33.gd3c17

Re: [PATCH] attr: don't confuse prefixes with leading directories

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Jan 10, 2012 at 11:23:01AM -0800, Junio C Hamano wrote:
>
>> > I'm not sure if the right solution is to change the popping loop to:
>> >
>> >   /* we will never run out of stack, because we always have the root */
>> >   while (attr_stack->origin) {
>> >           ...
>> 
>> Yeah, that makes sense, as that existing check "attr_stack &&" was a
>> misguided defensive coding, that was _not_ defensive at all as we didn't
>> do anything after we stop iterating from that loop and without checking
>> dereferenced attr_stack->origin, which was a simple bogosity.
>> 
>> >
>> > Or to be extra defensive and put:
>> >
>> >   if (!attr_stack)
>> >           die("BUG: we ran out of attr stack!?");
>> >
>> > after the loop, or to somehow handle the case of an empty attr stack
>> > below (which is hard to do, because it can't be triggered, so I have no
>> > idea what it would mean).
>> 
>> And this is even more so.
>
> I wasn't clear: the second one is "even more so" making sense, or "even
> more so" misguided defensive coding?

Sorry for sending a half-baked response.  The initial draft of my response
had just "that makes sense" and nothing else in the first paragraph.

If the original meant to be defensive, it should have had your "extra
defensive" die(), but it didn't.

But the condition to break out of that loop is either we hit an elem that
satisfy the condition (in which case that elem cannot be NULL) or we
successfully saw that attr_stack->origin is NULL (in which case attr_stack
couldn't have been NULL), so it is pointless to check the NULLness of
attr_stack itself. Assertion _before_ going into the while loop might make
sense, but if we look at what bootstrap_attr_stack() does, it should be
pretty obvious that that cannot happen.

An assert(attr_stack->origin) before we use it may make sense, though, in
order to make sure we do not mistakenly pop the root one and expose the
built-in ones whose origin are set to NULL.

diff --git a/attr.c b/attr.c
index ad7eb9c..4d3b61a 100644
--- a/attr.c
+++ b/attr.c
@@ -566,7 +567,9 @@ static void prepare_attr_stack(const char *path, int dirlen)
 
 	/*
 	 * Pop the ones from directories that are not the prefix of
-	 * the path we are checking.
+	 * the path we are checking. Break out of the loop when we see
+	 * the root one (whose origin is an empty string "") or the builtin
+	 * one (whose origin is NULL) without popping it.
 	 */
 	while (attr_stack->origin) {
 		int namelen = strlen(attr_stack->origin);
@@ -586,6 +589,13 @@ static void prepare_attr_stack(const char *path, int dirlen)
 	 * Read from parent directories and push them down
 	 */
 	if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
+		/*
+		 * bootstrap_attr_stack() should have added, and the
+		 * above loop should have stopped before popping, the
+		 * root element whose attr_stack->origin is set to an
+		 * empty string.
+		 */ 
+		assert(attr_stack->origin);
 		while (1) {
 			char *cp;
 

Re: [PATCH] attr: don't confuse prefixes with leading directories

[Jeff King]

On Tue, Jan 10, 2012 at 12:25:09PM -0800, Junio C Hamano wrote:

> Sorry for sending a half-baked response.  The initial draft of my response
> had just "that makes sense" and nothing else in the first paragraph.
> 
> If the original meant to be defensive, it should have had your "extra
> defensive" die(), but it didn't.
> [...]
> diff --git a/attr.c b/attr.c
> index ad7eb9c..4d3b61a 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -566,7 +567,9 @@ static void prepare_attr_stack(const char *path, int dirlen)
>  
>  	/*
>  	 * Pop the ones from directories that are not the prefix of
> -	 * the path we are checking.
> +	 * the path we are checking. Break out of the loop when we see
> +	 * the root one (whose origin is an empty string "") or the builtin
> +	 * one (whose origin is NULL) without popping it.
>  	 */
>  	while (attr_stack->origin) {
>  		int namelen = strlen(attr_stack->origin);
> @@ -586,6 +589,13 @@ static void prepare_attr_stack(const char *path, int dirlen)
>  	 * Read from parent directories and push them down
>  	 */
>  	if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
> +		/*
> +		 * bootstrap_attr_stack() should have added, and the
> +		 * above loop should have stopped before popping, the
> +		 * root element whose attr_stack->origin is set to an
> +		 * empty string.
> +		 */ 
> +		assert(attr_stack->origin);
>  		while (1) {
>  			char *cp;

Yeah, this version looks good to me (though I thought we usually spelled
assert as die("BUG: ...")).

-Peff

Re: [PATCH] attr: don't confuse prefixes with leading directories

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> diff --git a/attr.c b/attr.c
> index 76b079f..fa975da 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -582,7 +582,8 @@ static void prepare_attr_stack(const char *path)
>  
>  		elem = attr_stack;
>  		if (namelen <= dirlen &&
> -		    !strncmp(elem->origin, path, namelen))
> +		    !strncmp(elem->origin, path, namelen) &&
> +		    (!namelen || path[namelen] == '/'))
>  			break;

Thanks for the fix; I was looking at path_matches() and wondering about
the same thing.

Re: [BUG] gitattribute macro expansion oddity

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Bisection points to ec775c4 (attr: Expand macros immediately when
> encountered., 2010-04-06), but it's too late for me to dig further
> tonight. Cc'ing Junio as the author of the attr code and Henrik as the
> author of ec775c4.

Thanks for getting the ball rolling.

Regardless of this unrelated regression, after looking at what ec775c4
wanted to do again, I am very much tempted to just revert it.

It aimed to take these three

        *       ident
        foo     mybin
        bar     mybin ident

and wanted to omit 'ident' from "foo" when there is this macro definition
elsewhere:

	[attr] mybin binary -ident

But the real point of the macro was that the users do not have to know
their internals, iow, if you explicitly specify a pattern that overrides
the contents of the macro, that explicit pattern should win. When deciding
the value of "ident" attribute for path "foo", "* ident" is stronger than
"foo mybin" (the latter of which does not say anything about 'ident'
explicitly).

Re: [BUG] gitattribute macro expansion oddity

[Michael Haggerty]

On 01/10/2012 06:22 PM, Junio C Hamano wrote:
> Regardless of this unrelated regression, after looking at what ec775c4
> wanted to do again, I am very much tempted to just revert it.
> 
> It aimed to take these three
> 
>         *       ident
>         foo     mybin
>         bar     mybin ident
> 
> and wanted to omit 'ident' from "foo" when there is this macro definition
> elsewhere:
> 
> 	[attr] mybin binary -ident
> 
> But the real point of the macro was that the users do not have to know
> their internals, iow, if you explicitly specify a pattern that overrides
> the contents of the macro, that explicit pattern should win. When deciding
> the value of "ident" attribute for path "foo", "* ident" is stronger than
> "foo mybin" (the latter of which does not say anything about 'ident'
> explicitly).

I like the simplicity of the rule "apply attributes in the order found
in the .gitattributes files" better than the rule you are proposing,
which seems like it will become more complicated to explain.

For example, it would seem under your rule for the above example that
the "mybin" macro should bestow on file foo the "binary" attribute and
also the "mybin" attribute (given that macros are themselves
attributes), but not "-ident".

You would also have to decide and explain whether a macro that invokes a
macro that sets or clears attribute "foo" is "weaker" than a simple
macro that clears or sets attribute "foo".

I have one real-life use case that would become more difficult with your
rule:

# Marker for textlike files whose EOL characters haven't been
# normalized yet:
[attr]eol-fixme -text !eol

*.cc text eol=lf


# Then later, perhaps in some subdirectory's .gitattributes:
SomeParticularScrewedUpFile.cc eol-fixme


The point of the eol-fixme macro is (1) to prevent git from throwing a
tantrum and (2) to mark the file as needing cleanup sometime in the future.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/