fix(turborepo): Size comes from the thing being read.

[nathanhammond]

By splitting into two separate sources the metadata for hashing does not remain in sync with the contents of the file.

Further, fs::read_to_end already calls metadata on the open file, so this is duplicated work.

As a side effect, this PR also restores <= 1.10.6 (Go) behavior for hashing of symlinks, which is stat, not lstat:

turbo/cli/internal/fs/hash.go

Lines 22 to 46 in 66d6d7e

	// GitLikeHashFile is a function that mimics how Git
	// calculates the SHA1 for a file (or, in Git terms, a "blob") (without git)
	func GitLikeHashFile(filePath turbopath.AbsoluteSystemPath) (string, error) {
	file, err := filePath.Open()
	if err != nil {
	return "", err
	}
	defer file.Close()
	stat, err := file.Stat()
	if err != nil {
	return "", err
	}
	hash := sha1.New()
	hash.Write([]byte("blob"))
	hash.Write([]byte(" "))
	hash.Write([]byte(strconv.FormatInt(stat.Size(), 10)))
	hash.Write([]byte{0})
	if _, err := io.Copy(hash, file); err != nil {
	return "", err
	}
	return hex.EncodeToString(hash.Sum(nil)), nil
	}

Fixes #6046

Closes TURBO-1417

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
examples-basic-web	🔄 Building (Inspect)	Visit Preview	💬 Add feedback	Oct 16, 2023 10:48pm
examples-kitchensink-blog	🔄 Building (Inspect)	Visit Preview	💬 Add feedback	Oct 16, 2023 10:48pm
examples-native-web	🔄 Building (Inspect)	Visit Preview	💬 Add feedback	Oct 16, 2023 10:48pm

8 Ignored Deployments

Name	Status	Preview	Comments	Updated (UTC)
examples-cra-web	⬜️ Ignored (Inspect)	Visit Preview		Oct 16, 2023 10:48pm
examples-designsystem-docs	⬜️ Ignored (Inspect)	Visit Preview		Oct 16, 2023 10:48pm
examples-gatsby-web	⬜️ Ignored (Inspect)	Visit Preview		Oct 16, 2023 10:48pm
examples-nonmonorepo	⬜️ Ignored (Inspect)	Visit Preview		Oct 16, 2023 10:48pm
examples-svelte-web	⬜️ Ignored (Inspect)	Visit Preview		Oct 16, 2023 10:48pm
examples-tailwind-web	⬜️ Ignored (Inspect)	Visit Preview		Oct 16, 2023 10:48pm
examples-vite-web	⬜️ Ignored (Inspect)	Visit Preview		Oct 16, 2023 10:48pm
turbo-site	⬜️ Ignored (Inspect)	Visit Preview		Oct 16, 2023 10:48pm

[vercel]

Someone is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

[nathanhammond]

Reviewer's Guide!

[nathanhammond]

read_to_end already attempts to do this optimally in terms of syscall count, we don't really need to help out.

read_to_end is this:

// Reserves space in the buffer based on the file size when available.
fn read_to_end(&mut self, buf: &mut Vec<u8>) -> io::Result<usize> {
    let size = buffer_capacity_required(self);
    buf.reserve(size.unwrap_or(0));
    io::default_read_to_end(self, buf, size)
}

buffer_capacity_required is this:

fn buffer_capacity_required(mut file: &File) -> Option<usize> {
    let size = file.metadata().map(|m| m.len()).ok()?;
    let pos = file.stream_position().ok()?;
    // Don't worry about `usize` overflow because reading will fail regardless
    // in that case.
    Some(size.saturating_sub(pos) as usize)
}

[nathanhammond]

Point of failure one: lstat. The size that we get off of a symlink_metadata call is nonsense in the event of the thing being a symlink.

[nathanhammond]

Point of failure two: fopen. fopen follows symlinks, which means that fopen can fail while symlink_metadata succeeds.

[nathanhammond]

So, now we just attempt to hash the file, and let read_to_end do its thing.

We can then investigate the returned error to see if it is ENOENT.

[nathanhammond]

This line of code + follow_links = false means that if the symlink target is outside of the package or in the package but otherwise ignored we get a bad hash.

I believe that we should be following symlinks, and if it is a broken symlink, hash it the same way git does (by target).

[nathanhammond]

This would also mean that if we ls-tree and have a symlink, we need to replace it with the target hash if the target exists and is a file, and, if the target is a directory, walk it.

What should we do about .gitignore when walking a symlink'd directory's contents? 😅🤷‍♂️

[arlyon]

Interesting behaviour, did a little test and it seems that ls-tree does indeed not expose symlinks. Git does at least give the file mode so we can manually check for symlinks (120000).

I think in this case, there is an optimisation we could apply with a caveat:

• Evaluate the symlink, and try to find files with the same path prefix as the destination in the ls-tree output rather than having to walk the fs
• In that case the 'gitignore stack' would be the of the target rather than of the source
• If the destination is not in the ls-tree output (ie. outside the repo) we would either need to warn and ignore or explicitly
• With the target being outside the repo it would not have a 'gitignore stack' so we would have to use the stack of the source

These two behaviours conflict but I don't think we should support symlinks outside the repo root anyways so perhaps erroring is sufficient.

[gsoltis]

The intention with the change was to hash symlink files, not their targets. There's been a series of differently-incorrect behavior here:

• As @arlyon notes, ls-tree does list a hash for the symlink. So without inputs, they are wholesale ignored.
• when using inputs, and at least since switching to the encoder setup in Go, symlinks caused an error and we fell back to manual hashing. It's very possible that was the behavior before also, I haven't checked
• manual hashing in Go calls os.Open, which reads the target.
• Rust ls-tree has the same restriction as the Go implementation, it ignores symlinks
• Rust git-based hashing, with inputs, was intended to switch to hashing the contents of the symlink, but there are several problems:
  1. We eventually get to fopen as @nathanhammond notes. This follows the link. The Go manual file hashing implementation has the same behavior. At least for the Rust implementation, that was not the intention, and I would consider it a bug at least insofar as it doesn't do what was intended.
  2. As noted in the slack thread, the raw contents of the link is not portable, so probably not what we want to hash
  3. We currently have checks in place to skip over hashing symlinks only when using manual hashing

In terms of what I think should happen is the following:

• Short term: update all paths through hashing to match. Probably ignore symlinks for now. The link target should be required to be covered by inputs anyways. This leaves the existing bug of not hashing symlinks in place, but makes us consistent about it.
• Medium term: Hash a portable representation of the contents of the symlink, not the target. This saves us from having to care about file vs directory, broken vs working, and in-repo vs not, which in the presence of inputs is not even necessarily a clear distinction (do we follow in-repo, but out-of-inputs links?). Users will be required to ensure that the target is covered by their inputs if they want the target hashed. This is mirrors our behavior for caching: we don't cache the target of symlinks. If you want it cached, ensure that the target is also covered by the outputs globs.
• Longer term: determine how to handle symlinks when inputs is not specified.

[arlyon]

No changes, just some comments

[arlyon]

Interesting behaviour, did a little test and it seems that ls-tree does indeed not expose symlinks. Git does at least give the file mode so we can manually check for symlinks (120000).

I think in this case, there is an optimisation we could apply with a caveat:

• Evaluate the symlink, and try to find files with the same path prefix as the destination in the ls-tree output rather than having to walk the fs
• In that case the 'gitignore stack' would be the of the target rather than of the source
• If the destination is not in the ls-tree output (ie. outside the repo) we would either need to warn and ignore or explicitly
• With the target being outside the repo it would not have a 'gitignore stack' so we would have to use the stack of the source

These two behaviours conflict but I don't think we should support symlinks outside the repo root anyways so perhaps erroring is sufficient.

[gsoltis]

Let's make sure we're all on the same page about where this is going before merging.

Getting the size from the thing that's read seems fine, I'm more concerned about what we intend behavior to be with symlinks vs targets.

I'll defer to @arlyon for Rust notes.

[gsoltis]

Documented read_to_end behavior re: symlinks, updating internal discussion on desired eventual symlink handling.