{"thread":{"id":"27781","subject":"[PATCH] Do not trust PWD blindly","startedAt":"2011-07-09T17:38:08Z","lastAt":"2011-07-11T17:18:10Z","messageCount":8,"participants":["Johannes Schindelin","Sebastian Schuberth","Johannes Sixt","Randal L. Schwartz","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"171032","messageId":"alpine.DEB.1.00.1107091935210.1985@bonsai2","threadId":"27781","inReplyTo":"CABNJ2GKgzXGDq9FhKcVP380bs=rEKqYdrOaCb+A99_TBm7A4_A@mail.gmail.com","subject":"[PATCH] Do not trust PWD blindly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2011-07-09T17:38:08Z","receivedAt":"2011-07-09T17:38:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nAt least on Windows, chdir() does not update PWD. Unfortunately, stat()\ndoes not fill any ino or dev fields anymore, so get_pwd_cwd() is not\nable to tell.\n\nBut there is a telltale: both ino and dev are 0 when they are not filled\ncorrectly, so let's be extra cautious.\n\nThis happens to fix a bug in \"get-receive-pack working_directory/\" when\nthe GIT_DIR would not be set correctly due to absolute_path(\".\")\nreturning the wrong value.\n\nSigned-off-by: Johannes Schindelin \n---\n\n\tOn Fri, 8 Jul 2011, Pat Thoyts wrote:\n\n\t> ! t5516-fetch-push (60 receive.denyCurrentBranch = updateInstead)\n\n\tThis patch fixes that.\n\n\tHannes, I have no idea whether you meant 10c4c881 to fix anything \n\ton Windows.\n\n abspath.c | 3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/abspath.c b/abspath.c\nindex 01858eb..37287f8 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -102,7 +102,8 @@ static const char *get_pwd_cwd(void)\n \tpwd = getenv(\"PWD\");\n \tif (pwd && strcmp(pwd, cwd)) {\n \t\tstat(cwd, &cwd_stat);\n-\t\tif (!stat(pwd, &pwd_stat) &&\n+\t\tif ((cwd_stat.st_dev || cwd_stat.st_ino) &&\n+\t\t !stat(pwd, &pwd_stat) &&\n \t\t pwd_stat.st_dev == cwd_stat.st_dev &&\n \t\t pwd_stat.st_ino == cwd_stat.st_ino) {\n \t\t\tstrlcpy(cwd, pwd, PATH_MAX);\n-- \n1.7.6.rc0.4047.g15f89\n"},{"id":"171033","messageId":"CAHGBnuO_p8WfBowiBw=4t8432nc0UvpNS_emWZEYBn1zNY+c5Q@mail.gmail.com","threadId":"27781","inReplyTo":"alpine.DEB.1.00.1107091935210.1985@bonsai2","subject":"Re: [PATCH] Do not trust PWD blindly","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2011-07-09T20:06:49Z","receivedAt":"2011-07-09T20:06:49Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Sat, Jul 9, 2011 at 19:38, Johannes Schindelin\n wrote:\n\n>        On Fri, 8 Jul 2011, Pat Thoyts wrote:\n>\n>        > ! t5516-fetch-push      (60 receive.denyCurrentBranch = updateInstead)\n>\n>        This patch fixes that.\n\nI can confirm that the patch fixes test 5516 on Windows. Thanks Dscho!\n\n-- \nSebastian Schuberth\n"},{"id":"171038","messageId":"4E1A0FCC.7080308@kdbg.org","threadId":"27781","inReplyTo":"alpine.DEB.1.00.1107091935210.1985@bonsai2","subject":"Re: [PATCH] Do not trust PWD blindly","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-07-10T20:47:08Z","receivedAt":"2011-07-10T20:47:08Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 09.07.2011 19:38, schrieb Johannes Schindelin:\n> \n> At least on Windows, chdir() does not update PWD.\n\nVery strange wording. chdir() should not update PWD even on POSIX.\n\n> Unfortunately, stat()\n> does not fill any ino or dev fields anymore, so get_pwd_cwd() is not\n> able to tell.\n> \n> But there is a telltale: both ino and dev are 0 when they are not filled\n> correctly, so let's be extra cautious.\n> \n> This happens to fix a bug in \"get-receive-pack working_directory/\" when\n> the GIT_DIR would not be set correctly due to absolute_path(\".\")\n> returning the wrong value.\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> \n> \tOn Fri, 8 Jul 2011, Pat Thoyts wrote:\n> \n> \t> ! t5516-fetch-push (60 receive.denyCurrentBranch = updateInstead)\n> \n> \tThis patch fixes that.\n> \n> \tHannes, I have no idea whether you meant 10c4c881 to fix anything \n> \ton Windows.\n\nI think this fix worked for me because when git is called from CMD, PWD\nis not in the enviornment and the if (pwd && ...) branch is never taken.\n\n> \n> abspath.c | 3 ++-\n> 1 files changed, 2 insertions(+), 1 deletions(-)\n> \n> diff --git a/abspath.c b/abspath.c\n> index 01858eb..37287f8 100644\n> --- a/abspath.c\n> +++ b/abspath.c\n> @@ -102,7 +102,8 @@ static const char *get_pwd_cwd(void)\n> \tpwd = getenv(\"PWD\");\n> \tif (pwd && strcmp(pwd, cwd)) {\n> \t\tstat(cwd, &cwd_stat);\n> -\t\tif (!stat(pwd, &pwd_stat) &&\n> +\t\tif ((cwd_stat.st_dev || cwd_stat.st_ino) &&\n> +\t\t !stat(pwd, &pwd_stat) &&\n> \t\t pwd_stat.st_dev == cwd_stat.st_dev &&\n> \t\t pwd_stat.st_ino == cwd_stat.st_ino) {\n> \t\t\tstrlcpy(cwd, pwd, PATH_MAX);\n\nAcked-by: Johannes Sixt \n\n-- Hannes\n"},{"id":"171040","messageId":"alpine.DEB.1.00.1107110057120.3379@bonsai2","threadId":"27781","inReplyTo":"4E1A0FCC.7080308@kdbg.org","subject":"Re: [PATCH] Do not trust PWD blindly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2011-07-10T22:59:03Z","receivedAt":"2011-07-10T22:59:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 10 Jul 2011, Johannes Sixt wrote:\n\n> Am 09.07.2011 19:38, schrieb Johannes Schindelin:\n> > \n> > At least on Windows, chdir() does not update PWD.\n> \n> Very strange wording. chdir() should not update PWD even on POSIX.\n\nWell, you might think it is strange wording. But then, it expresses \nexactly what I meant it to say. chdir() does not update PWD on Windows.\n\nYou might be very surprised, but that is not true on the Linux system \nwhere one of the 4msysgit.git test cases does _not_ break, while it does \non Windows.\n\nI hoped to make that clear with the wording, but apparently I failed \nrather blatantly.\n\nAll the more surprising do I find that:\n\n> Acked-by: Johannes Sixt \n\nCiao,\nJohannes\n"},{"id":"171042","messageId":"86k4bpporf.fsf@red.stonehenge.com","threadId":"27781","inReplyTo":"alpine.DEB.1.00.1107110057120.3379@bonsai2","subject":"Re: [PATCH] Do not trust PWD blindly","fromName":"Randal L. Schwartz","fromEmail":"merlyn@stonehenge.com","sentAt":"2011-07-11T01:52:36Z","receivedAt":"2011-07-11T01:52:36Z","isPatch":true,"sender":{"key":"merlyn@stonehenge.com","avatar":"https://gravatar.com/avatar/dc528d210743ff0333e6213f9ee7b33b23f1b7bc1f3c5a8c2d819074ecd7ab19?d=mp&s=160"},"body":">>>>> \"Johannes\" == Johannes Schindelin writes:\n\nJohannes> You might be very surprised, but that is not true on the Linux system \nJohannes> where one of the 4msysgit.git test cases does _not_ break, while it does \nJohannes> on Windows.\n\nIf you ever depend on a userspace PWD to be your actual current\ndirectory without at least stat()ing it, you've failed.\n\nIn my experience, it is *never* reliable. It's just a hint.\n\n-- \nRandal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095\n \nSmalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.\nSee http://methodsandmessages.posterous.com/ for Smalltalk discussion\n"},{"id":"171049","messageId":"alpine.DEB.1.00.1107111121390.3379@bonsai2","threadId":"27781","inReplyTo":"86k4bpporf.fsf@red.stonehenge.com","subject":"Re: [PATCH] Do not trust PWD blindly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2011-07-11T09:26:34Z","receivedAt":"2011-07-11T09:26:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Randal,\n\nOn Sun, 10 Jul 2011, Randal L. Schwartz wrote:\n\n> >>>>> \"Johannes\" == Johannes Schindelin writes:\n> \n> Johannes> You might be very surprised, but that is not true on the Linux \n> Johannes> system where one of the 4msysgit.git test cases does _not_ \n> Johannes> break, while it does on Windows.\n> \n> If you ever depend on a userspace PWD to be your actual current \n> directory without at least stat()ing it, you've failed.\n> \n> In my experience, it is *never* reliable. It's just a hint.\n\nTo be precise, get_pwd_cwd() _does_ stat() what's in PWD, and _does_ \ncompare with the stat() of what comes out of getcwd(), but that comparison \nuses only st_dev and st_ino, both of which happen to be 0 in my case -- \nfor each and every file/directory.\n\nI can only _guess_ at the reasoning behind get_pwd_cwd(). I _think_ it was \nmeant to catch the case when getcwd() and PWD refer to the same directory, \nbut PWD goes through symbolic links. I was tempted to just throw that PWD \nhandling out for Windows, since we do not have symbolic link handling yet. \nBut that is currently actively discussed, so we might need it in the \nfuture, in which case I have to figure out how to fake reliable st_dev and \nst_ino values into our stat() code.\n\nCiao,\nDscho\n"},{"id":"171082","messageId":"7vbox0g3hn.fsf@alter.siamese.dyndns.org","threadId":"27781","inReplyTo":"alpine.DEB.1.00.1107111121390.3379@bonsai2","subject":"Re: [PATCH] Do not trust PWD blindly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-11T16:56:52Z","receivedAt":"2011-07-11T16:56:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Sun, 10 Jul 2011, Randal L. Schwartz wrote:\n> ...\n>> If you ever depend on a userspace PWD to be your actual current \n>> directory without at least stat()ing it, you've failed.\n>> \n>> In my experience, it is *never* reliable. It's just a hint.\n>\n> To be precise, get_pwd_cwd() _does_ stat() what's in PWD, and _does_ \n> compare with the stat() of what comes out of getcwd(), but that comparison \n> uses only st_dev and st_ino, both of which happen to be 0 in my case -- \n> for each and every file/directory.\n>\n> I can only _guess_ at the reasoning behind get_pwd_cwd(). I _think_ it was \n> meant to catch the case when getcwd() and PWD refer to the same directory, \n> but PWD goes through symbolic links.\n\nThanks for a much clearer explanation than before. I tried to reword the\nproposed commit log message using the description above.\n\nI feel that the title is still not optimal. If the original code used to\nreturn getenv(\"PWD\") if the environment variable is set, and otherwise\nfell back to getcwd(), and the updated code tries to make sure they refer\nto the same directory, then \"Do not trust PWD blindly\" would be a good\ndescription for the fix, but the code you fixed the bug in tried not to\ntrust PWD blindly but failed to realize that on some systems dev/ino field\nmay be unreliable.\n\n\"Do not trust st.st_ino/st.st_dev blindly\" might be a better title in that\nsense.\n\nIn any case, thanks for a fix; will queue.\n\nAuthor: Johannes Schindelin \nDate: Sat Jul 9 19:38:08 2011 +0200\n\n Do not trust PWD blindly\n \n 10c4c88 (Allow add_path() to add non-existent directories to the path,\n 2008-07-21) introduced get_pwd_cwd() function in order to favor $PWD when\n getenv(\"PWD\") and getcwd() refer to the same directory but are different\n strings (e.g. the former gives a nicer looking name via a symbolic link to\n an uglier looking automounted path). The function tried to determine if\n two directories are the same by running stat(2) on both and comparing\n ino/dev fields.\n \n Unfortunately, stat() does not fill any ino or dev fields in msysgit. But\n there is a telltale: both ino and dev are 0 when they are not filled\n correctly, so let's be extra cautious.\n \n This happens to fix a bug in \"get-receive-pack working_directory/\" when\n the GIT_DIR would not be set correctly due to absolute_path(\".\")\n returning the wrong value.\n \n Signed-off-by: Johannes Schindelin \n Acked-by: Johannes Sixt \n Signed-off-by: Junio C Hamano \n"},{"id":"171086","messageId":"alpine.DEB.1.00.1107111917330.1534@s15462909.onlinehome-server.info","threadId":"27781","inReplyTo":"7vbox0g3hn.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Do not trust PWD blindly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2011-07-11T17:18:10Z","receivedAt":"2011-07-11T17:18:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 11 Jul 2011, Junio C Hamano wrote:\n\n> \"Do not trust st.st_ino/st.st_dev blindly\" might be a better title in \n> that sense.\n\nMaybe prefix it with \"get_pwd_cwd(): \"?\n\n> In any case, thanks for a fix; will queue.\n\nThanks!\nJohannes\n"}]}