Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean fix #447

Merged
merged 8 commits into from Jul 6, 2015
Merged

Clean fix #447

merged 8 commits into from Jul 6, 2015

Conversation

technoweenie
Copy link
Contributor

git push errors if you have Git LFS pointers without an object in .git/lfs/objects. This prevents the client from making a successful Git push without a successful Git LFS push. But, the error message is very confusing.

Cannot clean a Git LFS pointer. Skipping.

The confusing error message was written to prevent git add from turning a Git LFS pointer to another pointer in the clean filter. So here's an attempt at a better error:

new.dat is an LFS pointer to 7aa7a5359173d05b63cfd682e3c38487f3cb4f7f1d60659fe59fab1505977d4c, which does not exist in .git/lfs/objects.

Run 'git lfs fsck' to verify Git LFS objects.

This hopefully makes a little more sense. It also points the user to the fsck command, which will show the user all of their missing objects. Thoughts?

The first commits are some light refactoring as I was going over some code. Some properties were no longer used, or could be simplified. For instance, no need to send an *os.File in the *cleanedAsset object if every caller is going to immediately close it. Just close it and send the filename instead.

Also, the error message change in commands/command_pre_push.go is a hack. lfs.NewUploadable should return a *WrappedError with the Panic bool set to false. This way both push and pre-push get the correct error message for free.

@@ -100,7 +101,9 @@ func ensureFile(smudgePath, cleanPath string) error {
return err
}

cleaned.Close()
if cleaned != nil {
defer cleaned.Teardown()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this section is quite right, for a couple reasons. Primarily, it's possible for PointerClean to return an error with and a valid cleanedAsset if the CopyWithCallback returns an error. In this case, ensureFile will return without removing the cleaned file.

Less importantly, I don't think we need to defer the Teardown(), It's only removing the underlying file and we're not using it before the end of the function.

Maybe something like:

diff --git a/lfs/upload_queue.go b/lfs/upload_queue.go
index cbd075a..4a26f08 100644
--- a/lfs/upload_queue.go
+++ b/lfs/upload_queue.go
@@ -97,12 +97,12 @@ func ensureFile(smudgePath, cleanPath string) error {
    }

    cleaned, err := PointerClean(file, stat.Size(), nil)
-   if err != nil {
-       return err
+   if cleaned != nil {
+       cleaned.Teardown()
    }

-   if cleaned != nil {
-       defer cleaned.Teardown()
+   if err != nil {
+       return err
    }

    if expectedOid != cleaned.Oid {

@technoweenie technoweenie self-assigned this Jul 6, 2015
@technoweenie technoweenie mentioned this pull request Jul 6, 2015
11 tasks
@technoweenie
Copy link
Contributor Author

Okay, I made the recommended change. 🤘

@technoweenie technoweenie mentioned this pull request Jul 6, 2015
13 tasks
technoweenie added a commit that referenced this pull request Jul 6, 2015
@technoweenie technoweenie merged commit 1f24533 into master Jul 6, 2015
@technoweenie technoweenie deleted the clean-fix branch July 6, 2015 17:00
technoweenie added a commit that referenced this pull request Jul 22, 2015
technoweenie added a commit that referenced this pull request Jul 22, 2015
@technoweenie technoweenie removed their assignment Jul 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants