{"thread":{"id":"19238","subject":"[PATCH RESEND] Git.pm: Set GIT_WORK_TREE if we set GIT_DIR","startedAt":"2009-05-07T13:41:27Z","lastAt":"2009-05-27T13:46:17Z","messageCount":7,"participants":["Frank Lichtenheld","Petr Baudis","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"113247","messageId":"1241703688-6892-1-git-send-email-frank@lichtenheld.de","threadId":"19238","inReplyTo":null,"subject":"[PATCH RESEND] Git.pm: Set GIT_WORK_TREE if we set GIT_DIR","fromName":"Frank Lichtenheld","fromEmail":"frank@lichtenheld.de","sentAt":"2009-05-07T13:41:27Z","receivedAt":"2009-05-07T13:41:27Z","isPatch":true,"sender":{"key":"frank@lichtenheld.de","avatar":"https://gravatar.com/avatar/b9f1d4b120e138f157c9e480d0818197c474628923786adb98f30017cdb99c3c?d=mp&s=160"},"body":"From: Frank Lichtenheld <flichtenheld@astaro.com>\n\nOtherwise git will use the current directory as work tree which will\nlead to unexpected results if we operate in sub directory of the\nwork tree.\n\nSigned-off-by: Frank Lichtenheld <flichtenheld@astaro.com>\n---\n perl/Git.pm         |    2 ++\n t/t9700-perl-git.sh |    4 ++++\n t/t9700/test.pl     |   13 +++++++++++++\n 3 files changed, 19 insertions(+), 0 deletions(-)\n\nNo comments and doesn't seem to have been applied, so resent unchanged.\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 291ff5b..4313db7 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -1280,6 +1280,8 @@ sub _cmd_exec {\n \tmy ($self, @args) = @_;\n \tif ($self) {\n \t\t$self->repo_path() and $ENV{'GIT_DIR'} = $self->repo_path();\n+\t\t$self->repo_path() and $self->wc_path()\n+\t\t\tand $ENV{'GIT_WORK_TREE'} = $self->wc_path();\n \t\t$self->wc_path() and chdir($self->wc_path());\n \t\t$self->wc_subdir() and chdir($self->wc_subdir());\n \t}\ndiff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh\nindex b4ca244..4eb7d3f 100755\n--- a/t/t9700-perl-git.sh\n+++ b/t/t9700-perl-git.sh\n@@ -29,6 +29,10 @@ test_expect_success \\\n      git add . &&\n      git commit -m \"first commit\" &&\n \n+     echo \"new file in subdir 2\" > directory2/file2 &&\n+     git add . &&\n+     git commit -m \"commit in directory2\" &&\n+\n      echo \"changed file 1\" > file1 &&\n      git commit -a -m \"second commit\" &&\n \ndiff --git a/t/t9700/test.pl b/t/t9700/test.pl\nindex 697daf3..d9b29ea 100755\n--- a/t/t9700/test.pl\n+++ b/t/t9700/test.pl\n@@ -98,3 +98,16 @@ TODO: {\n \ttodo_skip 'config after wc_chdir', 1;\n \tis($r->config(\"color.string\"), \"value\", \"config after wc_chdir\");\n }\n+\n+# Object generation in sub directory\n+chdir(\"directory2\");\n+my $r2 = Git->repository();\n+is($r2->repo_path, $abs_repo_dir . \"/.git\", \"repo_path (2)\");\n+is($r2->wc_path, $abs_repo_dir . \"/\", \"wc_path (2)\");\n+is($r2->wc_subdir, \"directory2/\", \"wc_subdir initial (2)\");\n+\n+# commands in sub directory\n+my $last_commit = $r2->command_oneline(qw(rev-parse --verify HEAD));\n+like($last_commit, qr/^[0-9a-fA-F]{40}$/, 'rev-parse returned hash');\n+my $dir_commit = $r2->command_oneline('log', '-n1', '--pretty=format:%H', '.');\n+isnt($last_commit, $dir_commit, 'log . does not show last commit');\n-- \n1.6.2.1\n"},{"id":"113248","messageId":"1241703688-6892-2-git-send-email-frank@lichtenheld.de","threadId":"19238","inReplyTo":"1241703688-6892-1-git-send-email-frank@lichtenheld.de","subject":"[PATCH RESEND] Git.pm: Always set Repository to absolute path if autodetecting","fromName":"Frank Lichtenheld","fromEmail":"frank@lichtenheld.de","sentAt":"2009-05-07T13:41:28Z","receivedAt":"2009-05-07T13:41:28Z","isPatch":true,"sender":{"key":"frank@lichtenheld.de","avatar":"https://gravatar.com/avatar/b9f1d4b120e138f157c9e480d0818197c474628923786adb98f30017cdb99c3c?d=mp&s=160"},"body":"From: Frank Lichtenheld <flichtenheld@astaro.com>\n\nSo far we only set it to absolute paths in some cases which lead\nto problems like wc_chdir not working.\n\nSigned-off-by: Frank Lichtenheld <flichtenheld@astaro.com>\n---\n perl/Git.pm     |    2 +-\n t/t9700/test.pl |   10 ++--------\n 2 files changed, 3 insertions(+), 9 deletions(-)\n\nResent unchanged. There was one comment which I've reponded too and\nargued that it didn't apply and there was no further objections.\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 4313db7..e8df55d 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -185,7 +185,7 @@ sub repository {\n \n \t\tif ($dir) {\n \t\t\t$dir =~ m#^/# or $dir = $opts{Directory} . '/' . $dir;\n-\t\t\t$opts{Repository} = $dir;\n+\t\t\t$opts{Repository} = abs_path($dir);\n \n \t\t\t# If --git-dir went ok, this shouldn't die either.\n \t\t\tmy $prefix = $search->command_oneline('rev-parse', '--show-prefix');\ndiff --git a/t/t9700/test.pl b/t/t9700/test.pl\nindex d9b29ea..6c70aec 100755\n--- a/t/t9700/test.pl\n+++ b/t/t9700/test.pl\n@@ -86,18 +86,12 @@ close TEMPFILE;\n unlink $tmpfile;\n \n # paths\n-is($r->repo_path, \"./.git\", \"repo_path\");\n+is($r->repo_path, $abs_repo_dir . \"/.git\", \"repo_path\");\n is($r->wc_path, $abs_repo_dir . \"/\", \"wc_path\");\n is($r->wc_subdir, \"\", \"wc_subdir initial\");\n $r->wc_chdir(\"directory1\");\n is($r->wc_subdir, \"directory1\", \"wc_subdir after wc_chdir\");\n-TODO: {\n-\tlocal $TODO = \"commands do not work after wc_chdir\";\n-\t# Failure output is active even in non-verbose mode and thus\n-\t# annoying.  Hence we skip these tests as long as they fail.\n-\ttodo_skip 'config after wc_chdir', 1;\n-\tis($r->config(\"color.string\"), \"value\", \"config after wc_chdir\");\n-}\n+is($r->config(\"test.string\"), \"value\", \"config after wc_chdir\");\n \n # Object generation in sub directory\n chdir(\"directory2\");\n-- \n1.6.2.1\n"},{"id":"113250","messageId":"20090507194047.GA17989@machine.or.cz","threadId":"19238","inReplyTo":"1241703688-6892-1-git-send-email-frank@lichtenheld.de","subject":"Re: [PATCH RESEND] Git.pm: Set GIT_WORK_TREE if we set GIT_DIR","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2009-05-07T19:40:47Z","receivedAt":"2009-05-07T19:40:47Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Thu, May 07, 2009 at 03:41:27PM +0200, Frank Lichtenheld wrote:\n> From: Frank Lichtenheld <flichtenheld@astaro.com>\n> \n> Otherwise git will use the current directory as work tree which will\n> lead to unexpected results if we operate in sub directory of the\n> work tree.\n> \n> Signed-off-by: Frank Lichtenheld <flichtenheld@astaro.com>\n> ---\n>  perl/Git.pm         |    2 ++\n>  t/t9700-perl-git.sh |    4 ++++\n>  t/t9700/test.pl     |   13 +++++++++++++\n>  3 files changed, 19 insertions(+), 0 deletions(-)\n> \n> No comments and doesn't seem to have been applied, so resent unchanged.\n> \n> diff --git a/perl/Git.pm b/perl/Git.pm\n> index 291ff5b..4313db7 100644\n> --- a/perl/Git.pm\n> +++ b/perl/Git.pm\n> @@ -1280,6 +1280,8 @@ sub _cmd_exec {\n>  \tmy ($self, @args) = @_;\n>  \tif ($self) {\n>  \t\t$self->repo_path() and $ENV{'GIT_DIR'} = $self->repo_path();\n> +\t\t$self->repo_path() and $self->wc_path()\n> +\t\t\tand $ENV{'GIT_WORK_TREE'} = $self->wc_path();\n>  \t\t$self->wc_path() and chdir($self->wc_path());\n>  \t\t$self->wc_subdir() and chdir($self->wc_subdir());\n>  \t}\n\nThis looks obviously correct?\n\nYou could even skip the first chdir and use $self->wc_path() .\n$self->wc_subdir() in the second one to save a syscall, I guess. ;-)\n\nI've really forgot most of the code already so it's not worth much, but\n\nAcked-by: Petr Baudis <pasky@suse.cz>\n"},{"id":"114629","messageId":"4A1A49C0.7040102@viscovery.net","threadId":"19238","inReplyTo":"1241703688-6892-2-git-send-email-frank@lichtenheld.de","subject":"Re: [PATCH RESEND] Git.pm: Always set Repository to absolute path if autodetecting","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-05-25T07:33:20Z","receivedAt":"2009-05-25T07:33:20Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Frank Lichtenheld schrieb:\n> From: Frank Lichtenheld <flichtenheld@astaro.com>\n> \n> So far we only set it to absolute paths in some cases which lead\n> to problems like wc_chdir not working.\n> \n> Signed-off-by: Frank Lichtenheld <flichtenheld@astaro.com>\n> ---\n>  perl/Git.pm     |    2 +-\n>  t/t9700/test.pl |   10 ++--------\n>  2 files changed, 3 insertions(+), 9 deletions(-)\n> \n> Resent unchanged. There was one comment which I've reponded too and\n> argued that it didn't apply and there was no further objections.\n> \n> diff --git a/perl/Git.pm b/perl/Git.pm\n> index 4313db7..e8df55d 100644\n> --- a/perl/Git.pm\n> +++ b/perl/Git.pm\n> @@ -185,7 +185,7 @@ sub repository {\n>  \n>  \t\tif ($dir) {\n>  \t\t\t$dir =~ m#^/# or $dir = $opts{Directory} . '/' . $dir;\n> -\t\t\t$opts{Repository} = $dir;\n> +\t\t\t$opts{Repository} = abs_path($dir);\n\nUnfortunately, this change breaks MinGW git because the absolute path that\nthis produces is MSYS-style /c/path/to/repo, but git does not understand\nthis; it should be c:/path/to/repo. This value is ultimately assigned to\nGIT_DIR, but the path name mangling that usually happens when an MSYS\nprogram (like perl) spawns a non-MSYS program (like git) does not happen.\n\nYour commit message is quite vague about the problems that you have seen.\nI vote to revert this change.\n\n-- Hannes\n"},{"id":"114793","messageId":"20090527105454.GW17706@mail-vs.djpig.de","threadId":"19238","inReplyTo":"4A1A49C0.7040102@viscovery.net","subject":"Re: [PATCH RESEND] Git.pm: Always set Repository to absolute path if autodetecting","fromName":"Frank Lichtenheld","fromEmail":"frank@lichtenheld.de","sentAt":"2009-05-27T10:54:55Z","receivedAt":"2009-05-27T10:54:55Z","isPatch":true,"sender":{"key":"frank@lichtenheld.de","avatar":"https://gravatar.com/avatar/b9f1d4b120e138f157c9e480d0818197c474628923786adb98f30017cdb99c3c?d=mp&s=160"},"body":"On Mon, May 25, 2009 at 09:33:20AM +0200, Johannes Sixt wrote:\n> Frank Lichtenheld schrieb:\n> > From: Frank Lichtenheld <flichtenheld@astaro.com>\n> > \n> > So far we only set it to absolute paths in some cases which lead\n> > to problems like wc_chdir not working.\n> > \n> > Signed-off-by: Frank Lichtenheld <flichtenheld@astaro.com>\n> > ---\n> >  perl/Git.pm     |    2 +-\n> >  t/t9700/test.pl |   10 ++--------\n> >  2 files changed, 3 insertions(+), 9 deletions(-)\n> > \n> > Resent unchanged. There was one comment which I've reponded too and\n> > argued that it didn't apply and there was no further objections.\n> > \n> > diff --git a/perl/Git.pm b/perl/Git.pm\n> > index 4313db7..e8df55d 100644\n> > --- a/perl/Git.pm\n> > +++ b/perl/Git.pm\n> > @@ -185,7 +185,7 @@ sub repository {\n> >  \n> >  \t\tif ($dir) {\n> >  \t\t\t$dir =~ m#^/# or $dir = $opts{Directory} . '/' . $dir;\n> > -\t\t\t$opts{Repository} = $dir;\n> > +\t\t\t$opts{Repository} = abs_path($dir);\n> \n> Unfortunately, this change breaks MinGW git because the absolute path that\n> this produces is MSYS-style /c/path/to/repo, but git does not understand\n> this; it should be c:/path/to/repo. This value is ultimately assigned to\n> GIT_DIR, but the path name mangling that usually happens when an MSYS\n> program (like perl) spawns a non-MSYS program (like git) does not happen.\n> \n> Your commit message is quite vague about the problems that you have seen.\n> I vote to revert this change.\n\nNote that abs_path is already used twice in the same function. Why are those\nusages not problematic? I would be happy to work with you on finding a patch\nthat doesn't break, but I have to admit that I have no idea of the\nWindows<->Perl<->git interactions.\n\nAs for the problems, a part of the public API of the module simply doesn't work\n(i.e. wc_chdir) which I fixed. If we can't fix it we should at least not pretend\nthat it works.\n\nGruesse,\n-- \nFrank Lichtenheld <frank@lichtenheld.de>\nwww: http://www.djpig.de/\n"},{"id":"114791","messageId":"4A1D2100.5040903@viscovery.net","threadId":"19238","inReplyTo":"20090527105454.GW17706@mail-vs.djpig.de","subject":"Re: [PATCH RESEND] Git.pm: Always set Repository to absolute path if autodetecting","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-05-27T11:16:16Z","receivedAt":"2009-05-27T11:16:16Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Frank Lichtenheld schrieb:\n> On Mon, May 25, 2009 at 09:33:20AM +0200, Johannes Sixt wrote:\n>> Frank Lichtenheld schrieb:\n>>> --- a/perl/Git.pm\n>>> +++ b/perl/Git.pm\n>>> @@ -185,7 +185,7 @@ sub repository {\n>>>  \n>>>  \t\tif ($dir) {\n>>>  \t\t\t$dir =~ m#^/# or $dir = $opts{Directory} . '/' . $dir;\n>>> -\t\t\t$opts{Repository} = $dir;\n>>> +\t\t\t$opts{Repository} = abs_path($dir);\n>> Unfortunately, this change breaks MinGW git because the absolute path that\n>> this produces is MSYS-style /c/path/to/repo, but git does not understand\n>> this; it should be c:/path/to/repo. This value is ultimately assigned to\n>> GIT_DIR, but the path name mangling that usually happens when an MSYS\n>> program (like perl) spawns a non-MSYS program (like git) does not happen.\n>>\n>> Your commit message is quite vague about the problems that you have seen.\n>> I vote to revert this change.\n> \n> Note that abs_path is already used twice in the same function. Why are those\n> usages not problematic? I would be happy to work with you on finding a patch\n> that doesn't break, but I have to admit that I have no idea of the\n> Windows<->Perl<->git interactions.\n\nThe result of abs_path() three lines below the cited context is never\npassed to git; only its trailing part is ever used. This does not seem to\nbe problematic on Windows, according to the test suite.\n\nThe other use if abs_path() is about bare repositories and that is\ncertainly problematic, but nobody uses the tools written in perl in a bare\nrepository on Windows, obviously, otherwise we would have heard complaints. ;)\n\n> As for the problems, a part of the public API of the module simply doesn't work\n> (i.e. wc_chdir) which I fixed. If we can't fix it we should at least not pretend\n> that it works.\n\nSince you keep repeating \"does not work\", without any specifics, I can't\nhelp (and I'm not going to find out myself what \"does not work\").\n\n-- Hannes\n"},{"id":"114799","messageId":"20090527134617.GY17706@mail-vs.djpig.de","threadId":"19238","inReplyTo":"4A1D2100.5040903@viscovery.net","subject":"Re: [PATCH RESEND] Git.pm: Always set Repository to absolute path if autodetecting","fromName":"Frank Lichtenheld","fromEmail":"frank@lichtenheld.de","sentAt":"2009-05-27T13:46:17Z","receivedAt":"2009-05-27T13:46:17Z","isPatch":true,"sender":{"key":"frank@lichtenheld.de","avatar":"https://gravatar.com/avatar/b9f1d4b120e138f157c9e480d0818197c474628923786adb98f30017cdb99c3c?d=mp&s=160"},"body":"On Wed, May 27, 2009 at 01:16:16PM +0200, Johannes Sixt wrote:\n> Frank Lichtenheld schrieb:\n> > As for the problems, a part of the public API of the module simply doesn't work\n> > (i.e. wc_chdir) which I fixed. If we can't fix it we should at least not pretend\n> > that it works.\n> \n> Since you keep repeating \"does not work\", without any specifics, I can't\n> help (and I'm not going to find out myself what \"does not work\").\n\nOh, sorry, I thought that the core problem would be obvious from the related test\nsuite changes. I can elaborate on that later this evening when I'm not at work.\n\nGruesse,\n-- \nFrank Lichtenheld <frank@lichtenheld.de>\nwww: http://www.djpig.de/\n"}]}