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

On Monday 03 March 2008, Daniel Barkalow wrote:

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