Re: [PATCH] Add test for cloning with "--reference" repo being a subset of source repo

[Johan Herland <johan@herland.net>]

On Monday 03 March 2008, Daniel Barkalow wrote:
> On Mon, 3 Mar 2008, Johan Herland wrote:
> 
> > Not sure what's going on here, yet, but I thought I'd give you a heads up.
> 
> I figured it out, and pushed out a fix; it was doing everything correctly, 
> but it wrote to the alternates files after the library had read that file, 
> so it then didn't notice that it actually had the objects that are in the 
> second alternate repository.

Thanks. After looking a bit more at the original test repo where I found
this issue, I discovered another, similar bug. This one seems ugly; brace
yourself:

In some cases (I'm not exactly sure of all the preconditions) when
cloning with "--reference", it seems git tries to access a loose object
in the "--reference" repo instead of in the cloned repo, even if that
object is already present in the cloned repo and _missing_ in the
"--reference" repo. The symptom is this error message:
    error: Trying to write ref $ref with nonexistant object $sha1

After playing around with this in gdb, it seems the problem is all the
way down in sha1_file_name() (sha1_file.c). This function is responsible
for generating the loose object filename for a given $sha1. It keeps a
static char *base which is initially set to the object directory name,
and then calls fill_sha1_path() to copy the rest of the object filename
into the following bytes. On subsequent calls, only the fill_sha1_path()
part is done, thereby reusing the base from the previous invocation.

What I observe is that this base is not reset after accessing loose
objects in the "--reference" repo. Thus, later when accessing objects in
the cloned repo, sha1_file_name() generates incorrect filenames (pointing
to the "--reference" repo instead of the cloned repo).

Of course, this often goes undetected since the "--reference" repo often
have the same loose objects as the clone.

Unfortunately (from a builtin git-clone's POV) this seems to be
symptomatic of a deeper problem in this part of the code: Using
function-static variables as caches only works as far as the cache
is in sync with reality. Especially when switching between multiple
repositories within the same process, it seems that several of these
variables are left with invalid data in them. This needs to be fixed,
if not only for now, then at least as part of the libification effort.

I'm not sure what is the best way of fixing this issue; my initial guess
is to move these function-static variables out to file-level, and make
sure they're properly reset whenever the appropriate context is changed
(typically when set_git_dir() is called, I guess).

Here are the function-static variables I immediately found in sha1_file.c
(there may be more, both in sha1_file.c and in other files):
- sha1_file_name(): static char *base
- sha1_pack_name(): static char *base
- sha1_pack_index_name(): static char *base
- find_pack_entry(): static struct packed_git *last_found
  (not sure about this one)

I will follow up this email with two patches, one adding the failing test,
and one providing a simple fix for that specific test (although very much
insufficient as a fix for the actual issue described above).


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net