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

proposal: x/tools/txtar: add Extract function or method #61386

Closed
adonovan opened this issue Jul 16, 2023 · 11 comments
Closed

proposal: x/tools/txtar: add Extract function or method #61386

adonovan opened this issue Jul 16, 2023 · 11 comments
Labels
Proposal
Milestone
Proposal

Comments

@adonovan
Copy link
Member

adonovan commented Jul 16, 2023 •
edited

While developing tests, I've written this function at least four times this week:

package txtar

// Extract writes each file in the archive to the corresponding location beneath dir.
func Extract(ar *Archive, dir string) error {
	for _, file := range ar.Files {
		name := filepath.Join(dir, file.Name)
		if err := os.MkdirAll(filepath.Dir(name), 0777); err != nil {
			return err
		}
		if err := os.WriteFile(name, file.Data, 0666); err != nil {
			return err
		}
	}
	return nil
}

I propose that we add it to the txtar package, either as a function or a method of Archive.
@gopherbot gopherbot added this to the Proposal milestone Jul 16, 2023
@mvdan
Copy link
Member

mvdan commented Jul 17, 2023

Written a while back, but it might be useful: https://pkg.go.dev/github.com/rogpeppe/go-internal/txtar#Write cc @rogpeppe @myitcv

Also, shouldn't we use #56219?

@adonovan
Copy link
Member Author

Personally I only use txtar for tests, but rejecting up-level references seems prudent. I'm less convinced about O_EXCL: I expect an extract operation to write files, even over existing ones.

@apparentlymart

This comment was marked as off-topic.

Sign in to view
@ianlancetaylor

This comment was marked as resolved.

Sign in to view
@seankhliao
Copy link
Member

alternatively, txtar could implement fs.FS for #44158
and, os can get a WriteFS(dir string, fsys fs.FS) error that would be more flexible

@apparentlymart

This comment was marked as off-topic.

Sign in to view
@rsc
Copy link
Contributor

rsc commented Jul 18, 2023

I'm fine with this as txtar.Extract, but the implementation should use filepath.ToSlash and filepath.IsLocal, as @mvdan noted.

I would rather not introduce a method since right now the Archive type is trivial, pure data, and I'd like to keep it that way.

@rsc
Copy link
Contributor

rsc commented Aug 30, 2023

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Sep 6, 2023

If we do #44158 and then add os.CopyFS from #45757 (comment), then like @seankhliao said above, that seems like a nicer outcome.

We would have to figure out what to do about txtar files that say ../ at the start, but archive/zip has to do that too and we can do whatever it does (drop them?).

@rsc rsc mentioned this issue Sep 6, 2023
Closed
@rsc rsc mentioned this issue Sep 20, 2023
Open
@rsc
Copy link
Contributor

rsc commented Jan 24, 2024

#44158 is accepted, and #62484 is going to be accepted very soon, so probably we can close this one.

@rsc
Copy link
Contributor

rsc commented Jan 26, 2024

This proposal has been declined as retracted.
— rsc for the proposal review group

@rsc rsc closed this as completed Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Proposal
Projects
Proposals
Status: Declined
Milestone
Proposal
Development

No branches or pull requests
7 participants
@apparentlymart @rsc @ianlancetaylor @mvdan @adonovan @gopherbot @seankhliao