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

I have a project organized as a number of nested git modules (not using git-submodule), and frequently use git-new-workdir to create the nested modules.

Since the above merge-commit, git-gui is confused by this arrangement and reports every file in every nested module as being an untracked file in the top-level (super) project. Prior to the above merge, git-gui properly stops recursing when finding a .git directory. git-gui also continues working correctly when the modules are full clones, it just doesn't work correctly when the .git directory contains symlinks to the real .git directory contents.

git-gui works correctly on either parent of the above merge, just not after the merge. As the merge was not clean, I guess Junio gets to decide who owns the problem :^).

Note that core git is fine up to current master, it is only git-gui that has become confused (e.g., git-status shows the top-level directory of each nested module as untracked, but does not list files in the nested modules).

Mark

Could you come up with a simple reproduction recipe that prepares a superproject that has no file of its own, a submodule with a single file tracked, and attaches another workdir? If running git-gui in the resulting directory makes it misbehave, we could then isolate what git command that is invoked by git-gui has changed its behaviour.

Thanks.

The following shows the problem for me:

#!/bin/bash
mkdir super sub
cd sub
git init
touch a
git add a
git commit -m 'file' a
git pack-refs --all
cd ../super
git init
git new-workdir ../sub sub
git-gui

git-gui shows "sub/a" in the list of Unstaged Changes. Note that the "git pack-refs" call is needed to get the failure.

Mark

Please bear with me because I don't use git-gui so I don't really know what to expect.

When I check out 2c5c66b and run the above script (actually, the script listed below) what I see in git-gui is:

* In the "Unstaged Changes" window, "sub" is listed (not "sub/a").

* When I click on "sub", then in the "Untracked, not staged" window, "Git Repository (subproject)" appears.

I see the exact same thing when I run the same test script on the version before merge 2c5c66b.

What do you see?

What do you expect to see?

What versions of git, exactly, are you testing (what version do you consider "good"; presumably it is version 2c5c66b that you consider "bad")?

Are you certain that you are using the same git version for all commands ("git", "git-gui", and "git-new-workdir")? Please especially note that git-new-workdir is not part of a default git install, and therefore it would be easy to accidentally use a different version of this script than of the other commands.

Michael

#!/bin/bash

SRC=$(cd $(dirname $0); pwd)
GIT=$SRC/git GIT_NEW_WORKDIR=$SRC/contrib/workdir/git-new-workdir GITGUI=$SRC/git-gui/git-gui

rm -rf super sub
mkdir super sub
cd sub
$GIT init
touch a
$GIT add a
$GIT commit -m 'file' a
$GIT pack-refs --all
cd ../super
$GIT init
$GIT_NEW_WORKDIR ../sub sub
$GITGUI &

Michael Haggerty
[email protected]
http://softwareswirl.blogspot.com/

Michael,

Thanks for looking....

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.

What I expect to see is what you saw: the "sub" directory listed under Unstaged Changes. What I get when I have installed version 2c5c66b (or current master) is the file "sub/a" listed under Unstaged Changes, in other words git-gui no longer recognizes that sub is a submodule.

Mark

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
[email protected]
http://softwareswirl.blogspot.com/

If you are testing directly out of the build directory, you need to set GIT_EXEC_PATH, too. The bin-wrappers/git script will do this for you (and is what the test scripts use).

But note that there's a catch with git-gui, as its built version doesn't live in the top-level. So running:

bin-wrappers/git gui

will try to exec the git-gui directory. You can work around it with:

bin-wrappers/git gui/git-gui

-Peff

Thanks for digging this through before I got to it. Very much appreciated, and sorry or the mismerge (incidentally this was why I wanted to merge early these two topics that tried to improve different things that happened to touch the same part of the code, as I knew such a merge was risky and needed plenty time before it hits released versions).

2c5c66b (Merge branch 'jp/get-ref-dir-unsorted', 2011-10-10) merged a topic that forked from the mainline before a new helper function get_packed_refs() refactored code to read packed-refs file. The merge made the call to the helper function with an incorrect argument. The parameter to the function has to be a path to the submodule.

Fix the mismerge.

---

Let's do this on the 'master' front while mh/refs-api and mh/refs-api-2 (the new 14 patch series) are cooking in the 'next' branch. The added test does not pass until the second-to-last patch in your new series.

I made trial merges of this with mh/refs-api and mh/refs-api-2 and both were trivial to resolve (merge with mh/refs-api will keep the fix, and merge with mh/refs-api-2 can be made by dropping this change), so this is purely as interim fix plus---the value of the patch lies primarily in the test for regression avoidance.

 refs.c                     |   12 +++++++++++-
 t/t3000-ls-files-others.sh |   19 +++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletions(-)

index 9911c97..cab4394 100644
@@ -393,12 +393,22 @@ static struct ref_array *get_loose_refs(const char *submodule)
 #define MAXDEPTH 5
 #define MAXREFLEN (1024)
 
+/*
+ * Called by resolve_gitlink_ref_recursive() after it failed to read
+ * from "name", which is "module/.git/<refname>". Find <refname> in
+ * the packed-refs file for the submodule.
+ */
 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);
+	struct ref_array *array;
 
+	/* being defensive: resolve_gitlink_ref() did this for us */
+	if (pathlen < 6 || memcmp(name + pathlen - 6, "/.git/", 6))
+		die("Oops");
+	name[pathlen - 6] = '\0'; /* make it path to the submodule */
+	array = get_packed_refs(name);
 	ref = search_ref_array(array, refname);
 	if (ref != NULL) {
 		memcpy(result, ref->sha1, 20);

index 2eec011..e9160df 100755
@@ -65,4 +65,23 @@ test_expect_success '--no-empty-directory hides empty directory' '
 	test_cmp expected3 output
 '
 
+test_expect_success SYMLINKS 'ls-files --others with symlinked submodule' '
+	git init super &&
+	git init sub &&
+	(
+		cd sub &&
+		>a &&
+		git add a &&
+		git commit -m sub &&
+		git pack-refs --all
+	) &&
+	(
+		cd super &&
+		"$TEST_DIRECTORY/../contrib/workdir/git-new-workdir" ../sub sub
+		git ls-files --others --exclude-standard >../actual
+	) &&
+	echo sub/ >expect &&
+	test_cmp expect actual
+'
+
 test_done

1.7.7.370.gefe43

Thank you, this fixes the problem for me.

Mark

Thanks for a quick bug report and helping with the test.