* [PATCH] Do not trust PWD blindly
[not found] <CABNJ2GKgzXGDq9FhKcVP380bs=rEKqYdrOaCb+A99_TBm7A4_A@mail.gmail.com>
@ 2011-07-09 17:38 ` Johannes Schindelin
2011-07-09 20:06 ` Sebastian Schuberth
2011-07-10 20:47 ` Johannes Sixt
0 siblings, 2 replies; 8+ messages in thread
From: Johannes Schindelin @ 2011-07-09 17:38 UTC (permalink / raw)
To: Pat Thoyts, gitster, j6t; +Cc: msysGit, Sebastian Schuberth, git
At least on Windows, chdir() does not update PWD. Unfortunately, stat()
does not fill any ino or dev fields anymore, so get_pwd_cwd() is not
able to tell.
But there is a telltale: both ino and dev are 0 when they are not filled
correctly, so let's be extra cautious.
This happens to fix a bug in "get-receive-pack working_directory/" when
the GIT_DIR would not be set correctly due to absolute_path(".")
returning the wrong value.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
On Fri, 8 Jul 2011, Pat Thoyts wrote:
> ! t5516-fetch-push (60 receive.denyCurrentBranch = updateInstead)
This patch fixes that.
Hannes, I have no idea whether you meant 10c4c881 to fix anything
on Windows.
abspath.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/abspath.c b/abspath.c
index 01858eb..37287f8 100644
--- a/abspath.c
+++ b/abspath.c
@@ -102,7 +102,8 @@ static const char *get_pwd_cwd(void)
pwd = getenv("PWD");
if (pwd && strcmp(pwd, cwd)) {
stat(cwd, &cwd_stat);
- if (!stat(pwd, &pwd_stat) &&
+ if ((cwd_stat.st_dev || cwd_stat.st_ino) &&
+ !stat(pwd, &pwd_stat) &&
pwd_stat.st_dev == cwd_stat.st_dev &&
pwd_stat.st_ino == cwd_stat.st_ino) {
strlcpy(cwd, pwd, PATH_MAX);
--
1.7.6.rc0.4047.g15f89
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] Do not trust PWD blindly
2011-07-09 17:38 ` [PATCH] Do not trust PWD blindly Johannes Schindelin
@ 2011-07-09 20:06 ` Sebastian Schuberth
2011-07-10 20:47 ` Johannes Sixt
1 sibling, 0 replies; 8+ messages in thread
From: Sebastian Schuberth @ 2011-07-09 20:06 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Pat Thoyts, gitster, j6t, msysGit, git
On Sat, Jul 9, 2011 at 19:38, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Fri, 8 Jul 2011, Pat Thoyts wrote:
>
> > ! t5516-fetch-push (60 receive.denyCurrentBranch = updateInstead)
>
> This patch fixes that.
I can confirm that the patch fixes test 5516 on Windows. Thanks Dscho!
--
Sebastian Schuberth
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Do not trust PWD blindly
2011-07-09 17:38 ` [PATCH] Do not trust PWD blindly Johannes Schindelin
2011-07-09 20:06 ` Sebastian Schuberth
@ 2011-07-10 20:47 ` Johannes Sixt
2011-07-10 22:59 ` Johannes Schindelin
1 sibling, 1 reply; 8+ messages in thread
From: Johannes Sixt @ 2011-07-10 20:47 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Pat Thoyts, gitster, msysGit, Sebastian Schuberth, git
Am 09.07.2011 19:38, schrieb Johannes Schindelin:
>
> At least on Windows, chdir() does not update PWD.
Very strange wording. chdir() should not update PWD even on POSIX.
> Unfortunately, stat()
> does not fill any ino or dev fields anymore, so get_pwd_cwd() is not
> able to tell.
>
> But there is a telltale: both ino and dev are 0 when they are not filled
> correctly, so let's be extra cautious.
>
> This happens to fix a bug in "get-receive-pack working_directory/" when
> the GIT_DIR would not be set correctly due to absolute_path(".")
> returning the wrong value.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>
> On Fri, 8 Jul 2011, Pat Thoyts wrote:
>
> > ! t5516-fetch-push (60 receive.denyCurrentBranch = updateInstead)
>
> This patch fixes that.
>
> Hannes, I have no idea whether you meant 10c4c881 to fix anything
> on Windows.
I think this fix worked for me because when git is called from CMD, PWD
is not in the enviornment and the if (pwd && ...) branch is never taken.
>
> abspath.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/abspath.c b/abspath.c
> index 01858eb..37287f8 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -102,7 +102,8 @@ static const char *get_pwd_cwd(void)
> pwd = getenv("PWD");
> if (pwd && strcmp(pwd, cwd)) {
> stat(cwd, &cwd_stat);
> - if (!stat(pwd, &pwd_stat) &&
> + if ((cwd_stat.st_dev || cwd_stat.st_ino) &&
> + !stat(pwd, &pwd_stat) &&
> pwd_stat.st_dev == cwd_stat.st_dev &&
> pwd_stat.st_ino == cwd_stat.st_ino) {
> strlcpy(cwd, pwd, PATH_MAX);
Acked-by: Johannes Sixt <j6t@kdbg.org>
-- Hannes
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Do not trust PWD blindly
2011-07-10 20:47 ` Johannes Sixt
@ 2011-07-10 22:59 ` Johannes Schindelin
2011-07-11 1:52 ` Randal L. Schwartz
0 siblings, 1 reply; 8+ messages in thread
From: Johannes Schindelin @ 2011-07-10 22:59 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Pat Thoyts, gitster, msysGit, Sebastian Schuberth, git
Hi,
On Sun, 10 Jul 2011, Johannes Sixt wrote:
> Am 09.07.2011 19:38, schrieb Johannes Schindelin:
> >
> > At least on Windows, chdir() does not update PWD.
>
> Very strange wording. chdir() should not update PWD even on POSIX.
Well, you might think it is strange wording. But then, it expresses
exactly what I meant it to say. chdir() does not update PWD on Windows.
You might be very surprised, but that is not true on the Linux system
where one of the 4msysgit.git test cases does _not_ break, while it does
on Windows.
I hoped to make that clear with the wording, but apparently I failed
rather blatantly.
All the more surprising do I find that:
> Acked-by: Johannes Sixt <j6t@kdbg.org>
Ciao,
Johannes
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Do not trust PWD blindly
2011-07-10 22:59 ` Johannes Schindelin
@ 2011-07-11 1:52 ` Randal L. Schwartz
2011-07-11 9:26 ` Johannes Schindelin
0 siblings, 1 reply; 8+ messages in thread
From: Randal L. Schwartz @ 2011-07-11 1:52 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Johannes Sixt, Pat Thoyts, gitster, msysGit, Sebastian Schuberth, git
>>>>> "Johannes" == Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Johannes> You might be very surprised, but that is not true on the Linux system
Johannes> where one of the 4msysgit.git test cases does _not_ break, while it does
Johannes> on Windows.
If you ever depend on a userspace PWD to be your actual current
directory without at least stat()ing it, you've failed.
In my experience, it is *never* reliable. It's just a hint.
--
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Smalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.
See http://methodsandmessages.posterous.com/ for Smalltalk discussion
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Do not trust PWD blindly
2011-07-11 1:52 ` Randal L. Schwartz
@ 2011-07-11 9:26 ` Johannes Schindelin
2011-07-11 16:56 ` Junio C Hamano
0 siblings, 1 reply; 8+ messages in thread
From: Johannes Schindelin @ 2011-07-11 9:26 UTC (permalink / raw)
To: Randal L. Schwartz
Cc: Johannes Sixt, Pat Thoyts, gitster, msysGit, Sebastian Schuberth, git
Hi Randal,
On Sun, 10 Jul 2011, Randal L. Schwartz wrote:
> >>>>> "Johannes" == Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> Johannes> You might be very surprised, but that is not true on the Linux
> Johannes> system where one of the 4msysgit.git test cases does _not_
> Johannes> break, while it does on Windows.
>
> If you ever depend on a userspace PWD to be your actual current
> directory without at least stat()ing it, you've failed.
>
> In my experience, it is *never* reliable. It's just a hint.
To be precise, get_pwd_cwd() _does_ stat() what's in PWD, and _does_
compare with the stat() of what comes out of getcwd(), but that comparison
uses only st_dev and st_ino, both of which happen to be 0 in my case --
for each and every file/directory.
I can only _guess_ at the reasoning behind get_pwd_cwd(). I _think_ it was
meant to catch the case when getcwd() and PWD refer to the same directory,
but PWD goes through symbolic links. I was tempted to just throw that PWD
handling out for Windows, since we do not have symbolic link handling yet.
But that is currently actively discussed, so we might need it in the
future, in which case I have to figure out how to fake reliable st_dev and
st_ino values into our stat() code.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Do not trust PWD blindly
2011-07-11 9:26 ` Johannes Schindelin
@ 2011-07-11 16:56 ` Junio C Hamano
2011-07-11 17:18 ` Johannes Schindelin
0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2011-07-11 16:56 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Randal L. Schwartz, Johannes Sixt, Pat Thoyts, msysGit,
Sebastian Schuberth, git
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Sun, 10 Jul 2011, Randal L. Schwartz wrote:
> ...
>> If you ever depend on a userspace PWD to be your actual current
>> directory without at least stat()ing it, you've failed.
>>
>> In my experience, it is *never* reliable. It's just a hint.
>
> To be precise, get_pwd_cwd() _does_ stat() what's in PWD, and _does_
> compare with the stat() of what comes out of getcwd(), but that comparison
> uses only st_dev and st_ino, both of which happen to be 0 in my case --
> for each and every file/directory.
>
> I can only _guess_ at the reasoning behind get_pwd_cwd(). I _think_ it was
> meant to catch the case when getcwd() and PWD refer to the same directory,
> but PWD goes through symbolic links.
Thanks for a much clearer explanation than before. I tried to reword the
proposed commit log message using the description above.
I feel that the title is still not optimal. If the original code used to
return getenv("PWD") if the environment variable is set, and otherwise
fell back to getcwd(), and the updated code tries to make sure they refer
to the same directory, then "Do not trust PWD blindly" would be a good
description for the fix, but the code you fixed the bug in tried not to
trust PWD blindly but failed to realize that on some systems dev/ino field
may be unreliable.
"Do not trust st.st_ino/st.st_dev blindly" might be a better title in that
sense.
In any case, thanks for a fix; will queue.
Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Date: Sat Jul 9 19:38:08 2011 +0200
Do not trust PWD blindly
10c4c88 (Allow add_path() to add non-existent directories to the path,
2008-07-21) introduced get_pwd_cwd() function in order to favor $PWD when
getenv("PWD") and getcwd() refer to the same directory but are different
strings (e.g. the former gives a nicer looking name via a symbolic link to
an uglier looking automounted path). The function tried to determine if
two directories are the same by running stat(2) on both and comparing
ino/dev fields.
Unfortunately, stat() does not fill any ino or dev fields in msysgit. But
there is a telltale: both ino and dev are 0 when they are not filled
correctly, so let's be extra cautious.
This happens to fix a bug in "get-receive-pack working_directory/" when
the GIT_DIR would not be set correctly due to absolute_path(".")
returning the wrong value.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Acked-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Do not trust PWD blindly
2011-07-11 16:56 ` Junio C Hamano
@ 2011-07-11 17:18 ` Johannes Schindelin
0 siblings, 0 replies; 8+ messages in thread
From: Johannes Schindelin @ 2011-07-11 17:18 UTC (permalink / raw)
To: Junio C Hamano
Cc: Randal L. Schwartz, Johannes Sixt, Pat Thoyts, msysGit,
Sebastian Schuberth, git
Hi,
On Mon, 11 Jul 2011, Junio C Hamano wrote:
> "Do not trust st.st_ino/st.st_dev blindly" might be a better title in
> that sense.
Maybe prefix it with "get_pwd_cwd(): "?
> In any case, thanks for a fix; will queue.
Thanks!
Johannes
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-07-11 17:18 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CABNJ2GKgzXGDq9FhKcVP380bs=rEKqYdrOaCb+A99_TBm7A4_A@mail.gmail.com>
2011-07-09 17:38 ` [PATCH] Do not trust PWD blindly Johannes Schindelin
2011-07-09 20:06 ` Sebastian Schuberth
2011-07-10 20:47 ` Johannes Sixt
2011-07-10 22:59 ` Johannes Schindelin
2011-07-11 1:52 ` Randal L. Schwartz
2011-07-11 9:26 ` Johannes Schindelin
2011-07-11 16:56 ` Junio C Hamano
2011-07-11 17:18 ` Johannes Schindelin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.