Implement file_util::CountFilesCreatedAfter() for posix environments....

Implement file_util::CountFilesCreatedAfter() for posix environments.

BUG=9833

[Evan Martin, 2009-04-21 00:16:37]

Does the Windows version look in subdirectories, too?

http://codereview.chromium.org/87003/diff/1005/8
File base/file_util_posix.cc (right):

http://codereview.chromium.org/87003/diff/1005/8#newcode73
Line 73: LOG(ERROR) << "stat64 failed: " << strerror(errno);
I guess there should be a "continue;" here?

[hamaji, 2009-04-21 00:50:17]

Thanks for the review!

Both Windows version and POSIX version don't look in subdirectories.

http://codereview.chromium.org/87003/diff/1005/8
File base/file_util_posix.cc (right):

http://codereview.chromium.org/87003/diff/1005/8#newcode73
Line 73: LOG(ERROR) << "stat64 failed: " << strerror(errno);
On 2009/04/21 00:16:37, Evan Martin wrote:
> I guess there should be a "continue;" here?

Ah thanks, fixed.

[Evan Martin, 2009-04-21 00:54:29]

minor bug below.  LGTM after tha

http://codereview.chromium.org/87003/diff/1005/9
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/87003/diff/1005/9#newcode322
Line 322: #if defined(OS_WINDOWS)
OS_WIN, not OS_WINDOWS

[Evan Martin, 2009-04-21 00:54:47]

On 2009/04/21 00:54:29, Evan Martin wrote:
> minor bug below.  LGTM after tha

minor bug below.  LGTM after that.


> 
> http://codereview.chromium.org/87003/diff/1005/9
> File base/file_util_unittest.cc (right):
> 
> http://codereview.chromium.org/87003/diff/1005/9#newcode322
> Line 322: #if defined(OS_WINDOWS)
> OS_WIN, not OS_WINDOWS

[hamaji, 2009-04-21 01:33:00]

Thanks you for your review. If there are no further issues, could you check this
change in? I don't have permission yet.

Thanks!

http://codereview.chromium.org/87003/diff/1005/9
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/87003/diff/1005/9#newcode322
Line 322: #if defined(OS_WINDOWS)
On 2009/04/21 00:54:29, Evan Martin wrote:
> OS_WIN, not OS_WINDOWS

Oops thanks, fixed.

[Erik does not do reviews, 2009-04-21 16:10:21]

It looks like this review was already submitted.  One issue, one nit for you for
a follow-on.

http://codereview.chromium.org/87003/diff/15/17
File base/file_util_posix.cc (right):

http://codereview.chromium.org/87003/diff/15/17#newcode65
Line 65: while ((ent = readdir(dir)) != NULL) {
lint says to use readdir_r instead of readdir (see the error column on the
rietvelg front page for this review)

http://codereview.chromium.org/87003/diff/15/18
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/87003/diff/15/18#newcode327
Line 327: sleep(1);
nit: for new code, it's probably better to use PlatformThread::Sleep, even
though this is POSIX specific.

[hamaji, 2009-04-21 18:19:56]

Thanks for the review! I'll send another change which fixes the issues.

On 2009/04/21 16:10:21, Erik Kay wrote:
> It looks like this review was already submitted.  One issue, one nit for you
for
> a follow-on.
> 
> http://codereview.chromium.org/87003/diff/15/17
> File base/file_util_posix.cc (right):
> 
> http://codereview.chromium.org/87003/diff/15/17#newcode65
> Line 65: while ((ent = readdir(dir)) != NULL) {
> lint says to use readdir_r instead of readdir (see the error column on the
> rietvelg front page for this review)
> 
> http://codereview.chromium.org/87003/diff/15/18
> File base/file_util_unittest.cc (right):
> 
> http://codereview.chromium.org/87003/diff/15/18#newcode327
> Line 327: sleep(1);
> nit: for new code, it's probably better to use PlatformThread::Sleep, even
> though this is POSIX specific.

[Hironori Bono, 2009-04-22 04:46:27]

This is just a nit for your follow-on.

http://codereview.chromium.org/87003/diff/15/17
File base/file_util_posix.cc (right):

http://codereview.chromium.org/87003/diff/15/17#newcode76
Line 76: if (st.st_ctime >= comparison_time.ToTimeT())
nit: If I recall correctly, base::Time::ToTimeT() discards microseconds from
|comparison_time|, i.e. |comparison_time.ToTimeT()| returns 100 if
|comparison_time.us_| is 100100000 (usecs) = 100.1 (secs).
So, this condition becomes true if |st_ctime| is 100 (secs) and
|comparison_time.us_| is 100100000 (usecs) = 100.1 (secs).
It may be better to call base::Time::FromTimeT() to convert |st.st_ctime| to
base::Time or call base::Time::ToDoubleT().

[hamaji, 2009-04-22 05:12:23]

Thanks for your review! As I wrote below, I want to keep the current code. If
you have a better idea, please let me know.

http://codereview.chromium.org/87003/diff/15/17
File base/file_util_posix.cc (right):

http://codereview.chromium.org/87003/diff/15/17#newcode76
Line 76: if (st.st_ctime >= comparison_time.ToTimeT())
On 2009/04/22 04:46:27, hbono wrote:
> nit: If I recall correctly, base::Time::ToTimeT() discards microseconds from
> |comparison_time|, i.e. |comparison_time.ToTimeT()| returns 100 if
> |comparison_time.us_| is 100100000 (usecs) = 100.1 (secs).
> So, this condition becomes true if |st_ctime| is 100 (secs) and
> |comparison_time.us_| is 100100000 (usecs) = 100.1 (secs).
> It may be better to call base::Time::FromTimeT() to convert |st.st_ctime| to
> base::Time or call base::Time::ToDoubleT().

Good point. However, if we use Time::DoubleT(), I think there would be another
issue. Please suppose the following case:

1. Get comparison_time by Time::Now() and the value is 100.1 (secs).
2. Create a file and the current time is 100.3 (secs).

As POSIX doesn't have millisecond precision for st_ctime, the creation time of
the file created in the step 2 is 100 and the file is considered older than
|comparison_time|. After all, we may have to accept either of the two issues: 1.
files which are older than |comparison_time| are considered newer (current
implementation) 2. files newer than |comparison_time| are considered older (your
suggestion). I chose the choice 1 just because we have already had sleep before
getting |comparison_time| in file_util_unittest.cc .

[Hironori Bono, 2009-04-22 05:35:58]

http://codereview.chromium.org/87003/diff/15/17
File base/file_util_posix.cc (right):

http://codereview.chromium.org/87003/diff/15/17#newcode76
Line 76: if (st.st_ctime >= comparison_time.ToTimeT())
On 2009/04/22 05:12:23, hamaji wrote:
> Good point. However, if we use Time::DoubleT(), I think there would be another
> issue. Please suppose the following case:
> 
> 1. Get comparison_time by Time::Now() and the value is 100.1 (secs).
> 2. Create a file and the current time is 100.3 (secs).
> 
> As POSIX doesn't have millisecond precision for st_ctime, the creation time of
> the file created in the step 2 is 100 and the file is considered older than
> |comparison_time|. After all, we may have to accept either of the two issues:
1.
> files which are older than |comparison_time| are considered newer (current
> implementation) 2. files newer than |comparison_time| are considered older
(your
> suggestion). I chose the choice 1 just because we have already had sleep
before
> getting |comparison_time| in file_util_unittest.cc.

I'm wondering why you did not write this comment in your code, which is
definitely useful for convincing readers.

[hamaji, 2009-04-22 06:02:02]

On 2009/04/22 05:35:58, hbono wrote:
> http://codereview.chromium.org/87003/diff/15/17
> File base/file_util_posix.cc (right):
> 
> http://codereview.chromium.org/87003/diff/15/17#newcode76
> Line 76: if (st.st_ctime >= comparison_time.ToTimeT())
> On 2009/04/22 05:12:23, hamaji wrote:
> > Good point. However, if we use Time::DoubleT(), I think there would be
another
> > issue. Please suppose the following case:
> > 
> > 1. Get comparison_time by Time::Now() and the value is 100.1 (secs).
> > 2. Create a file and the current time is 100.3 (secs).
> > 
> > As POSIX doesn't have millisecond precision for st_ctime, the creation time
of
> > the file created in the step 2 is 100 and the file is considered older than
> > |comparison_time|. After all, we may have to accept either of the two
issues:
> 1.
> > files which are older than |comparison_time| are considered newer (current
> > implementation) 2. files newer than |comparison_time| are considered older
> (your
> > suggestion). I chose the choice 1 just because we have already had sleep
> before
> > getting |comparison_time| in file_util_unittest.cc.
> 
> I'm wondering why you did not write this comment in your code, which is
> definitely useful for convincing readers.

Ah, yes. I should have wrote this comment... Thanks for the suggestion. I put
the comments in the another review:

http://codereview.chromium.org/87039

Thanks!