code review 1103041: os: add Utime function

os: add Utime function

[brad_danga_com, 2010-05-04 19:40:15]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[r, 2010-05-05 00:56:07]

LGTM

leaving for rsc

http://codereview.appspot.com/1103041/diff/2001/3002
File src/pkg/os/os_test.go (right):

http://codereview.appspot.com/1103041/diff/2001/3002#newcode493
src/pkg/os/os_test.go:493: fd.Write([]byte("hello, world\n"))
you could use WriteString and let the library do the conversion. your choice.

http://codereview.appspot.com/1103041/diff/2001/3002#newcode502
src/pkg/os/os_test.go:502: const SecondNanos = 1000000000
how about 
const OneSecond = 1e9 // units of nanoseconds

[brad_danga_com, 2010-05-05 14:33:26]

Hello rsc, r (cc: golang-dev@googlegroups.com),

Please take another look.

[brad_danga_com, 2010-05-05 14:35:02]

Hello rsc, r (cc: golang-dev@googlegroups.com),

Please take another look.

[brad_danga_com, 2010-05-05 14:36:34]

PTAL

http://codereview.appspot.com/1103041/diff/2001/3002
File src/pkg/os/os_test.go (right):

http://codereview.appspot.com/1103041/diff/2001/3002#newcode493
src/pkg/os/os_test.go:493: fd.Write([]byte("hello, world\n"))
On 2010/05/05 00:56:07, r wrote:
> you could use WriteString and let the library do the conversion. your choice.

I didn't want to break consistency with the rest of this file.

http://codereview.appspot.com/1103041/diff/2001/3002#newcode502
src/pkg/os/os_test.go:502: const SecondNanos = 1000000000
On 2010/05/05 00:56:07, r wrote:
> how about 
> const OneSecond = 1e9 // units of nanoseconds

Done.

[rog, 2010-05-05 15:21:05]

i still think that Chtime or Chtimes would be a better name
for this call, given that the name Utime doesn't
correspond directly to the system call name. under
Windows, the name Utime would be even less appropriate, as the
the relevant call is SetFileInformationByHandle.



On 5 May 2010 15:36,  <bradfitz@gmail.com> wrote:
> PTAL
>
>
> http://codereview.appspot.com/1103041/diff/2001/3002
> File src/pkg/os/os_test.go (right):
>
> http://codereview.appspot.com/1103041/diff/2001/3002#newcode493
> src/pkg/os/os_test.go:493: fd.Write([]byte("hello, world\n"))
> On 2010/05/05 00:56:07, r wrote:
>>
>> you could use WriteString and let the library do the conversion. your
>
> choice.
>
> I didn't want to break consistency with the rest of this file.
>
> http://codereview.appspot.com/1103041/diff/2001/3002#newcode502
> src/pkg/os/os_test.go:502: const SecondNanos = 1000000000
> On 2010/05/05 00:56:07, r wrote:
>>
>> how about
>> const OneSecond = 1e9 // units of nanoseconds
>
> Done.
>
> http://codereview.appspot.com/1103041/show
>

[dangabrad, 2010-05-05 16:34:18]

I'm happy with whatever, including Chtimes.  The win32 argument is good, but
I imagine pkg os already has a bunch of Unix-isms in it that don't map to
Windows names either, so consistency with that naming family might be
preferred over deviating in a few places.

I'll leave this to Russ and Rob to decide, though.

- Brad

On Wed, May 5, 2010 at 8:21 AM, roger peppe <rogpeppe@gmail.com> wrote:

> i still think that Chtime or Chtimes would be a better name
> for this call, given that the name Utime doesn't
> correspond directly to the system call name. under
> Windows, the name Utime would be even less appropriate, as the
> the relevant call is SetFileInformationByHandle.
>
>
>
> On 5 May 2010 15:36,  <bradfitz@gmail.com> wrote:
> > PTAL
> >
> >
> > http://codereview.appspot.com/1103041/diff/2001/3002
> > File src/pkg/os/os_test.go (right):
> >
> > http://codereview.appspot.com/1103041/diff/2001/3002#newcode493
> > src/pkg/os/os_test.go:493: fd.Write([]byte("hello, world\n"))
> > On 2010/05/05 00:56:07, r wrote:
> >>
> >> you could use WriteString and let the library do the conversion. your
> >
> > choice.
> >
> > I didn't want to break consistency with the rest of this file.
> >
> > http://codereview.appspot.com/1103041/diff/2001/3002#newcode502
> > src/pkg/os/os_test.go:502: const SecondNanos = 1000000000
> > On 2010/05/05 00:56:07, r wrote:
> >>
> >> how about
> >> const OneSecond = 1e9 // units of nanoseconds
> >
> > Done.
> >
> > http://codereview.appspot.com/1103041/show
> >
>

[rsc, 2010-05-05 16:34:59]

On Wed, May 5, 2010 at 08:21, roger peppe <rogpeppe@gmail.com> wrote:
> i still think that Chtime or Chtimes would be a better name
> for this call, given that the name Utime doesn't
> correspond directly to the system call name. under
> Windows, the name Utime would be even less appropriate, as the
> the relevant call is SetFileInformationByHandle.

utime does correspond directly to the original system call.
the s variant was added to add sub-second precision.
i agree that it's a clumsy name, but it's what people
coming from unix (which is a lot of our developers)
expect, and no one expects Chtime.
(and SetFileInformationByHandle is right out.)

[rog, 2010-05-05 17:09:58]

i guess.

FWIW, there are other functions in os that don't correspond
directly to their unix equivalents (e.g. Remove, ReadAt),
so i don't think it would matter so much if Chtime was similarly
different. it's not hard to look up.

i don't think there are any other functions in os that modify
the environment that don't have an active verb in the name.



On 5 May 2010 17:34, Russ Cox <rsc@golang.org> wrote:
> On Wed, May 5, 2010 at 08:21, roger peppe <rogpeppe@gmail.com> wrote:
>> i still think that Chtime or Chtimes would be a better name
>> for this call, given that the name Utime doesn't
>> correspond directly to the system call name. under
>> Windows, the name Utime would be even less appropriate, as the
>> the relevant call is SetFileInformationByHandle.
>
> utime does correspond directly to the original system call.
> the s variant was added to add sub-second precision.
> i agree that it's a clumsy name, but it's what people
> coming from unix (which is a lot of our developers)
> expect, and no one expects Chtime.
> (and SetFileInformationByHandle is right out.)
>

[dangabrad, 2010-05-05 17:11:58]

Arguably, "U" is Update in the same way "Ch" is Change, if you're calling
"Ch" a verb.

On Wed, May 5, 2010 at 10:09 AM, roger peppe <rogpeppe@gmail.com> wrote:

> i guess.
>
> FWIW, there are other functions in os that don't correspond
> directly to their unix equivalents (e.g. Remove, ReadAt),
> so i don't think it would matter so much if Chtime was similarly
> different. it's not hard to look up.
>
> i don't think there are any other functions in os that modify
> the environment that don't have an active verb in the name.
>
>
>
> On 5 May 2010 17:34, Russ Cox <rsc@golang.org> wrote:
> > On Wed, May 5, 2010 at 08:21, roger peppe <rogpeppe@gmail.com> wrote:
> >> i still think that Chtime or Chtimes would be a better name
> >> for this call, given that the name Utime doesn't
> >> correspond directly to the system call name. under
> >> Windows, the name Utime would be even less appropriate, as the
> >> the relevant call is SetFileInformationByHandle.
> >
> > utime does correspond directly to the original system call.
> > the s variant was added to add sub-second precision.
> > i agree that it's a clumsy name, but it's what people
> > coming from unix (which is a lot of our developers)
> > expect, and no one expects Chtime.
> > (and SetFileInformationByHandle is right out.)
> >
>

[rog, 2010-05-05 17:32:59]

ah, maybe that's the case. my addled brain could
only make "user", which naturally made no sense.
in my defense, the man page (on os x at least) doesn't
contain the word "update".

On 5 May 2010 18:11, Brad Fitzpatrick <brad@danga.com> wrote:
> Arguably, "U" is Update in the same way "Ch" is Change, if you're calling
> "Ch" a verb.
> On Wed, May 5, 2010 at 10:09 AM, roger peppe <rogpeppe@gmail.com> wrote:
>>
>> i guess.
>>
>> FWIW, there are other functions in os that don't correspond
>> directly to their unix equivalents (e.g. Remove, ReadAt),
>> so i don't think it would matter so much if Chtime was similarly
>> different. it's not hard to look up.
>>
>> i don't think there are any other functions in os that modify
>> the environment that don't have an active verb in the name.
>>
>>
>>
>> On 5 May 2010 17:34, Russ Cox <rsc@golang.org> wrote:
>> > On Wed, May 5, 2010 at 08:21, roger peppe <rogpeppe@gmail.com> wrote:
>> >> i still think that Chtime or Chtimes would be a better name
>> >> for this call, given that the name Utime doesn't
>> >> correspond directly to the system call name. under
>> >> Windows, the name Utime would be even less appropriate, as the
>> >> the relevant call is SetFileInformationByHandle.
>> >
>> > utime does correspond directly to the original system call.
>> > the s variant was added to add sub-second precision.
>> > i agree that it's a clumsy name, but it's what people
>> > coming from unix (which is a lot of our developers)
>> > expect, and no one expects Chtime.
>> > (and SetFileInformationByHandle is right out.)
>> >
>
>

[r2, 2010-05-05 17:36:06]

i can go either way but i must admit i like Chtime[s] better.

-rob

On May 5, 2010, at 10:32 AM, roger peppe wrote:

> ah, maybe that's the case. my addled brain could
> only make "user", which naturally made no sense.
> in my defense, the man page (on os x at least) doesn't
> contain the word "update".
>
> On 5 May 2010 18:11, Brad Fitzpatrick <brad@danga.com> wrote:
>> Arguably, "U" is Update in the same way "Ch" is Change, if you're  
>> calling
>> "Ch" a verb.
>> On Wed, May 5, 2010 at 10:09 AM, roger peppe <rogpeppe@gmail.com>  
>> wrote:
>>>
>>> i guess.
>>>
>>> FWIW, there are other functions in os that don't correspond
>>> directly to their unix equivalents (e.g. Remove, ReadAt),
>>> so i don't think it would matter so much if Chtime was similarly
>>> different. it's not hard to look up.
>>>
>>> i don't think there are any other functions in os that modify
>>> the environment that don't have an active verb in the name.
>>>
>>>
>>>
>>> On 5 May 2010 17:34, Russ Cox <rsc@golang.org> wrote:
>>>> On Wed, May 5, 2010 at 08:21, roger peppe <rogpeppe@gmail.com>  
>>>> wrote:
>>>>> i still think that Chtime or Chtimes would be a better name
>>>>> for this call, given that the name Utime doesn't
>>>>> correspond directly to the system call name. under
>>>>> Windows, the name Utime would be even less appropriate, as the
>>>>> the relevant call is SetFileInformationByHandle.
>>>>
>>>> utime does correspond directly to the original system call.
>>>> the s variant was added to add sub-second precision.
>>>> i agree that it's a clumsy name, but it's what people
>>>> coming from unix (which is a lot of our developers)
>>>> expect, and no one expects Chtime.
>>>> (and SetFileInformationByHandle is right out.)
>>>>
>>
>>

[dangabrad, 2010-05-05 17:38:34]

How about I name it Chtimes and mention utime/utimes in the docs for people
doing quick searches?

On Wed, May 5, 2010 at 10:35 AM, Rob 'Commander' Pike <r@google.com> wrote:

> i can go either way but i must admit i like Chtime[s] better.
>
> -rob
>
>
> On May 5, 2010, at 10:32 AM, roger peppe wrote:
>
>  ah, maybe that's the case. my addled brain could
>> only make "user", which naturally made no sense.
>> in my defense, the man page (on os x at least) doesn't
>> contain the word "update".
>>
>> On 5 May 2010 18:11, Brad Fitzpatrick <brad@danga.com> wrote:
>>
>>> Arguably, "U" is Update in the same way "Ch" is Change, if you're calling
>>> "Ch" a verb.
>>> On Wed, May 5, 2010 at 10:09 AM, roger peppe <rogpeppe@gmail.com> wrote:
>>>
>>>>
>>>> i guess.
>>>>
>>>> FWIW, there are other functions in os that don't correspond
>>>> directly to their unix equivalents (e.g. Remove, ReadAt),
>>>> so i don't think it would matter so much if Chtime was similarly
>>>> different. it's not hard to look up.
>>>>
>>>> i don't think there are any other functions in os that modify
>>>> the environment that don't have an active verb in the name.
>>>>
>>>>
>>>>
>>>> On 5 May 2010 17:34, Russ Cox <rsc@golang.org> wrote:
>>>>
>>>>> On Wed, May 5, 2010 at 08:21, roger peppe <rogpeppe@gmail.com> wrote:
>>>>>
>>>>>> i still think that Chtime or Chtimes would be a better name
>>>>>> for this call, given that the name Utime doesn't
>>>>>> correspond directly to the system call name. under
>>>>>> Windows, the name Utime would be even less appropriate, as the
>>>>>> the relevant call is SetFileInformationByHandle.
>>>>>>
>>>>>
>>>>> utime does correspond directly to the original system call.
>>>>> the s variant was added to add sub-second precision.
>>>>> i agree that it's a clumsy name, but it's what people
>>>>> coming from unix (which is a lot of our developers)
>>>>> expect, and no one expects Chtime.
>>>>> (and SetFileInformationByHandle is right out.)
>>>>>
>>>>>
>>>
>>>
>

[rsc, 2010-05-05 17:48:50]

Sounds good.

On Wed, May 5, 2010 at 10:38, Brad Fitzpatrick <brad@danga.com> wrote:
> How about I name it Chtimes and mention utime/utimes in the docs for people
> doing quick searches?
>
> On Wed, May 5, 2010 at 10:35 AM, Rob 'Commander' Pike <r@google.com> wrote:
>>
>> i can go either way but i must admit i like Chtime[s] better.
>>
>> -rob
>>
>> On May 5, 2010, at 10:32 AM, roger peppe wrote:
>>
>>> ah, maybe that's the case. my addled brain could
>>> only make "user", which naturally made no sense.
>>> in my defense, the man page (on os x at least) doesn't
>>> contain the word "update".
>>>
>>> On 5 May 2010 18:11, Brad Fitzpatrick <brad@danga.com> wrote:
>>>>
>>>> Arguably, "U" is Update in the same way "Ch" is Change, if you're
>>>> calling
>>>> "Ch" a verb.
>>>> On Wed, May 5, 2010 at 10:09 AM, roger peppe <rogpeppe@gmail.com> wrote:
>>>>>
>>>>> i guess.
>>>>>
>>>>> FWIW, there are other functions in os that don't correspond
>>>>> directly to their unix equivalents (e.g. Remove, ReadAt),
>>>>> so i don't think it would matter so much if Chtime was similarly
>>>>> different. it's not hard to look up.
>>>>>
>>>>> i don't think there are any other functions in os that modify
>>>>> the environment that don't have an active verb in the name.
>>>>>
>>>>>
>>>>>
>>>>> On 5 May 2010 17:34, Russ Cox <rsc@golang.org> wrote:
>>>>>>
>>>>>> On Wed, May 5, 2010 at 08:21, roger peppe <rogpeppe@gmail.com> wrote:
>>>>>>>
>>>>>>> i still think that Chtime or Chtimes would be a better name
>>>>>>> for this call, given that the name Utime doesn't
>>>>>>> correspond directly to the system call name. under
>>>>>>> Windows, the name Utime would be even less appropriate, as the
>>>>>>> the relevant call is SetFileInformationByHandle.
>>>>>>
>>>>>> utime does correspond directly to the original system call.
>>>>>> the s variant was added to add sub-second precision.
>>>>>> i agree that it's a clumsy name, but it's what people
>>>>>> coming from unix (which is a lot of our developers)
>>>>>> expect, and no one expects Chtime.
>>>>>> (and SetFileInformationByHandle is right out.)
>>>>>>
>>>>
>>>>
>>
>
>

[brad_danga_com, 2010-05-05 17:50:52]

Hello rsc, r (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2010-05-05 18:02:04]

LGTM modulo commentary
leaving for rsc

http://codereview.appspot.com/1103041/diff/5002/23002
File src/pkg/os/file.go (right):

http://codereview.appspot.com/1103041/diff/5002/23002#newcode414
src/pkg/os/file.go:414: // Note that Chtimes takes nanosecond granularity times
but the
s/Note that //

but a rewrite is shorter anyway:

The argument times are in nanoseconds, although the underlying...

http://codereview.appspot.com/1103041/diff/5002/23002#newcode417
src/pkg/os/file.go:417: // subsequent Stat call.
delete this last sentence please. it's jargony and redundant.

http://codereview.appspot.com/1103041/diff/5002/23003
File src/pkg/os/os_test.go (right):

http://codereview.appspot.com/1103041/diff/5002/23003#newcode498
src/pkg/os/os_test.go:498: t.Fatalf("stat %s: %s", Path, err)
s/s/S/

http://codereview.appspot.com/1103041/diff/5002/23003#newcode505
src/pkg/os/os_test.go:505: t.Fatalf("chtimes %s: %s", Path, err)
s/c/C/

[brad_danga_com, 2010-05-05 18:22:00]

Hello rsc, r (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2010-05-05 18:27:17]

LGTM

Thanks.

[rsc, 2010-05-20 00:49:01]

*** Submitted as http://code.google.com/p/go/source/detail?r=a79cd7447eaa ***

os: add Chtimes function

R=rsc, r
CC=golang-dev
http://codereview.appspot.com/1103041

Committer: Russ Cox <rsc@golang.org>