Merge pull request #498 from github/release-0.5-backport-447

Backport #447 for v0.5.x: Clean fix

commands/command_clean.go

@@ -46,7 +46,6 @@ func cleanCommand(cmd *cobra.Command, args []string) {
 	}
 
 	if cleaned != nil {
-		cleaned.Close()
 		defer cleaned.Teardown()
 	}
 
@@ -59,7 +58,7 @@ func cleanCommand(cmd *cobra.Command, args []string) {
 		Panic(err, "Error cleaning asset.")
 	}
 
-	tmpfile := cleaned.File.Name()
+	tmpfile := cleaned.Filename
 	mediafile, err := lfs.LocalMediaPath(cleaned.Oid)
 	if err != nil {
 		Panic(err, "Unable to get local media path.")

commands/command_pre_push.go

@@ -81,7 +81,10 @@ func prePushCommand(cmd *cobra.Command, args []string) {
 
 		u, wErr := lfs.NewUploadable(pointer.Oid, pointer.Name, i+1, len(pointers))
 		if wErr != nil {
-			if Debugging || wErr.Panic {
+			if cleanPointerErr, ok := wErr.Err.(*lfs.CleanedPointerError); ok {
+				Exit("%s is an LFS pointer to %s, which does not exist in .git/lfs/objects.\n\nRun 'git lfs fsck' to verify Git LFS objects.",
+					pointer.Name, cleanPointerErr.Pointer.Oid)
+			} else if Debugging || wErr.Panic {
 				Panic(wErr.Err, wErr.Error())
 			} else {
 				Exit(wErr.Error())

lfs/pointer_clean.go

@@ -9,13 +9,13 @@ import (
 )
 
 type cleanedAsset struct {
-	File          *os.File
-	mediafilepath string
+	Filename string
 	*Pointer
 }
 
 type CleanedPointerError struct {
-	Bytes []byte
+	Pointer *Pointer
+	Bytes   []byte
 }
 
 func (e *CleanedPointerError) Error() string {
@@ -28,29 +28,27 @@ func PointerClean(reader io.Reader, size int64, cb CopyCallback) (*cleanedAsset,
 		return nil, err
 	}
 
+	defer tmp.Close()
+
 	oidHash := sha256.New()
 	writer := io.MultiWriter(oidHash, tmp)
 
 	if size == 0 {
 		cb = nil
 	}
 
-	by, _, err := DecodeFrom(reader)
+	by, ptr, err := DecodeFrom(reader)
 	if err == nil && len(by) < 512 {
-		return nil, &CleanedPointerError{by}
+		return nil, &CleanedPointerError{ptr, by}
 	}
 
 	multi := io.MultiReader(bytes.NewReader(by), reader)
 	written, err := CopyWithCallback(writer, multi, size, cb)
 
 	pointer := NewPointer(hex.EncodeToString(oidHash.Sum(nil)), written)
-	return &cleanedAsset{tmp, "", pointer}, err
-}
-
-func (a *cleanedAsset) Close() error {
-	return a.File.Close()
+	return &cleanedAsset{tmp.Name(), pointer}, err
 }
 
 func (a *cleanedAsset) Teardown() error {
-	return os.Remove(a.File.Name())
+	return os.Remove(a.Filename)
 }

lfs/upload_queue.go

@@ -96,12 +96,14 @@ func ensureFile(smudgePath, cleanPath string) error {
 	}
 
 	cleaned, err := PointerClean(file, stat.Size(), nil)
+	if cleaned != nil {
+		cleaned.Teardown()
+	}
+
 	if err != nil {
 		return err
 	}
 
-	cleaned.Close()
-
 	if expectedOid != cleaned.Oid {
 		return fmt.Errorf("Expected %s to have an OID of %s, got %s", smudgePath, expectedOid, cleaned.Oid)
 	}

test/test-clean.sh

@@ -2,33 +2,52 @@
 
 . "test/testlib.sh"
 
+clean_setup () {
+  mkdir "$1"
+  cd "$1"
+  git init
+}
 
-begin_test "clean"
+begin_test "clean simple file"
 (
   set -e
+  clean_setup "simple"
 
-  mkdir repo
-  cd repo
-  git init
-
-  # clean a simple file
   echo "whatever" | git lfs clean | tee clean.log
   [ "$(pointer cd293be6cea034bd45a0352775a219ef5dc7825ce55d1f7dae9762d80ce64411 9)" = "$(cat clean.log)" ]
+)
+end_test
+
+begin_test "clean a pointer"
+(
+  set -e
+  clean_setup "pointer"
 
-  # clean a git lfs pointer
   pointer cd293be6cea034bd45a0352775a219ef5dc7825ce55d1f7dae9762d80ce64411 9 | git lfs clean | tee clean.log
   [ "$(pointer cd293be6cea034bd45a0352775a219ef5dc7825ce55d1f7dae9762d80ce64411 9)" = "$(cat clean.log)" ]
+)
+end_test
+
+begin_test "clean pseudo pointer"
+(
+  set -e
+  clean_setup "pseudo"
 
-  # clean a pseudo pointer with extra data
   echo "version https://git-lfs.github.com/spec/v1
 oid sha256:7cd8be1d2cd0dd22cd9d229bb6b5785009a05e8b39d405615d882caac56562b5
 size 1024
 
 This is my test pointer.  There are many like it, but this one is mine." | git lfs clean | tee clean.log
   [ "$(pointer f492acbebb5faa22da4c1501c022af035469f624f426631f31936575873fefe1 202)" = "$(cat clean.log)" ]
+)
+end_test
+
+begin_test "clean pseudo pointer with extra data"
+(
+  set -e
+  clean_setup "extra-data"
 
-  # clean a pseudo pointer with extra data separated by enough white space to
-  # fill the 'git lfs clean' buffer
+  # pointer includes enough extra data to fill the 'git lfs clean' buffer
   echo "version https://git-lfs.github.com/spec/v1
 oid sha256:7cd8be1d2cd0dd22cd9d229bb6b5785009a05e8b39d405615d882caac56562b5
 size 1024

test/test-pre-push.sh

@@ -24,7 +24,6 @@ begin_test "pre-push"
   git add hi.dat
   git commit -m "add hi.dat"
   git show
-  git lfs env
 
   # file isn't on the git lfs server yet
   curl -v "$GITSERVER/$reponame.git/info/lfs/objects/98ea6e4f216f2fb4b69fff9b3a44842c38686ca685f3f55dc48c5d3fb1107be4" \
@@ -78,7 +77,6 @@ begin_test "pre-push dry-run"
   git add hi.dat
   git commit -m "add hi.dat"
   git show
-  git lfs env
 
   # file doesn't exist yet
   curl -v "$GITSERVER/$reponame.git/info/lfs/objects/2840e0eafda1d0760771fe28b91247cf81c76aa888af28a850b5648a338dc15b" \
@@ -121,7 +119,6 @@ begin_test "pre-push 307 redirects"
   git add hi.dat
   git commit -m "add hi.dat"
   git show
-  git lfs env
 
   # push file to the git lfs server
   echo "refs/heads/master master refs/heads/master 0000000000000000000000000000000000000000" |
@@ -149,7 +146,6 @@ begin_test "pre-push 307 redirects"
   git add hi2.dat
   git commit -m "add hi2.dat"
   git show
-  git lfs env
 
   # push file to the git lfs server
   echo "refs/heads/master master refs/heads/master 0000000000000000000000000000000000000000" |
@@ -160,3 +156,86 @@ begin_test "pre-push 307 redirects"
 
 )
 end_test
+
+begin_test "pre-push with existing file"
+(
+  set -e
+
+  reponame="$(basename "$0" ".sh")-existing-file"
+  setup_remote_repo "$reponame"
+
+  clone_repo "$reponame" existing-file
+  echo "existing" > existing.dat
+  git add existing.dat
+  git commit -m "add existing dat"
+
+  git lfs track "*.dat"
+  echo "new" > new.dat
+  git add new.dat
+  git add .gitattributes
+  git commit -m "add new file through git lfs"
+
+  # push file to the git lfs server
+  echo "refs/heads/master master refs/heads/master 0000000000000000000000000000000000000000" |
+    git lfs pre-push origin "$GITSERVER/$reponame" 2>&1 |
+    tee push.log
+  grep "(1 of 1 files)" push.log
+
+  # now the file exists
+  curl -v "$GITSERVER/$reponame.git/info/lfs/objects/7aa7a5359173d05b63cfd682e3c38487f3cb4f7f1d60659fe59fab1505977d4c" \
+    -u "user:pass" \
+    -o lfs.json \
+    -H "Accept: application/vnd.git-lfs+json" 2>&1 |
+    tee http.log
+  grep "200 OK" http.log
+
+  grep "download" lfs.json || {
+    cat lfs.json
+    exit 1
+  }
+)
+end_test
+
+begin_test "pre-push with existing pointer"
+(
+  set -e
+
+  reponame="$(basename "$0" ".sh")-existing-pointer"
+  setup_remote_repo "$reponame"
+  clone_repo "$reponame" existing-pointer
+
+  echo "$(pointer "7aa7a5359173d05b63cfd682e3c38487f3cb4f7f1d60659fe59fab1505977d4c" 4)" > new.dat
+  git add new.dat
+  git commit -m "add new pointer"
+  mkdir -p .git/lfs/objects/7a/a7
+  echo "new" > .git/lfs/objects/7a/a7/7aa7a5359173d05b63cfd682e3c38487f3cb4f7f1d60659fe59fab1505977d4c
+
+  # push file to the git lfs server
+  echo "refs/heads/master master refs/heads/master 0000000000000000000000000000000000000000" |
+    git lfs pre-push origin "$GITSERVER/$reponame" 2>&1 |
+    tee push.log
+  grep "(1 of 1 files)" push.log
+)
+end_test
+
+begin_test "pre-push with missing pointer"
+(
+  set -e
+
+  reponame="$(basename "$0" ".sh")-missing-pointer"
+  setup_remote_repo "$reponame"
+  clone_repo "$reponame" missing-pointer
+
+  echo "$(pointer "7aa7a5359173d05b63cfd682e3c38487f3cb4f7f1d60659fe59fab1505977d4c" 4)" > new.dat
+  git add new.dat
+  git commit -m "add new pointer"
+
+  # assert that push fails
+  set +e
+  echo "refs/heads/master master refs/heads/master 0000000000000000000000000000000000000000" |
+    git lfs pre-push origin "$GITSERVER/$reponame" 2>&1 |
+    tee push.log
+  set -e
+  grep "new.dat is an LFS pointer to 7aa7a5359173d05b63cfd682e3c38487f3cb4f7f1d60659fe59fab1505977d4c, which does not exist in .git/lfs/objects" push.log
+)
+end_test