Clean fix #447

technoweenie · 2015-06-28T13:41:37Z

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.

rubyist · 2015-06-29T15:20:47Z

lfs/upload_queue.go

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 · 2015-07-06T16:59:32Z

Okay, I made the recommended change. 🤘

Clean fix

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

Our t/t-pre-push.sh and t/t-push-failures-local.sh test scripts include a number of tests which validate the behaviour of the "git lfs pre-push" and "git push" commands when Git LFS objects are missing or corrupt. While these tests are largely similar in their implementation, they vary in a number of formatting and implementation details unrelated to the specifics of the different conditions they simulate. These variations are artifacts of the evolution of our test suite over time; for example, the tests in the t/t-push-failures-local.sh script were refactored and collected from several earlier tests in commit 4d52e08 of PR git-lfs#3109, and the original versions of the tests in the t/t-pre-push.sh script were added incrementally in PRs git-lfs#447, git-lfs#2199, and git-lfs#2574. In a subsequent commit in this PR we expect to update the Git LFS client to remove some non-functional code which attempts to recreate missing Git LFS objects during push operations. In many cases this change will cause the client to report missing objects in an earlier stage of push operations than it does now. We also expect to reword the error message the client will output in such cases. Before we make these changes, we first revise the related tests in our test suite so they are as simple and similar as possible. This will ensure that when we update the Git LFS client we can clearly identify the changes that we need to make in our tests to accommodate the client's new behaviour. In the "pre-push with missing pointer not on server" test in the t/t-pre-push.sh script we remove the use of the "set +e" shell option, which is not necessary to avoid test failure. Although we expect the "git lfs pre-push" command to fail, its output is piped into the tee(1) command, and when the "set -e" option is in effect the shell will exit immediately if the last command in a pipeline returns a non-zero exit code, but not if other commands in the pipeline return such a code. We do, however, add a check that the "git lfs pre-push" command exits with a non-zero code by testing the appropriate PIPESTATUS array value, following the example of the other related tests. These other tests include the "pre-push with missing and present pointers (lfs.allowincompletepush true)" and "pre-push reject missing pointers (lfs.allowincompletepush default)" tests in the t/t-pre-push.sh script, plus the four tests in the t/t-push-failures-local.sh script. In all these tests we adjust the error messages generated in the case that a "git lfs pre-push" or "git push" command fails or succeeds unexpectedly. We rewrite these messages so they are consistent with each other and with those found in many of our other test scripts. Note that in the "push reject missing objects (lfs.allowincompletepush false)" test we also correct the message reported if the "git push" command were to succeed unexpectedly. At present, the message states that we expect the command to succeed, but we actually expect it to fail. Next, in the "push reject missing objects (lfs.allowincompletepush default)" and "push reject corrupt objects (lfs.allowincompletepush default)" tests, we update our checks of the "git push" command's exit code so that we confirm the command exits with a specific non-zero exit code of 1 rather than simply checking that its exit code is not zero. This change brings these checks into alignment with those made by the other related tests. Lastly, we remove and adjust some whitespace so that these tests are all formatted in a similar manner to each other.

Our t/t-pre-push.sh and t/t-push-failures-local.sh test scripts include a number of tests which validate the behaviour of the "git lfs pre-push" and "git push" commands when Git LFS objects are missing or corrupt. While these tests are largely similar in their implementation, they vary in a number of formatting and implementation details unrelated to the specifics of the different conditions they simulate. These variations are artifacts of the evolution of our test suite over time; for example, the tests in the t/t-push-failures-local.sh script were refactored and collected from several earlier tests in commit 4d52e08 of PR git-lfs#3109, and the original versions of the tests in the t/t-pre-push.sh script were added incrementally in PRs git-lfs#447, git-lfs#2199, and git-lfs#2574. In a subsequent commit in this PR we expect to update the Git LFS client to remove some non-functional code which attempts to recreate missing Git LFS objects during push operations. In many cases this change will cause the client to report missing objects in an earlier stage of push operations than it does now. We also expect to reword the error message the client will output in such cases. Before we make these changes, we first revise the related tests in our test suite so they are as simple and similar as possible. This will ensure that when we update the Git LFS client we can clearly identify the changes that we need to make in our tests to accommodate the client's new behaviour. In a pair of tests in t/t-pre-push.sh script, and in another pair of tests in the t/t-push-failures-local.sh script, the tests' initial setup code creates several Git LFS objects and then removes the object file in the .git/lfs/objects directory hierarchy for one of them. In each case, we can replace this code with a simple call to our delete_local_object() test helper function, which performs the equivalent action of removing an object's file from the internal Git LFS storage directories.

Our t/t-pre-push.sh test script includes a number of tests which validate the behaviour of the "git lfs pre-push" command when Git LFS objects are present, missing, or corrupt. While these tests are largely similar in their implementation, they vary in a number of formatting and implementation details unrelated to the specifics of the different conditions they simulate. These variations are artifacts of the evolution of our test suite over time; for example, the original versions of the tests in the t/t-pre-push.sh script were added incrementally in PRs git-lfs#447, git-lfs#2199, and git-lfs#2574. In a subsequent commit in this PR we expect to update the Git LFS client to remove some non-functional code which attempts to recreate missing Git LFS objects during push operations. In many cases this change will cause the client to report missing objects in an earlier stage of push operations than it does now. We also expect to reword the error message the client will output in such cases. Before we make these changes, we first revise the related tests in our test suite so they are as simple and similar as possible. This will ensure that when we update the Git LFS client we can clearly identify the changes that we need to make in our tests to accommodate the client's new behaviour. The "pre-push with existing file" and "pre-push with existing pointer" tests in the t/t-pre-push.sh script check similar conditions, with the only difference betwen that the latter test does not invoke the Git LFS client's "clean" filter and instead creates an object file in the .git/lfs/objects storage directories directly. Like the "pre-push with existing file" test, though, the "pre-push with existing pointer" test then pushes the new Git LFS object to the remote server and expect the object to be transferred successfully. However, unlike the "pre-push with existing file" test, the "pre-push with existing pointer" test does not confirm that the object exists in the remote server's storage,, so we add that confirmation step now using an assert_server_object() test helper function call identical to the one made at the end of the "pre-push with existing file" test.

Our t/t-pre-push.sh test script includes a number of tests which validate the behaviour of the "git lfs pre-push" command when Git LFS objects are present, missing, or corrupt. While these tests are largely similar in their implementation, they vary in a number of formatting and implementation details unrelated to the specifics of the different conditions they simulate. These variations are artifacts of the evolution of our test suite over time; for example, the original versions of the tests in the t/t-pre-push.sh script were added incrementally in PRs git-lfs#447, git-lfs#2199, and git-lfs#2574. In a subsequent commit in this PR we expect to update the Git LFS client to remove some non-functional code which attempts to recreate missing Git LFS objects during push operations. In many cases this change will cause the client to report missing objects in an earlier stage of push operations than it does now. We also expect to reword the error message the client will output in such cases. Before we make these changes, we first revise the related tests in our test suite so they are as simple and similar as possible. This will ensure that when we update the Git LFS client we can clearly identify the changes that we need to make in our tests to accommodate the client's new behaviour. Some of the tests in the t/t-pre-push.sh script which verify the behaviour of the "git lfs pre-push" command when Git LFS objects are present or missing use an older form of the common initial test repository setup steps, where the basename(1) command is called to generate a prefix from the script's name for each test repository's name on the remote server, but then a fixed string without that prefix is used when cloning the repository from the remote. This older approach to establishing a test repository is not uncommon in our test suite and is entirely functional. However, we expect to rename a number of these particular tests in the t/t-pre-push.sh script in a subsequent commit in this PR, and also revise the repository names used by the tests at the same time, to help make the tests as consistent with each other as possible. Therefore, before renaming these tests and their repositories, we adjust their initial setup steps to use follow the same pattern as, for example, the related tests in the same script and also those found in the t/t-push-failures-local.sh script.

Our t/t-pre-push.sh and t/t-push-failures-local.sh test scripts include a number of tests which validate the behaviour of the "git lfs pre-push" and "git push" commands when Git LFS objects are present, missing, or corrupt. While these tests are largely similar in their implementation, they vary in a number of formatting and implementation details unrelated to the specifics of the different conditions they simulate. These variations are artifacts of the evolution of our test suite over time; for example, the tests in the t/t-push-failures-local.sh script were refactored and collected from several earlier tests in commit 4d52e08 of PR git-lfs#3109, and the original versions of the tests in the t/t-pre-push.sh script were added incrementally in PRs git-lfs#447, git-lfs#2199, and git-lfs#2574. In a subsequent commit in this PR we expect to update the Git LFS client to remove some non-functional code which attempts to recreate missing Git LFS objects during push operations. In many cases this change will cause the client to report missing objects in an earlier stage of push operations than it does now. We also expect to reword the error message the client will output in such cases. Before we make these changes, we first revise the related tests in our test suite so they are as simple and similar as possible. This will ensure that when we update the Git LFS client we can clearly identify the changes that we need to make in our tests to accommodate the client's new behaviour. In previous commits in this PR we have revised and reformatted these tests to increase their consistency with each other. One additional adjustment we make now to further increase this consistency, and to provide greater clarity as to each test's purpose, is to rename both the tests and test repositories they create and clone. Note that since these tests typically only create a single missing or corrupt object, we now use the singular word "object" rather than "objects" in the test and repository names.

Our t/t-pre-push.sh and t/t-push-failures-local.sh test scripts include a number of tests which validate the behaviour of the "git lfs pre-push" and "git push" commands when Git LFS objects are missing or corrupt. While these tests are largely similar in their implementation, they vary in a number of formatting and implementation details unrelated to the specifics of the different conditions they simulate. These variations are artifacts of the evolution of our test suite over time; for example, the tests in the t/t-push-failures-local.sh script were refactored and collected from several earlier tests in commit 4d52e08 of PR git-lfs#3109, and the original versions of the tests in the t/t-pre-push.sh script were added incrementally in PRs git-lfs#447, git-lfs#2199, and git-lfs#2574. In a subsequent commit in this PR we expect to update the Git LFS client to remove some non-functional code which attempts to recreate missing Git LFS objects during push operations. In many cases this change will cause the client to report missing objects in an earlier stage of push operations than it does now. We also expect to reword the error message the client will output in such cases. Before we make these changes, we first revise the related tests in our test suite so they are as simple and similar as possible. This will ensure that when we update the Git LFS client we can clearly identify the changes that we need to make in our tests to accommodate the client's new behaviour. In the "pre-push with missing pointer not on server" test in the t/t-pre-push.sh script we remove the use of the "set +e" shell option, which is not necessary to avoid test failure. Although we expect the "git lfs pre-push" command to fail, its output is piped into the tee(1) command, and when the "set -e" option is in effect the shell will exit immediately if the last command in a pipeline returns a non-zero exit code, but not if other commands in the pipeline return such a code. We do, however, add a check that the "git lfs pre-push" command exits with a non-zero code by testing the appropriate PIPESTATUS array value, following the example of the other related tests. These other tests include the "pre-push with missing and present pointers (lfs.allowincompletepush true)" and "pre-push reject missing pointers (lfs.allowincompletepush default)" tests in the t/t-pre-push.sh script, plus the four tests in the t/t-push-failures-local.sh script. In all these tests we adjust the error messages generated in the case that a "git lfs pre-push" or "git push" command fails or succeeds unexpectedly. We rewrite these messages so they are consistent with each other and with those found in many of our other test scripts. Note that in the "push reject missing objects (lfs.allowincompletepush false)" test we also correct the message reported if the "git push" command were to succeed unexpectedly. At present, the message states that we expect the command to succeed, but we actually expect it to fail. Next, in the "push reject missing objects (lfs.allowincompletepush default)" and "push reject corrupt objects (lfs.allowincompletepush default)" tests, we update our checks of the "git push" command's exit code so that we confirm the command exits with a specific non-zero exit code of 1 rather than simply checking that its exit code is not zero. This change brings these checks into alignment with those made by the other related tests. Lastly, we remove and adjust some whitespace so that these tests are all formatted in a similar manner to each other.

Our t/t-pre-push.sh and t/t-push-failures-local.sh test scripts include a number of tests which validate the behaviour of the "git lfs pre-push" and "git push" commands when Git LFS objects are missing or corrupt. While these tests are largely similar in their implementation, they vary in a number of formatting and implementation details unrelated to the specifics of the different conditions they simulate. These variations are artifacts of the evolution of our test suite over time; for example, the tests in the t/t-push-failures-local.sh script were refactored and collected from several earlier tests in commit 4d52e08 of PR git-lfs#3109, and the original versions of the tests in the t/t-pre-push.sh script were added incrementally in PRs git-lfs#447, git-lfs#2199, and git-lfs#2574. In a subsequent commit in this PR we expect to update the Git LFS client to remove some non-functional code which attempts to recreate missing Git LFS objects during push operations. In many cases this change will cause the client to report missing objects in an earlier stage of push operations than it does now. We also expect to reword the error message the client will output in such cases. Before we make these changes, we first revise the related tests in our test suite so they are as simple and similar as possible. This will ensure that when we update the Git LFS client we can clearly identify the changes that we need to make in our tests to accommodate the client's new behaviour. In a pair of tests in t/t-pre-push.sh script, and in another pair of tests in the t/t-push-failures-local.sh script, the tests' initial setup code creates several Git LFS objects and then removes the object file in the .git/lfs/objects directory hierarchy for one of them. In each case, we can replace this code with a simple call to our delete_local_object() test helper function, which performs the equivalent action of removing an object's file from the internal Git LFS storage directories.

Our t/t-pre-push.sh test script includes a number of tests which validate the behaviour of the "git lfs pre-push" command when Git LFS objects are present, missing, or corrupt. While these tests are largely similar in their implementation, they vary in a number of formatting and implementation details unrelated to the specifics of the different conditions they simulate. These variations are artifacts of the evolution of our test suite over time; for example, the original versions of the tests in the t/t-pre-push.sh script were added incrementally in PRs git-lfs#447, git-lfs#2199, and git-lfs#2574. In a subsequent commit in this PR we expect to update the Git LFS client to remove some non-functional code which attempts to recreate missing Git LFS objects during push operations. In many cases this change will cause the client to report missing objects in an earlier stage of push operations than it does now. We also expect to reword the error message the client will output in such cases. Before we make these changes, we first revise the related tests in our test suite so they are as simple and similar as possible. This will ensure that when we update the Git LFS client we can clearly identify the changes that we need to make in our tests to accommodate the client's new behaviour. The "pre-push with existing file" and "pre-push with existing pointer" tests in the t/t-pre-push.sh script check similar conditions, with the only difference betwen that the latter test does not invoke the Git LFS client's "clean" filter and instead creates an object file in the .git/lfs/objects storage directories directly. Like the "pre-push with existing file" test, though, the "pre-push with existing pointer" test then pushes the new Git LFS object to the remote server and expect the object to be transferred successfully. However, unlike the "pre-push with existing file" test, the "pre-push with existing pointer" test does not confirm that the object exists in the remote server's storage,, so we add that confirmation step now using an assert_server_object() test helper function call identical to the one made at the end of the "pre-push with existing file" test.

Our t/t-pre-push.sh test script includes a number of tests which validate the behaviour of the "git lfs pre-push" command when Git LFS objects are present, missing, or corrupt. While these tests are largely similar in their implementation, they vary in a number of formatting and implementation details unrelated to the specifics of the different conditions they simulate. These variations are artifacts of the evolution of our test suite over time; for example, the original versions of the tests in the t/t-pre-push.sh script were added incrementally in PRs git-lfs#447, git-lfs#2199, and git-lfs#2574. In a subsequent commit in this PR we expect to update the Git LFS client to remove some non-functional code which attempts to recreate missing Git LFS objects during push operations. In many cases this change will cause the client to report missing objects in an earlier stage of push operations than it does now. We also expect to reword the error message the client will output in such cases. Before we make these changes, we first revise the related tests in our test suite so they are as simple and similar as possible. This will ensure that when we update the Git LFS client we can clearly identify the changes that we need to make in our tests to accommodate the client's new behaviour. Some of the tests in the t/t-pre-push.sh script which verify the behaviour of the "git lfs pre-push" command when Git LFS objects are present or missing use an older form of the common initial test repository setup steps, where the basename(1) command is called to generate a prefix from the script's name for each test repository's name on the remote server, but then a fixed string without that prefix is used when cloning the repository from the remote. This older approach to establishing a test repository is not uncommon in our test suite and is entirely functional. However, we expect to rename a number of these particular tests in the t/t-pre-push.sh script in a subsequent commit in this PR, and also revise the repository names used by the tests at the same time, to help make the tests as consistent with each other as possible. Therefore, before renaming these tests and their repositories, we adjust their initial setup steps to use follow the same pattern as, for example, the related tests in the same script and also those found in the t/t-push-failures-local.sh script.

Our t/t-pre-push.sh and t/t-push-failures-local.sh test scripts include a number of tests which validate the behaviour of the "git lfs pre-push" and "git push" commands when Git LFS objects are present, missing, or corrupt. While these tests are largely similar in their implementation, they vary in a number of formatting and implementation details unrelated to the specifics of the different conditions they simulate. These variations are artifacts of the evolution of our test suite over time; for example, the tests in the t/t-push-failures-local.sh script were refactored and collected from several earlier tests in commit 4d52e08 of PR git-lfs#3109, and the original versions of the tests in the t/t-pre-push.sh script were added incrementally in PRs git-lfs#447, git-lfs#2199, and git-lfs#2574. In a subsequent commit in this PR we expect to update the Git LFS client to remove some non-functional code which attempts to recreate missing Git LFS objects during push operations. In many cases this change will cause the client to report missing objects in an earlier stage of push operations than it does now. We also expect to reword the error message the client will output in such cases. Before we make these changes, we first revise the related tests in our test suite so they are as simple and similar as possible. This will ensure that when we update the Git LFS client we can clearly identify the changes that we need to make in our tests to accommodate the client's new behaviour. In previous commits in this PR we have revised and reformatted these tests to increase their consistency with each other. One additional adjustment we make now to further increase this consistency, and to provide greater clarity as to each test's purpose, is to rename both the tests and test repositories they create and clone. Note that since these tests typically only create a single missing or corrupt object, we now use the singular word "object" rather than "objects" in the test and repository names.

Our t/t-pre-push.sh and t/t-push-failures-local.sh test scripts include a number of tests which validate the behaviour of the "git lfs pre-push" and "git push" commands when Git LFS objects are missing or corrupt. While these tests are largely similar in their implementation, they vary in a number of formatting and implementation details unrelated to the specifics of the different conditions they simulate. These variations are artifacts of the evolution of our test suite over time; for example, the tests in the t/t-push-failures-local.sh script were refactored and collected from several earlier tests in commit 4d52e08 of PR git-lfs#3109, and the original versions of the tests in the t/t-pre-push.sh script were added incrementally in PRs git-lfs#447, git-lfs#2199, and git-lfs#2574. In a subsequent commit in this PR we expect to update the Git LFS client to remove some non-functional code which attempts to recreate missing Git LFS objects during push operations. In many cases this change will cause the client to report missing objects in an earlier stage of push operations than it does now. We also expect to reword the error message the client will output in such cases. Before we make these changes, we first revise the related tests in our test suite so they are as simple and similar as possible. This will ensure that when we update the Git LFS client we can clearly identify the changes that we need to make in our tests to accommodate the client's new behaviour. A pair of tests in the t/t-push-failures-local.sh script, specifically the "push reject missing objects (lfs.allowincompletepush default)" and "push reject corrupt objects (lfs.allowincompletepush default)" tests, perform their setup of Git LFS objects and Git commits in a different sequence than the other tests in the same script. In order to align the code in all these tests as closely as possible, we simply revise the setup steps of the last two tests in the t/t-push-failures-local.sh script so they follow the same pattern as those of the other tests. This change has no functional effect; the only notable detail is that the tests now create all their Git LFS objects in a single commit instead of using a separate commit to create each object. As well, we reorder the lists of files we pass to the "git add" command in the first two tests in the t/t-push-failures-local.sh script so they now follow the same pattern as those of the other tests in both that script and in the t/t-pre-push.sh script. We also adjust the commit message used when creating Git LFS objects in a pair of tests in the t/t-pre-push.sh script, specifically the "pre-push with missing and present pointers (lfs.allowincompletepush true)" and "pre-push reject missing pointers (lfs.allowincompletepush default)" tests, so they are equivalent to those used in the related tests in the t/t-push-failures-local.sh script.

Our t/t-pre-push.sh test script includes a number of tests which validate the behaviour of the "git lfs pre-push" command when Git LFS objects are present, missing, or corrupt. While these tests are largely similar in their implementation, they vary in a number of formatting and implementation details unrelated to the specifics of the different conditions they simulate. These variations are artifacts of the evolution of our test suite over time; for example, the original versions of the tests in the t/t-pre-push.sh script were added incrementally in PRs git-lfs#447, git-lfs#2199, and git-lfs#2574. In a subsequent commit in this PR we expect to update the Git LFS client to remove some non-functional code which attempts to recreate missing Git LFS objects during push operations. In many cases this change will cause the client to report missing objects in an earlier stage of push operations than it does now. We also expect to reword the error message the client will output in such cases. Before we make these changes, we first revise the related tests in our test suite so they are as simple and similar as possible. This will ensure that when we update the Git LFS client we can clearly identify the changes that we need to make in our tests to accommodate the client's new behaviour. The "pre-push with existing file" and "pre-push with existing pointer" tests in the t/t-pre-push.sh script check similar conditions, with the only difference betwen that the latter test does not invoke the Git LFS client's "clean" filter and instead creates an object file in the .git/lfs/objects storage directories directly. Like the "pre-push with existing file" test, though, the "pre-push with existing pointer" test then pushes the new Git LFS object to the remote server and expect the object to be transferred successfully. However, unlike the "pre-push with existing file" test, the "pre-push with existing pointer" test does not confirm that the object exists in the remote server's storage, so we add that confirmation step now using an assert_server_object() test helper function call identical to the one made at the end of the "pre-push with existing file" test. Another pair of tests, the "pre-push with missing pointer not on server" test in the t/t-pre-push.sh script and the "push reject missing objects (lfs.allowincompletepush default)" test in the t/t-push-failures-local.sh script, also check similar conditions, with the difference that the former test never creates an object file in the .git/lfs/objects storage directories, while the latter allows an object file to be created by the "clean" filter, and then deletes it. In both cases, though, they expect the absent object file to cause a failure to be reported by the "git lfs pre-push" or "git push" commands, respectively. However, the "pre-push with missing pointer not on server" test, unlike the "push reject missing objects (lfs.allowincompletepush default)" test, does not call the refute_server_object() helper function to confirm the that the object does not exist in the remote server's storage after the push operation runs, and also does not check for the message "LFS upload failed" in the output of the push operation, so we add those checks now.