code review 7181047: path/filepath: SplitList shouldn't split quoted paths o...

path/filepath, os/exec: unquote PATH elements on Windows

On Windows, directory names in PATH can be fully or partially quoted
in double quotes ('"'), but the path names as used by most APIs must
be unquoted. In addition, quoted names can contain the semicolon
(';') character, which is otherwise used as ListSeparator.

This CL changes SplitList in path/filepath and LookPath in os/exec
to only	treat unquoted semicolons as separators, and to unquote the
separated elements.

(In addition, fix harmless test bug I introduced for LookPath on Unix.)

Related discussion thread:
https://groups.google.com/d/msg/golang-nuts/PXCr10DsRb4/sawZBM7scYgJ

[speter, 2013-01-22 14:48:15]

Hello golang-dev@googlegroups.com,

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

[rsc, 2013-01-22 19:06:27]

Shouldn't it remove the quotes? I'm a bit skeptical about this. I don't
want to get into unquoting here. It's really unfortunate that Windows does
this.

[speter, 2013-01-22 23:49:34]

Perhaps a more succinct description of the change would be: "on Windows,
only split at semicolons that are separators". Splitting at semicolons that
are part of a directory name is clearly always problematic.

Although unquoting is a related issue, I think it could be considered and
(if needed) addressed separately. My opinion on unquoting is that SplitList
shouldn't be changed to remove quotes as the Go1 docs for it don't mention
unquoting. (Even if it is considered unspecified behavior, the benefits of
the change are not obvious.)

Not fixing splitting would mean that Go programs on Windows cannot use
SplitList at all (in a safe manner). Not unquoting just means that Go
programs handling PATH on Windows have to do their own unquoting -- which
some may as well already be doing. (Note that although quoting is only
required for directory names containing semicolons, I think it is much more
commonly used in other cases, often when people think directory names
including spaces or some other perceived "special" characters need or
should be quoted.)




On Wed, Jan 23, 2013 at 4:06 AM, Russ Cox <rsc@golang.org> wrote:

> Shouldn't it remove the quotes? I'm a bit skeptical about this. I don't
> want to get into unquoting here. It's really unfortunate that Windows does
> this.
>
>

[rsc, 2013-01-23 02:44:53]

I certainly expect to be able to use the split path elements directly in
constructing path names, without further processing. There's no unquoting
on non-Windows systems.

[minux1, 2013-01-23 11:53:05]

On Wed, Jan 23, 2013 at 7:49 AM, speter <speter.go1@gmail.com> wrote:

> Not fixing splitting would mean that Go programs on Windows cannot use
> SplitList at all (in a safe manner). Not unquoting just means that Go
> programs handling PATH on Windows have to do their own unquoting -- which
> some may as well already be doing. (Note that although quoting is only
> required for directory names containing semicolons, I think it is much more
> commonly used in other cases, often when people think directory names
> including spaces or some other perceived "special" characters need or
> should be quoted.)
>
both of the cases mean that a Go user must do something specially on
Windows, and
i think this defeated the goal of path/filepath which serve as portable
file path handling
package.

[mccoyst_gmail.com, 2013-01-23 14:23:40]

On Wednesday, January 23, 2013 6:52:44 AM UTC-5, minux wrote:

>
> On Wed, Jan 23, 2013 at 7:49 AM, speter <spete...@gmail.com
<javascript:>>wrote:
>
>> Not fixing splitting would mean that Go programs on Windows cannot use 
>> SplitList at all (in a safe manner). Not unquoting just means that Go 
>> programs handling PATH on Windows have to do their own unquoting -- which 
>> some may as well already be doing. (Note that although quoting is only 
>> required for directory names containing semicolons, I think it is much more 
>> commonly used in other cases, often when people think directory names 
>> including spaces or some other perceived "special" characters need or 
>> should be quoted.)
>>
> both of the cases mean that a Go user must do something specially on 
> Windows, and
> i think this defeated the goal of path/filepath which serve as portable 
> file path handling
> package.
>

Thirded. Double quotes are purely control characters in Windows's PATH and 
they aren't valid inside of path or file names, so they shouldn't come out 
of a function like SplitList that is (potentially) interpreting them.

[speter, 2013-01-23 14:43:30]

Sorry, I'm afraid I wasn't clear / specific enough.

I agree that returning the "bare" (on Windows, unquoted) path elements is
more commonly needed, and that splitting and unquoting would be the more
desirable semantics when designing a new library.

My concern was that the original behavior of SplitList, i.e. splitting
without unquoting, can also be considered a meaningful semantics (although
it is useful in much fewer cases). So changing SplitList to unquote path
elements on Windows has the potential to break existing valid programs that
rely on the existing (non-unquoting) behavior. (Since the original docs for
SplitList mention splitting but don't mention unquoting, it is not
unreasonable for programs to expect non-unquoted strings.)

But in practice this is very unlikely to affect any programs in a negative
way, so it is mostly a "legal" concern. If it is not considered to be an
issue, I'm glad to update the CL to also do the unquoting in SplitList.

(An example of an alternative solution to the unquoting problem that
doesn't involve what can be considered a potentially incompatible change
would be to add a new function, such as SplitListBare, and document that
programs that want to obtain the "bare" path elements in a portable manner
should use the new function.)

[brainman, 2013-01-30 06:34:21]

On 2013/01/23 14:43:30, speter wrote:
> 
> I agree that returning the "bare" (on Windows, unquoted) path elements is
> more commonly needed, and that splitting and unquoting would be the more
> desirable semantics when designing a new library.
> 

It looks like Windows APIs we use do not accept paths with " in it -
http://play.golang.org/p/HxFuvF_YEi outputs:

2013/01/30 17:28:00 openfile("\"C:\\WINDOWS\\system32\\cmd.exe\""): open "C:\WIN
DOWS\system32\cmd.exe": The filename, directory name, or volume label syntax is
incorrect.
exit status 1

on my system. So paths with " aren't very useful.

> My concern was that the original behavior of SplitList, i.e. splitting
> without unquoting, can also be considered a meaningful semantics (although
> it is useful in much fewer cases). So changing SplitList to unquote path
> elements on Windows has the potential to break existing valid programs that
> rely on the existing (non-unquoting) behavior. (Since the original docs for
> SplitList mention splitting but don't mention unquoting, it is not
> unreasonable for programs to expect non-unquoted strings.)

Like others said, the purpose of this package is to deal with file-paths in a
portable way: you start with a string, and you end up with something that can be
used to open files and navigate filesystem. In that regard, paths with " aren't
useable. It is a shortcoming on our part that we aren't removing quotes. I think
we should implement and document this behavior.

That is, if Go Team considers that change not breaking Go1 "promise".

> (An example of an alternative solution to the unquoting problem that
> doesn't involve what can be considered a potentially incompatible change
> would be to add a new function, such as SplitListBare, and document that
> programs that want to obtain the "bare" path elements in a portable manner
> should use the new function.)

I am against creating new function. It would be hard to come-up with a good
name. Also everyone is using old function already anyway, so we would have to
educate users. Hopefully number of users of SplitList that need to keep quotes
is small, and they can use strings.Split(path, string(ListSeparator)) instead.

Alex

[speter, 2013-01-30 13:46:35]

I believe everyone who has commented on this issue so far agrees that:

1. SplitList splitting a path at semicolons that are not separators (but parts
of directory names) is clearly a bug that must be fixed. 
2. Disregarding backwards compatibility, the behavior that would benefit the
most users would be for SplitList to unquote list elements on Windows.

But as SplitList is part of Go 1, I don't think we can disregard backwards
compatibility.

My interpretation of the situation is that:
"1." above (and the current version of this CL) is a change that fixes an
implementation bug, and thus brings the implementation in line with the
specification (API docs), which describe splitting on separators, but do not
mention unquoting.
"2." above would be a change from one meaningful semantics to another. Although
the new one would be more "popular", I don't think the decision on a potentially
incompatible change should be made based on popularity.

I'm fairly certain that changing "1." doesn't break any existing (meaningful)
program. On the other hand, changing "2." has the potential of breaking programs
that have been implemented to be consistent with the existing specification (API
docs) and tested to work correctly. Note that programs can do a lot of things
with the splitted path besides passing it to Go file handling API's (such as
passing it to other programs that may rely on quotation being retained), so I
don't think how other Go API's handle quotes should be a decisive factor in this
case (although it undoubtedly affects "popularity" significantly).

The likelihood that such a change actually breaks some programs in practice may
be quite low (although there is no way to really know), but if it is so, it is
just because there are still relatively few Go users (especially on Windows).
But I don't think that should be a decisive factor either. And even if no one is
affected in this case, such a change could also send a negative signal regarding
the protections of go1compat.html. So far my expectation has been that
incompatible changes (other than clear bugfixes) are generally avoided (but the
right to do so is retained for some cases, for situations when it is practically
inevitable). I think that a precedent of making a potentially incompatible
change based on popularity, or based on some estimation that it affects just a
small minority of users (when it could be easily avoided by just adding a new
function) could make many people think twice before considering Go for serious
projects.

But all this is just my individual interpretation, opinion and expectation, so
it in no way authoritative.

Could someone from the Go Team explain how go1compat.html is intended to apply
in this case?

[iant2, 2013-01-30 15:02:27]

On Wed, Jan 30, 2013 at 5:46 AM,  <speter.go1@gmail.com> wrote:
>
> Could someone from the Go Team explain how go1compat.html is intended to
> apply in this case?

The Go 1 compatibility rules permit us to change a library function if
it does not implement its documented behaviour.  In this case the
documented behaviour is to split a list of paths.  This function is
documented as being OS-specific.  If a Windows path list naturally
includes quoted entries, and if the normal behaviour of a function
that splits a Windows path would include unquoting entries, then I
think it would be reasonable for this function to unquote entries.
That would be a reasonable application of the Go 1 compatibility
rules.

It's worth considering what would happen to a reasonable program
moving from Go 1 to Go 1.1.  Presumably a reasonable program would
either 1) ignore quoting, or 2) do the unquoting itself.  A program
that ignores unquoting would continue to work under Go 1.1, and in
fact might work better.  What would happen to a program that currently
does unquoting itself?  I don't know the quoting rules.

It's also worth considering whether we provide any facility for using
SplitList to pull apart a path list, then add or remove an entry, and
then put the result back into a path list again.  As far as I know we
have no facility for that.  Any such facility would presumably need to
do quoting on Windows.  And, actually, that is a case where a Go 1
program that ignores quoting when using SplitList would fail if
SplitList is changed.

Anyhow, I don't see a very strong argument, but I think there is a
reasonable argument that the Go 1 rules permit us to change the
function to do unquoting if we think it is the right thing to do.

Ian

[rsc, 2013-01-30 16:17:56]

If we're going to change anything on Windows, I believe the correct change
is to:

1. Make SplitList know about quotes _and_ do the unquoting.
2. Make JoinList (when it happens) reintroduced quotes as needed.

Russ

[brainman, 2013-01-31 02:10:00]

I can see 3 choices here:

1) We accept this CL as is (means no unquoting). Then we have a potential
problem with go command - it uses filepath.SplitPath, and filepath.SplitPath
will return quoted directory names, and these will fail to be walked (all this
needs to be verified).

2) We could change filepath.SplitPath to unquote.

3) We accept this CL as is (means no unquoting). And we add new split-path
function that does unquoting. And we use new function everywhere in go command
instead of filepath.SplitPath.

I think 1) is unacceptable. I think 3) is too convoluted (2 different functions
to do the same thing). 2) wins for me.

I do not see downside with 2). I cannot see any reasonable program that works
now, but would be broken if we change as per 2). Can you?

Maybe there other alternatives. What are they?

Alex

[rsc, 2013-01-31 02:11:06]

I agree with you: (2) is the right choice.

Russ

[speter, 2013-02-05 23:11:50]

Thank you for the comments, and apologies for the delay and the lengthy message
-- I should have discussed these things before sending the CL.

I did not intend the CL by itself ("(1)" above) to solve all related issues. And
in fact I don't think (2) by itself is a solution, although it can be part of
the implementation of a solution.

I'd also like to clarify that in (3) above, the two functions would not be
"doing the same thing" -- there are two different meaningful semantics. In most
cases callers will need the unquoted version. But in general, if elements of a
path are later used as part of a new path, it can be necessary and/or preferable
to use the non-unquoted version, rather than (or in addition to) the unquoted
one. Although the two functions would behave the same for non-Windows platforms,
I think keeping this distinction in mind is essential for writing programs that
handle paths correctly in a portable manner.

I think there are several issues here, with the underlying problem being the
lack of consideration for this distinction throughout the Go distribution (it
was neither considered for SplitList in path/filepath, nor for the go tool). I
don't think of SplitList as being the cause of issues for the go tool, but the
two being occurrences of the same underlying problem -- the problem could have
been discovered either by more thorough testing of SplitList or by more thorough
testing of the go tool. And I've found that LookPath in os/exec also has the
same issue (and a separate issue on Unix as well).

I think the fundamental solution would be to review all path handling code in
the Go distribution (be it using filepath.SplitList, strings.Split,
strings.Index, for loop, or anything) and check at each location whether
unquoting is needed or not. E.g., while it is quite possible that all calls to
SplitList in the go tool need the unquoted version, I think the only way to be
really certain is to actually review the code at those invocations. (And, since
we can't review all code in the world that uses SplitList, users should be
educated about the necessity of distinction for portable code.)

The advantage of basing the solution on (3) would be that it can be implemented
in an incremental manner, assessing the concrete effects of each step while
reviewing related code, and being able to ensure that changes do not affect
other parts. In contrast, I think for (2) the effects can only be assessed
probabilistically. When working on code that is being used in production, I
generally prefer incremental changes where the concrete effects can be assessed.

More specifically, I believe (2) makes two assumptions: first that all users of
SplitList also lacked the consideration for the distinction among the two
semantics, and second that such users would have always intended the unquoting
semantics. While I agree that both are quite likely true in the majority of
cases, I prefer to avoid making changes based on such assumptions.

But I admit that these criteria are somewhat subjective, that I'm quite possibly
overly cautious, and that there are other important factors. Most importantly, I
agree it would be easier for those writing new code if they could use SplitList
for doing split+unquote, rather than having to use a longer function name. And I
also agree that the probability of issues with other approaches seems to be very
low.

To summarize, I can see a range of possible approaches, all including either (2)
or (3), fixing exec.LookPath, and potentially reviewing some or all related
parts of the Go distribution source:

(2-1)
Do (2) above and fix exec.LookPath. Hope that everything else works out right:
the change fixes the go tool, there are no other affected parts in the Go
distribution, and it doesn't break any external code.

(2-2)
First, do (2), review / fix the go tool, and fix exec.LookPath. After these
changes are committed, consider it tentatively fixed, but continue reviewing
relevant code in the Go distribution to make sure there are no other affected
parts. Hope that it doesn't break any external code, and that the initial change
doesn't cause temporary degradation (that could be later fixed throughout the
review).

(2-3)
Review all relevant code in the Go distribution. After identifying all
potentially affected parts, implement and commit (2) with all relevant fixes,
including for go tool and exec.LookPath. Hope that the change does not break any
external code.

(3-1)
Same as (2-3) but use (3) instead of (2):
Review all relevant code in the Go distribution. After identifying all
potentially affected parts, implement and commit (3) with all relevant fixes,
including for go tool and exec.LookPath. (External code is not affected.)

(3-2)
Incrementally implement (3) and continue to review and fix relevant parts of the
Go distribution source step by step. This way each step fixes a specific issue
and effects to other code can be assessed specifically. E.g.:
S1: fix SplitList to not split at characters that are not separators (this CL)
S2: add a new fuction that also unquotes list elements on Windows
S3 onwards: review each component of the Go distribution that handles
    paths, one by one, starting with the os/exec and the go tool;
    decide whether the quoted or unquoted version is needed at each
    location, and fix the implementation where necessary

My preferred one is (3-2) as it allows to fix the issues in an incremental
manner and assess the concrete effect at each step. Having said that, I'm not
aware of any specific issues with the other approaches. (But neither have I done
a review of related code -- although I think reviewing some or all related code
should be done, I realized I should confirm the preferred approach and extent
first. The only part I did review was exec.LookPath, which I stumbled upon
mostly by chance.)

Which of these (or what other approach) would you consider to be suitable in
this case?

(Sorry again for not starting with discussing these things.)

Peter

[speter, 2013-02-05 23:21:21]

As for the quoting rules, I haven't found a definitive source but based on some
experimenting the following seems to be consistent with Path behavior on Windows
(XP SP3 32bit with NTFS).

1. Semicolon (';') characters preceded by an even number of double quote ('"')
characters are list separators, whereas semicolons preceded by an odd number of
double quotes are part of a directory name.
2. After splitting at separators, remove all double-quote characters from the
list elements.

This means that any part or parts of a list entry can be quoted, and unmatched
double quote is tolerated.

I will post the findings in more detail later on.

Peter

[brainman, 2013-02-06 06:35:14]

On 2013/02/05 23:11:50, speter wrote:
> ... And I've found that LookPath in os/exec also has the
> same issue ...

Good catch. I think we should fix it too.

> My preferred one is (3-2) ...

I prefer to do (2) above and fix exec.LookPath.

Alex

[brainman, 2013-02-06 06:39:13]

On 2013/02/05 23:21:21, speter wrote:
> As for the quoting rules, I haven't found a definitive source but based on
some
> experimenting the following seems to be consistent with Path behavior on
Windows
> ...

Sounds reasonable. Tests will help us clarify things.

> I will post the findings in more detail later on.

Looking forward to see the results. Thank you.

Alex

[speter, 2013-02-17 16:02:30]

Please take another look.

Sorry for the delay again. Changes from previous version:
- updated description and implementation based on (2) above
- applied the same fix to LookPath in os/exec
- moved godoc back to path.go to share documentation among platforms
  (so that Unix users who want to write portable code are made aware
  of the quoted/unquoted distinction on Windows)
- fixed harmless test bug for LookPath on Unix that I introduced in
  CL 7305053 (which fixed a related issue I found while looking into this)

I also created, as a separate CL (not for submission), a validating test
to check that the unquoting behavior, as described in the previous
comments and implemented in the current CL, is in fact consistent with
the Windows behavior: https://codereview.appspot.com/7346049/

Peter

[minux1, 2013-02-17 16:08:12]

On Mon, Feb 18, 2013 at 12:02 AM, <speter.go1@gmail.com> wrote:

> I also created, as a separate CL (not for submission), a validating test
> to check that the unquoting behavior, as described in the previous
> comments and implemented in the current CL, is in fact consistent with
> the Windows behavior:
https://codereview.appspot.**com/7346049/<https://codereview.appspot.com/7346...

how long does this test take?
it it can finish in reasonable time (a few second), i suggest we include
the test.
perhaps we can only run it in short mode, but as not everybody develop
on Windows, making the builder test it seems worthwhile.

[speter, 2013-02-18 18:03:47]

Please take another look.

Cleaned up and merged the validation test as path_windows_test.go.

[brainman, 2013-02-19 01:38:36]

Thank you. Most of your changes look good to me. Just test needs bit of
polishing.

Alex

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path....
src/pkg/path/filepath/path.go:178: // On Windows, directory names in PATH can be
fully or partially quoted in
I think quotes / semicolons in path are exceptions. I do not think we need to
describe gritty details of how this splitting works on Windows here. It provides
no help to most users of this function, just makes them read more of useless
text.

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
File src/pkg/path/filepath/path_windows_test.go (right):

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows_test.go:12: // Validate that expected results
in the tests for SplitList on Windows
Maybe if you rename this test to TestWinSplitListTestsAreValid you will not need
any comment here.

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows_test.go:53: for ti, tt := range
winsplitlisttests {
Everything inside this loop needs to go into a separate function. This will
solve some of your problems:
- "defer os.RemoveAll(tmp)" will actually execute after every iteration, so you
won't need to delete selected files at the end;
- you won't need "continue outer";

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows_test.go:55: errFatalf(err, "TempDir failed:
%v", err)
Please, do not use errFatalf. t.Fatalf will print source code line number, and
that will be useless, if you insist on using errFatalf.

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows_test.go:58: err = os.Chdir(tmp)
Do not os.Chdir. It is not safe to do in general. Your test leaves unremoved
directories in TMP, because of os.Chdir. You can do everything without os.Chdir
- just use full paths everywhere where it is required.

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows_test.go:65: t.Logf("%d,%d: already exists,
skipping: %#q", ti, i, d)
Shouldn't this be an error instead? Maybe with some exceptions when d == "".

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows_test.go:71: t.Logf("%d,%d: %#q refers outside
working directory, skipping", ti, i, d)
This should be an Errorf, not Logf. I don't think we want tests that do not meet
this condition.

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows_test.go:76: t.Logf("%d,%d: MkdirAll(%#q)
failed, skipping: %v", ti, i, d, err)
This should be an Errorf, not Logf.

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows_test.go:80: f, err := os.OpenFile(fn,
os.O_CREATE|os.O_EXCL, perm)
Maybe http://tip.golang.org/pkg/io/ioutil/#WriteFile

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows_test.go:82: t.Logf("%d,%d: OpenFile(%#q)
failed, skipping: %v", ti, i, fn, err)
This should be an Errorf, not Logf.

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows_test.go:108: t.Errorf("%d,%d: expected %v,
got %v", ti, i, exp, out)
s/%v/%q/
otherwise it is difficult to see the output

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows_test.go:111: err = os.Remove(filepath.Join(d,
cmdfile))
Do you really need to do that? os.RemoveAll up above should delete everything
anyway.

[rsc, 2013-02-19 15:58:10]

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
File src/pkg/path/filepath/path_windows.go (right):

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows.go:86: fp = fp + string(c)
This generates a quadratic amount of garbage, because if you're processing
abcdefghi it has to allocate the strings a, ab, abc, abcd, abcde, abcdef,
abcdefg, abcdefgh, abcdefghi. And paths do get long on Windows. Since package
strings is already imported, you can do something like this:

// Split path, respecting but preserving quotes.
list := []string{}
start := 0
quo := false
for i := 0; i < len(path); i++ {
    switch c := path[i]; {
    case c == '"':
        quo = !quo
    case c == ListSeparator && !quo:
        list = append(list, path[start:i])
        start = i+1
    }
}
list = append(list, path[start:])

// Remove quotes.
for i, s := range list {
    if strings.Contains(s, `"`) {
        list[i] = strings.Replace(s, `"`, ``, -1)
    }
}

return list

[speter, 2013-02-19 17:20:10]

Please take another look.

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path....
src/pkg/path/filepath/path.go:178: // On Windows, directory names in PATH can be
fully or partially quoted in
On 2013/02/19 01:38:36, brainman wrote:
> I think quotes / semicolons in path are exceptions. I do not think we need to
> describe gritty details of how this splitting works on Windows here. It
provides
> no help to most users of this function, just makes them read more of useless
> text.

Done.

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
File src/pkg/path/filepath/path_windows.go (right):

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows.go:86: fp = fp + string(c)
On 2013/02/19 15:58:10, rsc wrote:
> This generates a quadratic amount of garbage, because if you're processing
> abcdefghi it has to allocate the strings a, ab, abc, abcd, abcde, abcdef,
> abcdefg, abcdefgh, abcdefghi. And paths do get long on Windows. Since package
> strings is already imported, you can do something like this:
> 
> // Split path, respecting but preserving quotes.
> list := []string{}
> start := 0
> quo := false
> for i := 0; i < len(path); i++ {
>     switch c := path[i]; {
>     case c == '"':
>         quo = !quo
>     case c == ListSeparator && !quo:
>         list = append(list, path[start:i])
>         start = i+1
>     }
> }
> list = append(list, path[start:])
> 
> // Remove quotes.
> for i, s := range list {
>     if strings.Contains(s, `"`) {
>         list[i] = strings.Replace(s, `"`, ``, -1)
>     }
> }
> 
> return list

Done.

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
File src/pkg/path/filepath/path_windows_test.go (right):

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows_test.go:12: // Validate that expected results
in the tests for SplitList on Windows
On 2013/02/19 01:38:36, brainman wrote:
> Maybe if you rename this test to TestWinSplitListTestsAreValid you will not
need
> any comment here.

Done.

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows_test.go:53: for ti, tt := range
winsplitlisttests {
On 2013/02/19 01:38:36, brainman wrote:
> Everything inside this loop needs to go into a separate function. This will
> solve some of your problems:
> - "defer os.RemoveAll(tmp)" will actually execute after every iteration, so
you
> won't need to delete selected files at the end;
> - you won't need "continue outer";

Done.

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows_test.go:55: errFatalf(err, "TempDir failed:
%v", err)
On 2013/02/19 01:38:36, brainman wrote:
> Please, do not use errFatalf. t.Fatalf will print source code line number, and
> that will be useless, if you insist on using errFatalf.

Done.

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows_test.go:58: err = os.Chdir(tmp)
On 2013/02/19 01:38:36, brainman wrote:
> Do not os.Chdir. It is not safe to do in general. Your test leaves unremoved
> directories in TMP, because of os.Chdir. You can do everything without
os.Chdir
> - just use full paths everywhere where it is required.

Done.

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows_test.go:65: t.Logf("%d,%d: already exists,
skipping: %#q", ti, i, d)
On 2013/02/19 01:38:36, brainman wrote:
> Shouldn't this be an error instead? Maybe with some exceptions when d == "".

Done.

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows_test.go:71: t.Logf("%d,%d: %#q refers outside
working directory, skipping", ti, i, d)
On 2013/02/19 01:38:36, brainman wrote:
> This should be an Errorf, not Logf. I don't think we want tests that do not
meet
> this condition.

Done.

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows_test.go:76: t.Logf("%d,%d: MkdirAll(%#q)
failed, skipping: %v", ti, i, d, err)
On 2013/02/19 01:38:36, brainman wrote:
> This should be an Errorf, not Logf.

Done.

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows_test.go:80: f, err := os.OpenFile(fn,
os.O_CREATE|os.O_EXCL, perm)
On 2013/02/19 01:38:36, brainman wrote:
> Maybe http://tip.golang.org/pkg/io/ioutil/#WriteFile

Done.

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows_test.go:82: t.Logf("%d,%d: OpenFile(%#q)
failed, skipping: %v", ti, i, fn, err)
On 2013/02/19 01:38:36, brainman wrote:
> This should be an Errorf, not Logf.

Done.

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows_test.go:108: t.Errorf("%d,%d: expected %v,
got %v", ti, i, exp, out)
On 2013/02/19 01:38:36, brainman wrote:
> s/%v/%q/
> otherwise it is difficult to see the output

Done.

https://codereview.appspot.com/7181047/diff/33001/src/pkg/path/filepath/path_...
src/pkg/path/filepath/path_windows_test.go:111: err = os.Remove(filepath.Join(d,
cmdfile))
On 2013/02/19 01:38:36, brainman wrote:
> Do you really need to do that? os.RemoveAll up above should delete everything
> anyway.
Current test flow requires removing cmdfile to unshadow the one in the next
directory (added comment). Other potential approaches are iterating from last
dir to first placing one command a time (shadowing the previous ones), or using
a different command name in each dir (although this doesn't validate the order
of results).

[rsc, 2013-02-19 17:22:04]

LGTM

Leaving for Alex.

[brainman, 2013-02-20 05:19:35]

LGTM

Thank you.

Alex

[brainman, 2013-02-20 05:20:00]

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

path/filepath, os/exec: unquote PATH elements on Windows

On Windows, directory names in PATH can be fully or partially quoted
in double quotes ('"'), but the path names as used by most APIs must
be unquoted. In addition, quoted names can contain the semicolon
(';') character, which is otherwise used as ListSeparator.

This CL changes SplitList in path/filepath and LookPath in os/exec
to only	treat unquoted semicolons as separators, and to unquote the
separated elements.

(In addition, fix harmless test bug I introduced for LookPath on Unix.)

Related discussion thread:
https://groups.google.com/d/msg/golang-nuts/PXCr10DsRb4/sawZBM7scYgJ

R=rsc, minux.ma, mccoyst, alex.brainman, iant
CC=golang-dev
https://codereview.appspot.com/7181047

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