patchDo not trust PWD blindly

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

On Sat, Jul 9, 2011 at 19:38, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

I can confirm that the patch fixes test 5516 on Windows. Thanks Dscho!

Am 09.07.2011 19:38, schrieb Johannes Schindelin:

Very strange wording. chdir() should not update PWD even on POSIX.

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.

Acked-by: Johannes Sixt <j6t@kdbg.org>

-- Hannes

Hi,

On Sun, 10 Jul 2011, Johannes Sixt wrote:

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:

Ciao, Johannes

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.

Hi Randal,

On Sun, 10 Jul 2011, Randal L. Schwartz wrote:

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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>

Hi,

On Mon, 11 Jul 2011, Junio C Hamano wrote:

Maybe prefix it with "get_pwd_cwd(): "?

Thanks! Johannes