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

Mark Levedahl <mlevedahl@gmail.com> writes:

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.

On 10/16/2011 08:35 PM, Junio C Hamano wrote:

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

On 10/17/2011 03:38 AM, Mark Levedahl wrote:

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 &

On 10/16/2011 11:40 PM, Michael Haggerty wrote:

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

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

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

On Mon, Oct 17, 2011 at 03:35:56PM +0200, Michael Haggerty wrote:

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

Michael Haggerty <mhagger@alum.mit.edu> writes:

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.

Helped-by: Mark Levedahl <mlevedahl@gmail.com>
Helped-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

  Junio C Hamano <gitster@pobox.com> writes:

  > Michael Haggerty <mhagger@alum.mit.edu> writes:
  > ...
  >> 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()

  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(-)

On 10/17/2011 02:43 PM, Junio C Hamano wrote:

Thank you, this fixes the problem for me.

Mark

Mark Levedahl <mlevedahl@gmail.com> writes:

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