cmd/vendor/golang.org/x/tools: vet binary checked in to repo, rewrite history #28899
Comments
|
Ouch. Except this happened 6 days ago, so a git cleanup would be more disruptive than last time. I suppose we could teach git-codereview's "git sync" to have a list of known force pushes to not fail (if it does, which I assume). That would at least address the users who use that tool (many of our contributors I assume?). Thoughts? /cc @andybons @dmitshur @ianlancetaylor @bcmills @katiehockman |
bradfitz
added
NeedsDecision
|
I'm in favor of rewriting the history to drop the binary for the following reasons:
How long would this take to implement? I don't use This should serve as an increase on priority of #10658. |
|
The binary seems large enough to me that we should force-push. Mildly annoying a few dozen developers for a week with a force push seems better than mildly annoying thousands with a larger git repository forever :) |
|
@ianlancetaylor, you cool with a git history rewrite? |
|
Okay, @andybons @dmitshur @mvdan and I vote yes, and @ianlancetaylor says whatever we decide. So I can do this. I'll first post a shell script here for what I plan to run before I do it so people can review. |
|
Here's the plan, tested: #!/bin/bash
set -e
git checkout master
git branch -D rewrite_28899 || true
git change rewrite_28899
git fetch origin
git reset --hard origin/master
HEAD=$(git rev-parse HEAD)
# The commit with the bad blob.
BAD=50941e92414d5f51d1188285f593bfd8f6fbf515
git reset --hard $BAD
git rm src/cmd/vendor/golang.org/x/tools/go/analysis/vet
EDITOR=true git commit --amend
for COMMIT in $(git log --format='%H' $BAD..$HEAD | tac); do
git cherry-pick $COMMIT;
done
# Should print:
# src/cmd/vendor/golang.org/x/tools/go/analysis/vet | Bin 10207870 -> 0 bytes
git diff $HEAD --stat
# Then a force push to gerrit & github. |
# Then a force push to gerrit & github.
echo "If it all looks good, run:"
echo "$ git push https://go.googlesource.com/go HEAD:refs/heads/master"
echo "$ git push git@github.com:golang/go.git HEAD:refs/heads/master" |
|
Script looks fine to me at a glance, but why not use You'll probably need those pushes to be --force. |
|
Or |
|
I tried |
|
I'll try filter-branch again, because @dmitshur pointed out that the cherry-pick way is dropping Committer name/time. |
|
IIRC, |
|
Okay, that works. This isn't blazing fast, but acceptable: EDIT: we later learned, below, that we needed |
|
v2: #!/bin/bash
set -e
git checkout master
git branch -D rewrite_28899 || true
git change rewrite_28899
git fetch origin
git reset --hard origin/master
HEAD=$(git rev-parse HEAD)
# The commit with the bad blob.
BAD=50941e92414d5f51d1188285f593bfd8f6fbf515
git filter-branch --force --index-filter \
'git rm --cached --ignore-unmatch src/cmd/vendor/golang.org/x/tools/go/analysis/vet' \
--prune-empty -- $BAD..HEAD
# Should print:
# src/cmd/vendor/golang.org/x/tools/go/analysis/vet | Bin 10207870 -> 0 bytes
git diff $HEAD --stat
# Then a force push to gerrit & github.
echo "If it all looks good, run:"
echo "$ git push --force-with-lease https://go.googlesource.com/go HEAD:refs/heads/master"
echo "$ git push --force-with-lease git@github.com:golang/go.git HEAD:refs/heads/master" |
|
v3, to remove prune-empty for @dmitshur, and add some debug prints: set -e
git checkout master
git branch -D rewrite_28899 || true
git change rewrite_28899
git fetch origin
git reset --hard origin/master
OLD_HEAD=$(git rev-parse HEAD)
# The commit with the bad blob.
BAD=50941e92414d5f51d1188285f593bfd8f6fbf515
git filter-branch --force --index-filter \
'git rm --cached --ignore-unmatch src/cmd/vendor/golang.org/x/tools/go/analysis/vet' \
-- $BAD..HEAD
NEW_HEAD=$(git rev-parse HEAD)
echo "Moving from $OLD_HEAD to $NEW_HEAD ..."
# Should print:
# src/cmd/vendor/golang.org/x/tools/go/analysis/vet | Bin 10207870 -> 0 bytes
git diff $OLD_HEAD --stat
# Then a force push to gerrit & github.
echo "If it all looks good, run:"
echo "$ git push --force-with-lease https://go.googlesource.com/go HEAD:refs/heads/master"
echo "$ git push --force-with-lease git@github.com:golang/go.git HEAD:refs/heads/master" |
|
For posterity, the old history: |
|
v4, with an off-by-one fixed in the range that would've made v3 be totally useless: #!/bin/bash
set -e
git checkout master
git branch -D rewrite_28899 || true
git change rewrite_28899
git fetch origin
git reset --hard origin/master
OLD_HEAD=$(git rev-parse HEAD)
# The commit with the bad blob.
BAD=50941e92414d5f51d1188285f593bfd8f6fbf515
echo "Old history:"
git log --format=oneline $BAD^..HEAD
git filter-branch --force --index-filter \
'git rm --cached --ignore-unmatch src/cmd/vendor/golang.org/x/tools/go/analysis/vet' \
-- $BAD^..HEAD
NEW_HEAD=$(git rev-parse HEAD)
echo "Moving from $OLD_HEAD to $NEW_HEAD ..."
# Should print:
# src/cmd/vendor/golang.org/x/tools/go/analysis/vet | Bin 10207870 -> 0 bytes
git diff $OLD_HEAD --stat
# Then a force push to gerrit & github.
echo "If it all looks good, run:"
echo "$ git push --force-with-lease https://go.googlesource.com/go HEAD:refs/heads/master"
echo "$ git push --force-with-lease git@github.com:golang/go.git HEAD:refs/heads/master"Thanks, @dmitshur. |
|
And to confirm, the bad commit and its new version both have the same parent: |
|
Done. Gerrit didn't like |
dmitshur
added
NeedsFix
|
Thanks for getting this done! |
|
golang-dev announcement: https://groups.google.com/d/msg/golang-dev/8JmYrD3DWY0/oNVcA29FBgAJ. Closing since this is fixed. |
|
Thanks. Not sure if it is on the checklist: |
|
It seems https://build.golang.org/ somehow got confused by this. It still shows the old git hashes and is latest change which was tested. |
|
@tklauser Thanks for reporting. @bradfitz mentioned remembering that at least one service needs to be restarted after a force push. https://build.golang.org/ not showing the latest commits is definitely related to that. Re-opening to resolve that. Once resolved, we should also make note of this, in case this information is needed in the future. (I didn't see it mentioned in #24020, but perhaps it's because that issue dealt with force pushing to a subrepo rather than the main repo.) |
|
I restarted gitmirror, which cleaned up the build.golang.org datastore, but that's now a lot of rows to re-build-and-test. (I should really prioritize #28950) Off to bed. Hopefully it makes some good progress while I sleep. |
|
Thanks Brad. So for future reference, gitmirror is the service that needs to be restarted after the main repository history is rewritten. It looks like it has made good progress, the build dashboard is mostly filled in by now. Closing as fixed. |
bradfitz
changed the title
revision 50941e9 from https://go-review.googlesource.com/c/go/+/149604 included a 9.7MB compiled vet binary in src/cmd/vendor/golang.org/x/tools/go/analysis/
The text was updated successfully, but these errors were encountered: