config: support a configurable, and turn-off-able, pack.window #587

strib · 2017-09-09T20:32:29Z

One use of go-git is to transfer git data from a non-standard git repo (not stored in a file system, for example) to a "remote" backed by a standard, local .git repo.

In this scenario, delta compression is not needed to reduce transfer time over the "network", because there is no network. The underlying storage layer has already taken care of the data tranfer, and sending the objects to local .git storage doesn't require compression. So this PR gives the user the option to turn off compression when it isn't needed.

Of course, this results in a larger, uncompressed local .git repo, but the user can then run git gc or git repack on that repo if they care about the storage costs.

Turning the pack window to 0 reduces total push time of a 36K repo by 50 seconds (out of a pre-PR total of 3m26s).

orirawlings · 2017-09-09T21:47:20Z

Is this a feature in the standard git tooling?

strib · 2017-09-09T22:32:25Z

Is this a feature in the standard git tooling?

Yes: https://stackoverflow.com/questions/7102053/git-pull-without-remotely-compressing-objects

mcuadros · 2017-09-10T18:24:57Z

Thanks for your contribution, make a lot of sense for people using memory based storage or others.

I cannot see a config flag exactly for this feature, the core.compression flag in stackoverflow, has a different behavior.

As you can see in the document this kind of configuration it's done at config level. So instead of having it as a flag of the PullOptions, should be implemented as property in the config file.

If this property doesn't exists on the standard git, I am open to merge this PR with a flag call gogit.deltaCompression or something similar.

orirawlings · 2017-09-10T19:21:40Z

It looks like this is controlled via a gitattributes option on specific file glob patterns. Ideally we would control this behavior via the same gitattributes, but I don't believe that currently the go-git library supports gitattributes. The are a bunch of other behaviors that can be controlled via gitattributes as well.

mcuadros · 2017-09-10T20:07:59Z

This is handle by the config package current we only support a few config settings, but we already have the Core property.

go-git/config/config.go

Lines 35 to 42 in bb3217c

    
           type Config struct { 
        
           	Core struct { 
        
           		// IsBare if true this repository is assumed to be bare and has no 
        
           		// working directory associated with it. 
        
           		IsBare bool 
        
           		// Worktree is the path to the root of the working tree. 
        
           		Worktree string 
        
           	}

strib · 2017-09-11T00:03:55Z

Thanks for taking a look guys. I don't think this is really equivalent to a core config param. It's more equivalent to setting --window=0 when calling git-pack-objects: https://git-scm.com/docs/git-pack-objects#git-pack-objects---windowltngt . That can also be controlled in the config file with pack.window: https://git-scm.com/docs/git-config#git-config-packwindow

I'm not sure why git doesn't make it a per-remote option, that seems more reasonable to me. But if you guys want to keep it closer to the real git, I can turn this into support for pack.window, where window=0 means delta compression is turned off. What do you think?

mcuadros · 2017-09-11T00:58:24Z

Yes, I think this is the best solution, since implementing this as pack.window is giving support a feature from git. Also will be nice change the the packfile encoder to accept the argument of the window.

codecov · 2017-09-11T04:57:41Z

Codecov Report

Merging #587 into master will decrease coverage by 0.6%.
The diff coverage is 74.41%.

@@            Coverage Diff             @@
##           master     #587      +/-   ##
==========================================
- Coverage   78.32%   77.72%   -0.61%     
==========================================
  Files         130      130              
  Lines       10092    10122      +30     
==========================================
- Hits         7905     7867      -38     
- Misses       1342     1418      +76     
+ Partials      845      837       -8

Impacted Files	Coverage Δ
plumbing/transport/server/server.go	`65.74% <100%> (ø)`	⬆️
plumbing/format/packfile/encoder.go	`80.85% <100%> (ø)`	⬆️
remote.go	`74.28% <40%> (-0.3%)`	⬇️
config/config.go	`84.05% <61.11%> (-3.45%)`	⬇️
plumbing/format/packfile/delta_selector.go	`80.12% <93.75%> (+1.17%)`	⬆️
plumbing/transport/ssh/common.go	`20.54% <0%> (-45.21%)`	⬇️
plumbing/transport/ssh/auth_method.go	`31.57% <0%> (-22.81%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb3217c...52c1f98. Read the comment docs.

strib · 2017-09-11T04:58:28Z

Thanks @mcuadros, done. Please take another look.

mcuadros

Excellent work! Just one small comment and ready to merge.

mcuadros · 2017-09-11T07:42:37Z

plumbing/format/packfile/delta_selector.go

-	otp, err := dw.objectsToPack(hashes)
+func (dw *deltaSelector) ObjectsToPack(
+	hashes []plumbing.Hash,
+	packWindow uint,


Please update the comment about this new argument.

mcuadros · 2017-09-11T07:43:01Z

plumbing/format/packfile/encoder.go

-	objects, err := e.selector.ObjectsToPack(hashes)
+func (e *Encoder) Encode(
+	hashes []plumbing.Hash,
+	packWindow uint,


erizocosmico · 2017-09-11T08:00:59Z

lgtm once the requests by @mcuadros are added

bzz · 2017-09-11T08:11:12Z

Looks great.

Probably a nitpick, but could you also please double-check if 0d74736 is intended to be part of these changes?

One use of go-git is to transfer git data from a non-standard git repo (not stored in a file system, for example) to a "remote" backed by a standard, local .git repo. In this scenario, delta compression is not needed to reduce transfer time over the "network", because there is no network. The underlying storage layer has already taken care of the data tranfer, and sending the objects to local .git storage doesn't require compression. So this PR gives the user the option to turn off compression when it isn't needed. Of course, this results in a larger, uncompressed local .git repo, but the user can then run git gc or git repack on that repo if they care about the storage costs. Turning the pack window to 0 on reduces total push time of a 36K repo by 50 seconds (out of a pre-PR total of 3m26s).

strib · 2017-09-11T15:57:43Z

Added comments and rebased it correctly. Thanks all!

strib mentioned this pull request Sep 9, 2017

vendor: pull in more go-git optimizations keybase/kbfs#1158

Merged

strib force-pushed the strib/skip-compression-gh-master branch from 032196f to db279de Compare September 11, 2017 04:57

strib changed the title ~~remote: option to skip delta compression while encoding packfiles~~ config: support a configurable, and turn-off-able, pack.window Sep 11, 2017

mcuadros suggested changes Sep 11, 2017

View reviewed changes

erizocosmico approved these changes Sep 11, 2017

View reviewed changes

strib force-pushed the strib/skip-compression-gh-master branch from a1681c8 to 52c1f98 Compare September 11, 2017 15:57

mcuadros approved these changes Sep 11, 2017

View reviewed changes

mcuadros merged commit 032ec28 into src-d:master Sep 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: support a configurable, and turn-off-able, pack.window #587

config: support a configurable, and turn-off-able, pack.window #587

strib commented Sep 9, 2017 •

edited

Loading

orirawlings commented Sep 9, 2017

strib commented Sep 9, 2017

mcuadros commented Sep 10, 2017

orirawlings commented Sep 10, 2017

mcuadros commented Sep 10, 2017

strib commented Sep 11, 2017

mcuadros commented Sep 11, 2017

codecov bot commented Sep 11, 2017 •

edited

Loading

strib commented Sep 11, 2017

mcuadros left a comment

mcuadros Sep 11, 2017

mcuadros Sep 11, 2017

erizocosmico commented Sep 11, 2017

bzz commented Sep 11, 2017

strib commented Sep 11, 2017

config: support a configurable, and turn-off-able, pack.window #587

config: support a configurable, and turn-off-able, pack.window #587

Conversation

strib commented Sep 9, 2017 • edited Loading

orirawlings commented Sep 9, 2017

strib commented Sep 9, 2017

mcuadros commented Sep 10, 2017

orirawlings commented Sep 10, 2017

mcuadros commented Sep 10, 2017

strib commented Sep 11, 2017

mcuadros commented Sep 11, 2017

codecov bot commented Sep 11, 2017 • edited Loading

Codecov Report

strib commented Sep 11, 2017

mcuadros left a comment

Choose a reason for hiding this comment

mcuadros Sep 11, 2017

Choose a reason for hiding this comment

mcuadros Sep 11, 2017

Choose a reason for hiding this comment

erizocosmico commented Sep 11, 2017

bzz commented Sep 11, 2017

strib commented Sep 11, 2017

strib commented Sep 9, 2017 •

edited

Loading

codecov bot commented Sep 11, 2017 •

edited

Loading