Re: regression in git-gui since 2c5c66b... Merge branch 'jp/get-ref-dir-unsorted

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Oct 17, 2011, 13:35 UTC
Message-ID: <4E9C2F3C.7080405@alum.mit.edu>
In-Reply-To: <4E9BFE66.5070906@gmail.com>

On 10/17/2011 12:07 PM, Mark Levedahl wrote:

Show 6 quoted lines

> Your modification of my script does not show the error for me, unless I
> have *installed* a version of git with the failure: I suspect git-gui
> invokes installed components, and not what is in the build directory, so
> having a good version of git installed with the bad version in the build
> directory does not show the error. And yes, I am quite sure that all of
> the git commands I am running are from the one version.

Yes, you seem to be right. Even if I set PATH to list my git build directory before the directory where it is installed, "git-gui" sometimes invokes git-rev-parse from the libexec path of the installed version.

When I install the compiled git, then I see the behavior that you describe.

The invocation that behaves differently is

    git ls-files --others -z --exclude-standard

(run in the "super" directory). It doesn't seem to matter which version of git is used to create the test repository. Under 2c5c66b, it outputs "sub/a", whereas under either of the merge commit's ancestors, the command outputs "sub/".

    git ls-files --others

I believe that the problem originates in code in resolve_gitlink_packed_ref() that was invented during the merge 2c5c66b:

static int resolve_gitlink_packed_ref(char *name, int pathlen, const
char *refname, unsigned char *result)
{
	int retval = -1;
	struct ref_entry *ref;
	struct ref_array *array = get_packed_refs(name);

	ref = search_ref_array(array, refname);
	if (ref != NULL) {
		memcpy(result, ref->sha1, 20);
		retval = 0;
	}
	return retval;
}

The problem is that the parameter "name" is not NUL-terminated. The old code turned it into a (NUL-terminated) filename via

    strcpy(name + pathlen, "packed-refs");

but the new code passes it (unterminated) to get_packed_refs()

Coincidentally, the old (correct) behavior is restored by a patch that I submitted earlier today:

    "Pass a (ref_cache *) to the resolve_gitlink_*() helper functions".

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/