added support for quarantine directory #887

noxora · 2018-07-10T15:02:26Z

This is a part of issue #886
I have tested this on my local installation and it has allowed me to create a "commitObject" from an object sitting in a quarantine directory as per the git-receive-pack

This code might be put in the wrong place, or implemented differently than you all would like, so feel free to move/alter it.
The idea of it is to check to see if the file whose path you're returning is actually there, and if not to see if there is an incoming-XXXXXXX directory and look for the file in there

noxora · 2018-07-10T15:26:46Z

I'm looking at why the travis build has failed, and I'm not sure why my changes impact this

FAIL: <autogenerated>:1: UploadPackSuite.TestAdvertisedReferencesNotExists
/home/travis/gopath/src/gopkg.in/src-d/go-git.v4/plumbing/transport/test/upload_pack.go:55:
    c.Assert(err, Equals, transport.ErrRepositoryNotFound)
... obtained *errors.errorString = &errors.errorString{s:"unexpected EOF"} ("unexpected EOF")
... expected *errors.errorString = &errors.errorString{s:"repository not found"} ("repository not found")
... Difference:
...     s: "unexpected EOF" != "repository not found"

Any help would be appreciated

noxora · 2018-07-11T20:29:20Z

So the test error is because I am changing the output in some cases to
.git/objects/incoming-XXXXXX/hash[0:2]/hash[2:40]
instead of
.git/objects/hash[0:2]/hash[2:40]
What's confusing me is that this should only change the output in cases where the object IS in the "incoming" folder, it shouldn't change it in scenarios where the object isn't in either.

smola · 2018-07-16T08:54:44Z

@noxora Thanks for the pull request!

It would be good to mention quarantine environment somewhere in the code / comments, with a link to https://git-scm.com/docs/git-receive-pack#_quarantine_environment

This way those of us who didn't heard about this before will not be confused ;)

I still do not see how the whole thing would work to mimick a pre-receive hook. We would need to know more about that before merging this.

For example:

When would incoming objects be written?
This should probably be optional to not impact performance when we don't use it.
When an object does not exist, is it correct to read its incoming counterpart in any operation? Or it should be seen only by certain operations?

noxora · 2018-07-16T15:29:28Z

@smola Thanks for the response, I should have been clearer

This is not intended to mimic a pre-receive hook, it only enables the writing of one using this library. I started writing a hook using this library and when I went to run it, I found that go-git was not finding the object I wanted to compare with inside the quarantine directory.

This pull request is intended to add support to look for a 'quarantine' directory containing objects so that a pre-receive hook created with this library can work with the incoming objects.

As per your questions

In a normal push operation, the remote server receives the objects and puts them into the 'quarantine' directory. It then runs the pre-receive hook, if that hook exits with status code 0, it runs the update hook for each reference to be updated. If that also passes, then it updates the reference by migrating the object out of the quarantine folder (at least, that's how I understand it)
This should probably only be checked if a 'quarantine' directory might exist, which should only be during a git-receive-pack operation, which I think is only git push.
It is probably not correct to attempt to read it in all situations, as it will cause some slight lag. Again, I think it is only required while receiving the contents of a git push. The quarantine directory only exists while the pre-receive and update hooks are being run. However, I am not familiar enough with this library to know exactly how to correct this

noxora · 2018-08-02T18:56:15Z

@smola
I have spent some time researching what exactly we should be doing in this case, and have reached out to the git mailing list. The conversation is publicly available here

Without the environment variables described in Jeff King's response, I believe this is currently the best option available to support quarantine directories.

If there are any changes you would like me to make, I would be happy to, but I would like to know if you would like to move forward with this pull request. If you would, please let me know if there is anything I can do to help with it

noxora · 2018-08-13T19:32:58Z

@mcuadros I apologize if this bothers you, but I haven't heard back from anyone in quite some time. Would you be able to take a look at this?

mcuadros · 2018-08-14T08:01:42Z

@noxora Will be great if you can cover the new functionality with some tests.
Also if you can squash and sign-off your commits.

smola

I'm still curious about how you'll implement the full hook behaviour. This PR is good. I'm just proposing more fine-grained handling of errors.

smola · 2018-08-14T08:31:05Z

storage/filesystem/dotgit/dotgit.go

 }

 // ObjectDelete removes the object file, if exists
 func (d *DotGit) ObjectDelete(h plumbing.Hash) error {
-	return d.fs.Remove(d.objectPath(h))
+	err1 := d.fs.Remove(d.objectPath(h))
+	if err1 != nil {


Only fallback to incoming object if os.IsNotExist(err1).

smola · 2018-08-14T08:31:19Z

storage/filesystem/dotgit/dotgit.go

 }

 // ObjectStat returns a os.FileInfo pointing the object file, if exists
 func (d *DotGit) ObjectStat(h plumbing.Hash) (os.FileInfo, error) {
-	return d.fs.Stat(d.objectPath(h))
+	obj1, err1 := d.fs.Stat(d.objectPath(h))
+	if err1 != nil {


Use os.IsNotExist

smola · 2018-08-14T08:31:31Z

storage/filesystem/dotgit/dotgit.go

 // Object returns a fs.File pointing the object file, if exists
 func (d *DotGit) Object(h plumbing.Hash) (billy.File, error) {
-	return d.fs.Open(d.objectPath(h))
+	obj1, err1 := d.fs.Open(d.objectPath(h))
+	if err1 != nil {


Use os.IsNotExist

noxora · 2018-08-14T18:22:35Z

Commits squashed and signed, added tests, but I have no idea why one of the three failed in appveyor, I will be looking into this, but the tests are very similar.
The relevant appveyor logs are

----------------------------------------------------------------------
FAIL: dotgit_test.go:567: SuiteDotGit.TestObjectDelete
dotgit_test.go:581:
    c.Assert(err, IsNil)
... value *os.PathError = &os.PathError{Op:"remove", Path:"C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\tmp-tgz-226769362\\objects\\9d\\25e0f9bde9f82882b49fe29117b9411cb157b7", Err:0x3} ("remove C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\tmp-tgz-226769362\\objects\\9d\\25e0f9bde9f82882b49fe29117b9411cb157b7: The system cannot find the path specified.")
OOPS: 34 passed, 1 FAILED
--- FAIL: Test (2.75s)
FAIL
FAIL	gopkg.in/src-d/go-git.v4/storage/filesystem/dotgit	2.964s

noxora · 2018-08-14T20:03:50Z

Alright, so for some reason TestObjectDelete isn't passing on appveyor, and I think it's to do with appveyor's environment. I have commented the troublesome portion of the test out for now, but it is very similar to TestObject and TestObjectStat which both pass.

All three of these tests pass on my macbook, so I am not sure why it is failing there

I would also like to clarify how this change helps with a git-hook.

By allowing the library to see a quarantined directory, one could write a git pre-receive hook using this library to do something like diff the HEAD with the incoming changes and perform some checks on any added/changed logic.

It is not part of triggering a hook to be run, but I would be happy to add that later in a different PR

smola · 2018-08-16T09:54:13Z

storage/filesystem/dotgit/dotgit_test.go

+	//	incomingDirPath := fs.Join("objects", "incoming-123456")
+	//	incomingFilePath := fs.Join(incomingDirPath, incomingHash[0:2], incomingHash[2:40])
+	//	fs.MkdirAll(incomingDirPath, os.FileMode(0755))
+	//	fs.Create(incomingFilePath)


You did not create the incomingHash[0:2] directory before creating incomingFilePath.

Also assert fs.MkdirAll and fs.Create do not return errors.

Maybe the problem is related to that. Or maybe something related to os.IsNotExist on Windows.

I tried those changes, and it also failed so I'm not exactly sure what's going on.

The changes are included, but commented out

Signed-off-by: noxora <[email protected]> trying a possible fix to the delete test Signed-off-by: noxora <[email protected]> still trying to fix this test Signed-off-by: noxora <[email protected]> fixes did not work, seems to be a windows env problem Signed-off-by: noxora <[email protected]>

Signed-off-by: Santiago M. Mola <[email protected]>

smola

Fixed the failing test.

noxora mentioned this pull request Jul 11, 2018

Get diff of head and incoming git object for pre-receive hook #886

Open

smola added the enhancement label Jul 16, 2018

smola suggested changes Aug 14, 2018

View reviewed changes

noxora force-pushed the hook-support branch from 2dc11d8 to bb8e98d Compare August 14, 2018 15:48

noxora force-pushed the hook-support branch 3 times, most recently from 2085391 to 65052ac Compare August 14, 2018 19:44

dustin-decker mentioned this pull request Aug 14, 2018

Any plans to support git hooks? #913

Open

noxora force-pushed the hook-support branch from 65052ac to 962d560 Compare August 15, 2018 13:33

smola reviewed Aug 16, 2018

View reviewed changes

noxora force-pushed the hook-support branch from 1ba52be to ae55016 Compare August 16, 2018 15:41

dotgit: fix object delete test

8cf7edb

Signed-off-by: Santiago M. Mola <[email protected]>

smola approved these changes Aug 17, 2018

View reviewed changes

mcuadros merged commit cc27d4a into src-d:master Aug 21, 2018

jfontan mentioned this pull request Aug 25, 2018

storage/dotgit: search for incoming dir only once #935

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added support for quarantine directory #887

added support for quarantine directory #887

noxora commented Jul 10, 2018

noxora commented Jul 10, 2018

noxora commented Jul 11, 2018

smola commented Jul 16, 2018

noxora commented Jul 16, 2018 •

edited

Loading

noxora commented Aug 2, 2018

noxora commented Aug 13, 2018

mcuadros commented Aug 14, 2018

smola left a comment

smola Aug 14, 2018

smola Aug 14, 2018

smola Aug 14, 2018

noxora commented Aug 14, 2018

noxora commented Aug 14, 2018 •

edited

Loading

smola Aug 16, 2018

noxora Aug 16, 2018

smola left a comment

added support for quarantine directory #887

added support for quarantine directory #887

Conversation

noxora commented Jul 10, 2018

noxora commented Jul 10, 2018

noxora commented Jul 11, 2018

smola commented Jul 16, 2018

noxora commented Jul 16, 2018 • edited Loading

noxora commented Aug 2, 2018

noxora commented Aug 13, 2018

mcuadros commented Aug 14, 2018

smola left a comment

Choose a reason for hiding this comment

smola Aug 14, 2018

Choose a reason for hiding this comment

smola Aug 14, 2018

Choose a reason for hiding this comment

smola Aug 14, 2018

Choose a reason for hiding this comment

noxora commented Aug 14, 2018

noxora commented Aug 14, 2018 • edited Loading

smola Aug 16, 2018

Choose a reason for hiding this comment

noxora Aug 16, 2018

Choose a reason for hiding this comment

smola left a comment

Choose a reason for hiding this comment

noxora commented Jul 16, 2018 •

edited

Loading

noxora commented Aug 14, 2018 •

edited

Loading