Issue 4635042: code review 4635042: src/pkg/http/cgi: Handler.Dir for specify working direc... (Closed)
|
Created:
13 years, 9 months ago by mattn Modified:
13 years, 8 months ago Reviewers:
CC:
golang-dev_googlecode.com, bradfitz, brainman, golang-dev Visibility:
Public. |
Descriptionhttp/cgi: add Handler.Dir to specify working directory
Patch Set 1 #Patch Set 2 : diff -r dd3913e4b4b9 http://go.googlecode.com/hg/ #Patch Set 3 : diff -r dd3913e4b4b9 http://go.googlecode.com/hg/ #
Total comments: 4
Patch Set 4 : diff -r 9fef9a578278 http://go.googlecode.com/hg/ #Patch Set 5 : diff -r 9fef9a578278 http://go.googlecode.com/hg/ #Patch Set 6 : diff -r 4795e1786223 http://go.googlecode.com/hg/ #Patch Set 7 : diff -r 4795e1786223 http://go.googlecode.com/hg #Patch Set 8 : diff -r 4795e1786223 http://go.googlecode.com/hg #
Total comments: 6
Patch Set 9 : diff -r 4795e1786223 http://go.googlecode.com/hg #
Total comments: 6
Patch Set 10 : diff -r 7768f924c335 http://go.googlecode.com/hg/ #Patch Set 11 : diff -r 7768f924c335 http://go.googlecode.com/hg/ #Patch Set 12 : diff -r 7768f924c335 http://go.googlecode.com/hg/ #Patch Set 13 : diff -r b295b8bda20b http://go.googlecode.com/hg/ #
MessagesTotal messages: 23
Hello golang-dev@googlecode.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to http://go.googlecode.com/hg/
Sign in to reply to this message.
Could you add a new test, too? http://codereview.appspot.com/4635042/diff/3001/src/pkg/http/cgi/host.go File src/pkg/http/cgi/host.go (right): http://codereview.appspot.com/4635042/diff/3001/src/pkg/http/cgi/host.go#newc... src/pkg/http/cgi/host.go:49: Dir string // working directory of CGI, if empty, use base directory of Path or "." this comment is a bit awkward We'll probably need to break this into its own section and have a multiple-line comment: // Dir specifies the CGI executable's working directory. // If Dir is empty, the base directory of Path is used. // If Path has no base directory, the current working // directory is used. Dir string http://codereview.appspot.com/4635042/diff/3001/src/pkg/http/cgi/host.go#newc... src/pkg/http/cgi/host.go:157: pathBase = h.Path this variable is now named a bit weird. might just call this "path" now.
Sign in to reply to this message.
http://codereview.appspot.com/4635042/diff/3001/src/pkg/http/cgi/host.go File src/pkg/http/cgi/host.go (right): http://codereview.appspot.com/4635042/diff/3001/src/pkg/http/cgi/host.go#newc... src/pkg/http/cgi/host.go:49: Dir string // working directory of CGI, if empty, use base directory of Path or "." On 2011/06/16 21:51:00, bradfitz wrote: > this comment is a bit awkward > > We'll probably need to break this into its own section and have a multiple-line > comment: > > // Dir specifies the CGI executable's working directory. > // If Dir is empty, the base directory of Path is used. > // If Path has no base directory, the current working > // directory is used. > Dir string Done. http://codereview.appspot.com/4635042/diff/3001/src/pkg/http/cgi/host.go#newc... src/pkg/http/cgi/host.go:157: pathBase = h.Path On 2011/06/16 21:51:00, bradfitz wrote: > this variable is now named a bit weird. might just call this "path" now. Done.
Sign in to reply to this message.
Hello golang-dev@googlecode.com, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Were you going to add a test too? On Thu, Jun 16, 2011 at 4:52 PM, <mattn.jp@gmail.com> wrote: > Hello golang-dev@googlecode.com, bradfitz@golang.org (cc: > golang-dev@googlegroups.com), > > Please take another look. > > > > http://codereview.appspot.com/**4635042/<http://codereview.appspot.com/4635042/> >
Sign in to reply to this message.
Hello golang-dev@googlecode.com, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
http://codereview.appspot.com/4635042/diff/5003/src/pkg/http/cgi/host_test.go File src/pkg/http/cgi/host_test.go (right): http://codereview.appspot.com/4635042/diff/5003/src/pkg/http/cgi/host_test.go... src/pkg/http/cgi/host_test.go:314: Dir: cwd, you should make one of the tests (or a new one) be in a directory that's not os.Getwd(). right now both tests have expectedMap "cwd" == os.Getwd(), right? http://codereview.appspot.com/4635042/diff/5003/src/pkg/http/cgi/testdata/tes... File src/pkg/http/cgi/testdata/test.cgi (right): http://codereview.appspot.com/4635042/diff/5003/src/pkg/http/cgi/testdata/tes... src/pkg/http/cgi/testdata/test.cgi:47: print "cwd=".$cwd."\n"; print "cwd=$cwd\n"; ... works http://codereview.appspot.com/4635042/diff/5003/src/pkg/http/cgi/testdata/tes... src/pkg/http/cgi/testdata/test.cgi:49: print "cwd=".`pwd`."n"; why don't you chomp `pwd` here? But really, can't you just do: use Cwd; my $dir = getcwd; That should work on Windows too.
Sign in to reply to this message.
http://codereview.appspot.com/4635042/diff/5003/src/pkg/http/cgi/host_test.go File src/pkg/http/cgi/host_test.go (right): http://codereview.appspot.com/4635042/diff/5003/src/pkg/http/cgi/host_test.go... src/pkg/http/cgi/host_test.go:314: Dir: cwd, On 2011/06/17 01:04:55, bradfitz wrote: > you should make one of the tests (or a new one) be in a directory that's not > os.Getwd(). > > right now both tests have expectedMap "cwd" == os.Getwd(), right? I did test that in following. the first is execute CGI in $GOROOT/src/pkg/http/cgi the second is execute CGI in $GOROOT/src/pkg/http/cgi/testdir Do you need one more another test? http://codereview.appspot.com/4635042/diff/5003/src/pkg/http/cgi/testdata/tes... File src/pkg/http/cgi/testdata/test.cgi (right): http://codereview.appspot.com/4635042/diff/5003/src/pkg/http/cgi/testdata/tes... src/pkg/http/cgi/testdata/test.cgi:47: print "cwd=".$cwd."\n"; On 2011/06/17 01:04:55, bradfitz wrote: > print "cwd=$cwd\n"; > ... works Done. http://codereview.appspot.com/4635042/diff/5003/src/pkg/http/cgi/testdata/tes... src/pkg/http/cgi/testdata/test.cgi:49: print "cwd=".`pwd`."n"; On 2011/06/17 01:04:55, bradfitz wrote: > why don't you chomp `pwd` here? > > But really, can't you just do: > > use Cwd; > my $dir = getcwd; > > That should work on Windows too. Done.
Sign in to reply to this message.
http://codereview.appspot.com/4635042/diff/19001/src/pkg/http/cgi/host_test.go File src/pkg/http/cgi/host_test.go (right): http://codereview.appspot.com/4635042/diff/19001/src/pkg/http/cgi/host_test.g... src/pkg/http/cgi/host_test.go:321: cwd, _ = os.Getwd() this is same as the test above http://codereview.appspot.com/4635042/diff/19001/src/pkg/http/cgi/host_test.g... src/pkg/http/cgi/host_test.go:328: "cwd": cwd, this is same as the test above
Sign in to reply to this message.
http://codereview.appspot.com/4635042/diff/19001/src/pkg/http/cgi/host_test.go File src/pkg/http/cgi/host_test.go (right): http://codereview.appspot.com/4635042/diff/19001/src/pkg/http/cgi/host_test.g... src/pkg/http/cgi/host_test.go:321: cwd, _ = os.Getwd() On 2011/06/20 15:18:35, bradfitz wrote: > this is same as the test above If don't specify Dir, Dir is set base of "/testdata/test.cgi", then "$GOROOT/src/pkg/http/cgi/testdir". First is specified Dir as cwd, then "$GOROOT/src/pkg/http/cgi" Not same. http://codereview.appspot.com/4635042/diff/19001/src/pkg/http/cgi/host_test.g... src/pkg/http/cgi/host_test.go:328: "cwd": cwd, On 2011/06/20 15:18:35, bradfitz wrote: > this is same as the test above same above.
Sign in to reply to this message.
Yes, you've tested that Input1 => Output1 and Input2 => Output1 I want to see: Input3 => Output2 On Mon, Jun 20, 2011 at 5:04 PM, <mattn.jp@gmail.com> wrote: > > http://codereview.appspot.com/**4635042/diff/19001/src/pkg/** > http/cgi/host_test.go<http://codereview.appspot.com/4635042/diff/19001/src/pkg/http/cgi/host_test.go> > File src/pkg/http/cgi/host_test.go (right): > > http://codereview.appspot.com/**4635042/diff/19001/src/pkg/** > http/cgi/host_test.go#**newcode321<http://codereview.appspot.com/4635042/diff/19001/src/pkg/http/cgi/host_test.go#newcode321> > src/pkg/http/cgi/host_test.go:**321: cwd, _ = os.Getwd() > On 2011/06/20 15:18:35, bradfitz wrote: > >> this is same as the test above >> > > If don't specify Dir, Dir is set base of "/testdata/test.cgi", then > "$GOROOT/src/pkg/http/cgi/**testdir". First is specified Dir as cwd, then > > "$GOROOT/src/pkg/http/cgi" > > Not same. > > > http://codereview.appspot.com/**4635042/diff/19001/src/pkg/** > http/cgi/host_test.go#**newcode328<http://codereview.appspot.com/4635042/diff/19001/src/pkg/http/cgi/host_test.go#newcode328> > src/pkg/http/cgi/host_test.go:**328: "cwd": cwd, > On 2011/06/20 15:18:35, bradfitz wrote: > >> this is same as the test above >> > > same above. > > > http://codereview.appspot.com/**4635042/<http://codereview.appspot.com/4635042/> >
Sign in to reply to this message.
The first of expectedMap is {cwd: "$GOROOT/src/pkg/http/cgi" }
(I guess this is meaning Output1)
But second of one is {cwd: "$GOROOT/src/pkg/http/cgi/testdir" }
(I guess this is meaning Output2)
Sorry, I can't what you said.
On 2011/06/21 00:09:48, bradfitz wrote:
> Yes, you've tested that
>
> Input1 => Output1
> and
> Input2 => Output1
>
> I want to see:
>
> Input3 => Output2
>
>
> On Mon, Jun 20, 2011 at 5:04 PM, <mailto:mattn.jp@gmail.com> wrote:
>
> >
> > http://codereview.appspot.com/**4635042/diff/19001/src/pkg/**
> >
>
http/cgi/host_test.go<http://codereview.appspot.com/4635042/diff/19001/src/pkg/http/cgi/host_test.go>
> > File src/pkg/http/cgi/host_test.go (right):
> >
> > http://codereview.appspot.com/**4635042/diff/19001/src/pkg/**
> >
>
http/cgi/host_test.go#**newcode321<http://codereview.appspot.com/4635042/diff/19001/src/pkg/http/cgi/host_test.go#newcode321>
> > src/pkg/http/cgi/host_test.go:**321: cwd, _ = os.Getwd()
> > On 2011/06/20 15:18:35, bradfitz wrote:
> >
> >> this is same as the test above
> >>
> >
> > If don't specify Dir, Dir is set base of "/testdata/test.cgi", then
> > "$GOROOT/src/pkg/http/cgi/**testdir". First is specified Dir as cwd, then
> >
> > "$GOROOT/src/pkg/http/cgi"
> >
> > Not same.
> >
> >
> > http://codereview.appspot.com/**4635042/diff/19001/src/pkg/**
> >
>
http/cgi/host_test.go#**newcode328<http://codereview.appspot.com/4635042/diff/19001/src/pkg/http/cgi/host_test.go#newcode328>
> > src/pkg/http/cgi/host_test.go:**328: "cwd": cwd,
> > On 2011/06/20 15:18:35, bradfitz wrote:
> >
> >> this is same as the test above
> >>
> >
> > same above.
> >
> >
> >
>
http://codereview.appspot.com/**4635042/%3Chttp://codereview.appspot.com/4635...>
> >
Sign in to reply to this message.
LGTM http://codereview.appspot.com/4635042/diff/19001/src/pkg/http/cgi/host_test.go File src/pkg/http/cgi/host_test.go (right): http://codereview.appspot.com/4635042/diff/19001/src/pkg/http/cgi/host_test.g... src/pkg/http/cgi/host_test.go:322: cwd = filepath.Join(cwd, "testdata") I stupid and missed line 322. :/
Sign in to reply to this message.
Never, not LGTM... found a bug in the test. Also, change the first line of the commit message to: http/cgi: add Handler.Dir to specify working directory Then a blank line, then whatever else you'd like, but that's probably enough. http://codereview.appspot.com/4635042/diff/19001/src/pkg/http/cgi/testdata/te... File src/pkg/http/cgi/testdata/test.cgi (right): http://codereview.appspot.com/4635042/diff/19001/src/pkg/http/cgi/testdata/te... src/pkg/http/cgi/testdata/test.cgi:47: $dir = system("cmd /c cd"); system returns the exit status, not the output. You want backticks here: $dir = `cmd /c cd`; Did you test this on Windows? Just to make sure I'm not crazy I just tried it on the EC2 machine and it returns 0.
Sign in to reply to this message.
> Did you test this on Windows? Just to make sure I'm not crazy I just tried it > on the EC2 machine and it returns 0. Sorry, test was skipped. X-( I added code for windows. This require perl. If you don't have perl, test will be skipped. I checked this with strawberry perl and msys's perl. Thanks.
Sign in to reply to this message.
Hmm, way too much "if syscall.OS == "windows"" in this CL.
It should be runtime.GOOS == "windows" if anything, but even that is gross.
Why does just this one test need all this Windows-specific stuff, but all
the other tests are fine unmodified? The #!/usr/bin/perl works for the
other tests. Why do you need to exec.LookPath("perl")?
It was a nice small patch before, but now it's littered with if (WINDOWS)
everywhere. :-/
And why is $| = 1 (auto-flush) needed now?
On Wed, Jun 22, 2011 at 7:05 PM, <mattn.jp@gmail.com> wrote:
> Did you test this on Windows? Just to make sure I'm not crazy I just
>>
> tried it
>
>> on the EC2 machine and it returns 0.
>>
>
> Sorry, test was skipped. X-(
>
> I added code for windows. This require perl. If you don't have perl,
> test will be skipped. I checked this with strawberry perl and msys's
> perl.
>
> Thanks.
>
>
>
>
http://codereview.appspot.com/**4635042/<http://codereview.appspot.com/4635042/>
>
Sign in to reply to this message.
I updated this CL. I tried to make this patch more simply. But, after all, I splitted function TestDirUnix/TestDirWindows.
Sign in to reply to this message.
This fails for me if I patch it in to my Windows box. === RUN cgi.TestDirWindows sh: C:Windowssystem32cmd.exe: command not found sh: C:Windowssystem32cmd.exe: command not found --- FAIL: cgi.TestDirWindows (0.51 seconds) for key "cwd" got ""; expected "c:\\go\\src\\pkg\\http\\cgi" for key "cwd" got ""; expected "C:\\MinGW\\msys\\1.0\\bin" I still don't understand why you're doing this test differently for Windows, but all the other tests run fine without knowing about Windows-vs-Posix. You must be using some different Windows environment, but it's not the same one the rest of the Windows Go team is using?
Sign in to reply to this message.
On 2011/06/24 23:33:43, bradfitz wrote: > > I still don't understand why you're doing this test differently for Windows, On unix, you could run your perl shell script in 2 different ways: 1) /usr/bin/perl ./test.cgi 2) ./test.cgi You can do 2) because ./test.cgi is marked as executable and it has #!/usr/bin/perl at the start of the script. Windows doesn't have such convention. Your only choice is 1). If you want unix and windows code to be the same, you have to use approach 1). Alex PS: Don't know why the test fails. I know nothing about perl.
Sign in to reply to this message.
> PS: Don't know why the test fails. I know nothing about perl. Thanks for your expaining. I'll look into the fails tomorrow.
Sign in to reply to this message.
PTAL On 2011/06/26 13:40:04, mattn wrote: > > PS: Don't know why the test fails. I know nothing about perl. > > Thanks for your expaining. I'll look into the fails tomorrow.
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=04322400fed3 *** http/cgi: add Handler.Dir to specify working directory R=golang-dev, bradfitz, alex.brainman CC=golang-dev http://codereview.appspot.com/4635042 Committer: Brad Fitzpatrick <bradfitz@golang.org>
Sign in to reply to this message.
LGTM Tests pass on Windows and Linux. On Thu, Jun 30, 2011 at 9:44 AM, <bradfitz@golang.org> wrote: > *** Submitted as > http://code.google.com/p/go/**source/detail?r=04322400fed3<http://code.google... > > > http/cgi: add Handler.Dir to specify working directory > > R=golang-dev, bradfitz, alex.brainman > CC=golang-dev > > http://codereview.appspot.com/**4635042<http://codereview.appspot.com/4635042> > > Committer: Brad Fitzpatrick <bradfitz@golang.org> > > > > http://codereview.appspot.com/**4635042/<http://codereview.appspot.com/4635042/> >
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
