Add vendor/.git exists check in SafeWriter.Write #464
Conversation
|
@sdboyer should we handle cross-device rename errors here, as in Masterminds/glide#699 ? Added a unit test for Didn't add integration test for Need help in adding integration test for |
txn_writer.go
Outdated
| @@ -294,6 +294,14 @@ func (sw *SafeWriter) Write(root string, sm gps.SourceManager) error { | |||
| } | |||
| } | |||
|
|
|||
| // Ensure vendor/.git is preserved if present | |||
| if hasDotGit(vpath) { | |||
| err = os.Rename(filepath.Join(vpath, ".git"), filepath.Join(td, "vendor/.git")) | |||
There was a problem hiding this comment.
dep.renameWithFallback does what you have here, plus a bunch more error handling, let's use that instead of os.Rename directly.
txn_writer.go
Outdated
| func hasDotGit(path string) bool { | ||
| gitfilepath := filepath.Join(path, ".git") | ||
| _, err := os.Stat(gitfilepath) | ||
| if err != nil { |
There was a problem hiding this comment.
This can be simplified to return err == nil.
| @@ -522,3 +524,17 @@ func TestSafeWriter_DiffLocks(t *testing.T) { | |||
| t.Fatal(err) | |||
| } | |||
| } | |||
|
|
|||
| func TestHasDotGit(t *testing.T) { | |||
There was a problem hiding this comment.
Let's add one more test that populates vendor/.git, and then writes out vendor using SafeWriter, verifying that this feature is plumbed in properly. You can look at https://github.com/golang/dep/blob/master/txn_writer_test.go#L215 for an example of how to call the safewriter and force vendor to be rewritten.
|
See my review comments for how to deal with cross device rename problems, and how to test your changes. |
- Adds TestSafeWriter_VendorDotGitPreservedWithForceVendor test. - Replaces os.Rename with renameWithFallback
|
@carolynvs thanks for the review :) Made the changes and tests are passing. Lemme know if anything else is required and if I should squash the commits. |
|
LGTM - thanks for this, @darkowlzz! |
Add vendor/.git exists check in SafeWriter.Write
Fixes #460