code review 13652043: cmd/api/run: make api check directory per-user

cmd/api: make api check directory per-user

Fixes issue 6353.

[kortschak, 2013-09-10 20:02:37]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2013-09-10 20:10:38]

LGTM

But change subject to just cmd/api

Leaving for others to submit after that.
On Sep 10, 2013 1:02 PM, <dan.kortschak@adelaide.edu.au> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> cmd/api/run: make api check directory per-user
>
> Fixes issue 6353.
>
> Please review this at
https://codereview.appspot.**com/13652043/<https://codereview.appspot.com/136...
>
> Affected files (+7, -1 lines):
>   M src/cmd/api/run.go
>
>
> Index: src/cmd/api/run.go
> ==============================**==============================**=======
> --- a/src/cmd/api/run.go
> +++ b/src/cmd/api/run.go
> @@ -19,6 +19,7 @@
>         "net/http"
>         "os"
>         "os/exec"
> +       "os/user"
>         "path/filepath"
>         "strconv"
>         "strings"
> @@ -99,8 +100,13 @@
>  func prepGoPath() string {
>         const tempBase = "go.tools.TMP"
>
> +       u, err := user.Current()
> +       if err != nil {
> +               log.Fatalf("Error getting current user: %v", err)
> +       }
> +
>         // The GOPATH we'll return
> -       gopath := filepath.Join(os.TempDir(), "gopath-api", goToolsVersion)
> +       gopath := filepath.Join(os.TempDir(), "gopath-api-"+u.Username,
> goToolsVersion)
>
>         // cloneDir is where we run "hg clone".
>         cloneDir := filepath.Join(gopath, "src", "code.google.com", "p")
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>

[brainman, 2013-09-10 23:28:39]

https://codereview.appspot.com/13652043/diff/5001/src/cmd/api/run.go
File src/cmd/api/run.go (right):

https://codereview.appspot.com/13652043/diff/5001/src/cmd/api/run.go#newcode109
src/cmd/api/run.go:109: gopath := filepath.Join(os.TempDir(),
"gopath-api-"+u.Username, goToolsVersion)
u.Username is "DOMAIN\brainman" on my computer (windows). Given that \ is a path
delimiter on windows, do you think it is good idea to use u.Username as part of
filename?

[kortschak, 2013-09-10 23:39:42]

On 2013/09/10 23:28:39, brainman wrote:
> https://codereview.appspot.com/13652043/diff/5001/src/cmd/api/run.go
> File src/cmd/api/run.go (right):
> 
>
https://codereview.appspot.com/13652043/diff/5001/src/cmd/api/run.go#newcode109
> src/cmd/api/run.go:109: gopath := filepath.Join(os.TempDir(),
> "gopath-api-"+u.Username, goToolsVersion)
> u.Username is "DOMAIN\brainman" on my computer (windows). Given that \ is a
path
> delimiter on windows, do you think it is good idea to use u.Username as part
of
> filename?

I have close to zero knowledge of the windows environment. Can you suggest a
reasonable alternative. If I use u.UID will that work for you?

[kortschak, 2013-09-10 23:41:51]

On 2013/09/10 23:39:42, kortschak wrote:
> I have close to zero knowledge of the windows environment. Can you suggest a
> reasonable alternative. If I use u.UID will that work for you?

Alternatively filepath.Base(u.Username)?

[brainman, 2013-09-10 23:46:40]

On 2013/09/10 23:39:42, kortschak wrote:
> ... If I use u.UID will that work for you?

u.UID is long and cryptic string. Please do not use that.

On 2013/09/10 23:41:51, kortschak wrote:
> ... Alternatively filepath.Base(u.Username)?

Maybe. Alternatively, you can replace these chars \ / : in u.Username with
something more acceptable, like _. It might work better.

Alex

[kortschak, 2013-09-11 00:18:22]

PTAL

[brainman, 2013-09-11 00:24:24]

https://codereview.appspot.com/13652043/diff/16001/src/cmd/api/run.go
File src/cmd/api/run.go (right):

https://codereview.appspot.com/13652043/diff/16001/src/cmd/api/run.go#newcode149
src/cmd/api/run.go:149: func cleanUsername(n string) string {
I think you code will read simpler if you just do 3 single line replaces like
this:

https://github.com/golang/glog/blob/master/glog_file.go#L69

for '\\', '/' and ':'.

[kortschak, 2013-09-11 00:44:30]

PTAL

On Wed, 2013-09-11 at 00:24 +0000, alex.brainman@gmail.com wrote:
> I think you code will read simpler if you just do 3 single line
> replaces
> like this:
> 
> https://github.com/golang/glog/blob/master/glog_file.go#L69

3x strings.Replace for single code point changes feels like abuse.

> for '\\', '/' and ':'.

Done.

[brainman, 2013-09-11 00:46:03]

LGTM

Should I submit?

Alex

[kortschak, 2013-09-11 00:49:31]

On Wed, 2013-09-11 at 00:46 +0000, alex.brainman@gmail.com wrote:
> Should I submit?

Please.

[brainman, 2013-09-11 00:51:07]

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

cmd/api: make api check directory per-user

Fixes issue 6353.

R=golang-dev, bradfitz, alex.brainman
CC=golang-dev
https://codereview.appspot.com/13652043

Committer: Alex Brainman <alex.brainman@gmail.com>