Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

txtar-c: add -script flag #181

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

txtar-c: add -script flag #181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions cmd/txtar-c/savedir.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
//
// See https://godoc.org/github.com/rogpeppe/go-internal/txtar for details of the format
// and how to parse a txtar file.
//
package main

import (
Expand All @@ -35,8 +34,9 @@ func usage() {
}

var (
quoteFlag = flag.Bool("quote", false, "quote files that contain txtar file markers instead of failing")
allFlag = flag.Bool("a", false, "include dot files too")
quoteFlag = flag.Bool("quote", false, "quote files that contain txtar file markers instead of failing")
scriptFlag = flag.String("script", "", "include testscript `FILE` (after any 'unquote' lines)")
allFlag = flag.Bool("a", false, "include dot files too")
)

func main() {
Expand Down Expand Up @@ -105,7 +105,13 @@ func main1() int {
})
return nil
})

if *scriptFlag != "" {
script, err := os.ReadFile(*scriptFlag)
if err != nil {
log.Fatal(err)
}
a.Comment = append(a.Comment, script...)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the scenario when the -script argument contains unquote lines itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I see what you mean. Could you say a little more?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

He means that if the script file contains txtar syntax, like -- foo --, then it will leak out of the comment section and affect the rest of the txtar.

The flag should probably reject those scripts, because I don't think there is any easy quoting or escaping we can reach for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I still don't get it. Could you give an example of a script which would produce the wrong result?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I actually misunderstood what Paul said, so ignore me.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming I have gotten the quoting right:

unquote expect
unquote test.txtar
unquote blah/needsquote.txtar
exec txtar-c -quote -script test.txtar blah
cmp stdout expect

-- test.txtar --
>unquote my.txtar
>exec cat my.txtar
>
>-- my.txtar --
>># This is a comment in my.txtar
>>
>>-- a.txt --
>>// This is a.txt within my.txtar
>>
-- blah/go.mod --
module example.com/blah

-- blah/main.go --
package main

import "fmt"

func main() {
  fmt.Println("Hello, world!")
}

-- blah/needsquote.txtar --
>-- file_entry.txt --

-- expect --
>unquote needsquote.txtar
>unquote my.txtar
>
>exec cat my.txtar
>
>-- my.txtar --
>># This is a comment in my.txtar
>>
>>-- a.txt --
>>// This is a.txt within my.txtar
>>
>-- go.mod --
>module example.com/blah
>
>-- main.go --
>package main
>
>import "fmt"
>
>func main() {
>  fmt.Println("Hello, world!")
>}
>
>-- needsquote.txtar --
>>-- file_entry.txt --
>>

Which also points out that in this PR only the comment in the -script argument is used. Which seems quite specific. Was that intentional?

txtar files don't naturally compose as far as I can see, hence my question is really directed at questioning whether this could be adequately solved in a purpose built program, rather than trying to adapt txtar-c.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does fail, but only because the script produces an extra unexpected blank line—is that what you meant?

> cmp stdout expect
--- stdout
+++ expect
@@ -1,5 +1,6 @@
 unquote needsquote.txtar
 unquote my.txtar
+
 exec cat my.txtar
 
 -- my.txtar --

FAIL: /var/folders/q3/ybqqxyh92yd0yc865zqk0vpm0000gn/T/testscript1226362500/tmp.txtar/script.txtar:5: stdout and expect differ

only the comment in the -script argument is used. Which seems quite specific. Was that intentional?

I'm not sure I understand the question. If you mean "only the comment part of the script makes it into the resulting txtar, ignoring any file inclusions", that's not the case. The whole of the file referenced by -script FILE is included, and if it contains file inclusions, they will end up in the final txtar too. At least, that was the intention.

If you mean "only the comment part of the resulting txtar is modified, by including the script in it", that's correct, but I can't think of an alternative way of doing this that would make sense.

my question is really directed at questioning whether this could be adequately solved in a purpose built program, rather than trying to adapt txtar-c.

Certainly it could.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand the question. If you mean "only the comment part of the script makes it into the resulting txtar, ignoring any file inclusions", that's not the case. The whole of the file referenced by -script FILE is included, and if it contains file inclusions, they will end up in the final txtar too. At least, that was the intention.

I actually misread the code. As you say, the entire file passed to -script is included after unquote lines that are added by txtar-c. What threw me is that the contents of this file are added to the new archive's comments, even though that []byte contains files as interpreted by txtar.

Here's a case where I think things go wrong:

unquote expect
unquote test.txtar
unquote blah/needsquote.txtar
# With both -script and -quote, it should include test.txtar after any 'unquote's
exec txtar-c -quote -script test.txtar blah
cmp stdout expect

-- test.txtar --
>unquote needsquote.txtar
>exec cat my.txtar
>
>-- needsquote.txtar --
>># This is a comment in my.txtar
>>
>>-- a.txt --
>>// This is a.txt within my.txtar
>>
-- blah/go.mod --
module example.com/blah

-- blah/main.go --
package main

import "fmt"

func main() {
  fmt.Println("Hello, world!")
}

-- blah/needsquote.txtar --
>-- file_entry.txt --

-- expect --
>unquote needsquote.txtar
>unquote needsquote.txtar
>exec cat my.txtar
>
>-- needsquote.txtar --
>># This is a comment in my.txtar
>>
>>-- a.txt --
>>// This is a.txt within my.txtar
>>
>-- go.mod --
>module example.com/blah
>
>-- main.go --
>package main
>
>import "fmt"
>
>func main() {
>  fmt.Println("Hello, world!")
>}
>
>-- needsquote.txtar --
>>-- file_entry.txt --
>>

The double unquote of needsquote.txtar is incorrect; there should only be one unquote.

The flag name -script is also a bit of a smell for me here. txtar-c is a command that is only concerned with the txtar format. It knows nothing about testscript. If we take the name "script" away, we are effectively passing in another txtar archive as the argument here. In that case, how would you describe the operation we are performing on what are two txtar archives? Merge? Append?

If you mean "only the comment part of the resulting txtar is modified, by including the script in it", that's correct, but I can't think of an alternative way of doing this that would make sense.

my question is really directed at questioning whether this could be adequately solved in a purpose built program, rather than trying to adapt txtar-c.

Certainly it could.

My sense is that we're struggling to define what this operation between two txtar archives is here. And as I said, I think that comes down to the fact that in the general case, operations on archives are not well defined: they don't compose, prepend, append etc. Hence I feel this probably belongs as a purpose-built tool for your situation, rather than something that is generally useful in txtar-c.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this probably belongs as a purpose-built tool for your situation, rather than something that is generally useful in txtar-c.

That's perfectly fair—and I have exactly that tool, which is working perfectly for my use case. (It is, in fact, just vanilla txtar-c with this patch applied.)

I feel the sense of the meeting is that while a feature like this might be occasionally useful, to provide a really robust solution that works in theory, as well as merely in practice, would take a fair bit more work, if it's even possible at all. Accordingly, please close, with my thanks.

}
data := txtar.Format(a)
os.Stdout.Write(data)

Expand Down
35 changes: 35 additions & 0 deletions cmd/txtar-c/testdata/script.txtar
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
unquote expect
# With the -script flag, it should include the contents of test.txtar as a comment
exec txtar-c -script test.txtar blah
cmp stdout expect

-- test.txtar --
exec echo hello

-- blah/go.mod --
module example.com/blah

-- blah/main.go --
package main

import "fmt"

func main() {
fmt.Println("Hello, world!")
}

-- expect --
>exec echo hello
>
>-- go.mod --
>module example.com/blah
>
>-- main.go --
>package main
>
>import "fmt"
>
>func main() {
> fmt.Println("Hello, world!")
>}
>
43 changes: 43 additions & 0 deletions cmd/txtar-c/testdata/script_quote.txtar
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
unquote expect
unquote blah/needsquote.txtar
# With both -script and -quote, it should include test.txtar after any 'unquote's
exec txtar-c -quote -script test.txtar blah
cmp stdout expect

-- test.txtar --
exec echo hello

-- blah/go.mod --
module example.com/blah

-- blah/main.go --
package main

import "fmt"

func main() {
fmt.Println("Hello, world!")
}

-- blah/needsquote.txtar --
>-- file_entry.txt --

-- expect --
>unquote needsquote.txtar
>exec echo hello
>
>-- go.mod --
>module example.com/blah
>
>-- main.go --
>package main
>
>import "fmt"
>
>func main() {
> fmt.Println("Hello, world!")
>}
>
>-- needsquote.txtar --
>>-- file_entry.txt --
>>