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.

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

Fix create archive to a continuous writing source file failed #388

Merged
merged 2 commits into from
Dec 13, 2023

Conversation

halfcrazy
Copy link
Contributor

Fix #387

@mholt
Copy link
Owner

mholt commented Sep 21, 2023

Thanks! Why does this fix it, though? Wouldn't Copy() just copy the 2 GB and finish at EOF? Why do we need to limit the number of bytes?

@halfcrazy
Copy link
Contributor Author

I suppose the problem is caused by writing a header with the original file size and the reader reading the new append content.

image
0880c4c80710ddd68215
0880c4c80710ddd6a214

https://github.com/mholt/archiver/blob/v4.0.0-alpha.8/tar.go#L74-L100

func (Tar) writeFileToArchive(ctx context.Context, tw *tar.Writer, file File) error {
	if err := ctx.Err(); err != nil {
		return err // honor context cancellation
	}

	hdr, err := tar.FileInfoHeader(file, file.LinkTarget)
	if err != nil {
		return fmt.Errorf("file %s: creating header: %w", file.NameInArchive, err)
	}
	hdr.Name = file.NameInArchive // complete path, since FileInfoHeader() only has base name

==>	if err := tw.WriteHeader(hdr); err != nil {
		return fmt.Errorf("file %s: writing header: %w", file.NameInArchive, err)
	}

	// only proceed to write a file body if there is actually a body
	// (for example, directories and links don't have a body)
	if hdr.Typeflag != tar.TypeReg {
		return nil
	}

	if err := openAndCopyFile(file, tw); err != nil {
		return fmt.Errorf("file %s: writing data: %w", file.NameInArchive, err)
	}

	return nil
}

@halfcrazy
Copy link
Contributor Author

ping @mholt

@mholt
Copy link
Owner

mholt commented Dec 1, 2023

Thanks, sorry for the delay. Just had a baby. I just need to think about this one more time but I suppose it is likely to be merged soon 🙂

Copy link
Owner

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Ok, I mean, I think I have no problems with this, but I also don't really know what the implications are for not your use case (which I don't fully understand still from code screenshots).

Could we add a comment maybe explaining why we're doing it this way?

Even better, tests to ensure correct behavior in your use case and the 'standard' use cases. I haven't really gotten around to doing a lot of testing in this package yet but if you want to make me even more confident in this change, then that'd be good.

archiver.go Show resolved Hide resolved
Copy link
Owner

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thanks for the improvement, we'll try it out

@mholt mholt merged commit 09bbccc into mholt:master Dec 13, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

file 2G.file: writing data: archive/tar: write too long
2 participants