[PATCH] "sleep 1" sleeps too little on cygwin

[PATCH] "sleep 1" sleeps too little on cygwin

[Alex Riesen]

[-- Attachment #1: Type: text/plain, Size: 115 bytes --]

Probably another one windows quirk, or just the moon phases,
but I have to make damn sure it sleeps long enough.

[-- Attachment #2: cygwin-sleeps-too-little.patch --]
[-- Type: text/x-patch, Size: 530 bytes --]

diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh
index e3ebf38..8c5b614 100755
--- a/t/t4011-diff-symlink.sh
+++ b/t/t4011-diff-symlink.sh
@@ -56,10 +56,11 @@ cat > expected << EOF
 diff --git a/frotz b/frotz
 EOF
 
+secs="$SECONDS"
+while [ "$SECONDS" = "$secs" ]; do sleep 1; done
 test_expect_success \
     'diff identical, but newly created symlink' \
-    'sleep 1 &&
-    ln -s xyzzy frotz &&
+    'ln -s xyzzy frotz &&
     git-diff-index -M -p $tree > current &&
     compare_diff_patch current expected'
 

Re: [PATCH] "sleep 1" sleeps too little on cygwin

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Probably another one windows quirk, or just the moon phases,
> but I have to make damn sure it sleeps long enough.

IIRC, DOS file timestamps have 2 seconds granularity, so
sleeping for one second might not be enough to begin with.  Also
I run my cygwin test on sufficiently slow machine, so that may
explain why I have not notice the problem ;-).

Instead of depending on $SECONDS (isn't it a bashism?), how
about doing something like this?

---
diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh
index e3ebf38..9a5fa38 100755
--- a/t/t4011-diff-symlink.sh
+++ b/t/t4011-diff-symlink.sh
@@ -23,6 +23,7 @@ EOF
 test_expect_success \
     'diff new symlink' \
     'ln -s xyzzy frotz &&
+    touch -t 199901010000 frotz &&
     git-update-index &&
     tree=$(git-write-tree) &&
     git-update-index --add frotz &&
@@ -58,8 +59,8 @@ EOF
 
 test_expect_success \
     'diff identical, but newly created symlink' \
-    'sleep 1 &&
-    ln -s xyzzy frotz &&
+    'ln -s xyzzy frotz &&
+    touch -t 200201010000 frotz &&
     git-diff-index -M -p $tree > current &&
     compare_diff_patch current expected'
 

Re: [PATCH] "sleep 1" sleeps too little on cygwin

[Alex Riesen]

On 1/18/06, Junio C Hamano <junkio@cox.net> wrote:
> Instead of depending on $SECONDS (isn't it a bashism?), how
> about doing something like this?

Probably not, even QNX6 ksh has it, and being the most broken shell
I ever met, I'd consider $SECONDS safe 8-)

Re: [PATCH] "sleep 1" sleeps too little on cygwin

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> On 1/18/06, Junio C Hamano <junkio@cox.net> wrote:
>> Instead of depending on $SECONDS (isn't it a bashism?), how
>> about doing something like this?
>
> Probably not, even QNX6 ksh has it, and being the most broken shell
> I ever met, I'd consider $SECONDS safe 8-)

When I have to answer these questions myself, I mostly do so by
checking http://www.opengroup.org/onlinepubs/009695399/.

Re: [PATCH] "sleep 1" sleeps too little on cygwin

[Christopher Faylor]

On Tue, Jan 17, 2006 at 05:41:34PM -0800, Junio C Hamano wrote:
>Alex Riesen <raa.lkml@gmail.com> writes:
>>Probably another one windows quirk, or just the moon phases, but I have
>>to make damn sure it sleeps long enough.
>
>IIRC, DOS file timestamps have 2 seconds granularity, so sleeping for
>one second might not be enough to begin with.  Also I run my cygwin
>test on sufficiently slow machine, so that may explain why I have not
>notice the problem ;-).

Cygwin really does sleep a second if you tell it to, but on FAT
filesystems, you're right, the granularity something like 2 seconds.

cgf

Re: [PATCH] "sleep 1" sleeps too little on cygwin

[Junio C Hamano]

Sorry, but the previous patch turns out to be bogus as well.
'touch' did not affect the timestamp of the symbolic link but
ended up creating a file pointed by it, and since we (hopefully)
correctly do lstat() not stat(), it did not have any good
effect.  Here is an replacement.

-- >8 --
Subject: [PATCH] t4011: "sleep 1" sleeps too little on cygwin

This test depended on "sleep 1" not to return until the next
second boundary, to get a dirty index entry.  An initial fix was
provided by Alex Riesen using a bashism $SECONDS, but make it a
bit more portable, and also work around a potential problem on a
filesystem with coarser-than-a-second timestamp granularity,
this patch fixes it a bit differently.

The test is checking if we correctly notice the stat changes,
and the timestamp is not the only thing we have in the stat part
of the index.  This commit fixes the problem by leaving the
original symlink we create on the filesystem, so that it keeps
the inode number used for the original symlink in use, and then
create a second one which should get a different inode number.

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

 t/t4011-diff-symlink.sh |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

5f6849e7b60bc4908bf10a7946fd93d4aaaf9c35
diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh
index e3ebf38..f0e3491 100755
--- a/t/t4011-diff-symlink.sh
+++ b/t/t4011-diff-symlink.sh
@@ -48,7 +48,7 @@ EOF
 
 test_expect_success \
     'diff removed symlink' \
-    'rm frotz &&
+    'mv frotz nitfol &&
     git-diff-index -M -p $tree > current &&
     compare_diff_patch current expected'
 
@@ -58,8 +58,7 @@ EOF
 
 test_expect_success \
     'diff identical, but newly created symlink' \
-    'sleep 1 &&
-    ln -s xyzzy frotz &&
+    'ln -s xyzzy frotz &&
     git-diff-index -M -p $tree > current &&
     compare_diff_patch current expected'
 
-- 
1.1.3.gce7b

Re: [PATCH] "sleep 1" sleeps too little on cygwin

[Johannes Schindelin]

Hi,

On Wed, 18 Jan 2006, Junio C Hamano wrote:

> @@ -48,7 +48,7 @@ EOF
>  
>  test_expect_success \
>      'diff removed symlink' \
> -    'rm frotz &&
> +    'mv frotz nitfol &&
>      git-diff-index -M -p $tree > current &&
>      compare_diff_patch current expected'
>  
> @@ -58,8 +58,7 @@ EOF
>  
>  test_expect_success \
>      'diff identical, but newly created symlink' \
> -    'sleep 1 &&
> -    ln -s xyzzy frotz &&
> +    'ln -s xyzzy frotz &&

And where does the time lag (required for the test) come from now?

Ciao,
Dscho

Re: [PATCH] "sleep 1" sleeps too little on cygwin

[Junio C Hamano]

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

> And where does the time lag (required for the test) come from now?

The point of the change is that it avoids to depend on the time
lag.  The test is to make sure we catch dirty index; instead of
timestamp difference it now uses i-num difference for that.

Re: [PATCH] "sleep 1" sleeps too little on cygwin

[Alex Riesen]

Junio C Hamano, Wed, Jan 18, 2006 18:00:30 +0100:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > And where does the time lag (required for the test) come from now?
> 
> The point of the change is that it avoids to depend on the time
> lag.  The test is to make sure we catch dirty index; instead of
> timestamp difference it now uses i-num difference for that.
> 

this is probable unsafe too: not all systems export inodes
(the recent cygwin breakage comes to mind).

Re: [PATCH] "sleep 1" sleeps too little on cygwin

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> this is probable unsafe too: not all systems export inodes
> (the recent cygwin breakage comes to mind).

Hmph.  I thought that breakage was about struct dirent, not what
we read from struct stat; d_ino is XSI extension so that may
have been the reason cygwin folks removed it, but st_ino is in
BASE --- did they remove that as well?

But you are right.  Among "struct stat" members, only S_IFMT
part from st_mode and st_size are meaningful for symlinks after
lstat() and other members are unspecified.  Which means that, in
the strictest sense, the original test that tried to see if the
timestamp change makes the entry for the symlink cache-dirty,
was pointless.

HOWEVER.

We live in the real world, not in a strict POSIX world.  We (the
index file) relies on lstat() to update st_mtime and/or st_ino,
and I think it is OK (IOW, we declare that platforms on which we
cannot rely on these members are not worth supporting).


By the way, if you have an access to git on cygwin with FAT,
could you test your patch ($SECONDS) and then i-num patch (the
machine with cygwin I can borrow has only NTFS) please?

Re: [PATCH] "sleep 1" sleeps too little on cygwin

[Christopher Faylor]

On Wed, Jan 18, 2006 at 05:18:50PM -0800, Junio C Hamano wrote:
>Alex Riesen <raa.lkml@gmail.com> writes:
>
>> this is probable unsafe too: not all systems export inodes
>> (the recent cygwin breakage comes to mind).
>
>Hmph.  I thought that breakage was about struct dirent, not what
>we read from struct stat; d_ino is XSI extension so that may
>have been the reason cygwin folks removed it, but st_ino is in
>BASE --- did they remove that as well?

No, we didn't.  We (i.e., I) removed d_ino in cygwin because it couldn't
be reliably calculated without slowing things down.  st_ino will always
be there.

cgf

Re: [PATCH] "sleep 1" sleeps too little on cygwin

[Alex Riesen]

On 1/19/06, Junio C Hamano <junkio@cox.net> wrote:
> By the way, if you have an access to git on cygwin with FAT,
> could you test your patch ($SECONDS) and then i-num patch (the
> machine with cygwin I can borrow has only NTFS) please?

Works if sleep is for 2 secs (I completely forgot about that stupid
FAT granularity!)
st_ino is always the same (it is a hash of pathname).
Christopher, how is that supposed to work with hardlinks? (NTFS has
hardlinks, BTW)

Re: [PATCH] "sleep 1" sleeps too little on cygwin

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Works if sleep is for 2 secs (I completely forgot about that stupid
> FAT granularity!)
> st_ino is always the same (it is a hash of pathname).
> Christopher, how is that supposed to work with hardlinks? (NTFS has
> hardlinks, BTW)

So the verdict is to take your patch but wait for three seconds?
I still have mild aversion about $SECONDS though...

Re: [PATCH] "sleep 1" sleeps too little on cygwin

[Alex Riesen]

Junio C Hamano, Thu, Jan 19, 2006 19:24:27 +0100:
> 
> > Works if sleep is for 2 secs (I completely forgot about that stupid
> > FAT granularity!)
> > st_ino is always the same (it is a hash of pathname).
> > Christopher, how is that supposed to work with hardlinks? (NTFS has
> > hardlinks, BTW)
> 
> So the verdict is to take your patch but wait for three seconds?
> I still have mild aversion about $SECONDS though...
> 

Maybe just wait for 3 (three) seconds? It should guarantee the change
in mtime.

Re: [PATCH] "sleep 1" sleeps too little on cygwin

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Maybe just wait for 3 (three) seconds? It should guarantee the change
> in mtime.

That is certainly an easy way out.  Unless somebody comes up
with more simple and elegant solution I'd opt for this.

Re: [PATCH] "sleep 1" sleeps too little on cygwin

[Christopher Faylor]

On Thu, Jan 19, 2006 at 04:01:19PM +0100, Alex Riesen wrote:
>On 1/19/06, Junio C Hamano <junkio@cox.net> wrote:
>> By the way, if you have an access to git on cygwin with FAT,
>> could you test your patch ($SECONDS) and then i-num patch (the
>> machine with cygwin I can borrow has only NTFS) please?
>
>Works if sleep is for 2 secs (I completely forgot about that stupid
>FAT granularity!)
>st_ino is always the same (it is a hash of pathname).
>Christopher, how is that supposed to work with hardlinks? (NTFS has
>hardlinks, BTW)

There is OS support hardlinks work on NTFS/NT but not on FAT*
"filesystems" or Windows 9x variants.  Hardlink support on Cygwin
mirrors this.

cgf

Re: [PATCH] "sleep 1" sleeps too little on cygwin

[Alex Riesen]

Christopher Faylor, Thu, Jan 19, 2006 19:28:22 +0100:
> >> By the way, if you have an access to git on cygwin with FAT,
> >> could you test your patch ($SECONDS) and then i-num patch (the
> >> machine with cygwin I can borrow has only NTFS) please?
> >
> >Works if sleep is for 2 secs (I completely forgot about that stupid
> >FAT granularity!)
> >st_ino is always the same (it is a hash of pathname).
> >Christopher, how is that supposed to work with hardlinks? (NTFS has
> >hardlinks, BTW)
> 
> There is OS support hardlinks work on NTFS/NT but not on FAT*
> "filesystems" or Windows 9x variants.  Hardlink support on Cygwin
> mirrors this.
> 

No, I rephrase the question: how does the method of calculating
.st_ino from pathname handles hardlinked files on NTFS?
By briefly looking at the code it does not seem to be possible.

Re: [PATCH] "sleep 1" sleeps too little on cygwin

[Christopher Faylor]

On Thu, Jan 19, 2006 at 11:12:27PM +0100, Alex Riesen wrote:
>Christopher Faylor, Thu, Jan 19, 2006 19:28:22 +0100:
>>>>By the way, if you have an access to git on cygwin with FAT, could you
>>>>test your patch ($SECONDS) and then i-num patch (the machine with
>>>>cygwin I can borrow has only NTFS) please?
>>>
>>>Works if sleep is for 2 secs (I completely forgot about that stupid FAT
>>>granularity!) st_ino is always the same (it is a hash of pathname).
>>>Christopher, how is that supposed to work with hardlinks?  (NTFS has
>>>hardlinks, BTW)
>>
>>There is OS support hardlinks work on NTFS/NT but not on FAT*
>>"filesystems" or Windows 9x variants.  Hardlink support on Cygwin
>>mirrors this.
>
>No, I rephrase the question: how does the method of calculating .st_ino
>from pathname handles hardlinked files on NTFS?  By briefly looking at
>the code it does not seem to be possible.

Cygwin doesn't create an inode from the path name on NTFS.  NT gives you
a 64 bit unique number which identifies a file so we use that.  That
means that the inodes of hardlinked files should be the same, just like
on linux.

Inodes are only calculated by hashing the path name when the OS lacks
the support to provide a "real" inode and in that case there is no hard
link support available so it's a non-issue.

cgf

Re: [PATCH] "sleep 1" sleeps too little on cygwin

[Junio C Hamano]

Christopher Faylor <me@cgf.cx> writes:

> Inodes are only calculated by hashing the path name when the OS lacks
> the support to provide a "real" inode and in that case there is no hard
> link support available so it's a non-issue.

Does that mean on such filesystems "mv foo bar" would change the
i-num of the moved entity?

I am not complaining even if that is the case.  I just want to
understand what it does.

Re: [PATCH] "sleep 1" sleeps too little on cygwin

[Christopher Faylor]

On Thu, Jan 19, 2006 at 05:13:17PM -0800, Junio C Hamano wrote:
>Christopher Faylor <me@cgf.cx> writes:
>>Inodes are only calculated by hashing the path name when the OS lacks
>>the support to provide a "real" inode and in that case there is no hard
>>link support available so it's a non-issue.
>
>Does that mean on such filesystems "mv foo bar" would change the i-num
>of the moved entity?

I just tried this on Windows XP.  On a FAT32 or a NTFS filesystem the
inode is unchanged.  On a FAT filesystem, it changes.  I assume that
means that FAT doesn't support a real file ID.  The only thing I think
anyone would be using FAT for these days is possibly a boot partition.
That's the only reason I have one.  I use to to multi-boot various
flavors of Windows.  It's the lowest common denominator.

>I am not complaining even if that is the case.  I just want to
>understand what it does.

NP.  I complain about this fairly frequently myself.  :-)

cgf

Re: [PATCH] "sleep 1" sleeps too little on cygwin

[Alex Riesen]

On 1/20/06, Christopher Faylor <me@cgf.cx> wrote:
> On Thu, Jan 19, 2006 at 05:13:17PM -0800, Junio C Hamano wrote:
> >Christopher Faylor <me@cgf.cx> writes:
> >>Inodes are only calculated by hashing the path name when the OS lacks
> >>the support to provide a "real" inode and in that case there is no hard
> >>link support available so it's a non-issue.
> >
> >Does that mean on such filesystems "mv foo bar" would change the i-num
> >of the moved entity?

It is just a rename. It will not change inode by definition (the file is the
same, it is the name which was changed). To get inode change you
have to create it, like "cp foo bar; rm foo". And now, if a filesystem
has (or exports) inodes, would the inode change. If it does not, you
really can't say anything about the inode (it can even change, and
does in this case, being calculated from pathname).

> I just tried this on Windows XP.  On a FAT32 or a NTFS filesystem the

I have FAT32. It does not support inodes"
    $ : > foo
    $ stat foo |grep node
    Device: 111712fch/286724860d    Inode: 3595119992080318288  Links: 1
    $ mv foo bar
    $ stat bar |grep node
    Device: 111712fch/286724860d    Inode: 3595119974866484701  Links: 1

> inode is unchanged.  On a FAT filesystem, it changes.  I assume that
> means that FAT doesn't support a real file ID.  The only thing I think
> anyone would be using FAT for these days is possibly a boot partition.

It's still faster than NTFS, I use it as a temporary compilation partition,
and as NT is painfully slow about filesystems and throughput in general,
I keep my sources on the partition as well.

> That's the only reason I have one.  I use to to multi-boot various
> flavors of Windows.  It's the lowest common denominator.

Lowliest.

Re: [PATCH] "sleep 1" sleeps too little on cygwin

[Christopher Faylor]

On Fri, Jan 20, 2006 at 04:23:10PM +0100, Alex Riesen wrote:
>On 1/20/06, Christopher Faylor <me@cgf.cx> wrote:
>> On Thu, Jan 19, 2006 at 05:13:17PM -0800, Junio C Hamano wrote:
>> >Christopher Faylor <me@cgf.cx> writes:
>> >>Inodes are only calculated by hashing the path name when the OS lacks
>> >>the support to provide a "real" inode and in that case there is no hard
>> >>link support available so it's a non-issue.
>> >
>> >Does that mean on such filesystems "mv foo bar" would change the i-num
>> >of the moved entity?
>
>It is just a rename. It will not change inode by definition (the file is the
>same, it is the name which was changed). To get inode change you
>have to create it, like "cp foo bar; rm foo". And now, if a filesystem
>has (or exports) inodes, would the inode change. If it does not, you
>really can't say anything about the inode (it can even change, and
>does in this case, being calculated from pathname).
>
>> I just tried this on Windows XP.  On a FAT32 or a NTFS filesystem the
>
>I have FAT32. It does not support inodes"
>    $ : > foo
>    $ stat foo |grep node
>    Device: 111712fch/286724860d    Inode: 3595119992080318288  Links: 1
>    $ mv foo bar
>    $ stat bar |grep node
>    Device: 111712fch/286724860d    Inode: 3595119974866484701  Links: 1

Ok, I must have been hallucinating.  I just tried it again.  It doesn't
work on FAT32.

cgf