code review 127740043: os: make SameFile handle paths like c:a.txt properly

os: make SameFile handle paths like c:a.txt properly

Fixes issue 8490.

[brainman, 2014-08-07 05:59:08]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[brainman, 2014-08-07 05:59:40]

This CL introduces absPath function that is just a copy of syscall.getFullPath.
I was wandering, if I can put it in one of those "internal" packages or
something. I would need similar function in path/filepath later to fix issue
8145. Seems like a waste to repeat it 3 times. Suggestions are welcome.

Alex

[rsc, 2014-08-11 15:08:58]

This is a good test case for our policy. Let me discuss it with Rob and get back
to you. Thanks.

[rsc, 2014-08-11 20:32:08]

Rob is working on moving syscall to go.sys.
Once that's done we can export the code as FullPath, in both places.
But we need to finish the copy first.
Thanks.

[bradfitz, 2014-08-13 21:17:12]

Can this go shared AbsPath function go in the new
$GOROOT/src/pkg/internal/syscall package that I created yesterday?

[brainman, 2014-08-14 04:20:18]

Hello golang-codereviews@googlegroups.com, rsc@golang.org, bradfitz@golang.org
(cc: golang-codereviews@googlegroups.com, r@golang.org),

Please take another look.

[brainman, 2014-08-14 04:21:10]

On 2014/08/13 21:17:12, bradfitz wrote:
> Can this go shared AbsPath function go in the new
> $GOROOT/src/pkg/internal/syscall package that I created yesterday?

Sure. Done.

I wonder, if syscall is a good name for the new package. It might clash with old
syscall at times. Perhaps we could keep internal/syscall for "all OSes code",
but put windows specific code into "internal/syscall/windows" similar to go.sys?
Same for plan9 and unix. What do you think?

Alex

[bradfitz, 2014-08-14 04:53:04]

SGTM. Actually don't care either way. The internal system lets us change it
whenever.
 On Aug 13, 2014 9:21 PM, <alex.brainman@gmail.com> wrote:

> On 2014/08/13 21:17:12, bradfitz wrote:
>
>> Can this go shared AbsPath function go in the new
>> $GOROOT/src/pkg/internal/syscall package that I created yesterday?
>>
>
> Sure. Done.
>
> I wonder, if syscall is a good name for the new package. It might clash
> with old syscall at times. Perhaps we could keep internal/syscall for
> "all OSes code", but put windows specific code into
> "internal/syscall/windows" similar to go.sys? Same for plan9 and unix.
> What do you think?
>
> Alex
>
> https://codereview.appspot.com/127740043/
>

[r, 2014-08-14 05:00:36]

I'm not fussed either way but I thought the thinking was that the
function was useful anyway, so why not put it in syscall and go.sys
both?

-rob

[brainman, 2014-08-14 05:11:40]

On 2014/08/14 05:00:36, r wrote:
> I'm not fussed either way ...

Lets leave it as is for now in internal/syscall

> ... but I thought the thinking was that the
> function was useful anyway, so why not put it in syscall and go.sys
> both?

Do you mean syscall or internal/syscall? I though syscall is closed for *any*
changes (especially for additions). I don't really care if it gets coplied into
go.sys or not. I need this function in syscall, os and path/filepath. But I
could put it into go.sys, if we want ...

Alex

[r, 2014-08-14 13:57:53]

Here's what Russ wrote, and I think it's fine:

"Rob is working on moving syscall to go.sys.
Once that's done we can export the code as FullPath, in both places.
But we need to finish the copy first.
"

The syscall package is closed to development for things like new
syscalls, but to support the standard library I'm fine with a simple
change like this that seems useful. As time goes on, "internal" will
grow and the likelihood of such changes will go down, but this thing
seems fine to me. It looks like a useful function.

-rob

On Wed, Aug 13, 2014 at 10:11 PM,  <alex.brainman@gmail.com> wrote:
> On 2014/08/14 05:00:36, r wrote:
>>
>> I'm not fussed either way ...
>
>
> Lets leave it as is for now in internal/syscall
>
>> ... but I thought the thinking was that the
>>
>> function was useful anyway, so why not put it in syscall and go.sys
>> both?
>
>
> Do you mean syscall or internal/syscall? I though syscall is closed for
> *any* changes (especially for additions). I don't really care if it gets
> coplied into go.sys or not. I need this function in syscall, os and
> path/filepath. But I could put it into go.sys, if we want ...
>
> Alex
>
> https://codereview.appspot.com/127740043/

[brainman, 2014-08-15 00:29:24]

Hello golang-codereviews@googlegroups.com, rsc@golang.org, bradfitz@golang.org
(cc: golang-codereviews@googlegroups.com, r@golang.org),

Please take another look.

[r, 2014-08-15 00:40:17]

LGTM but let rsc approve too.
if he does, please push FullPath to go.sys/windows.

[brainman, 2014-08-15 01:12:10]

On 2014/08/15 00:40:17, r wrote:
> LGTM but let rsc approve too.
> if he does, please push FullPath to go.sys/windows.

Will do. Thank you.

Alex

[brainman, 2014-08-15 01:18:04]

Unrelated but ...

Should syscall related changes from https://codereview.appspot.com/122200043/ go
into syscall? Or should we move them into internal/syscall or
internal/syscall/windows? Thank you.

Alex

[r, 2014-08-15 02:15:04]

some syscall stuff has landed for 1.4. this change is so small i'm ok
with it, but make sure it gets into go.sys/windows as well.

after 1.4 the hammer comes down. it's halfway down already.

On Thu, Aug 14, 2014 at 6:18 PM,  <alex.brainman@gmail.com> wrote:
> Unrelated but ...
>
> Should syscall related changes from
> https://codereview.appspot.com/122200043/ go into syscall? Or should we
> move them into internal/syscall or internal/syscall/windows? Thank you.
>
> Alex
>
> https://codereview.appspot.com/127740043/

[brainman, 2014-08-19 02:23:24]

Russ, LGTY?

Alex

[rsc, 2014-08-19 02:29:01]

LGTM

[brainman, 2014-08-19 05:00:03]

*** Submitted as https://code.google.com/p/go/source/detail?r=71db3dc120af ***

os: make SameFile handle paths like c:a.txt properly

Fixes issue 8490.

LGTM=r, rsc
R=golang-codereviews, rsc, bradfitz, r
CC=golang-codereviews
https://codereview.appspot.com/127740043

[brainman, 2014-08-19 05:56:43]

On 2014/08/15 00:40:17, r wrote:
> ... please push FullPath to go.sys/windows.

https://codereview.appspot.com/130250043/

Alex