code review 4807048: goinstall: error out with paths that end with '/'

goinstall: error out with paths that end with '/'

[tarm, 2011-07-22 17:37:05]

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

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

[rsc, 2011-07-22 23:26:54]

>   goinstall github.com/example/MyHotCmd/
> should work

i disagree.
why not spell this

goinstall github.com/examples/MyHotCmd

?

it's important that each target have a unique name.
this is making each target have 2 names.

[adg, 2011-07-24 04:24:09]

On 23 July 2011 09:26, Russ Cox <rsc@golang.org> wrote:
>>   goinstall github.com/example/MyHotCmd/
>> should work
>
> i disagree.

+1

[tarm, 2011-08-16 19:09:09]

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

Please take another look.

[tarmigan+golang_gmail.com, 2011-08-16 19:09:16]

On Sat, Jul 23, 2011 at 9:23 PM, Andrew Gerrand <adg@golang.org> wrote:
> On 23 July 2011 09:26, Russ Cox <rsc@golang.org> wrote:
>>>   goinstall github.com/example/MyHotCmd/
>>> should work
>>
>> i disagree.
>
> +1

OK, but then I think goinstall should warn when you add the trailing
slash.  Currently it will execute without warning but the Makefile it
creates will have
   TARG=
which is not useful either.  Would a patch to add a warning be acceptable?

Thanks,
Tarmigan

[adg, 2011-08-16 20:07:27]

I think it's better to do this check here:

http://code.google.com/p/go/source/browse/src/cmd/goinstall/main.go#184

And you can just use strings.HasSuffix(pkg, "/") as the test.

Andrew

[tarm, 2011-08-16 20:47:58]

On Tue, Aug 16, 2011 at 1:07 PM, Andrew Gerrand <adg@golang.org> wrote:
> I think it's better to do this check here:
>
> http://code.google.com/p/go/source/browse/src/cmd/goinstall/main.go#184
>
> And you can just use strings.HasSuffix(pkg, "/") as the test.

OK, but I think we only want this check for commands (not packages),
right?  Also, the context where this originally bit me was using tab
completion in GOROOT (which included the trailing '/'), so it might be
nice for windows users if we used filepath (but I have not actually
tested on windows).

So we'd have to do something like:

diff -r 8ecb24f03c01 src/cmd/goinstall/main.go
--- a/src/cmd/goinstall/main.go Tue Aug 16 14:56:23 2011 -0400
+++ b/src/cmd/goinstall/main.go Tue Aug 16 13:44:41 2011 -0700
@@ -197,50 +197,55 @@
        // Download remote packages if not found or forced with -u flag.
        remote, public := isRemote(pkg), false
        if remote {
                if err == build.ErrNotFound || (err == nil && *update) {
                        // Download remote package.
                        printf("%s: download\n", pkg)
                        public, err = download(pkg, tree.SrcDir())
                } else {
                        // Test if this is a public repository
                        // (for reporting to dashboard).
                        m, _ := findPublicRepo(pkg)
                        public = m != nil
                }
        }
        if err != nil {
                errorf("%s: %v\n", pkg, err)
                return
        }
        dir := filepath.Join(tree.SrcDir(), pkg)

-       // Install prerequisites.
        dirInfo, err := build.ScanDir(dir, parent == "")
        if err != nil {
                errorf("%s: %v\n", pkg, err)
                return
        }
+       // Don't allow commands to have trailing '/'
+       if _, f := filepath.Split(pkg); dirInfo.IsCommand() && f != "" {
+               errorf("%s should not have trailing '/'", pkg)
+               return
+       }
        if len(dirInfo.GoFiles)+len(dirInfo.CgoFiles) == 0 {
                errorf("%s: package has no files\n", pkg)
                return
        }
+       // Install prerequisites.
        for _, p := range dirInfo.Imports {
                if p != "C" {
                        install(p, pkg)
                }
        }
        if errors {
                return
        }



Does that look better to you?

Thanks,
Tarmigan

[adg, 2011-08-16 21:08:26]

Why only for commands? It's not valid to import a package with a
trailing slash, either.

Using filepath.Split is fine, although probably unnecessary as all Go
imports should use forward slashes.

There might be some deeper ramifications here. Let me think more about this.

On 17 August 2011 06:47, Tarmigan Casebolt <tarmigan@gmail.com> wrote:
> On Tue, Aug 16, 2011 at 1:07 PM, Andrew Gerrand <adg@golang.org> wrote:
>> I think it's better to do this check here:
>>
>> http://code.google.com/p/go/source/browse/src/cmd/goinstall/main.go#184
>>
>> And you can just use strings.HasSuffix(pkg, "/") as the test.
>
> OK, but I think we only want this check for commands (not packages),
> right?  Also, the context where this originally bit me was using tab
> completion in GOROOT (which included the trailing '/'), so it might be
> nice for windows users if we used filepath (but I have not actually
> tested on windows).
>
> So we'd have to do something like:
>
> diff -r 8ecb24f03c01 src/cmd/goinstall/main.go
> --- a/src/cmd/goinstall/main.go Tue Aug 16 14:56:23 2011 -0400
> +++ b/src/cmd/goinstall/main.go Tue Aug 16 13:44:41 2011 -0700
> @@ -197,50 +197,55 @@
>        // Download remote packages if not found or forced with -u flag.
>        remote, public := isRemote(pkg), false
>        if remote {
>                if err == build.ErrNotFound || (err == nil && *update) {
>                        // Download remote package.
>                        printf("%s: download\n", pkg)
>                        public, err = download(pkg, tree.SrcDir())
>                } else {
>                        // Test if this is a public repository
>                        // (for reporting to dashboard).
>                        m, _ := findPublicRepo(pkg)
>                        public = m != nil
>                }
>        }
>        if err != nil {
>                errorf("%s: %v\n", pkg, err)
>                return
>        }
>        dir := filepath.Join(tree.SrcDir(), pkg)
>
> -       // Install prerequisites.
>        dirInfo, err := build.ScanDir(dir, parent == "")
>        if err != nil {
>                errorf("%s: %v\n", pkg, err)
>                return
>        }
> +       // Don't allow commands to have trailing '/'
> +       if _, f := filepath.Split(pkg); dirInfo.IsCommand() && f != "" {
> +               errorf("%s should not have trailing '/'", pkg)
> +               return
> +       }
>        if len(dirInfo.GoFiles)+len(dirInfo.CgoFiles) == 0 {
>                errorf("%s: package has no files\n", pkg)
>                return
>        }
> +       // Install prerequisites.
>        for _, p := range dirInfo.Imports {
>                if p != "C" {
>                        install(p, pkg)
>                }
>        }
>        if errors {
>                return
>        }
>
>
>
> Does that look better to you?
>
> Thanks,
> Tarmigan
>

[tarm, 2011-08-19 00:34:44]

Hello adg@golang.org, rsc@golang.org, tarmigan+golang@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[adg, 2011-08-20 23:44:42]

Is anyone else getting "Upload in progress." when they try to visit this URL?

http://codereview.appspot.com/4807048/diff/22001/src/cmd/goinstall/main.go

[adg, 2011-08-21 00:01:09]

Tarmigan, can you please run "hg upload" again?

On 21 August 2011 09:44, Andrew Gerrand <adg@golang.org> wrote:
> Is anyone else getting "Upload in progress." when they try to visit this URL?
>
> http://codereview.appspot.com/4807048/diff/22001/src/cmd/goinstall/main.go
>

[tarm, 2011-08-21 07:57:31]

On Sat, Aug 20, 2011 at 5:00 PM, Andrew Gerrand <adg@golang.org> wrote:
> Tarmigan, can you please run "hg upload" again?

Done.

Regarding the patch itself, it now checks both pkgs and cmds as you suggested.

Sorry about the trouble: a few days ago hg change gave me errors near
the end of the upload process, though I didn't write down the problem.
 I retried a few times before checking the issue tracker.  When I
checked, everything on codereview looked OK, but I obviously didn't
check that link.  It should be fixed now though.

Is there good documentation for the hg codereview extension?  I have
reread Contributing and even then have screwed up the codereview
commands/process a couple times.  I'm particularly confused about the
workflow and how to use the change, mail, file, and upload commands.
If I do hg change NNNN does that also upload?  If I do hg mail NNNN
does that upload?  hg upload looks at my working tree (not what I've
committed?) and uploads that, but I only for files that I have added
with hg file?  hg upload knows to use some "current issue number", but
hg mail, file, and change all take an issue number as an argument?  Is
there a way to have multiple issues open at the same time?  I have
some other trivial fixes that have been stuck behind this one.

Thanks,
Tarmigan

[adg, 2011-08-21 10:26:54]

LGTM

On 19 August 2011 10:34,  <tarmigan@gmail.com> wrote:
> Hello adg@golang.org, rsc@golang.org, tarmigan+golang@gmail.com (cc:
> golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/4807048/
>

[adg, 2011-08-21 10:29:11]

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

goinstall: error out with paths that end with '/'

R=adg, rsc, tarmigan+golang
CC=golang-dev
http://codereview.appspot.com/4807048

Committer: Andrew Gerrand <adg@golang.org>

[rsc, 2011-08-22 21:29:20]

http://codereview.appspot.com/4807048/diff/27001/src/cmd/goinstall/main.go
File src/cmd/goinstall/main.go (right):

http://codereview.appspot.com/4807048/diff/27001/src/cmd/goinstall/main.go#ne...
src/cmd/goinstall/main.go:186: if _, f := filepath.Split(pkg); f == "" {
This should be path, not filepath, fwiw.
Or just if strings.HasSuffix.

[tarmigan+golang_gmail.com, 2011-08-22 21:43:55]

On Mon, Aug 22, 2011 at 2:29 PM,  <rsc@golang.org> wrote:
>
> http://codereview.appspot.com/4807048/diff/27001/src/cmd/goinstall/main.go
> File src/cmd/goinstall/main.go (right):
>
>
http://codereview.appspot.com/4807048/diff/27001/src/cmd/goinstall/main.go#ne...
> src/cmd/goinstall/main.go:186: if _, f := filepath.Split(pkg); f == "" {
> This should be path, not filepath, fwiw.

Really?  Windows users may goinstall from $GOPATH.  Everywhere else in
main.go is using filepath.  And path is not imported anywhere in
goinstall from what I can tell.

> Or just if strings.HasSuffix.

If not filepath.

Thanks,
Tarm

[rsc, 2011-08-22 21:52:15]

> Really?  Windows users may goinstall from $GOPATH.  Everywhere else in
> main.go is using filepath.  And path is not imported anywhere in
> goinstall from what I can tell.

I think people should be typing

goinstall x/y/z

and not goinstall x\y\z, no matter what OS they are using.
The syntax of the goinstall arguments is exactly the syntax
of the import lines, and we can't start using different import
lines (import `gosqlite.googlecode.com\hg\sqlite`) on Windows.

Russ

[tarmigan+golang_gmail.com, 2011-08-23 03:24:12]

On Mon, Aug 22, 2011 at 2:52 PM, Russ Cox <rsc@golang.org> wrote:
>> Really?  Windows users may goinstall from $GOPATH.  Everywhere else in
>> main.go is using filepath.  And path is not imported anywhere in
>> goinstall from what I can tell.
>
> I think people should be typing
>
> goinstall x/y/z
>
> and not goinstall x\y\z, no matter what OS they are using.
> The syntax of the goinstall arguments is exactly the syntax
> of the import lines, and we can't start using different import
> lines (import `gosqlite.googlecode.com\hg\sqlite`) on Windows.

For packages that absolutely makes sense, but goinstall can be used
for commands too and for commands that are only on the local
filesystem where it seems likely that different tools (or command line
invokation) may pass `\`.

Aside from the change that I submitted, there are many other places in
goinstall that are using filepath that should be fixed at the same
time.  Additionally, would you want go/build.FindTree() to not split
with filepath as it currently does?  build.FindTree() also returns a
pkg import path that may contain a `\` as the separator on windows.
Should the \ character then be an invalid character in a pkg path?  Or
are you intending that a package (poorly) named `foo\bar` would be in
a nested directory on windows and in a single directory on windows?

I can send a patch if you want, but I don't have a clear vision of
where you want this to end up.

Thanks,
Tarmigan

[rsc, 2011-08-23 03:25:36]

Code manipulating file names should use filepath.
Code manipulating import paths should use path.
The arguments to goinstall should be import paths.

Russ