Re: Be more careful about updating refs

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 18, 2008, 01:43 UTC
Message-ID: <7vbq7jkixg.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <478F4CDC.2090703@viscovery.net>

Johannes Sixt <j.sixt@viscovery.net> writes:

Show 18 quoted lines

> Charles Bailey schrieb:
>> On Thu, Jan 17, 2008 at 11:52:23AM +0100, Johannes Sixt wrote:
>>> I observed the same (on Windows). The reason is that above-mentioned
>>> commit introduces a call to parse_objects(). But by the time that
>>> fast-import calls write_ref_sha1() (and, hence, this new parse_objects())
>>> it has not yet written a pack file, and parse_objects() fails. I don't
>>> have a clue how to fix this short of reverting the commit.
>>>
>> 
>> OK, so it's not just Mac OS X, then.  From your description and my
>> initial poke at the code,  I can't immediately see a good reason why
>> the test should succeed on Linux, though.
>
> My analysis is not correct. The pack file is present, but it seems to be
> incomplete.
>
> The reason is the NO_MMAP build flag. If you compile with
> NO_MMAP=YesPlease on Linux, t9301 fails as well. Does this ring a bell?

I ran strace and found that fast-import retains three windows to the same data that was opened while the pack was still being built (i.e. the filename is still tmp_pack_XXXXXX) when it dies:

    {next = 0x6f6d20, base = 0x6f77a0 "PACK", offset = 0, len = 907,
     last_used = 9, inuse_cnt = 0}
    {next = 0x728630, base = 0x6f7160 "PACK", offset = 0, len = 500, 
      last_used = 5, inuse_cnt = 0}
    {next = 0x0, base = 0x6f6d50 "PACK", offset = 0, len = 261, 
      last_used = 1, inuse_cnt = 0}

When it hits end_packfile() to install the built-pack to its final destination and makes it available to the rest of git, however, it has this clever code to reuse the still open window.

The object offset asked when it dies is at offset 887. However, tmp_pack was written after that last window with length 907 has been (re)opened, and reusing the window results in reading a stale data.

The following patch seems to fix the issue for me, but this is primarily meant for discussion, as I do not quite understand why the same issue does not manifest itself when NO_MMAP is not used.

diff --git a/fast-import.c b/fast-import.c
index 3609c24..bd0ddb1 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -926,7 +926,13 @@ static void end_packfile(void)
 		new_p = add_packed_git(idx_name, strlen(idx_name), 1);
 		if (!new_p)
 			die("core git rejected index %s", idx_name);
-		new_p->windows = old_p->windows;
+		while (old_p->windows) {
+			struct pack_window *w = old_p->windows;
+			munmap(w->base, w->len);
+			old_p->windows = w->next;
+			free(w);
+		}
+		new_p->windows = NULL;
 		all_packs[pack_id] = new_p;
 		install_packed_git(new_p);