From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Torsten Bögershausen" <tboegi@web.de>,
mhagger@alum.mit.edu, "Jeff King" <peff@peff.net>,
"Johannes Sixt" <j6t@kdbg.org>,
"Shawn O. Pearce" <spearce@spearce.org>,
mlevedahl@gmail.com, dpotapov@gmail.com,
"GIT Mailing-list" <git@vger.kernel.org>
Subject: Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
Date: Wed, 26 Jun 2013 22:39:41 +0100 [thread overview]
Message-ID: <51CB5F9D.3060202@ramsay1.demon.co.uk> (raw)
In-Reply-To: <7vy59y4w3r.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
>
>> Michael Haggerty and Jeff King have been re-vamping the reference
>> handling code. The failures noted above were provoked by patches
>> in the 'mh/ref-races' branch. At the time I wrote this patch, that
>> branch was only included in 'pu', but I notice that this topic has
>> now progressed to 'next' (see commit 71f1a182).
>
> I had an impression that up to 98eeb09e (for_each_ref: load all
> loose refs before packed refs, 2013-06-20) that is now in 'next'
> does not agressively use the lstat timestamp of the packed-refs
> file, and the "optional" bit 5d478f5c (refs: do not invalidate the
> packed-refs cache unnecessarily, 2013-06-20), and the one in 'next'
> should be safe with the cheating-lstat. Isn't it the case?
The next branch (from a couple of days ago, namely commit 4f488db) is
currently broken on cygwin, like so:
$ cd t
$ ./t3211-peel-ref.sh -i -v
[ ... ]
ok 7 - refs are peeled outside of refs/tags (old packed)
expecting success:
git pack-refs --all &&
cp .git/packed-refs fully-peeled &&
git branch yadda &&
git pack-refs --all &&
git branch -d yadda &&
test_cmp fully-peeled .git/packed-refs
fatal: internal error: packed-ref cache cleared while locked
not ok 8 - peeled refs survive deletion of packed ref
#
# git pack-refs --all &&
# cp .git/packed-refs fully-peeled &&
# git branch yadda &&
# git pack-refs --all &&
# git branch -d yadda &&
# test_cmp fully-peeled .git/packed-refs
#
$ cd trash\ directory.t3211-peel-ref/
$ ls
actual base.t expect
$ ../../bin-wrappers/git pack-refs --all
fatal: internal error: packed-ref cache cleared while locked
$ gdb ../../git.exe
GNU gdb 6.5.50.20060706-cvs (cygwin-special)
Copyright (C) 2006 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB. Type "show warranty" for details.
This GDB was configured as "i686-pc-cygwin"...
(gdb) b stat_validity_check
Breakpoint 1 at 0x48a33f: file read-cache.c, line 1965.
(gdb) run pack-refs --all
Starting program: /home/ramsay/git/git.exe pack-refs --all
Loaded symbols for /cygdrive/c/WINDOWS/system32/ntdll.dll
Loaded symbols for /cygdrive/c/WINDOWS/system32/kernel32.dll
Loaded symbols for /usr/bin/cygcrypto-0.9.8.dll
Loaded symbols for /usr/bin/cygwin1.dll
Loaded symbols for /cygdrive/c/WINDOWS/system32/advapi32.dll
Loaded symbols for /cygdrive/c/WINDOWS/system32/rpcrt4.dll
Loaded symbols for /cygdrive/c/WINDOWS/system32/secur32.dll
Loaded symbols for /usr/bin/cygiconv-2.dll
Loaded symbols for /usr/bin/cygz.dll
Breakpoint 1, stat_validity_check (sv=0xa611a4,
path=0x57d98c ".git/packed-refs") at read-cache.c:1965
1965 if (stat(path, &st) < 0)
(gdb) n
1967 if (!sv->sd)
(gdb) p sv
$1 = (struct stat_validity *) 0xa611a4
(gdb) p *sv
$2 = {sd = 0xa62478}
(gdb) p sv->sd
$3 = (struct stat_data *) 0xa62478
(gdb) p *sv->sd
$4 = {sd_ctime = {sec = 1372194879, nsec = 671875000}, sd_mtime = {
sec = 1372194879, nsec = 656250000}, sd_dev = 2899104371,
sd_ino = 180184, sd_uid = 1005, sd_gid = 513, sd_size = 296}
(gdb) p st
$5 = {st_dev = 0, st_ino = 0, st_mode = 33152, st_nlink = 1, st_uid = 0,
st_gid = 0, st_rdev = 0, st_size = 296, st_atim = {tv_sec = 1372195048,
tv_nsec = 890625000}, st_mtim = {tv_sec = 1372194879,
tv_nsec = 656250000}, st_ctim = {tv_sec = 1372194879,
tv_nsec = 15625000}, st_blksize = 5636381, st_blocks = 1, st_spare4 = {
10883480, 5757324}}
(gdb) c
Continuing.
fatal: internal error: packed-ref cache cleared while locked
Program exited with code 0200.
(gdb) quit
$ ../../test-stat .git/packed-refs
stat for '.git/packed-refs':
*dev: -1395862925, 0
*ino: 180184, 0
*mode: 100644 -rw-, 100600 -rw-
nlink: 1, 1
*uid: 1005, 0
*gid: 513, 0
*rdev: -1395862925, 0
size: 296, 296
atime: 1372195048, 1372195048 Tue Jun 25 22:17:28 2013
mtime: 1372194879, 1372194879 Tue Jun 25 22:14:39 2013
ctime: 1372194879, 1372194879 Tue Jun 25 22:14:39 2013
$
Note that, as noted before, (at least) the following tests fail
due to the 'mh/ref-races' branch:
t3210-pack-refs.sh (Wstat: 256 Tests: 16 Failed: 12)
Failed tests: 4-9, 11-16
Non-zero exit status: 1
t3211-peel-ref.sh (Wstat: 256 Tests: 8 Failed: 1)
Failed test: 8
Non-zero exit status: 1
t5500-fetch-pack.sh (Wstat: 256 Tests: 54 Failed: 1)
Failed test: 43
Non-zero exit status: 1
> In any case, if removing the cheating-lstat will improve the
> robustness without hurting performance, I am all for it.
I'm sure it will improve robustness (and help with maintainability),
but as for performance ...
> The less platform specific hacks, the better ;-).
Agreed.
ATB,
Ramsay Jones
next prev parent reply other threads:[~2013-06-26 22:07 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <51C5FD28.1070004@ramsay1.demon.co.uk>
[not found] ` <51C7A875.6020205@gmail.com>
[not found] ` <7va9mf6txq.fsf@alter.siamese.dyndns.org>
2013-06-25 19:23 ` [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions Johannes Sixt
2013-06-25 20:23 ` Torsten Bögershausen
2013-06-25 21:18 ` Junio C Hamano
2013-06-26 14:19 ` Torsten Bögershausen
2013-06-26 21:54 ` Ramsay Jones
2013-06-27 15:19 ` Torsten Bögershausen
2013-06-27 23:18 ` Ramsay Jones
2013-06-27 2:37 ` Mark Levedahl
[not found] ` <51C6BC4B.9030905@web.de>
[not found] ` <51C8BF2C.2050203@ramsay1.demon.co.uk>
[not found] ` <7vy59y4w3r.fsf@alter.siamese.dyndns.org>
2013-06-26 21:39 ` Ramsay Jones [this message]
[not found] ` <51C94425.7050006@alum.mit.edu>
2013-06-26 21:45 ` Ramsay Jones
2013-06-26 22:35 ` Jeff King
2013-06-26 22:43 ` Jeff King
2013-06-27 22:58 ` Ramsay Jones
2013-06-28 2:31 ` Mark Levedahl
2013-06-27 5:51 ` Michael Haggerty
2013-06-27 19:58 ` Jeff King
2013-06-27 21:04 ` Junio C Hamano
2013-06-27 23:09 ` Ramsay Jones
2013-06-30 17:28 ` Ramsay Jones
2013-06-30 19:50 ` Junio C Hamano
2013-07-04 18:18 ` Ramsay Jones
2013-07-09 11:02 ` Torsten Bögershausen
2013-07-11 17:31 ` Ramsay Jones
2013-06-27 22:17 ` Ramsay Jones
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=51CB5F9D.3060202@ramsay1.demon.co.uk \
--to=ramsay@ramsay1.demon.co.uk \
--cc=dpotapov@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=j6t@kdbg.org \
--cc=mhagger@alum.mit.edu \
--cc=mlevedahl@gmail.com \
--cc=peff@peff.net \
--cc=spearce@spearce.org \
--cc=tboegi@web.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).