Issue 8633043: code review 8633043: cmd/go: quote empty command line arguments in debug output (Closed)
|
Created:
11 years, 11 months ago by volker.dobler Modified:
11 years, 11 months ago Reviewers:
CC:
golang-dev, bradfitz, minux1, r Visibility:
Public. |
Descriptioncmd/go: quote command line arguments in debug output
Debug output from go test -x may contain empty arguments.
This CL quotes arguments if needed. E.g. the output of
go test -x is now
.../6g -o ./_go_.6 -p testmain -complete -D "" -I . -I $WORK ./_testmain.go
which is easier to grasp.
Patch Set 1 #Patch Set 2 : diff -r b27b1ff18f39 https://code.google.com/p/go/ #Patch Set 3 : diff -r b27b1ff18f39 https://code.google.com/p/go/ #Patch Set 4 : diff -r b27b1ff18f39 https://code.google.com/p/go/ #
Total comments: 1
Patch Set 5 : diff -r b3017cc3e17b https://code.google.com/p/go/ #Patch Set 6 : diff -r b3017cc3e17b https://code.google.com/p/go/ #
Total comments: 1
Patch Set 7 : diff -r 9bb42a7021c9 https://code.google.com/p/go/ #
Total comments: 1
MessagesTotal messages: 18
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
Sign in to reply to this message.
I don't think quotedJoin here needs to be as performance-sensitive as
strings.Join. I'd just make it super simple instead:
// quotedJoin works like strings.Join(a, " ") but joins empty strings as "".
func quotedJoin(a []string) string {
var b []byte
for i, s := range a {
if i > 0 {
b = append(b, ' ')
}
if s == "" {
b = append(b, '"', '"')
} else {
b = append(b, s...)
}
}
return string(b)
}
But this is still not a complete solution if the problem is "I want to
copy/paste this into my shell".
I'm not opposed to it, though. This seems fine.
On Wed, Apr 10, 2013 at 11:12 AM, <dr.volker.dobler@gmail.com> wrote:
> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> cmd/go: quote empty command line arguments in debug output
>
> Debug output from go test -x may contain empty arguments.
> This CL displays them as "". E.g. the output of go test -x
> is now
> .../6g -o ./_go_.6 -p testmain -complete -D "" -I . -I $WORK
> ./_testmain.go
> which is easier to grasp and can be re-run directly.
>
> Please review this at
https://codereview.appspot.**com/8633043/<https://codereview.appspot.com/8633...
>
> Affected files:
> M src/cmd/go/build.go
>
>
> Index: src/cmd/go/build.go
> ==============================**==============================**=======
> --- a/src/cmd/go/build.go
> +++ b/src/cmd/go/build.go
> @@ -1236,7 +1236,7 @@
> func (b *builder) runOut(dir string, desc string, cmdargs ...interface{})
> ([]byte, error) {
> cmdline := stringList(cmdargs...)
> if buildN || buildX {
> - b.showcmd(dir, "%s", strings.Join(cmdline, " "))
> + b.showcmd(dir, "%s", quotedJoin(cmdline))
> if buildN {
> return nil, nil
> }
> @@ -1303,6 +1303,41 @@
> }
> }
>
> +// quotedJoin works like strings.Join(a, " ") but joins empty strings as
> "".
> +func quotedJoin(a []string) string {
> + if len(a) == 0 {
> + return ""
> + }
> + if len(a) == 1 {
> + if a[0] == "" {
> + return `""`
> + }
> + return a[0]
> + }
> + n := len(a) - 1
> + for i := 0; i < len(a); i++ {
> + if w := len(a[i]); w > 0 {
> + n += w
> + } else {
> + n += 2
> + }
> + }
> +
> + b := make([]byte, n)
> + bp := 0
> + for i, s := range a {
> + if i > 0 {
> + bp += copy(b[bp:], " ")
> + }
> + if s == "" {
> + bp += copy(b[bp:], `""`)
> + } else {
> + bp += copy(b[bp:], s)
> + }
> + }
> + return string(b)
> +}
> +
> // mkdir makes the named directory.
> func (b *builder) mkdir(dir string) error {
> b.exec.Lock()
>
>
> --
>
> ---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...
> .
>
>
>
Sign in to reply to this message.
PTAL. I admit it is not a solution to copy/paste in the shell, but I struggled 10 minutes why I couldn't re-run the output. Looking up -D and counting spaces made it clear. This patch helps a bit here.
Sign in to reply to this message.
LGTM. I vote for get this in.
Sign in to reply to this message.
LGTM but waiting for r or adg before submitting. On Wed, Apr 10, 2013 at 1:21 PM, <minux.ma@gmail.com> wrote: > LGTM. > > I vote for get this in. > > https://codereview.appspot.**com/8633043/<https://codereview.appspot.com/8633... >
Sign in to reply to this message.
https://codereview.appspot.com/8633043/diff/9001/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/8633043/diff/9001/src/cmd/go/build.go#newcode1320 src/cmd/go/build.go:1320: } i see the point, but it's a little odd and specific. if you're going to quote, maybe quote for real? func quotedJoin(s []string) string { r := fmt.Sprintf("%q", s) return r[1:len(r)-1] } since this is debug output, it might be nice to see if there are frogs. i could see not doing that, too, since it means you can't cut and paste, but i think for debugging full information is more important. if you prefer the incomplete but easier to copy-paste version, use a byte buffer: func quotedJoin(strs []string) string { var b bytes.Buffer for _, s := range strs { b.WriteByte('"') b.WriteString(s) b.WriteByte('"') b.WriteByte(' ') } return b.String() } i don't believe the trailing space is worth the if statement to eliminate it, but put it in if you want. but what you have is also ok.
Sign in to reply to this message.
Notice my versions also protect spaces.
Sign in to reply to this message.
I'd prefer to avoid quoting things which don't need quoting. On Apr 10, 2013 1:39 PM, <r@golang.org> wrote: > Notice my versions also protect spaces. > > https://codereview.appspot.**com/8633043/<https://codereview.appspot.com/8633... >
Sign in to reply to this message.
Fine, but the CL as it stands doesn't quote things that *do* need quoting, it just prints "" for empty strings. -rob On Wed, Apr 10, 2013 at 9:23 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > I'd prefer to avoid quoting things which don't need quoting. > > On Apr 10, 2013 1:39 PM, <r@golang.org> wrote: >> >> Notice my versions also protect spaces. >> >> https://codereview.appspot.com/8633043/ > > -- > > --- > 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. > For more options, visit https://groups.google.com/groups/opt_out. > >
Sign in to reply to this message.
I will propose an alternative when I find a computer. On Apr 10, 2013 9:37 PM, "Rob Pike" <r@golang.org> wrote: > Fine, but the CL as it stands doesn't quote things that *do* need > quoting, it just prints "" for empty strings. > > -rob > > > On Wed, Apr 10, 2013 at 9:23 PM, Brad Fitzpatrick <bradfitz@golang.org> > wrote: > > I'd prefer to avoid quoting things which don't need quoting. > > > > On Apr 10, 2013 1:39 PM, <r@golang.org> wrote: > >> > >> Notice my versions also protect spaces. > >> > >> https://codereview.appspot.com/8633043/ > > > > -- > > > > --- > > 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. > > For more options, visit https://groups.google.com/groups/opt_out. > > > > >
Sign in to reply to this message.
func main() {
println(quotedJoin([]string{"foo", "-D", "", "with space",
"with\"quote", "bare"}))
}
func quotedJoin(a []string) string {
var buf bytes.Buffer
for i, s := range a {
if i > 0 {
buf.WriteByte(' ')
}
q := strconv.Quote(s)
if s == "" || strings.Contains(s, " ") || len(q) > len(s)+2
{
buf.WriteString(q)
} else {
buf.WriteString(s)
}
}
return buf.String()
}
On Wed, Apr 10, 2013 at 9:38 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:
> I will propose an alternative when I find a computer.
> On Apr 10, 2013 9:37 PM, "Rob Pike" <r@golang.org> wrote:
>
>> Fine, but the CL as it stands doesn't quote things that *do* need
>> quoting, it just prints "" for empty strings.
>>
>> -rob
>>
>>
>> On Wed, Apr 10, 2013 at 9:23 PM, Brad Fitzpatrick <bradfitz@golang.org>
>> wrote:
>> > I'd prefer to avoid quoting things which don't need quoting.
>> >
>> > On Apr 10, 2013 1:39 PM, <r@golang.org> wrote:
>> >>
>> >> Notice my versions also protect spaces.
>> >>
>> >> https://codereview.appspot.com/8633043/
>> >
>> > --
>> >
>> > ---
>> > 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.
>> > For more options, visit https://groups.google.com/groups/opt_out.
>> >
>> >
>>
>
Sign in to reply to this message.
PTAL adopted the suggestion of Brad.
Sign in to reply to this message.
LGTM Leaving for r to approve for go1.1 before submitting. On Thu, Apr 11, 2013 at 12:38 AM, <dr.volker.dobler@gmail.com> wrote: > PTAL > > adopted the suggestion of Brad. > > https://codereview.appspot.**com/8633043/<https://codereview.appspot.com/8633... >
Sign in to reply to this message.
https://codereview.appspot.com/8633043/diff/24001/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/8633043/diff/24001/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1308: // if needed. this comment is grammatically wrong and inaccurate. also the name is not the best, either, given what it does now. // quoteForShell prints the slice, quoting where necessary to make it closer // to being interpretable by the shell, such as showing empty strings and // quoting non-shell-safe strings. However it uses Go's quoting rules so // the result may not be perfect. func quoteForShell(...)
Sign in to reply to this message.
PTAL Sorry for the nonsensical doc. I do not think shell quoting should be mentioned as this is not the intention of this CL. So I'd like to propose a new version.
Sign in to reply to this message.
sorry about the long long tale here, but if we're going to do this, let's get it right. i'll write a proper shell quoter tonight and propose that. i learned long ago that it's worth making the printing of shell commands valid input to the shell.
Sign in to reply to this message.
LGTM This does only a small part of the problem but getting it right is a huge amount of work. I played with it some last night and this code is not set up to make proper printing work well. I'll file an issue to take care of it properly. https://codereview.appspot.com/8633043/diff/30002/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/8633043/diff/30002/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1309: func joinUnambiguously(a []string) string { that's a mouthful yet still not quite right. meh. this has dragged on long enough.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=eed0c8bb671f *** cmd/go: quote command line arguments in debug output Debug output from go test -x may contain empty arguments. This CL quotes arguments if needed. E.g. the output of go test -x is now .../6g -o ./_go_.6 -p testmain -complete -D "" -I . -I $WORK ./_testmain.go which is easier to grasp. R=golang-dev, bradfitz, minux.ma, r CC=golang-dev https://codereview.appspot.com/8633043 Committer: Rob Pike <r@golang.org>
Sign in to reply to this message.
|
