Skip to content

git: avoid "bad object" messages when force-pushing#4102

Merged
bk2204 merged 1 commit intogit-lfs:masterfrom
bk2204:bad-object-pre-push
Apr 23, 2020
Merged

git: avoid "bad object" messages when force-pushing#4102
bk2204 merged 1 commit intogit-lfs:masterfrom
bk2204:bad-object-pre-push

Conversation

@bk2204
Copy link
Member

@bk2204 bk2204 commented Apr 17, 2020

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.

Fixes #3977

@bk2204 bk2204 requested a review from a team April 17, 2020 14:27
@bk2204 bk2204 force-pushed the bad-object-pre-push branch 2 times, most recently from aede776 to 5affa79 Compare April 17, 2020 15:45
Copy link
Member

@chrisd8088 chrisd8088 left a comment

Choose a reason for hiding this comment

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

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.
@bk2204 bk2204 force-pushed the bad-object-pre-push branch from 5affa79 to aab142d Compare April 23, 2020 15:13
@bk2204 bk2204 merged commit 05310a2 into git-lfs:master Apr 23, 2020
@bk2204 bk2204 deleted the bad-object-pre-push branch April 23, 2020 16:56
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Force pushes can lead to "bad object" message from git rev-list

2 participants