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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter Protocol Support #1617

Merged
merged 90 commits into from Nov 11, 2016

Conversation

Projects
None yet
4 participants
@ttaylorr
Member

ttaylorr commented Nov 2, 2016

This pull-request is a continuation of #1382, originally authored by @larsxschneider.

It should serve as a place to merge in all of PRs in the filter-stream series (see description below), and then be merged into master as one huge thing. I'm suggesting this since the PR as of 46a8ee5 is incomplete, and not in a state that I would feel comfortable landing into master.

In terms of the PR series, here's what I'm thinking:

  1. #1617: Initial implementation of the filter protocol from @larsxschneider, that's this PR.
  2. #1618: Integration tests to make sure that all subsequent PRs keep working
  3. #1619: Unit tests around the protocol reading/writing to verify that its behavior is correct.
  4. #1620: Make the ObjectScanner behave like a Scanner.
  5. #1621: An io.Reader implementation for reading data off of the filter stream, and wiring that io.Reader up throughout the code
  6. #1622: An io.Writer implementation for writing streams of data to the filter stream, and wiring that up
  7. [pending]: A cleanup PR to rename types, document anything that I missed, and generally apply some 馃拝 EDIT: this was done in place

/cc @larsxschneider @technoweenie @sinbad @rubyist @sschuberth

@ttaylorr ttaylorr added the 1.5.0 label Nov 2, 2016

@ttaylorr ttaylorr added this to the Filter protocol support milestone Nov 2, 2016

wip

@ttaylorr ttaylorr force-pushed the filter-stream branch from 46a8ee5 to d874af9 Nov 2, 2016

@larsxschneider

This comment has been minimized.

Contributor

larsxschneider commented on test/test-filter-process.sh in bc23d69 Nov 2, 2016

Is it possible that the filter is globally defined?

Could we set clean/smudge to 'false' (also in the other test) ? Then Git would exit with an error code if it uses clean/smudge.

@larsxschneider

This comment has been minimized.

Contributor

larsxschneider commented Nov 10, 2016

The PR looks really good from my point of view. During my review I found 2 important problems that I think need to be addressed before merge and a couple of nits that could be/should be addressed.

(Yeah, I should have used the "Request changes" button instead of "Approve" ... but well 馃槃 )

@technoweenie

Great work 馃

@ttaylorr ttaylorr self-assigned this Nov 10, 2016

@larsxschneider

This comment has been minimized.

Contributor

larsxschneider commented on git/pkt_line_writer.go in 5c4fb03 Nov 11, 2016

s/preform/perform/

This comment has been minimized.

Member

ttaylorr replied Nov 11, 2016

No longer! 461ad41

@larsxschneider

This comment has been minimized.

Contributor

larsxschneider commented on git/pkt_line_writer.go in 5c4fb03 Nov 11, 2016

Are you sure about this? We call flush in the write call already:
https://github.com/github/git-lfs/blob/5c4fb035af2f564252ab4d6f674ba14a650852bf/git/pkt_line_writer.go#L81

The Flush (notice the uppercase!) is only necessary if the step in our self defined filter-protocol requires it.

This comment has been minimized.

Member

ttaylorr replied Nov 11, 2016

Good catch. The docs are a little unclear, which I've made a commit to address, but the gist is that we're calling the private flush() method to send any data buffered in the underlying *bufio.Writer out to the very-underlying io.Writer.

Flush(), the public version, you are correct, is used to both flush the underlying buffer and write the flush pkt 0000.

// Truncate "p" such that it no longer includes the data that we
// have in the internal buffer.
p = p[m:]
w.buf = append(w.buf, p[n:n+m]...)

This comment has been minimized.

@larsxschneider

larsxschneider Nov 11, 2016

Contributor

Do you know what clean/smudge operations triggered this bug with your special repo? Can we extract that case and add it to the integration tests?

This comment has been minimized.

@ttaylorr

ttaylorr Nov 11, 2016

Member

Great question. The bug was actually not here, but in the io.Reader, as patched in: 021511a. I'm not sure an integration test is appropriate for that level of detail, but the unit tests (specifically this one) do assert for that behavior.

As for the change in the io.Writer implementation, this is due to a contract that I failed to uphold when originally implementing this type:

Write must not modify the slice data, even temporarily.

Implementations must not retain p.

(see: https://golang.org/pkg/io#Writer).

That behavior is tested in this unit test, which would fail if the underlying dataums b1 and b2 were modified.

@ttaylorr ttaylorr merged commit 4935486 into master Nov 11, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ttaylorr ttaylorr deleted the filter-stream branch Nov 11, 2016

@ttaylorr

This comment has been minimized.

Member

ttaylorr commented Nov 11, 2016

Thanks everyone! 馃帀

@eminence

This comment has been minimized.

eminence commented Nov 12, 2016

Tremendous work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment