seg fault in "git format-patch"

seg fault in "git format-patch"

[Bruce Korb]

$ git format-patch -o patches --ignore-if-in-upstream 14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d
Segmentation fault
/u/gnu/proj/gnu-pw-mgr
$ git format-patch -o patches 14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d
patches/0001-remove-dead-code.patch
patches/0002-dead-code-removal.patch
patches/0003-add-sort-pw-cfg-program.patch
patches/0004-add-doc-for-sort-pw-cfg.patch
patches/0005-clean-up-doc-makefile.patch
patches/0006-clean-up-doc-makefile.patch
patches/0007-happy-2015-and-add-delete-option.patch
patches/0008-fix-doc-Makefile.am.patch
patches/0009-re-fix-copyright.patch
patches/0010-finish-debugging-remove_pwid.patch
patches/0011-only-update-file-if-something-was-removed.patch
patches/0012-update-NEWS.patch
patches/0013-bootstrap-cleanup.patch

Re: seg fault in "git format-patch"

[Christian Couder]

On Sun, May 31, 2015 at 9:13 PM, Bruce Korb <bruce.korb@gmail.com> wrote:
> $ git format-patch -o patches --ignore-if-in-upstream
> 14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d
> Segmentation fault
> /u/gnu/proj/gnu-pw-mgr
> $ git format-patch -o patches
> 14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d
> patches/0001-remove-dead-code.patch
> patches/0002-dead-code-removal.patch
> patches/0003-add-sort-pw-cfg-program.patch
> patches/0004-add-doc-for-sort-pw-cfg.patch
> patches/0005-clean-up-doc-makefile.patch
> patches/0006-clean-up-doc-makefile.patch
> patches/0007-happy-2015-and-add-delete-option.patch
> patches/0008-fix-doc-Makefile.am.patch
> patches/0009-re-fix-copyright.patch
> patches/0010-finish-debugging-remove_pwid.patch
> patches/0011-only-update-file-if-something-was-removed.patch
> patches/0012-update-NEWS.patch
> patches/0013-bootstrap-cleanup.patch

Could you tell us which git version you are using? You can use "git --version".
The operating system you are using could also be useful.
And maybe you could also run git under gdb and give us the output of
the "bt" (backtrace) gdb command when it crashes?

Thanks,
Christian.

Re: seg fault in "git format-patch"

[Bruce Korb]

bt won't help much:

Program received signal SIGSEGV, Segmentation fault.
0x000000000047e62f in ?? ()
(gdb) bt
#0  0x000000000047e62f in ?? ()
#1  0x000000000047e6ba in ?? ()
#2  0x000000000043cb9a in ?? ()
#3  0x000000000043e9cc in ?? ()
#4  0x000000000040647d in ?? ()
#5  0x0000000000405863 in ?? ()
#6  0x00007ffff6fc3be5 in __libc_start_main () from /lib64/libc.so.6
#7  0x0000000000405cd5 in ?? ()

$ git --version
git version 1.8.4.5
$ rpm -q -a|grep  '^git'
git-email-1.8.4.5-3.8.4.x86_64
gitg-0.2.7-3.1.4.x86_64
git-cvs-1.8.4.5-3.8.4.x86_64
git-svn-1.8.4.5-3.8.4.x86_64
git-web-1.8.4.5-3.8.4.x86_64
git-gui-1.8.4.5-3.8.4.x86_64
gitg-lang-0.2.7-3.1.4.noarch
git-1.8.4.5-3.8.4.x86_64
git-core-1.8.4.5-3.8.4.x86_64
gitk-1.8.4.5-3.8.4.x86_64

$ head -n 300 /etc/*eleas*
==> /etc/SuSE-release <==
openSUSE 13.1 (x86_64)
VERSION = 13.1
CODENAME = Bottle
# /etc/SuSE-release is deprecated and will be removed in the future,
use /etc/os-release instead

==> /etc/lsb-release <==
LSB_VERSION="core-2.0-noarch:core-3.2-noarch:core-4.0-noarch:core-2.0-x86_64:core-3.2-x86_64:core-4.0-x86_64"

==> /etc/lsb-release.d <==
head: error reading '/etc/lsb-release.d': Is a directory

==> /etc/os-release <==
NAME=openSUSE
VERSION="13.1 (Bottle)"
VERSION_ID="13.1"
PRETTY_NAME="openSUSE 13.1 (Bottle) (x86_64)"
ID=opensuse
ANSI_COLOR="0;32"
CPE_NAME="cpe:/o:opensuse:opensuse:13.1"
BUG_REPORT_URL="https://bugs.opensuse.org"
HOME_URL="https://opensuse.org/"
ID_LIKE="suse"


On Sun, May 31, 2015 at 1:26 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Sun, May 31, 2015 at 9:13 PM, Bruce Korb <bruce.korb@gmail.com> wrote:
>> $ git format-patch -o patches --ignore-if-in-upstream
>> 14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d
>> Segmentation fault
>> /u/gnu/proj/gnu-pw-mgr
>> $ git format-patch -o patches
>> 14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d
>> patches/0001-remove-dead-code.patch
>> patches/0002-dead-code-removal.patch
>> patches/0003-add-sort-pw-cfg-program.patch
>> patches/0004-add-doc-for-sort-pw-cfg.patch
>> patches/0005-clean-up-doc-makefile.patch
>> patches/0006-clean-up-doc-makefile.patch
>> patches/0007-happy-2015-and-add-delete-option.patch
>> patches/0008-fix-doc-Makefile.am.patch
>> patches/0009-re-fix-copyright.patch
>> patches/0010-finish-debugging-remove_pwid.patch
>> patches/0011-only-update-file-if-something-was-removed.patch
>> patches/0012-update-NEWS.patch
>> patches/0013-bootstrap-cleanup.patch
>
> Could you tell us which git version you are using? You can use "git --version".
> The operating system you are using could also be useful.
> And maybe you could also run git under gdb and give us the output of
> the "bt" (backtrace) gdb command when it crashes?
>
> Thanks,
> Christian.

Re: seg fault in "git format-patch"

[Bruce Korb]

Oh, you can also clone the gnu-pw-mgr and likely get the same result:

$ cat .git/config
[core]
        repositoryformatversion = 0
        filemode = true
        bare = false
        logallrefupdates = true
[remote "origin"]
        fetch = +refs/heads/*:refs/remotes/origin/*
        url = ssh://git.sv.gnu.org/srv/git/gnu-pw-mgr.git
[branch "master"]
        remote = origin
        merge = refs/heads/master

On Sun, May 31, 2015 at 1:41 PM, Bruce Korb <bruce.korb@gmail.com> wrote:
> bt won't help much:
>
> Program received signal SIGSEGV, Segmentation fault.
> 0x000000000047e62f in ?? ()
> (gdb) bt
> #0  0x000000000047e62f in ?? ()
> #1  0x000000000047e6ba in ?? ()
> #2  0x000000000043cb9a in ?? ()
> #3  0x000000000043e9cc in ?? ()
> #4  0x000000000040647d in ?? ()
> #5  0x0000000000405863 in ?? ()
> #6  0x00007ffff6fc3be5 in __libc_start_main () from /lib64/libc.so.6
> #7  0x0000000000405cd5 in ?? ()
>
> $ git --version
> git version 1.8.4.5
> $ rpm -q -a|grep  '^git'
> git-email-1.8.4.5-3.8.4.x86_64
> gitg-0.2.7-3.1.4.x86_64
> git-cvs-1.8.4.5-3.8.4.x86_64
> git-svn-1.8.4.5-3.8.4.x86_64
> git-web-1.8.4.5-3.8.4.x86_64
> git-gui-1.8.4.5-3.8.4.x86_64
> gitg-lang-0.2.7-3.1.4.noarch
> git-1.8.4.5-3.8.4.x86_64
> git-core-1.8.4.5-3.8.4.x86_64
> gitk-1.8.4.5-3.8.4.x86_64
>
> $ head -n 300 /etc/*eleas*
> ==> /etc/SuSE-release <==
> openSUSE 13.1 (x86_64)
> VERSION = 13.1
> CODENAME = Bottle
> # /etc/SuSE-release is deprecated and will be removed in the future,
> use /etc/os-release instead
>
> ==> /etc/lsb-release <==
> LSB_VERSION="core-2.0-noarch:core-3.2-noarch:core-4.0-noarch:core-2.0-x86_64:core-3.2-x86_64:core-4.0-x86_64"
>
> ==> /etc/lsb-release.d <==
> head: error reading '/etc/lsb-release.d': Is a directory
>
> ==> /etc/os-release <==
> NAME=openSUSE
> VERSION="13.1 (Bottle)"
> VERSION_ID="13.1"
> PRETTY_NAME="openSUSE 13.1 (Bottle) (x86_64)"
> ID=opensuse
> ANSI_COLOR="0;32"
> CPE_NAME="cpe:/o:opensuse:opensuse:13.1"
> BUG_REPORT_URL="https://bugs.opensuse.org"
> HOME_URL="https://opensuse.org/"
> ID_LIKE="suse"
>
>
> On Sun, May 31, 2015 at 1:26 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> On Sun, May 31, 2015 at 9:13 PM, Bruce Korb <bruce.korb@gmail.com> wrote:
>>> $ git format-patch -o patches --ignore-if-in-upstream
>>> 14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d
>>> Segmentation fault
>>> /u/gnu/proj/gnu-pw-mgr
>>> $ git format-patch -o patches
>>> 14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d
>>> patches/0001-remove-dead-code.patch
>>> patches/0002-dead-code-removal.patch
>>> patches/0003-add-sort-pw-cfg-program.patch
>>> patches/0004-add-doc-for-sort-pw-cfg.patch
>>> patches/0005-clean-up-doc-makefile.patch
>>> patches/0006-clean-up-doc-makefile.patch
>>> patches/0007-happy-2015-and-add-delete-option.patch
>>> patches/0008-fix-doc-Makefile.am.patch
>>> patches/0009-re-fix-copyright.patch
>>> patches/0010-finish-debugging-remove_pwid.patch
>>> patches/0011-only-update-file-if-something-was-removed.patch
>>> patches/0012-update-NEWS.patch
>>> patches/0013-bootstrap-cleanup.patch
>>
>> Could you tell us which git version you are using? You can use "git --version".
>> The operating system you are using could also be useful.
>> And maybe you could also run git under gdb and give us the output of
>> the "bt" (backtrace) gdb command when it crashes?
>>
>> Thanks,
>> Christian.

Re: seg fault in "git format-patch"

[Christian Couder]

On Sun, May 31, 2015 at 10:45 PM, Bruce Korb <bruce.korb@gmail.com> wrote:
> Oh, you can also clone the gnu-pw-mgr and likely get the same result:

Yeah, after cloning from http://git.savannah.gnu.org/r/gnu-pw-mgr.git
I get the following backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x00000000004b26b1 in clear_commit_marks_1 (plist=0x7fffffffbf78,
commit=0x84e8d0, mark=139) at commit.c:528
528                     while ((parents = parents->next))
(gdb) bt
#0  0x00000000004b26b1 in clear_commit_marks_1 (plist=0x7fffffffbf78,
commit=0x84e8d0, mark=139) at commit.c:528
#1  0x00000000004b2743 in clear_commit_marks_many (nr=-1,
commit=0x7fffffffbfa0, mark=139) at commit.c:544
#2  0x00000000004b2771 in clear_commit_marks (commit=0x84e8d0,
mark=139) at commit.c:549
#3  0x00000000004537cc in get_patch_ids (rev=0x7fffffffd190,
ids=0x7fffffffc910) at builtin/log.c:832
#4  0x0000000000455580 in cmd_format_patch (argc=1,
argv=0x7fffffffdc20, prefix=0x0) at builtin/log.c:1425
#5  0x0000000000405807 in run_builtin (p=0x80cac8 <commands+840>,
argc=5, argv=0x7fffffffdc20) at git.c:350
#6  0x0000000000405a15 in handle_builtin (argc=5, argv=0x7fffffffdc20)
at git.c:532
#7  0x0000000000405b31 in run_argv (argcp=0x7fffffffdafc,
argv=0x7fffffffdb10) at git.c:578
#8  0x0000000000405d29 in main (argc=5, av=0x7fffffffdc18) at git.c:686

(Please don't top post if you reply to this email as it is frown upon
on this list.)

Re: seg fault in "git format-patch"

[Christian Couder]

On Mon, Jun 1, 2015 at 1:14 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Sun, May 31, 2015 at 10:45 PM, Bruce Korb <bruce.korb@gmail.com> wrote:
>> Oh, you can also clone the gnu-pw-mgr and likely get the same result:
>
> Yeah, after cloning from http://git.savannah.gnu.org/r/gnu-pw-mgr.git
> I get the following backtrace:
>
> Program received signal SIGSEGV, Segmentation fault.
> 0x00000000004b26b1 in clear_commit_marks_1 (plist=0x7fffffffbf78,
> commit=0x84e8d0, mark=139) at commit.c:528
> 528                     while ((parents = parents->next))
> (gdb) bt
> #0  0x00000000004b26b1 in clear_commit_marks_1 (plist=0x7fffffffbf78,
> commit=0x84e8d0, mark=139) at commit.c:528
> #1  0x00000000004b2743 in clear_commit_marks_many (nr=-1,
> commit=0x7fffffffbfa0, mark=139) at commit.c:544
> #2  0x00000000004b2771 in clear_commit_marks (commit=0x84e8d0,
> mark=139) at commit.c:549
> #3  0x00000000004537cc in get_patch_ids (rev=0x7fffffffd190,
> ids=0x7fffffffc910) at builtin/log.c:832
> #4  0x0000000000455580 in cmd_format_patch (argc=1,
> argv=0x7fffffffdc20, prefix=0x0) at builtin/log.c:1425
> #5  0x0000000000405807 in run_builtin (p=0x80cac8 <commands+840>,
> argc=5, argv=0x7fffffffdc20) at git.c:350
> #6  0x0000000000405a15 in handle_builtin (argc=5, argv=0x7fffffffdc20)
> at git.c:532
> #7  0x0000000000405b31 in run_argv (argcp=0x7fffffffdafc,
> argv=0x7fffffffdb10) at git.c:578
> #8  0x0000000000405d29 in main (argc=5, av=0x7fffffffdc18) at git.c:686
>
> (Please don't top post if you reply to this email as it is frown upon
> on this list.)

When running the command that gives the above segfault:

$ git format-patch -o patches --ignore-if-in-upstream
14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d

It is interesting to note that the last sha1 refers to a tag:

$ git cat-file tag 2de9eef391259dfc8748dbaf76a5d55427f37b0d
object 524ccbdbe319068ab18a3950119b9e9a5d135783
type commit
tag v1.4
tagger Bruce Korb <bkorb@gnu.org> 1428847577 -0700

Release 1.4

* sort-pw-cfg: a sort/merge program for combining and organizing
  configurations.

* --delete: a new option to remove any entries for a password id

It works when the tag is replaced by the commit it points to, and the
segfault happens because the we try to access the "parents" field of
the tag object as if it was a commit.

Re: seg fault in "git format-patch"

[Christian Couder]

On Mon, Jun 1, 2015 at 1:53 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Mon, Jun 1, 2015 at 1:14 AM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> On Sun, May 31, 2015 at 10:45 PM, Bruce Korb <bruce.korb@gmail.com> wrote:
>>> Oh, you can also clone the gnu-pw-mgr and likely get the same result:
>>
>> Yeah, after cloning from http://git.savannah.gnu.org/r/gnu-pw-mgr.git
>> I get the following backtrace:
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x00000000004b26b1 in clear_commit_marks_1 (plist=0x7fffffffbf78,
>> commit=0x84e8d0, mark=139) at commit.c:528
>> 528                     while ((parents = parents->next))
>> (gdb) bt
>> #0  0x00000000004b26b1 in clear_commit_marks_1 (plist=0x7fffffffbf78,
>> commit=0x84e8d0, mark=139) at commit.c:528
>> #1  0x00000000004b2743 in clear_commit_marks_many (nr=-1,
>> commit=0x7fffffffbfa0, mark=139) at commit.c:544
>> #2  0x00000000004b2771 in clear_commit_marks (commit=0x84e8d0,
>> mark=139) at commit.c:549
>> #3  0x00000000004537cc in get_patch_ids (rev=0x7fffffffd190,
>> ids=0x7fffffffc910) at builtin/log.c:832
>> #4  0x0000000000455580 in cmd_format_patch (argc=1,
>> argv=0x7fffffffdc20, prefix=0x0) at builtin/log.c:1425
>> #5  0x0000000000405807 in run_builtin (p=0x80cac8 <commands+840>,
>> argc=5, argv=0x7fffffffdc20) at git.c:350
>> #6  0x0000000000405a15 in handle_builtin (argc=5, argv=0x7fffffffdc20)
>> at git.c:532
>> #7  0x0000000000405b31 in run_argv (argcp=0x7fffffffdafc,
>> argv=0x7fffffffdb10) at git.c:578
>> #8  0x0000000000405d29 in main (argc=5, av=0x7fffffffdc18) at git.c:686
>>
>> (Please don't top post if you reply to this email as it is frown upon
>> on this list.)
>
> When running the command that gives the above segfault:
>
> $ git format-patch -o patches --ignore-if-in-upstream
> 14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d
>
> It is interesting to note that the last sha1 refers to a tag:
>
> $ git cat-file tag 2de9eef391259dfc8748dbaf76a5d55427f37b0d
> object 524ccbdbe319068ab18a3950119b9e9a5d135783
> type commit
> tag v1.4
> tagger Bruce Korb <bkorb@gnu.org> 1428847577 -0700
>
> Release 1.4
>
> * sort-pw-cfg: a sort/merge program for combining and organizing
>   configurations.
>
> * --delete: a new option to remove any entries for a password id
>
> It works when the tag is replaced by the commit it points to, and the
> segfault happens because the we try to access the "parents" field of
> the tag object as if it was a commit.

Yeah, in builtin/log.c we are doing:

    o2 = rev->pending.objects[1].item;

and then we are casting the object into a commit when passing it to
clear_commit_marks():

    clear_commit_marks((struct commit *)o2,
            SEEN | UNINTERESTING | SHOWN | ADDED);

but I don't know where we should have peeled the tag to get a commit,
and it's late here so I will leave it someone else to find a fix.

Best,
Christian.

[PATCH] format-patch: dereference tags with --ignore-if-in-upstream

[brian m. carlson]

format-patch would segfault if provided a tag as one of the range
endpoints in conjunction with --ignore-if-in-upstream, as it assumed the
object was a commit and attempted to cast it to struct commit.
Dereference the tag as soon as possible to prevent this, but not until
after copying the necessary flags.

Reported-by: Bruce Korb <bruce.korb@gmail.com>
Diagnosed-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/log.c           |  6 ++++++
 t/t4014-format-patch.sh | 10 ++++++++++
 2 files changed, 16 insertions(+)

diff --git a/builtin/log.c b/builtin/log.c
index dd8f3fc..e0465ba 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -807,6 +807,12 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)
 	o2 = rev->pending.objects[1].item;
 	flags2 = o2->flags;
 
+	o1 = deref_tag(o1, NULL, 0);
+	o2 = deref_tag(o2, NULL, 0);
+
+	if (!o1 || !o2)
+		die(_("Invalid tag."));
+
 	if ((flags1 & UNINTERESTING) == (flags2 & UNINTERESTING))
 		die(_("Not a range."));
 
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index c39e500..60b9875 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -57,6 +57,16 @@ test_expect_success "format-patch --ignore-if-in-upstream" '
 
 '
 
+test_expect_success "format-patch --ignore-if-in-upstream handles tags" '
+
+	git tag -a v1 -m tag side &&
+	git format-patch --stdout \
+		--ignore-if-in-upstream master..v1 >patch1 &&
+	cnt=$(grep "^From " patch1 | wc -l) &&
+	test $cnt = 2
+
+'
+
 test_expect_success "format-patch doesn't consider merge commits" '
 
 	git checkout -b slave master &&
-- 
2.4.0

Re: [PATCH] format-patch: dereference tags with --ignore-if-in-upstream

[Jeff King]

On Mon, Jun 01, 2015 at 01:03:13AM +0000, brian m. carlson wrote:

> format-patch would segfault if provided a tag as one of the range
> endpoints in conjunction with --ignore-if-in-upstream, as it assumed the
> object was a commit and attempted to cast it to struct commit.
> Dereference the tag as soon as possible to prevent this, but not until
> after copying the necessary flags.

I bisected Bruce's case earlier to 895c5ba (revision: do not peel tags
used in range notation, 2013-09-19). This is an obvious fallout from
that commit; unlike most traversals which read from rev->commits, we
read straight from rev->pending here. So I wondered briefly if that
commit was not being sufficiently careful.

But as it turns out, this code was buggy long before then. 895c5ba only
changed the range notation. Even before then, if you did:

  git format-patch --ignore-if-in-upstream ^v2.2.0 v2.2.1

we would segfault. Anybody reading from rev->pending should be ready to
handle any kind of object.

Which also makes me wonder about...

> diff --git a/builtin/log.c b/builtin/log.c
> index dd8f3fc..e0465ba 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -807,6 +807,12 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)
>  	o2 = rev->pending.objects[1].item;
>  	flags2 = o2->flags;
>  
> +	o1 = deref_tag(o1, NULL, 0);
> +	o2 = deref_tag(o2, NULL, 0);
> +
> +	if (!o1 || !o2)
> +		die(_("Invalid tag."));

This will dereference tags, but it won't help at all with:

  git format-patch --ignore-if-in-upstream ^HEAD:Makefile HEAD:Documentation

where we end up with blobs. That is ridiculous, of course, but we should
complain, not segfault.

So I think what you really want is lookup_commit_reference. And the
error message is really not "invalid tag", but "not a commit". I think
you can just use lookup_commit_or_die.

>  	if ((flags1 & UNINTERESTING) == (flags2 & UNINTERESTING))
>  		die(_("Not a range."));

As an aside, now that we are dereferencing, these flags are from the
wrong object. They _should_ be the same (we mark the tag as
UNINTERESTING, too), but it's a little weird that at the end of the
function we restore the saved flags from the tag object onto the commit.
Just bumping the assignment of flags{1,2} would work (or just bump up
the lookup_commit_or_die call to where we assign to o{1,2}).

> +test_expect_success "format-patch --ignore-if-in-upstream handles tags" '
> +
> +	git tag -a v1 -m tag side &&
> +	git format-patch --stdout \
> +		--ignore-if-in-upstream master..v1 >patch1 &&
> +	cnt=$(grep "^From " patch1 | wc -l) &&
> +	test $cnt = 2

I think this avoids the usual "wc" whitespace pitfall because you don't
use double-quotes. But maybe:

  grep "^From " patch1 >count &&
  test_line_count = 2 patch1

would be more idiomatic.

-Peff

Re: [PATCH] format-patch: dereference tags with --ignore-if-in-upstream

[brian m. carlson]

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

On Mon, Jun 01, 2015 at 06:20:46AM -0400, Jeff King wrote:
> So I think what you really want is lookup_commit_reference. And the
> error message is really not "invalid tag", but "not a commit". I think
> you can just use lookup_commit_or_die.

Thanks.  That does seem to be what I want.

> As an aside, now that we are dereferencing, these flags are from the
> wrong object. They _should_ be the same (we mark the tag as
> UNINTERESTING, too), but it's a little weird that at the end of the
> function we restore the saved flags from the tag object onto the commit.
> Just bumping the assignment of flags{1,2} would work (or just bump up
> the lookup_commit_or_die call to where we assign to o{1,2}).

I tried looking up the flags after dereferencing the tags, but that led
to the die("Not a range.") being triggered.  That's why the commit
message ended up mentioning loading the flags before dereferencing.

> I think this avoids the usual "wc" whitespace pitfall because you don't
> use double-quotes. But maybe:
> 
>   grep "^From " patch1 >count &&
>   test_line_count = 2 patch1
> 
> would be more idiomatic.

I can certainly make that change.  I made the test as similar as
possible to other tests in the area, but I wasn't aware of
test_line_count.

I'll reroll the patch later today.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] format-patch: dereference tags with --ignore-if-in-upstream

[Jeff King]

On Mon, Jun 01, 2015 at 11:22:12AM +0000, brian m. carlson wrote:

> > As an aside, now that we are dereferencing, these flags are from the
> > wrong object. They _should_ be the same (we mark the tag as
> > UNINTERESTING, too), but it's a little weird that at the end of the
> > function we restore the saved flags from the tag object onto the commit.
> > Just bumping the assignment of flags{1,2} would work (or just bump up
> > the lookup_commit_or_die call to where we assign to o{1,2}).
> 
> I tried looking up the flags after dereferencing the tags, but that led
> to the die("Not a range.") being triggered.  That's why the commit
> message ended up mentioning loading the flags before dereferencing.

Oh, sorry, I somehow totally missed that mention in the commit message.

It seems doubly wrong then to pull the flags from the tag and then later
apply them to the commit at the end. And in fact, if you do not have the
UNINTERESTING flag on your commit here, that is a real problem. If we
make your test:

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 60b9875..37bf70a 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -60,8 +60,9 @@ test_expect_success "format-patch --ignore-if-in-upstream" '
 test_expect_success "format-patch --ignore-if-in-upstream handles tags" '
 
 	git tag -a v1 -m tag side &&
+	git tag -a v2 -m tag master &&
 	git format-patch --stdout \
-		--ignore-if-in-upstream master..v1 >patch1 &&
+		--ignore-if-in-upstream v2..v1 >patch1 &&
 	cnt=$(grep "^From " patch1 | wc -l) &&
 	test $cnt = 2
 

then it fails (the key is having the tag on the left-hand side, because
that is where we need the UNINTERESTING flag to be).

Normally this flag is propagated to the dereferenced commit as part of
prepare_revision_walk, but we are looking at the flags before that gets
called. So you'll have to either propagate it manually here, or just
feed the original tags to the sub-traversal. I think the latter is
probably simpler. Something like:

  1. Check the flags on the original objects (o1 and o2).

  2. Peel them to commits; complain if they're not both commits. Store
     the result in another variable (e.g., commit1, commit2).

  3. Feed o1 and o2 to the new check_rev traversal.

  4. Clear the commit flags off of commit1 and commit2.

  5. Restore the original flags to o1 and o2.

Yeesh. I would have thought that we could just do this as part of the
normal traversal by using "--cherry-pick" (I think format-patch predates
that option). We have to have a symmetric range to do that, but I wonder
if we could simulate it by converting "foo..bar" into "--cherry-pick
--right-only foo...bar".

I guess that is basically "--cherry", but we still have to massage
"foo..bar" into "foo...bar". I think that is basically just:

   o1 ^= ~UNINTERESTING;
   o1 |= SYMMETRIC_LEFT;

but there might be a hidden catch I am not considering.

> > I think this avoids the usual "wc" whitespace pitfall because you don't
> > use double-quotes. But maybe:
> > 
> >   grep "^From " patch1 >count &&
> >   test_line_count = 2 patch1
> > 
> > would be more idiomatic.
> 
> I can certainly make that change.  I made the test as similar as
> possible to other tests in the area, but I wasn't aware of
> test_line_count.

Ah, I just looked at the context in your patch, not at the whole test. I
don't mind matching the surrounding code. But I also don't mind a
preparatory modernization patch to the test script. :)

-Peff

Re: seg fault in "git format-patch"

[Christian Couder]

On Mon, Jun 1, 2015 at 2:01 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Mon, Jun 1, 2015 at 1:53 AM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> On Mon, Jun 1, 2015 at 1:14 AM, Christian Couder
>> <christian.couder@gmail.com> wrote:
>>> On Sun, May 31, 2015 at 10:45 PM, Bruce Korb <bruce.korb@gmail.com> wrote:
>>>> Oh, you can also clone the gnu-pw-mgr and likely get the same result:
>>>
>>> Yeah, after cloning from http://git.savannah.gnu.org/r/gnu-pw-mgr.git
>>> I get the following backtrace:
>>>
>>> Program received signal SIGSEGV, Segmentation fault.
>>> 0x00000000004b26b1 in clear_commit_marks_1 (plist=0x7fffffffbf78,
>>> commit=0x84e8d0, mark=139) at commit.c:528
>>> 528                     while ((parents = parents->next))
>>> (gdb) bt
>>> #0  0x00000000004b26b1 in clear_commit_marks_1 (plist=0x7fffffffbf78,
>>> commit=0x84e8d0, mark=139) at commit.c:528
>>> #1  0x00000000004b2743 in clear_commit_marks_many (nr=-1,
>>> commit=0x7fffffffbfa0, mark=139) at commit.c:544
>>> #2  0x00000000004b2771 in clear_commit_marks (commit=0x84e8d0,
>>> mark=139) at commit.c:549
>>> #3  0x00000000004537cc in get_patch_ids (rev=0x7fffffffd190,
>>> ids=0x7fffffffc910) at builtin/log.c:832
>>> #4  0x0000000000455580 in cmd_format_patch (argc=1,
>>> argv=0x7fffffffdc20, prefix=0x0) at builtin/log.c:1425
>>> #5  0x0000000000405807 in run_builtin (p=0x80cac8 <commands+840>,
>>> argc=5, argv=0x7fffffffdc20) at git.c:350
>>> #6  0x0000000000405a15 in handle_builtin (argc=5, argv=0x7fffffffdc20)
>>> at git.c:532
>>> #7  0x0000000000405b31 in run_argv (argcp=0x7fffffffdafc,
>>> argv=0x7fffffffdb10) at git.c:578
>>> #8  0x0000000000405d29 in main (argc=5, av=0x7fffffffdc18) at git.c:686
>>>
>>> (Please don't top post if you reply to this email as it is frown upon
>>> on this list.)
>>
>> When running the command that gives the above segfault:
>>
>> $ git format-patch -o patches --ignore-if-in-upstream
>> 14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d
>>
>> It is interesting to note that the last sha1 refers to a tag:
>>
>> $ git cat-file tag 2de9eef391259dfc8748dbaf76a5d55427f37b0d
>> object 524ccbdbe319068ab18a3950119b9e9a5d135783
>> type commit
>> tag v1.4
>> tagger Bruce Korb <bkorb@gnu.org> 1428847577 -0700
>>
>> Release 1.4
>>
>> * sort-pw-cfg: a sort/merge program for combining and organizing
>>   configurations.
>>
>> * --delete: a new option to remove any entries for a password id
>>
>> It works when the tag is replaced by the commit it points to, and the
>> segfault happens because the we try to access the "parents" field of
>> the tag object as if it was a commit.
>
> Yeah, in builtin/log.c we are doing:
>
>     o2 = rev->pending.objects[1].item;
>
> and then we are casting the object into a commit when passing it to
> clear_commit_marks():
>
>     clear_commit_marks((struct commit *)o2,
>             SEEN | UNINTERESTING | SHOWN | ADDED);
>
> but I don't know where we should have peeled the tag to get a commit,
> and it's late here so I will leave it someone else to find a fix.

The following seems to fix it, but I am not sure it is the right fix:

diff --git a/builtin/log.c b/builtin/log.c
index dd8f3fc..0ab9360 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -792,6 +792,16 @@ static int reopen_stdout(struct commit *commit,
const char *subject,
        return 0;
 }

+static void clear_object_marks(struct object *obj)
+{
+       struct commit *c = (struct commit *)peel_to_type(NULL, 0, obj,
+                                                        OBJ_COMMIT);
+       if (!c)
+               die(_("could not convert %s into a commit"),
+                   sha1_to_hex(obj->sha1));
+       clear_commit_marks(c, SEEN | UNINTERESTING | SHOWN | ADDED);
+}
+
 static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)
 {
        struct rev_info check_rev;
@@ -827,10 +837,8 @@ static void get_patch_ids(struct rev_info *rev,
struct patch_ids *ids)
        }

        /* reset for next revision walk */
-       clear_commit_marks((struct commit *)o1,
-                       SEEN | UNINTERESTING | SHOWN | ADDED);
-       clear_commit_marks((struct commit *)o2,
-                       SEEN | UNINTERESTING | SHOWN | ADDED);
+       clear_object_marks(o1);
+       clear_object_marks(o2);
        o1->flags = flags1;
        o2->flags = flags2;
 }

Re: seg fault in "git format-patch"

[Christian Couder]

On Mon, Jun 1, 2015 at 3:44 PM, Christian Couder
<christian.couder@gmail.com> wrote:
>
> The following seems to fix it, but I am not sure it is the right fix:

Ooops, I had not seen that Brian and Peff are already discussing a fix
in this thread:

http://thread.gmane.org/gmane.comp.version-control.git/270371

Re: seg fault in "git format-patch"

[Bruce Korb]

On Sun, May 31, 2015 at 4:53 PM, Christian Couder
<christian.couder@gmail.com> wrote:
>> (Please don't top post if you reply to this email as it is frown upon
>> on this list.)

WRT "top posting", two points:

1. Too many sites/lists now *require* top posting
2. MUA's (like Google Mail) hide the old mail as an obscure squiggle
at the bottom of the page.

Time to just get over it and accept the fact that there are two
conventions.  I did.  I only gave up a decade ago.

> $ git format-patch -o patches --ignore-if-in-upstream
> 14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d
>
> It is interesting to note that the last sha1 refers to a tag:

They both do.  I tried "git format-patch v1.3..v1.4" but that didn't work
and I knew that SHA1..SHA2 would, so I got the SHA's for the tags.

> It works when the tag is replaced by the commit it points to, and the
> segfault happens because the we try to access the "parents" field of
> the tag object as if it was a commit.

The other discussion started *after* this one, but thanks for the pointer.

Re: [PATCH] format-patch: dereference tags with --ignore-if-in-upstream

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> diff --git a/builtin/log.c b/builtin/log.c
> index dd8f3fc..e0465ba 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -807,6 +807,12 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)
>  	o2 = rev->pending.objects[1].item;
>  	flags2 = o2->flags;
>  
> +	o1 = deref_tag(o1, NULL, 0);
> +	o2 = deref_tag(o2, NULL, 0);
> +
> +	if (!o1 || !o2)
> +		die(_("Invalid tag."));

Shouldn't you ensure o1 and o2 are commits here?

Re: [PATCH] format-patch: dereference tags with --ignore-if-in-upstream

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
>> diff --git a/builtin/log.c b/builtin/log.c
>> index dd8f3fc..e0465ba 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -807,6 +807,12 @@ static void get_patch_ids(struct rev_info *rev,
>> struct patch_ids *ids)
>>  	o2 = rev->pending.objects[1].item;
>>  	flags2 = o2->flags;
>>  
>> +	o1 = deref_tag(o1, NULL, 0);
>> +	o2 = deref_tag(o2, NULL, 0);
>> +
>> +	if (!o1 || !o2)
>> +		die(_("Invalid tag."));
>
> Shouldn't you ensure o1 and o2 are commits here?

Heh, I should have read the remainder of the thread before
responding.

How about doing it this way?  We know and trust that existing
revision traversal machinery is doing the right thing, and it is
only that the clear_commit_marks() calls are botched.

diff --git a/builtin/log.c b/builtin/log.c
index dd8f3fc..23a42fa 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -795,7 +795,7 @@ static int reopen_stdout(struct commit *commit, const char *subject,
 static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)
 {
 	struct rev_info check_rev;
-	struct commit *commit;
+	struct commit *commit, *c1, *c2;
 	struct object *o1, *o2;
 	unsigned flags1, flags2;
 
@@ -803,9 +803,11 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)
 		die(_("Need exactly one range."));
 
 	o1 = rev->pending.objects[0].item;
-	flags1 = o1->flags;
 	o2 = rev->pending.objects[1].item;
+	flags1 = o1->flags;
 	flags2 = o2->flags;
+	c1 = lookup_commit_reference(o1->sha1);
+	c2 = lookup_commit_reference(o2->sha1);
 
 	if ((flags1 & UNINTERESTING) == (flags2 & UNINTERESTING))
 		die(_("Not a range."));
@@ -827,10 +829,8 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)
 	}
 
 	/* reset for next revision walk */
-	clear_commit_marks((struct commit *)o1,
-			SEEN | UNINTERESTING | SHOWN | ADDED);
-	clear_commit_marks((struct commit *)o2,
-			SEEN | UNINTERESTING | SHOWN | ADDED);
+	clear_commit_marks(c1, SEEN | UNINTERESTING | SHOWN | ADDED);
+	clear_commit_marks(c2, SEEN | UNINTERESTING | SHOWN | ADDED);
 	o1->flags = flags1;
 	o2->flags = flags2;
 }
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index c39e500..890db11 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -57,6 +57,14 @@ test_expect_success "format-patch --ignore-if-in-upstream" '
 
 '
 
+test_expect_success "format-patch --ignore-if-in-upstream handles tags" '
+	git tag -a v1 -m tag side &&
+	git tag -a v2 -m tag master &&
+	git format-patch --stdout --ignore-if-in-upstream v2..v1 >patch1 &&
+	cnt=$(grep "^From " patch1 | wc -l) &&
+	test $cnt = 2
+'
+
 test_expect_success "format-patch doesn't consider merge commits" '
 
 	git checkout -b slave master &&

Re: [PATCH] format-patch: dereference tags with --ignore-if-in-upstream

[Jeff King]

On Mon, Jun 01, 2015 at 10:44:21AM -0700, Junio C Hamano wrote:

> > Shouldn't you ensure o1 and o2 are commits here?
> 
> Heh, I should have read the remainder of the thread before
> responding.
> 
> How about doing it this way?  We know and trust that existing
> revision traversal machinery is doing the right thing, and it is
> only that the clear_commit_marks() calls are botched.

Yeah, I think this matches the recommendation I gave in the last round.

I do still think we could get rid of this "second" traversal entirely in
favor of using "--cherry", but that is a much larger topic. Even if
somebody wants to pursue that, the immediate fix should look like this.

-Peff

Re: [PATCH] format-patch: dereference tags with --ignore-if-in-upstream

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> How about doing it this way?  We know and trust that existing
> revision traversal machinery is doing the right thing, and it is
> only that the clear_commit_marks() calls are botched.

Another alternative may be to allow any object to clear_commit_marks()
and have the callee dereference as needed.  After all, the revision
walking machinery does such a dereferencing when leaving these marks
that the function wants to clear, so it might make sense from that
point of view.

A quick "git grep clear_commit_marks()" tells me that most of the
codepaths do make sure the object is a commit when they cast their
first argument to (struct commit *) when calling this function, but
some of them do look suspicous.

Re: [PATCH] format-patch: dereference tags with --ignore-if-in-upstream

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jun 01, 2015 at 10:44:21AM -0700, Junio C Hamano wrote:
>
>> > Shouldn't you ensure o1 and o2 are commits here?
>> 
>> Heh, I should have read the remainder of the thread before
>> responding.
>> 
>> How about doing it this way?  We know and trust that existing
>> revision traversal machinery is doing the right thing, and it is
>> only that the clear_commit_marks() calls are botched.
>
> Yeah, I think this matches the recommendation I gave in the last round.
>
> I do still think we could get rid of this "second" traversal entirely in
> favor of using "--cherry", but that is a much larger topic. Even if
> somebody wants to pursue that, the immediate fix should look like this.
>
> -Peff

Thanks.

-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 1 Jun 2015 10:44:21 -0700
Subject: [PATCH] format-patch: do not feed tags to clear_commit_marks()

"git format-patch --ignore-if-in-upstream A..B", when either A or B
is a tag, failed miserably.

This is because the code passes the tips it used for traversal to
clear_commit_marks(), after running a temporary revision traversal
to enumerate the commits on both branches to find if they have
commits that make equivalent changes.  The revision traversal
machinery knows how to enumerate commits reachable starting from a
tag, but clear_commit_marks() wants to take nothing but a commit.

In the longer term, it might be a more correct fix to teach
clear_commit_marks() to do the same "committish to commit"
dereferncing that is done in the revision traversal machinery, but
for now this fix should suffice.

Reported-by: Bruce Korb <bruce.korb@gmail.com>
Helped-by: Christian Couder <christian.couder@gmail.com>
Helped-by: brian m. carlson <sandals@crustytoothpaste.net>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/log.c           | 12 ++++++------
 t/t4014-format-patch.sh |  8 ++++++++
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 734aab3..39181e2 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -795,7 +795,7 @@ static int reopen_stdout(struct commit *commit, const char *subject,
 static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)
 {
 	struct rev_info check_rev;
-	struct commit *commit;
+	struct commit *commit, *c1, *c2;
 	struct object *o1, *o2;
 	unsigned flags1, flags2;
 
@@ -803,9 +803,11 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)
 		die(_("Need exactly one range."));
 
 	o1 = rev->pending.objects[0].item;
-	flags1 = o1->flags;
 	o2 = rev->pending.objects[1].item;
+	flags1 = o1->flags;
 	flags2 = o2->flags;
+	c1 = lookup_commit_reference(o1->sha1);
+	c2 = lookup_commit_reference(o2->sha1);
 
 	if ((flags1 & UNINTERESTING) == (flags2 & UNINTERESTING))
 		die(_("Not a range."));
@@ -827,10 +829,8 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)
 	}
 
 	/* reset for next revision walk */
-	clear_commit_marks((struct commit *)o1,
-			SEEN | UNINTERESTING | SHOWN | ADDED);
-	clear_commit_marks((struct commit *)o2,
-			SEEN | UNINTERESTING | SHOWN | ADDED);
+	clear_commit_marks(c1, SEEN | UNINTERESTING | SHOWN | ADDED);
+	clear_commit_marks(c2, SEEN | UNINTERESTING | SHOWN | ADDED);
 	o1->flags = flags1;
 	o2->flags = flags2;
 }
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 256affc..2ea12dd 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -57,6 +57,14 @@ test_expect_success "format-patch --ignore-if-in-upstream" '
 
 '
 
+test_expect_success "format-patch --ignore-if-in-upstream handles tags" '
+	git tag -a v1 -m tag side &&
+	git tag -a v2 -m tag master &&
+	git format-patch --stdout --ignore-if-in-upstream v2..v1 >patch1 &&
+	cnt=$(grep "^From " patch1 | wc -l) &&
+	test $cnt = 2
+'
+
 test_expect_success "format-patch doesn't consider merge commits" '
 
 	git checkout -b slave master &&
-- 
2.4.2-558-g3ddf4bb

Re: [PATCH] format-patch: dereference tags with --ignore-if-in-upstream

[brian m. carlson]

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

On Mon, Jun 01, 2015 at 01:35:17PM -0700, Junio C Hamano wrote:
> -- >8 --
> From: Junio C Hamano <gitster@pobox.com>
> Date: Mon, 1 Jun 2015 10:44:21 -0700
> Subject: [PATCH] format-patch: do not feed tags to clear_commit_marks()
> 
> "git format-patch --ignore-if-in-upstream A..B", when either A or B
> is a tag, failed miserably.
> 
> This is because the code passes the tips it used for traversal to
> clear_commit_marks(), after running a temporary revision traversal
> to enumerate the commits on both branches to find if they have
> commits that make equivalent changes.  The revision traversal
> machinery knows how to enumerate commits reachable starting from a
> tag, but clear_commit_marks() wants to take nothing but a commit.
> 
> In the longer term, it might be a more correct fix to teach
> clear_commit_marks() to do the same "committish to commit"
> dereferncing that is done in the revision traversal machinery, but

"dereferencing".  Otherwise, looks exactly like what I would have
written in my reroll had you not gotten to it before me.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] format-patch: dereference tags with --ignore-if-in-upstream

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

>> In the longer term, it might be a more correct fix to teach
>> clear_commit_marks() to do the same "committish to commit"
>> dereferncing that is done in the revision traversal machinery, but
>
> "dereferencing".  Otherwise, looks exactly like what I would have
> written in my reroll had you not gotten to it before me.

Heh thanks.

I do not mind if you sent in a replacement.  What I sent was done
primarily because I saw multiple people coming up with essentially
the same solution and I was afraid everybody would say "it is being
taken care of by others" and we end up not having any patch ;-).