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

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

[Mark Levedahl]

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

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

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> 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.

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.

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

[Mark Levedahl]

On 10/16/2011 08:35 PM, Junio C Hamano wrote:
> Mark Levedahl<mlevedahl@gmail.com>  writes:
>
>> 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.
> 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

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

[Michael Haggerty]

On 10/17/2011 03:38 AM, Mark Levedahl wrote:
> On 10/16/2011 08:35 PM, Junio C Hamano wrote:
>> Mark Levedahl<mlevedahl@gmail.com>  writes:
>>> 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.
> [...]
> 
> 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.

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
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

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

[Mark Levedahl]

On 10/16/2011 11:40 PM, Michael Haggerty 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&
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

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

[Michael Haggerty]

On 10/17/2011 12:07 PM, Mark Levedahl wrote:
> 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/

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

[Jeff King]

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

> On 10/17/2011 12:07 PM, Mark Levedahl wrote:
> > 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.

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

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

[Junio C Hamano]

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

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

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

[PATCH] resolve_gitlink_packed_ref(): fix mismerge

[Junio C Hamano]

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

diff --git a/refs.c b/refs.c
index 9911c97..cab4394 100644
--- a/refs.c
+++ b/refs.c
@@ -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);
diff --git a/t/t3000-ls-files-others.sh b/t/t3000-ls-files-others.sh
index 2eec011..e9160df 100755
--- a/t/t3000-ls-files-others.sh
+++ b/t/t3000-ls-files-others.sh
@@ -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

Re: [PATCH] resolve_gitlink_packed_ref(): fix mismerge

[Mark Levedahl]

On 10/17/2011 02:43 PM, Junio C Hamano wrote:
> 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>
> ---
>
Thank you, this fixes the problem for me.

Mark

Re: [PATCH] resolve_gitlink_packed_ref(): fix mismerge

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> On 10/17/2011 02:43 PM, Junio C Hamano wrote:
>> 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>
>> ---
>>
> Thank you, this fixes the problem for me.

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