Michael Haggerty wrote:
> *This patch series must be built on top of mh/reflife.*
>
[...]
> The other problem was that the for_each_ref() functions will die if
> the ref cache that they are iterating over is freed out from under
> them. This problem is solved by using reference counts to avoid
> freeing the old packed ref cache (even if it is no longer valid) until
> all users are done with it.
Yes, I found exactly this happened to me on cygwin, earlier this week,
with the previous version of this code. After seeing this mail, I had
decided not to describe the failure on the old version, but wait and
test this version instead.
This version is a great improvement, but it still has some failures on
cygwin. So, it may be worth (briefly) describing the old failure anyway!
Note that several tests failed, but I will only mention t3211-peel-ref.sh
tests #7-8.
$ pwd
/home/ramsay/git/t/trash directory.t3211-peel-ref
$
$ ../../bin-wrappers/git show-ref -d
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master
eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/outside/foo^{}
5 [main] git 3540 _cygtls::handle_exceptions: Error while dumping state (p
robably corrupted stack)
Segmentation fault (core dumped)
$
The stack-trace for the faulting code looks something like:
cmd_show_ref()
for_each_ref(show_ref, NULL)
do_for_each_ref(&ref_cache, "", show_ref, 0, 0, NULL)
do_for_each_entry(&ref_cache, "", do_one_ref, &data)
do_for_each_entry_in_dirs(packed_dir, loose_dir, do_one_ref, &data)
*dfeeid() recursive calls*
do_one_ref(entry, &data)
show_ref("refs/outside/foo", sha1, NULL) [2nd match]
peel_ref("refs/outside/foo", sha1)
peel_entry(entry, 0)
peel_object(name, sha1)
deref_tag_noverify(o)
parse_object(sha1 <eb0e854c2...>)
lookup_replace_object(sha1)
do_lookup_replace_object(sha1)
prepare_replace_object()
[un-indent here!]
for_each_replace_ref(register_replace_ref, NULL)
do_for_each_ref(&ref_cache, "refs/replace", fn, 13, 0, NULL)
do_for_each_entry(&ref_cache, "refs/replace", fn, &data)
get_packed_refs(&ref_cache)
clear_packed_ref_cache(&ref_cache) *free_ref_entries etc*
** return to show_ref() [2nd match] above **
** return to recursive dfeeid() call in original iteration
** dir1->entries has been free()-ed and reused => segmentation fault
[dir1->entries == 0x64633263 => dc2c => part of sha1 for refs/outside/foo]
So, the nested "replace-reference-iteration" causes the ref_cache to be
freed out from under the initial show-ref iteration, so this works:
$ GIT_NO_REPLACE_OBJECTS=1 ../../bin-wrappers/git show-ref -d
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master
eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/outside/foo^{}
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/base
eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/tags/foo
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/foo^{}
$
You may be wondering why clear_packed_ref_cache() is called? Well, that
is because stat_validity_check() *incorrectly* indicates that the
packed-refs file has changed. Why does it do that? Well, this is one
more example of the problems caused by the cygwin schizophrenic stat()
functions. :( [ARGHHHHHHHHH]
At this point, I tried running 'git show-ref' with core.checkstat set
on the command line; but that didn't work! I had to fix show-ref and
re-build git, and then, this works:
$ ../../bin-wrappers/git -c core.checkstat=minimal -c core.trustctime=f
alse show-ref -d
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master
eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/outside/foo^{}
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/base
eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/tags/foo
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/foo^{}
$
Now, turning to the new code, t3211-peel-ref.sh test #7 now works, but
test #8 still fails...
$ ./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/
$ ../../bin-wrappers/git pack-refs --all
fatal: internal error: packed-ref cache cleared while locked
$ ls
actual base.t expect
$ ls .git
COMMIT_EDITMSG branches/ description index logs/ packed-refs
HEAD config hooks-disabled/ info/ objects/ refs/
$ ls -l .git/packed-refs
-rw-r--r-- 1 ramsay None 296 Jun 14 20:34 .git/packed-refs
$ cat .git/packed-refs
# pack-refs with: peeled
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master
eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/base
eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/tags/foo
^d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e
$
Now, I have a test-stat program which prints the difference between
the two stat implementations used in cygwin git, thus:
$ ../../test-stat .git/packed-refs
stat for '.git/packed-refs':
*dev: -1395862925, 0
*ino: 166044, 0
*mode: 100644 -rw-, 100600 -rw-
nlink: 1, 1
*uid: 1005, 0
*gid: 513, 0
*rdev: -1395862925, 0
size: 296, 296
atime: 1371238550, 1371238550 Fri Jun 14 20:35:50 2013
mtime: 1371238469, 1371238469 Fri Jun 14 20:34:29 2013
ctime: 1371238469, 1371238469 Fri Jun 14 20:34:29 2013
$ ../../bin-wrappers/git -c core.checkstat=minimal pack-refs --all
fatal: internal error: packed-ref cache cleared while locked
$
Hmmm, that should have worked! Wait, fix 'git pack-refs' to support
setting config variables on the command line, rebuild and:
$ ../../bin-wrappers/git -c core.checkstat=minimal pack-refs --all
$ cat .git/packed-refs
# pack-refs with: peeled fully-peeled
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master
eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo
^d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/base
eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/tags/foo
^d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e
$
I haven't checked the remaining test failures to see if they are
caused by this code (I don't think so, but ...), but this failure
is clearly a cygwin specific issue.
HTH
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html