-
Notifications
You must be signed in to change notification settings - Fork 77
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.
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
txtar-c: add -script flag #181
base: master
Are you sure you want to change the base?
Conversation
if err != nil { | ||
log.Fatal(err) | ||
} | ||
a.Comment = append(a.Comment, script...) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 finaltxtar
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @myitcv points out, this tool isn't just about creating txtar files for testscript. How about naming this flag -comment
, as then it's applicable to both situations (the script occupies the "comment" section of the txtar file). I think the command should fail if the comment contains a line matching ^-- .* --$
.
Modulo the above, I think this will be useful functionality; thanks for suggesting it!
I should also mention that https://pkg.go.dev/golang.org/x/exp/cmd/txtar exists now, alongside https://pkg.go.dev/golang.org/x/tools/txtar. It already supports comments via stdout when extracting, and stdin when archiving. It doesn't appear to do any form of sanity checking on the input, so I guess we could do the same. I agree with Roger though - this tool should not be aware of testscript at all. |
TL;DR have
txtar-c
include an optional test script in the archive it creates.Use case
Suppose we want to use the marvellous
testscript
runner to run a given script against a set of files. For example, we might want to test a directory containing Go files using some scripttest.txtar
:Of course, we can't run this script as it stands, because it doesn't contain the Go files. So, how to add them?
We could create a
txtar
archive of the directory usingtxtar-c
, concatenate the result with our original script, and run the result of that withtestscript
. This is straightforward:Unfortunately, if
DIR
has files containingtxtar
file marker lines, this produces the wrong result.txtar-c -quote
prepends the necessaryunquote
lines to its archive:Simple-mindedly prepending our
test.txtar
script to this would produce:That's no good, because presumably we need the files to be unquoted before running
go test
. So the script needs to go after anyunquote
lines, but before the file data. This is not straightforward, and this shell script is unsatisfactory for any number of reasons (bonus points if you spot all twelve):How much nicer it would be if we could simply ask
txtar-c
to include our script file in the appropriate place!This makes it very easy and pleasant to use
testscript
to run scripts against some existing set of files, for example in a CI pipeline. A decent value-add for eight extra lines of code, I think.