{"thread":{"id":"28696","subject":"regression in git-gui since 2c5c66b... Merge branch 'jp/get-ref-dir-unsorted","startedAt":"2011-10-16T18:10:58Z","lastAt":"2011-10-17T23:14:30Z","messageCount":11,"participants":["Mark Levedahl","Junio C Hamano","Michael Haggerty","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"177799","messageId":"4E9B1E32.7030101@gmail.com","threadId":"28696","inReplyTo":null,"subject":"regression in git-gui since 2c5c66b... Merge branch 'jp/get-ref-dir-unsorted","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2011-10-16T18:10:58Z","receivedAt":"2011-10-16T18:10:58Z","isPatch":false,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"I have a project organized as a number of nested git modules (not using \ngit-submodule), and frequently use git-new-workdir to create the nested \nmodules.\n\nSince the above merge-commit, git-gui is confused by this arrangement \nand reports every file in every nested module as being an untracked file \nin the top-level (super) project. Prior to the above merge, git-gui \nproperly stops recursing when finding a .git directory. git-gui also \ncontinues working correctly when the modules are full clones, it just \ndoesn't work correctly when the .git directory contains symlinks to the \nreal .git directory contents.\n\ngit-gui works correctly on either parent of the above merge, just not \nafter the merge. As the merge was not clean, I guess Junio gets to \ndecide who owns the problem :^).\n\nNote that core git is fine up to current master, it is only git-gui that \nhas become confused (e.g., git-status shows the top-level directory of \neach nested module as untracked, but does not list files in the nested \nmodules).\n\n\nMark\n"},{"id":"177813","messageId":"7vfwispi8u.fsf@alter.siamese.dyndns.org","threadId":"28696","inReplyTo":"4E9B1E32.7030101@gmail.com","subject":"Re: regression in git-gui since 2c5c66b... Merge branch 'jp/get-ref-dir-unsorted","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-17T00:35:45Z","receivedAt":"2011-10-17T00:35:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Levedahl writes:\n\n> I have a project organized as a number of nested git modules (not\n> using git-submodule), and frequently use git-new-workdir to create the\n> nested modules.\n>\n> Since the above merge-commit, git-gui is confused by this arrangement\n> and reports every file in every nested module as being an untracked\n> file in the top-level (super) project.\n\nCould you come up with a simple reproduction recipe that prepares a\nsuperproject that has no file of its own, a submodule with a single file\ntracked, and attaches another workdir? If running git-gui in the resulting\ndirectory makes it misbehave, we could then isolate what git command that\nis invoked by git-gui has changed its behaviour.\n\nThanks.\n"},{"id":"177815","messageId":"4E9B8719.1090203@gmail.com","threadId":"28696","inReplyTo":"7vfwispi8u.fsf@alter.siamese.dyndns.org","subject":"Re: regression in git-gui since 2c5c66b... Merge branch 'jp/get-ref-dir-unsorted","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2011-10-17T01:38:33Z","receivedAt":"2011-10-17T01:38:33Z","isPatch":false,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"On 10/16/2011 08:35 PM, Junio C Hamano wrote:\n> Mark Levedahl writes:\n>\n>> I have a project organized as a number of nested git modules (not\n>> using git-submodule), and frequently use git-new-workdir to create the\n>> nested modules.\n>>\n>> Since the above merge-commit, git-gui is confused by this arrangement\n>> and reports every file in every nested module as being an untracked\n>> file in the top-level (super) project.\n> Could you come up with a simple reproduction recipe that prepares a\n> superproject that has no file of its own, a submodule with a single file\n> tracked, and attaches another workdir? If running git-gui in the resulting\n> directory makes it misbehave, we could then isolate what git command that\n> is invoked by git-gui has changed its behaviour.\n>\n> Thanks.\n>\n\nThe following shows the problem for me:\n\n#!/bin/bash\nmkdir super sub\ncd sub\ngit init\ntouch a\ngit add a\ngit commit -m 'file' a\ngit pack-refs --all\ncd ../super\ngit init\ngit new-workdir ../sub sub\ngit-gui\n\n\ngit-gui shows \"sub/a\" in the list of Unstaged Changes. Note that the \n\"git pack-refs\" call is needed to get the failure.\n\nMark\n"},{"id":"177830","messageId":"4E9BA39B.709@alum.mit.edu","threadId":"28696","inReplyTo":"4E9B8719.1090203@gmail.com","subject":"Re: regression in git-gui since 2c5c66b... Merge branch 'jp/get-ref-dir-unsorted","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-17T03:40:11Z","receivedAt":"2011-10-17T03:40:11Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/17/2011 03:38 AM, Mark Levedahl wrote:\n> On 10/16/2011 08:35 PM, Junio C Hamano wrote:\n>> Mark Levedahl writes:\n>>> I have a project organized as a number of nested git modules (not\n>>> using git-submodule), and frequently use git-new-workdir to create the\n>>> nested modules.\n>>>\n>>> Since the above merge-commit, git-gui is confused by this arrangement\n>>> and reports every file in every nested module as being an untracked\n>>> file in the top-level (super) project.\n> [...]\n> \n> The following shows the problem for me:\n> \n> #!/bin/bash\n> mkdir super sub\n> cd sub\n> git init\n> touch a\n> git add a\n> git commit -m 'file' a\n> git pack-refs --all\n> cd ../super\n> git init\n> git new-workdir ../sub sub\n> git-gui\n> \n> \n> git-gui shows \"sub/a\" in the list of Unstaged Changes. Note that the\n> \"git pack-refs\" call is needed to get the failure.\n\nPlease bear with me because I don't use git-gui so I don't really know\nwhat to expect.\n\nWhen I check out 2c5c66b and run the above script (actually, the script\nlisted below) what I see in git-gui is:\n\n* In the \"Unstaged Changes\" window, \"sub\" is listed (not \"sub/a\").\n\n* When I click on \"sub\", then in the \"Untracked, not staged\" window,\n\"Git Repository (subproject)\" appears.\n\nI see the exact same thing when I run the same test script on the\nversion before merge 2c5c66b.\n\nWhat do you see?\n\nWhat do you expect to see?\n\nWhat versions of git, exactly, are you testing (what version do you\nconsider \"good\"; presumably it is version 2c5c66b that you consider \"bad\")?\n\nAre you certain that you are using the same git version for all commands\n(\"git\", \"git-gui\", and \"git-new-workdir\")? Please especially note that\ngit-new-workdir is not part of a default git install, and therefore it\nwould be easy to accidentally use a different version of this script\nthan of the other commands.\n\nMichael\n\n#!/bin/bash\n\nSRC=$(cd $(dirname $0); pwd)\nGIT=$SRC/git\nGIT_NEW_WORKDIR=$SRC/contrib/workdir/git-new-workdir\nGITGUI=$SRC/git-gui/git-gui\n\nrm -rf super sub\nmkdir super sub\ncd sub\n$GIT init\ntouch a\n$GIT add a\n$GIT commit -m 'file' a\n$GIT pack-refs --all\ncd ../super\n$GIT init\n$GIT_NEW_WORKDIR ../sub sub\n$GITGUI &\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"177856","messageId":"4E9BFE66.5070906@gmail.com","threadId":"28696","inReplyTo":"4E9BA39B.709@alum.mit.edu","subject":"Re: regression in git-gui since 2c5c66b... Merge branch 'jp/get-ref-dir-unsorted","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2011-10-17T10:07:34Z","receivedAt":"2011-10-17T10:07:34Z","isPatch":false,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"On 10/16/2011 11:40 PM, Michael Haggerty wrote:\n\n> Please bear with me because I don't use git-gui so I don't really know\n> what to expect.\n>\n> When I check out 2c5c66b and run the above script (actually, the script\n> listed below) what I see in git-gui is:\n>\n> * In the \"Unstaged Changes\" window, \"sub\" is listed (not \"sub/a\").\n>\n> * When I click on \"sub\", then in the \"Untracked, not staged\" window,\n> \"Git Repository (subproject)\" appears.\n>\n> I see the exact same thing when I run the same test script on the\n> version before merge 2c5c66b.\n>\n> What do you see?\n>\n> What do you expect to see?\n>\n> What versions of git, exactly, are you testing (what version do you\n> consider \"good\"; presumably it is version 2c5c66b that you consider \"bad\")?\n>\n> Are you certain that you are using the same git version for all commands\n> (\"git\", \"git-gui\", and \"git-new-workdir\")? Please especially note that\n> git-new-workdir is not part of a default git install, and therefore it\n> would be easy to accidentally use a different version of this script\n> than of the other commands.\n>\n> Michael\n>\n> #!/bin/bash\n>\n> SRC=$(cd $(dirname $0); pwd)\n> GIT=$SRC/git\n> GIT_NEW_WORKDIR=$SRC/contrib/workdir/git-new-workdir\n> GITGUI=$SRC/git-gui/git-gui\n>\n> rm -rf super sub\n> mkdir super sub\n> cd sub\n> $GIT init\n> touch a\n> $GIT add a\n> $GIT commit -m 'file' a\n> $GIT pack-refs --all\n> cd ../super\n> $GIT init\n> $GIT_NEW_WORKDIR ../sub sub\n> $GITGUI&\nMichael,\n\nThanks for looking....\n\nYour modification of my script does not show the error for me, unless I \nhave *installed* a version of git with the failure: I suspect git-gui \ninvokes installed components, and not what is in the build directory, so \nhaving a good version of git installed with the bad version in the build \ndirectory does not show the error. And yes, I am quite sure that all of \nthe git commands I am running are from the one version.\n\nWhat I expect to see is what you saw: the \"sub\" directory listed under \nUnstaged Changes. What I get when I have installed version 2c5c66b (or \ncurrent master) is the file \"sub/a\" listed under Unstaged Changes, in \nother words git-gui no longer recognizes that sub is a submodule.\n\nMark\n"},{"id":"177859","messageId":"4E9C2F3C.7080405@alum.mit.edu","threadId":"28696","inReplyTo":"4E9BFE66.5070906@gmail.com","subject":"Re: regression in git-gui since 2c5c66b... Merge branch 'jp/get-ref-dir-unsorted","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-17T13:35:56Z","receivedAt":"2011-10-17T13:35:56Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/17/2011 12:07 PM, Mark Levedahl wrote:\n> Your modification of my script does not show the error for me, unless I\n> have *installed* a version of git with the failure: I suspect git-gui\n> invokes installed components, and not what is in the build directory, so\n> having a good version of git installed with the bad version in the build\n> directory does not show the error. And yes, I am quite sure that all of\n> the git commands I am running are from the one version.\n\nYes, you seem to be right. Even if I set PATH to list my git build\ndirectory before the directory where it is installed, \"git-gui\"\nsometimes invokes git-rev-parse from the libexec path of the installed\nversion.\n\nWhen I install the compiled git, then I see the behavior that you describe.\n\nThe invocation that behaves differently is\n\n git ls-files --others -z --exclude-standard\n\n(run in the \"super\" directory). It doesn't seem to matter which version\nof git is used to create the test repository. Under 2c5c66b, it outputs\n\"sub/a\", whereas under either of the merge commit's ancestors, the\ncommand outputs \"sub/\".\n\n git ls-files --others\n\nI believe that the problem originates in code in\nresolve_gitlink_packed_ref() that was invented during the merge 2c5c66b:\n\nstatic int resolve_gitlink_packed_ref(char *name, int pathlen, const\nchar *refname, unsigned char *result)\n{\n\tint retval = -1;\n\tstruct ref_entry *ref;\n\tstruct ref_array *array = get_packed_refs(name);\n\n\tref = search_ref_array(array, refname);\n\tif (ref != NULL) {\n\t\tmemcpy(result, ref->sha1, 20);\n\t\tretval = 0;\n\t}\n\treturn retval;\n}\n\nThe problem is that the parameter \"name\" is not NUL-terminated. The old\ncode turned it into a (NUL-terminated) filename via\n\n strcpy(name + pathlen, \"packed-refs\");\n\nbut the new code passes it (unterminated) to get_packed_refs()\n\nCoincidentally, the old (correct) behavior is restored by a patch that I\nsubmitted earlier today:\n\n \"Pass a (ref_cache *) to the resolve_gitlink_*() helper functions\".\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"177860","messageId":"20111017135501.GA4171@sigill.intra.peff.net","threadId":"28696","inReplyTo":"4E9C2F3C.7080405@alum.mit.edu","subject":"Re: regression in git-gui since 2c5c66b... Merge branch 'jp/get-ref-dir-unsorted","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-17T13:55:01Z","receivedAt":"2011-10-17T13:55:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 17, 2011 at 03:35:56PM +0200, Michael Haggerty wrote:\n\n> On 10/17/2011 12:07 PM, Mark Levedahl wrote:\n> > Your modification of my script does not show the error for me, unless I\n> > have *installed* a version of git with the failure: I suspect git-gui\n> > invokes installed components, and not what is in the build directory, so\n> > having a good version of git installed with the bad version in the build\n> > directory does not show the error. And yes, I am quite sure that all of\n> > the git commands I am running are from the one version.\n> \n> Yes, you seem to be right. Even if I set PATH to list my git build\n> directory before the directory where it is installed, \"git-gui\"\n> sometimes invokes git-rev-parse from the libexec path of the installed\n> version.\n\nIf you are testing directly out of the build directory, you need to set\nGIT_EXEC_PATH, too. The bin-wrappers/git script will do this for you\n(and is what the test scripts use).\n\nBut note that there's a catch with git-gui, as its built version doesn't\nlive in the top-level. So running:\n\n bin-wrappers/git gui\n\nwill try to exec the git-gui directory. You can work around it with:\n\n bin-wrappers/git gui/git-gui\n\n-Peff\n"},{"id":"177867","messageId":"7v4nz7o7mg.fsf@alter.siamese.dyndns.org","threadId":"28696","inReplyTo":"4E9C2F3C.7080405@alum.mit.edu","subject":"Re: regression in git-gui since 2c5c66b... Merge branch 'jp/get-ref-dir-unsorted","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-17T17:22:47Z","receivedAt":"2011-10-17T17:22:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> static int resolve_gitlink_packed_ref(char *name, int pathlen, const\n> char *refname, unsigned char *result)\n> {\n> \tint retval = -1;\n> \tstruct ref_entry *ref;\n> \tstruct ref_array *array = get_packed_refs(name);\n>\n> \tref = search_ref_array(array, refname);\n> \tif (ref != NULL) {\n> \t\tmemcpy(result, ref->sha1, 20);\n> \t\tretval = 0;\n> \t}\n> \treturn retval;\n> }\n>\n> The problem is that the parameter \"name\" is not NUL-terminated. The old\n> code turned it into a (NUL-terminated) filename via\n>\n> strcpy(name + pathlen, \"packed-refs\");\n>\n> but the new code passes it (unterminated) to get_packed_refs()\n\nThanks for digging this through before I got to it. Very much appreciated,\nand sorry or the mismerge (incidentally this was why I wanted to merge\nearly these two topics that tried to improve different things that\nhappened to touch the same part of the code, as I knew such a merge was\nrisky and needed plenty time before it hits released versions).\n"},{"id":"177871","messageId":"7vbotfmpbh.fsf_-_@alter.siamese.dyndns.org","threadId":"28696","inReplyTo":"7v4nz7o7mg.fsf@alter.siamese.dyndns.org","subject":"[PATCH] resolve_gitlink_packed_ref(): fix mismerge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-17T18:43:30Z","receivedAt":"2011-10-17T18:43:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"2c5c66b (Merge branch 'jp/get-ref-dir-unsorted', 2011-10-10) merged a\ntopic that forked from the mainline before a new helper function\nget_packed_refs() refactored code to read packed-refs file. The merge made\nthe call to the helper function with an incorrect argument. The parameter\nto the function has to be a path to the submodule.\n\nFix the mismerge.\n\nHelped-by: Mark Levedahl \nHelped-by: Michael Haggerty \nSigned-off-by: Junio C Hamano \n---\n\n Junio C Hamano writes:\n\n > Michael Haggerty writes:\n > ...\n >> The problem is that the parameter \"name\" is not NUL-terminated. The old\n >> code turned it into a (NUL-terminated) filename via\n >>\n >> strcpy(name + pathlen, \"packed-refs\");\n >>\n >> but the new code passes it (unterminated) to get_packed_refs()\n\n Let's do this on the 'master' front while mh/refs-api and mh/refs-api-2\n (the new 14 patch series) are cooking in the 'next' branch. The added\n test does not pass until the second-to-last patch in your new series.\n\n I made trial merges of this with mh/refs-api and mh/refs-api-2 and both\n were trivial to resolve (merge with mh/refs-api will keep the fix, and\n merge with mh/refs-api-2 can be made by dropping this change), so this is\n purely as interim fix plus---the value of the patch lies primarily in the\n test for regression avoidance.\n\n refs.c | 12 +++++++++++-\n t/t3000-ls-files-others.sh | 19 +++++++++++++++++++\n 2 files changed, 30 insertions(+), 1 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 9911c97..cab4394 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -393,12 +393,22 @@ static struct ref_array *get_loose_refs(const char *submodule)\n #define MAXDEPTH 5\n #define MAXREFLEN (1024)\n \n+/*\n+ * Called by resolve_gitlink_ref_recursive() after it failed to read\n+ * from \"name\", which is \"module/.git/\". Find in\n+ * the packed-refs file for the submodule.\n+ */\n static int resolve_gitlink_packed_ref(char *name, int pathlen, const char *refname, unsigned char *result)\n {\n \tint retval = -1;\n \tstruct ref_entry *ref;\n-\tstruct ref_array *array = get_packed_refs(name);\n+\tstruct ref_array *array;\n \n+\t/* being defensive: resolve_gitlink_ref() did this for us */\n+\tif (pathlen < 6 || memcmp(name + pathlen - 6, \"/.git/\", 6))\n+\t\tdie(\"Oops\");\n+\tname[pathlen - 6] = '\\0'; /* make it path to the submodule */\n+\tarray = get_packed_refs(name);\n \tref = search_ref_array(array, refname);\n \tif (ref != NULL) {\n \t\tmemcpy(result, ref->sha1, 20);\ndiff --git a/t/t3000-ls-files-others.sh b/t/t3000-ls-files-others.sh\nindex 2eec011..e9160df 100755\n--- a/t/t3000-ls-files-others.sh\n+++ b/t/t3000-ls-files-others.sh\n@@ -65,4 +65,23 @@ test_expect_success '--no-empty-directory hides empty directory' '\n \ttest_cmp expected3 output\n '\n \n+test_expect_success SYMLINKS 'ls-files --others with symlinked submodule' '\n+\tgit init super &&\n+\tgit init sub &&\n+\t(\n+\t\tcd sub &&\n+\t\t>a &&\n+\t\tgit add a &&\n+\t\tgit commit -m sub &&\n+\t\tgit pack-refs --all\n+\t) &&\n+\t(\n+\t\tcd super &&\n+\t\t\"$TEST_DIRECTORY/../contrib/workdir/git-new-workdir\" ../sub sub\n+\t\tgit ls-files --others --exclude-standard >../actual\n+\t) &&\n+\techo sub/ >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.7.370.gefe43\n"},{"id":"177897","messageId":"4E9CA853.10107@gmail.com","threadId":"28696","inReplyTo":"7vbotfmpbh.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH] resolve_gitlink_packed_ref(): fix mismerge","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2011-10-17T22:12:35Z","receivedAt":"2011-10-17T22:12:35Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"On 10/17/2011 02:43 PM, Junio C Hamano wrote:\n> 2c5c66b (Merge branch 'jp/get-ref-dir-unsorted', 2011-10-10) merged a\n> topic that forked from the mainline before a new helper function\n> get_packed_refs() refactored code to read packed-refs file. The merge made\n> the call to the helper function with an incorrect argument. The parameter\n> to the function has to be a path to the submodule.\n>\n> Fix the mismerge.\n>\n> Helped-by: Mark Levedahl\n> Helped-by: Michael Haggerty\n> Signed-off-by: Junio C Hamano\n> ---\n>\nThank you, this fixes the problem for me.\n\nMark\n"},{"id":"177908","messageId":"7vipnnjjmx.fsf@alter.siamese.dyndns.org","threadId":"28696","inReplyTo":"4E9CA853.10107@gmail.com","subject":"Re: [PATCH] resolve_gitlink_packed_ref(): fix mismerge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-17T23:14:30Z","receivedAt":"2011-10-17T23:14:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Levedahl writes:\n\n> On 10/17/2011 02:43 PM, Junio C Hamano wrote:\n>> 2c5c66b (Merge branch 'jp/get-ref-dir-unsorted', 2011-10-10) merged a\n>> topic that forked from the mainline before a new helper function\n>> get_packed_refs() refactored code to read packed-refs file. The merge made\n>> the call to the helper function with an incorrect argument. The parameter\n>> to the function has to be a path to the submodule.\n>>\n>> Fix the mismerge.\n>>\n>> Helped-by: Mark Levedahl\n>> Helped-by: Michael Haggerty\n>> Signed-off-by: Junio C Hamano\n>> ---\n>>\n> Thank you, this fixes the problem for me.\n\nThanks for a quick bug report and helping with the test.\n"}]}