git: avoid "bad object" messages when force-pushing#4102
Merged
bk2204 merged 1 commit intogit-lfs:masterfrom Apr 23, 2020
Merged
git: avoid "bad object" messages when force-pushing#4102bk2204 merged 1 commit intogit-lfs:masterfrom
bk2204 merged 1 commit intogit-lfs:masterfrom
Conversation
aede776 to
5affa79
Compare
chrisd8088
approved these changes
Apr 20, 2020
Member
chrisd8088
left a comment
There was a problem hiding this comment.
Really appreciate the clearly commented additional test, thank you!
If a user attempts to force-push over a ref and we don't currently have the object that the reference currently points to, our pre-push hook will fail with a message like the following: ref foo:: Error in git rev-list --stdin --objects --not --remotes=origin --: exit status 128 fatal: bad object 1aabb695d63c4d4afe731ab69573893390526896 This occurs because we pass the existing object name to git rev-list as something to exclude. Normally, this is a good idea, since it means we can short-circuit the traversal since we already know what the other side has. However, in this case, it causes unwanted problems. What we really want is for Git to just ignore an object if it's bad, and fortunately the rev-list --ignore-missing object does exactly this, so let's use it. Note that we must pass it before the --stdin option since otherwise it has no effect, so move the --stdin option to the end of the command.
5affa79 to
aab142d
Compare
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Oct 4, 2024
In previous commits in this PR we have adjusted the arguments we use when executing the "git ls-remote" command so as to limit the refs it returns to include only branches, in cases where we do not need to process any tag refs. Specifically, the "git lfs push" and "git lfs pre-push" commands invoke the NewGitScannerForPush() function in our "lfs" package, which uses the calcSkippedRefs() function in the same package to find the set of remote branch refs that are also cached locally. Any commits reachable from the commits referenced by the locally cached versions of these refs will then be excluded from the set of commits returned by the "git rev-list" command when that is subsequently called by the revListShas() function and the methods of the RevListScanner structure in our "git" package. No tag refs are considered during these steps, so can avoid retrieving them with the "git ls-remote" command. Earlier, in commit aab142d of PR git-lfs#4102, we added the "pre-push with force-pushed ref" test to our t/t-pre-push.sh test suite, which checks that the "git lfs pre-push" command handles the condition where a Git commit is referenced by a remote ref, and is not present locally, but also happens to be one that can be ignored because it has been replaced in the local version of the ref by a force-push. (The test runs the "git lfs pre-push" command indirectly, as it is executed by the "git push" command.) Fortunately, this test suffices to validate the changes we have made in this PR also, at least for the "git lfs pre-push" command, because the test creates and pushes a tag, and then updates and pushes the tag again. As a result, it runs the "git lfs pre-push" command in a context where a tag ref exists on the Git remote. Prior to our changes in this PR, the "git ls-remote" command (as performed by the RemoteRefs() function in the "git" package, when called by the calcSkippedRefs() function) would return the tag ref as well as the "main" branch ref. With this PR's changes, the "git ls-remote" command now only returns the "main" branch ref under this test's conditions, so we have an effective test of the "git lfs pre-push" command with a tag ref on a Git remote. However, we do not have a test of the "git lfs push" command with a tag ref on the Git remote, so we add such a test now. This test is not strictly necessary, but ensures we run the "git lfs push" command in a context where a tag has been created and pushed to the Git remote. Without this PR's other changes, the tag ref would then be returned by "git ls-remote" when that was performed as part of the "git lfs push" command's operation. With this PR's changes, though, the tag ref will not be returned by "git ls-remote". The test will succeed in either case because the tag ref is ultimately immaterial in the determination of the refs returned by the calcSkippedRefs() function.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Oct 4, 2024
In previous commits in this PR we have adjusted the arguments we use when executing the "git ls-remote" command so as to limit the refs it returns to include only branches, in cases where we do not need to process any tag refs. Specifically, the "git lfs push" and "git lfs pre-push" commands invoke the NewGitScannerForPush() function in our "lfs" package, which uses the calcSkippedRefs() function in the same package to find the set of remote branch refs that are also cached locally. Any commits reachable from the commits referenced by the locally cached versions of these refs will then be excluded from the set of commits returned by the "git rev-list" command when that is subsequently called by the revListShas() function and the methods of the RevListScanner structure in our "git" package. No tag refs are considered during these steps, so can avoid retrieving them with the "git ls-remote" command. Earlier, in commit aab142d of PR git-lfs#4102, we added the "pre-push with force-pushed ref" test to our t/t-pre-push.sh test suite, which checks that the "git lfs pre-push" command handles the condition where a Git commit is referenced by a remote ref, and is not present locally, but also happens to be one that can be ignored because it has been replaced in the local version of the ref by a force-push. (The test runs the "git lfs pre-push" command indirectly, as it is executed by the "git push" command.) Fortunately, this test suffices to validate the changes we have made in this PR also, at least for the "git lfs pre-push" command, because the test creates and pushes a tag, and then updates and pushes the tag again. As a result, it runs the "git lfs pre-push" command in a context where a tag ref exists on the Git remote. Prior to our changes in this PR, the "git ls-remote" command (as performed by the RemoteRefs() function in the "git" package, when called by the calcSkippedRefs() function) would return the tag ref as well as the "main" branch ref. With this PR's changes, the "git ls-remote" command now only returns the "main" branch ref under this test's conditions, so we have an effective test of the "git lfs pre-push" command with a tag ref on a Git remote. However, we do not have a test of the "git lfs push" command with a tag ref on the Git remote, so we add such a test now. This test is not strictly necessary, but ensures we run the "git lfs push" command in a context where a tag has been created and pushed to the Git remote. Without this PR's other changes, the tag ref would then be returned by "git ls-remote" when that was performed as part of the "git lfs push" command's operation. With this PR's changes, though, the tag ref will not be returned by "git ls-remote". The test will succeed in either case because the tag ref is ultimately immaterial in the determination of the refs returned by the calcSkippedRefs() function.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If a user attempts to force-push over a ref and we don't currently have the object that the reference currently points to, our pre-push hook will fail with a message like the following:
This occurs because we pass the existing object name to git rev-list as something to exclude. Normally, this is a good idea, since it means we can short-circuit the traversal since we already know what the other side has. However, in this case, it causes unwanted problems.
What we really want is for Git to just ignore an object if it's bad, and fortunately the rev-list
--ignore-missingobject does exactly this, so let's use it. Note that we must pass it before the--stdinoption since otherwise it has no effect, so move the--stdinoption to the end of the command.Fixes #3977