Filter Protocol Support #1617

ttaylorr · 2016-11-02T17:23:34Z

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:

Filter Protocol Support #1617: Initial implementation of the filter protocol from @larsxschneider, that's this PR.
filter_stream: Integration tests #1618: Integration tests to make sure that all subsequent PRs keep working
filter_stream: Protocol unit tests #1619: Unit tests around the protocol reading/writing to verify that its behavior is correct.
filter_stream: Make ObjectScanner behave like a Scanner #1620: Make the ObjectScanner behave like a Scanner.
filter_stream: provide io.Reader implementation #1621: An io.Reader implementation for reading data off of the filter stream, and wiring that io.Reader up throughout the code
filter_stream: provide io.Writer implementation #1622: An io.Writer implementation for writing streams of data to the filter stream, and wiring that up
~~[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

filter_stream: Integration tests

…ntered

larsxschneider · 2016-11-10T09:23:08Z

commands/command_filter_process.go

+		// instead of a pointer.
+		//
+		// TODO(taylor): figure out if there is more data on the reader,
+		// and buffer that as well.


I am not sure if I understand that 100% but I think according to the filter protocol there can't be more data on the reader. Because we need to acknowledge the data with a status packet before Git sends more data, no?

Do you want to remove or address this TODO before merge?

After discussing in meat-space, the consensus was there will be no more data on the io.Reader coming from Git, since we will have decoded the entire pointer (or at least, tried to) by this point, so I went ahead and removed the TODO.

@larsxschneider can clarify anything that I left out.

EDIT: Went ahead and removed the TODO note in 480ec1b.

larsxschneider · 2016-11-10T09:24:19Z

commands/command_filter_process.go

+}
+
+func filterCommand(cmd *cobra.Command, args []string) {
+	requireStdin("This command should be run by the Git filter process")


TODO: This seems not to work on Windows (same is true for the legacy clean/smudge functions). However, I don't think that should be addressed in this PR.

Awesome catch. I agree that this is out of scope of the current PR, so I opened #1639 to address.

larsxschneider · 2016-11-10T09:30:18Z

commands/command_filter_process.go

+
+	if err := s.Err(); err != nil && err != io.EOF {
+		ExitWithError(err)
+	}


s.Err() == io.EOF is a legitimate signal from Git telling GitLFS that it should shut down. Maybe we should document/make this more clear here?

More importantly: If we get an error during w.Flush() in L101 then we wouldn't exit with error here, right?

larsxschneider · 2016-11-10T09:34:30Z

commands/command_smudge.go

+	if err != nil {
+		ptr.Encode(to)
+		// Download declined error is ok to skip if we weren't requesting download
+		if !(errors.IsDownloadDeclinedError(err) && !download) {


How can we get a download error if we don't request a download?

Good question. This happens since we treat downloading a file excluded with the -X flag as optional. If we tried to download the file (i.e., repository was public), we just won't do the smudge in order to speed up the checkout. If the repo was private and we tried to download, we could get an error due to missing credentials, but that's safe to ignore, since we don't need the contents of the file anyway.

/refs: 417fd73 #759

larsxschneider · 2016-11-10T09:35:36Z

commands/command_smudge.go

@@ -49,42 +97,18 @@ func smudgeCommand(cmd *cobra.Command, args []string) {
 			Exit(err.Error())
 		}

-		stat, err := os.Stat(localPath)
-		if err != nil {
+		if stat, err := os.Stat(localPath); err != nil {


Just curious: What is the advantage of this style over the other?

Two things: It's 1 less line of code, and the err value is scoped to the if stat, err block.

larsxschneider · 2016-11-10T10:25:13Z

lfs/setup.go

-			"smudge": []string{"git-lfs smudge %f"},
+			"clean":   []string{"git-lfs clean %f"},
+			"smudge":  []string{"git-lfs smudge %f"},
+			"process": []string{"git-lfs filter"},


Important: Shouldn't that be filter-process here?

Good question. This is in the Upgradeables section of the setup code, meaning it's legit to overwrite this value so long as it matches one of the values in that slice. Since there was code publicly available during the course of this PR that referred to git-lfs filter instead of git-lfs filter-process, I'm going to keep this.

larsxschneider · 2016-11-10T10:25:23Z

lfs/setup.go

-			"smudge": []string{"git-lfs smudge --skip %f"},
+			"clean":   []string{"git-lfs clean %f"},
+			"smudge":  []string{"git-lfs smudge --skip %f"},
+			"process": []string{"git-lfs filter --skip"},


Important: Shouldn't that be filter-process here?

See #1617 (comment).

larsxschneider · 2016-11-10T10:26:40Z

test/test-filter-process.sh

+# Once 2.11 is released, replace this with:
+#
+# ```
+# ensure_git_version_isnt $VERSION_LOWER "2.11.0"


Maybe we should teach ensure_git_version the g format?

Good thought. Since adding support for the git-describe format would complicate this function (and I'm not planning on gating any more LFS features behind Git versions other than those released), I'm going to forgo this change for now, unless we need something like this in the future.

larsxschneider · 2016-11-10T10:28:33Z

test/test-filter-process.sh

+
+  diff -u <(echo "$expected") <(echo "$got")
+)
+end_test


I think I would prefer it if we set clean and smudge to false here to ensure they are not used.

larsxschneider · 2016-11-10T10:29:28Z

test/test-install.sh


  printf "$smudge" | grep "git-lfs smudge"
  printf "$clean" | grep "git-lfs clean"
+  printf "$filter" | grep "git-lfs filter"


s/lfs filter/lfs filter-process/

Nice catch, ha! Fixed it right up in: 4ae8afc.

larsxschneider · 2016-11-10T10:36:48Z

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 😄 )

…would fail

technoweenie

Great work 🤘

… total

larsxschneider · 2016-11-11T11:36:04Z

git/pkt_line_writer.go

-		// 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]...)



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?

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 · 2016-11-11T21:18:20Z

Thanks everyone! 🎉

eminence · 2016-11-12T03:24:44Z

Tremendous work!

During sync, repo runs `git read-tree --reset -u -v HEAD` which causes git-lfs's smudge filter to run, which fails because git-lfs does not work with bare repositories. This was fixed in I091ff37998131e2e6bbc59aa37ee352fe12d7fcd to automatically disable this smudge filter. However, later versions of Git (2.11.0) introduced a new filter protocol [1], to avoid spawning a new command for each filtered file. This was implemented in Git-LFS 1.5.0 [2]. This patch fixes the issue by setting the git lfs process filter, in addition to the smudge filter. For any projects that have LFS objects, `git lfs pull` must still be executed manually afterwards. [1] git/git@edcc858 [2] git-lfs/git-lfs#1617 Bug: https://crbug.com/gerrit/10911 Change-Id: I277fc68fdefc91514a2412b3887e3be9106cab48

ttaylorr added the 1.5.0 label Nov 2, 2016

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

ttaylorr mentioned this pull request Nov 2, 2016

filter_stream: Protocol unit tests #1619

Merged

wip

d874af9

ttaylorr force-pushed the filter-stream branch from 46a8ee5 to d874af9 Compare November 2, 2016 22:51

ttaylorr added 7 commits November 2, 2016 16:51

test: add an integration test for checkouts using filter protocol

d1dca3e

test: test staging a file using the filter process

bc23d69

test/fp: remove config setup, since testhelpers already works

3147b57

git: split protocol low/high-level

8bc726d

git/proto: test protocol reading/writing

17f9ead

git/object_scanner: add unit tests

31d0416

git/fp: assert writing a packet exactly the maximum size

1d184f7

larsxschneider mentioned this pull request Nov 3, 2016

Proposal: Access Git LFS cache from Git core (100x speed up!) #1329

Closed

larsxschneider and others added 17 commits November 3, 2016 13:28

Merge pull request #1618 from github/filter-stream-integration

77b084f

filter_stream: Integration tests

git: distinguish between missing and flush packets

9bc1d68

git/fp: teach Init() to return an error

3dca3ce

git/fp: remove "Error:" prefix from error messages

e33c595

git/fp: teach NegotiateCapabilities() to return an error

fc61845

git/fp: remove "Error:" prefixes

4ced5cd

commands,git/fp: introduce *git.Request type

c209f40

commands,git/fp: introduce Scanner-like functionality

fbd9e47

git/fp: use errors.Wrap

4a493a8

git/os: check success of calling o.writeStatus()

9827b90

git: implement type PacketReader

8a8a9b5

git/fp: publicize WriteStatus method

5a1df1c

commands,git/fp: use packetReader as request payload

83f17e5

git,tools: use tools.{Max,Min}Int instead of math package

3de701f

commands: remove superfluous line

dba2821

lfs: suppress io.EOF errors during pointer decodes

048abd1

commands: buffer the pointer file and return if any errors were encou…

e450c68

…ntered

technoweenie mentioned this pull request Nov 9, 2016

LFS Roadmap: v1.5.0 -> v2.0 #1632

Closed

larsxschneider approved these changes Nov 10, 2016

View reviewed changes

larsxschneider added the review label Nov 10, 2016

ttaylorr added 2 commits November 10, 2016 11:26

test/fp: explicitly set filter.lfs.{clean,smudge} to commands that …

792f5aa

…would fail

commands/fp: remove TODO, there is never more data on the io.Reader

480ec1b

ttaylorr mentioned this pull request Nov 10, 2016

requireStdin is broken on Windows #1639

Closed

ttaylorr added 4 commits November 10, 2016 12:17

git: refer to filter as filter-process

77bb7fb

git/writer: remove support for w.Write(nil)

5c4fb03

git/writer: be consistent with += operator

e9bab59

test/install: grep against filter-process, not filter

4ae8afc

technoweenie approved these changes Nov 10, 2016

View reviewed changes

ttaylorr added 2 commits November 10, 2016 12:27

git: annotate the presence of a flush packet

63f5418

git: be more clear about what happens when you exit

129030a

ttaylorr mentioned this pull request Nov 10, 2016

Filter protocol follow-up issue #1640

Closed

git: s/preform/perform/g

461ad41

ttaylorr self-assigned this Nov 10, 2016

ttaylorr added 4 commits November 10, 2016 14:54

git/pr: data is determined from how much remaining space "p" has, not…

021511a

… total

git/pw: leave "p" buffer unmodified

1c593d8

Merge branch 'master' into filter-stream

25c3fde

commands: place io.Writer before io.Reader in clean/smudge

1eb4415

larsxschneider reviewed Nov 11, 2016

View reviewed changes

git/pw: 💅 documentation

da319ba

ttaylorr merged commit 4935486 into master Nov 11, 2016

ttaylorr deleted the filter-stream branch November 11, 2016 21:08

ttaylorr mentioned this pull request Nov 12, 2016

WIP: promised downloads (proof of concept) #1646

Closed

technoweenie mentioned this pull request Nov 16, 2016

git lfs pull --recursive is slower than git lfs pull #1664

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter Protocol Support #1617

Filter Protocol Support #1617

ttaylorr commented Nov 2, 2016 •

edited

Loading

larsxschneider Nov 10, 2016

ttaylorr Nov 10, 2016 •

edited

Loading

larsxschneider Nov 10, 2016

ttaylorr Nov 10, 2016

larsxschneider Nov 10, 2016

larsxschneider Nov 10, 2016

ttaylorr Nov 10, 2016

larsxschneider Nov 10, 2016

technoweenie Nov 10, 2016

larsxschneider Nov 10, 2016

ttaylorr Nov 10, 2016

larsxschneider Nov 10, 2016

ttaylorr Nov 10, 2016

larsxschneider Nov 10, 2016

ttaylorr Nov 10, 2016

larsxschneider Nov 10, 2016

larsxschneider Nov 10, 2016

ttaylorr Nov 10, 2016

larsxschneider commented Nov 10, 2016

technoweenie left a comment

larsxschneider Nov 11, 2016

ttaylorr Nov 11, 2016

ttaylorr commented Nov 11, 2016

eminence commented Nov 12, 2016

Filter Protocol Support #1617

Filter Protocol Support #1617

Conversation

ttaylorr commented Nov 2, 2016 • edited Loading

Choose a reason for hiding this comment

ttaylorr Nov 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larsxschneider commented Nov 10, 2016

technoweenie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttaylorr commented Nov 11, 2016

eminence commented Nov 12, 2016

ttaylorr commented Nov 2, 2016 •

edited

Loading

ttaylorr Nov 10, 2016 •

edited

Loading