Heads up: this seems to work for me on macOS and Linux but I have a hanging process on Windows. Unfortunately I cannot share the repository that creates the hanging process and I haven't been able to create a small test case, yet.

I have a hanging process on Windows.

Are you running Go 1.7.3 on the Windows box?

Ah!! There was something about 1.7... ... let me try!

OK, I am convinced that this is the correct solution. Here's why:

When we try to clean a file, the first thing that we do is see if that file is already an LFS pointer, and if it is, we pass that file through directly. To check if the file is an LFS pointer, we only read part of it, since the whole file could be rather large. We read the first 1024 bytes of the file (as dictated by the blobSizeCutoff.

We buffer the data in that read by passing it through a io.TeeReader into a bytes.Buffer (so we can use the data later on if the file wasn't already a pointer). This happens here. Here's where things get interesting. We combine the buffered data with the actual reader (an *os.File, usually) if we buffered less data than the total length of the file. That happens here. We only concatenate if there is more data, since if we had read the entire contents of the file, there'd be no more data left to read, and the call would either a) block forever, or b) return an io.EOF immediately.

However! Our pkt-line implementation of io.Reader splits the last read and the EOF read into two. So if you're at the end of a file, you'll get:

1. Read([1024]byte) => (1024, nil)
2. Read[n]byte) => (0, io.EOF)

This is totally valid according to the Go documentation for the io.Reader interface, but introduces a very subtle problem in our code. Consider a 1024 byte file, and the pkt-line representation of it. We'd have:

1. A header with the pathname and command (consumed by the filter-process scanner)
2. A flush packet (consumed by the filter-process scanner)
3. 1024 bytes of the file's contents (consumed by the reader above)
4. A flush packet (NOT CONSUMED!)

Since we filled the buffer, and didn't reach into the next packet (which would have been a 0000 flush), the stream is left 4 bytes behind where it should be, thus the next command isn't recognized, and the command exits in a dirty state.

This fix is correct because it combines the last read into:

1. Read([1024]byte) => (1024, io.EOF)

because the reader reads packets until it has to buffer, not until the given p buffer is full. This causes it to read the flush packet against a perfectly sized buffer, which advances the buffer to the right state, even though we don't concatenate the readers.

This behavior is covered under our unit tests here and here.

I'd be happy to test this new version on my set of test data (the data that found this issue). It totals about 10GB and 138000 files. Would you happen to have a 64-bit linux binary handy?

@eminence: sure. Here are builds for both AMD64 and 386 architectures built against Linux containing all of the commits in this PR:

~/g/git-lfs (lars/test-1024-error) $ git show -q HEAD
commit 0899760263ad35ef11bd7e8af2799b8ea33ae624
Author: Taylor Blau <me@ttaylorr.com>
Date:   Tue Nov 22 10:57:18 2016 -0700

    git: remove extraneous length check

    `n`, representing the total number of bytes we have read into `p`, can never
    become `>len(p)`. As such, let's remove a check for that impossible condition.

git-lfs-linux-386-1.5.0.tar.gz
git-lfs-linux-amd64-1.5.0.tar.gz

Thanks! It's been running for about 30 minuets now with no errors. I'll report back here when it completes.

Edit: just finished! Took 32minutes in total.