New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/vendor/golang.org/x/tools: vet binary checked in to repo, rewrite history #28899

Closed
fesh0r opened this Issue Nov 20, 2018 · 27 comments

Comments

Projects
None yet
8 participants
@fesh0r

fesh0r commented Nov 20, 2018

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/

@mark-rushakoff

This comment has been minimized.

Contributor

mark-rushakoff commented Nov 20, 2018

There was precedent in #24020 to force push x/tools to remove a committed binary. /cc @andybons @bradfitz.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 20, 2018

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 bradfitz added this to the Go1.12 milestone Nov 20, 2018

@dmitshur

This comment has been minimized.

Member

dmitshur commented Nov 20, 2018

I'm in favor of rewriting the history to drop the binary for the following reasons:

  • This is the main go repo, not a subrepo, so only Go developers have a copy. Had it been a subrepo, it would be much more costly to pull off since it'd affect all users of golang.org/x/{subrepo}/... packages.
  • We can communicate that this will be done via golang-dev mailing list, so people will know it has been done intentionally (and why).
  • We're in freeze and it's holiday season (in the US), so the activity is lower right now. Fewer CLs will need to be rebased/updated, etc.

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).

How long would this take to implement? I don't use git sync, but it's mainly because I actively want to find out whether there's been a force push. As long as it's communicated properly (to avoid confusion about whether it's intentional), I wouldn't mind updating my local repo after a force push. So I'm not sure whether there's much value in doing this. Edit: Not to mention, this would only help those people who install the newer version of git-codereview before encountering the force push.

This should serve as an increase on priority of #10658.

@mvdan

This comment has been minimized.

Member

mvdan commented Nov 21, 2018

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 :)

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 21, 2018

@ianlancetaylor, you cool with a git history rewrite?

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 21, 2018

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.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 21, 2018

Here's the plan, tested:

$ cat ./rewrite-28899.sh
#!/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.                                                                                                                                                                     
@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 21, 2018

# 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"
@josharian

This comment has been minimized.

Contributor

josharian commented Nov 21, 2018

Script looks fine to me at a glance, but why not use git filter-branch, which is made for this kind of thing?

You'll probably need those pushes to be --force.

@dmitshur

This comment has been minimized.

Member

dmitshur commented Nov 21, 2018

Or --force-with-lease.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 21, 2018

I tried filter-branch but it was super slow and the more I read about how to make it fast the more gross it all got, so I reached out to familiar (and fast) tools instead.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 21, 2018

I'll try filter-branch again, because @dmitshur pointed out that the cherry-pick way is dropping Committer name/time.

@josharian

This comment has been minimized.

Contributor

josharian commented Nov 21, 2018

IIRC, filter-branch accepts a commit range, which should suffice to make it not-slow, since that commit range will be small. (Apologies if you've already tried that.)

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 21, 2018

Okay, that works. This isn't blazing fast, but acceptable:

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

EDIT: we later learned, below, that we needed $BAD^..HEAD instead.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 21, 2018

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"
@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 21, 2018

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"
@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 21, 2018

For posterity, the old history:

c9b4dca58f02f23640481086efeab14dd82f3edc cmd/compile: fix TestFormats by using valid formats
2a7f904e3cca87e27004b9f7fb48f82d77a10bee runtime: debug code to catch bad gcWork.puts
4d5791ca4f74e755b80e93dadc32269c7ecce15c runtime: improve "P has cached GC work" debug info
c64848595be16db1679932b134e682ad0b1f6b1e net/http: fix spelling mistake in a comment
ddccd95d07178ab01ee7d281b3544997d9819260 cmd/compile: add control flow graphs to ssa.html
ae65615fd8784919f11e744b3a26d9dfa844c222 runtime: add arg maps for sync/atomic functions in ARM64 race mode
a0edb0c6fbb891af1bb702df3093ab98ea3cf739 misc/wasm: add stub for fs.read on browsers
339e5ff189a637647bad0e4075a39b2ceedbd815 go/doc: disable playground for examples that use syscall/js  
7c71aa2df86e186c00a760a5f3cc2624e53a6b8a os: return PathError on RemoveAll with trailing dots
9f832f1a4a1555f53d291c7db0be96983dea011a os: permit RemoveAll with paths that end in ".."
0d29261d6b0f6caf236ec44692d3d1dc1492e0ab mime: remove allocation introduced in recent fix
b44cd15464a87be57446a7573f76846088826606 syscall: add dummy SIGTERM constant to js/wasm
8679b913a0009fcf2a048df0d21752425777fcee misc/wasm: use temporary directory provided by Node.js
977f431f5b6d751f062f03d45e992f2dad8eccba cmd/link, runtime: add initial cgo support for ppc64
d6f2fd989f3a4c46f728e46e6a0da01a85611a68 mime: correctly detect non-ASCII characters in FormatMediaType
b34ea7e5789e68939b8e352f428bee9b795d9de4 syscall/js: document ValueOf() panic
bbd1771f235a80b931bdf9ddba35bd437dd6e7d7 regexp: use backquotes for all regular expression examples   
0f02664666bb30e7cb4041dd0f8d7f739ffd54af doc/go1.12: announce deprecation of support for FreeBSD 10.x
90777a34cf250d2051dc6e4595625933249ca211 encoding/pem: test getLine does not include trailing whitespace
e58475b8fa0f8aedcd0bc84d48f1ddc2e8be5a0f regexp: add matching and finding examples
e337904b804358c05b3b363ce3adc195143363c4 cmd/link: directly get max pc value in findfunctab
47d5a4433513c3870a9994b50ed4fa2af628c742 cmd/go: parse dot-separated identifiers in build metadata
e003c6f971d74d67e743f6de1c5cc3af7f2eeb95 cmd/vendor: eliminate vet-lite
af21ffd86a48c4c8951971a2fb0b28dee1931197 cmd/go: improve go vet documentation
24ba63116468509094a84e7a1093054f45bdae0c cmd/vendor: update to golang.org/x/tools@139d099f
0b9004f1c08ae8c07454f5fb33b9c3848e07c932 cmd/compile: bulk rename
310f2e2465b1308f1f3beaac817e9b3914b7dda0 cmd/go: packages that use SWIG depend on "unsafe"
fe562cebf110b5ed3afdc1578d9cc2a5fc9ec296 os: make RemoveAll("") fail silently on unix
054640b54df68789d9df0e50575d21d9dbffe99f crypto/hmac: rename CheckHMAC to ValidHMAC in package docs
96eeaa18e4fe30bf779d535d8c8930ca8ad73e45 runtime: don't use thread local storage before it is set up on iOS
b65f5c8806cd8c93f35a16ff80747de4c015224b cmd/vet: remove pkgfact analyzer, left in by mistake
6797b32b7c48ccb5823819d92a8713e4d6823e7e cmd/cgo: recognized untyped Go constants as untyped constants
3faab79b55836f507987b46e799b838d827c52c2 cmd/asm: rename -symabis to -gensymabis
6f5be373e79e1468a2c8976fa79108f982c7c25c cmd/vet/all: remove skip when x/tools isn't in $GOPATH
2900943d0ada739197241a24134a2b62027c642a cmd/vet: basic tests of go vet -json -c=N flags
a54b7b2d99a4cceedb16b1c23319d5a189e689f0 cmd/vendor: update to golang.org/x/tools@8e5aba0a
6157dda0d8f7ca68849c62f110a0f1fa6f8b1dd7 cmd/go: accept @hash/branch in mod download
ed3a3570d24e1c45ba7ae9a35cd2adfc440f71cf cmd/go: vet: provide package ID to the vet tool
465c3114c995e023a73b0e5dc21007cf17216a21 build: clear GO111MODULE during make.bash etc
37572843f87d025ffb910d71aa3c179ed311e2da cmd/vet: reenable cgo test
478af803493dce74aee89d4cf63b4db48fd5fad1 doc/go_spec: tweak wording to avoid 'explicit assignment' misreading
b19adfd0ee52de3d6ad1e578944bfe075ceda34c cmd/go: fix experiment isolation in cache key
53ed92ddc4496c50f7888f3758d3f13c07448007 cmd/go: fix detection of unexpected files in downloaded zips
a1025ba4aab08a40efe9805829e1c626e73ef102 cmd/compile: provide updating mechanism for format test
a33cebfb8d0675cfedfb0de6e30f24c1938f1ee0 database/sql: add examples for opening and testing a DB pool
79257c425f84194945e6a9ddbb75d2664d4b4cdf testing: add example to package doc
6557af70107f1b56f046a557f170bf0fa4827815 net/http: fix typo in the SameSite docs
c6fe2083417c2043c9ad62ed629770735d50cb2a cmd/vet: fix two failing test cases
722ff6bf5f5b485fd15d2367741b0b81d4701c88 reflect: add comment for String method of Kind struct
657f6d6293b5ac4e43146259cc904413ffa6ad24 misc/cgo/testsanitizers: gofmt
34f1f5df5c204878fb4df16a7239510b29d5e1df cmd/cgo: fix comment grammar
14608976dbaff72b7ff6b41b66623c959eaded28 cmd/go: correctly suggest tidy instead of nonexistent fix for -fix
700575be813a6a34eb99af886c830097e5a66764 cmd/vet: switch to x/tools/go/analysis implementation
8fa789b68912d045de0e40a9b53951659d976a53 runtime: eliminate mheap.busy* lists
6bd85f70438258692a7f44f3e9b5edebe28e2b8c runtime: implement efficient page reclaimer
6d19461db39ed38aa07a80f33d64e0d26bb7b5cb runtime: mark span when marking any object on the span
c18600416d2310ad3f83657e4e5f2c448b66bba5 runtime: record in-use spans in a page-indexed bitmap
cd3b8785f1a8a571e2cb4bfe609c0c970c768f0b runtime: track all heap arenas in a slice
35770abfb0c1bf8f8ae4b2d2799dcb553c04f5ec cmd/vet/all: use x/tools/go/analysis/cmd/vet not cmd/vet
564a6526d1f2f1455a6d8e28a350e8fd33427098 cmd/vendor: update to golang.org/x/tools@f62bfb54
7a6ccece877232fc58872c85fdf0718629b83d91 x/net/route: use libc calls on Darwin
c7a4165ae7970b99f942aa5c770d06f89e9c5e05 cmd/link: fix isStmt DWARF info
0a40d45d8e76e2e1c7343d223d1e9e658e3aa9cb internal/cpu: move GODEBUGCPU options into GODEBUG
50941e92414d5f51d1188285f593bfd8f6fbf515 cmd/vendor/golang.org/x/tools: update to 7d6b83ca
@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 21, 2018

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.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 21, 2018

And to confirm, the bad commit and its new version both have the same parent:

bradfitz@gdev:~/go$ git rev-parse 50941e92414d5f51d1188285f593bfd8f6fbf515^
bfd9b94069e74b0c6516a045cbb83bf1024a1269
bradfitz@gdev:~/go$ git rev-parse 89a791749389f196d15ca873ae50b81a30780287^
bfd9b94069e74b0c6516a045cbb83bf1024a1269
@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 21, 2018

Done.

Gerrit didn't like --force-with-lease so I used --force and also -o nokeycheck, which was required.

To https://go.googlesource.com/go
 + c9b4dca58f...8c5c2b71f4 HEAD -> master (forced update)
...
To github.com:golang/go.git
 + c9b4dca58f...8c5c2b71f4 HEAD -> master (forced update)

@dmitshur dmitshur added NeedsFix and removed NeedsDecision labels Nov 21, 2018

@mvdan

This comment has been minimized.

Member

mvdan commented Nov 21, 2018

Thanks for getting this done!

@dmitshur

This comment has been minimized.

Member

dmitshur commented Nov 21, 2018

golang-dev announcement: https://groups.google.com/d/msg/golang-dev/8JmYrD3DWY0/oNVcA29FBgAJ.

Closing since this is fixed.

@martisch

This comment has been minimized.

Member

martisch commented Nov 22, 2018

Thanks. Not sure if it is on the checklist:
Are Go developer memories already rewritten to simplify matters going forward?
Context: #18548 (comment)

@tklauser

This comment has been minimized.

Member

tklauser commented Nov 22, 2018

It seems https://build.golang.org/ somehow got confused by this. It still shows the old git hashes and

c9b4dca58f02f23640481086efeab14dd82f3edc cmd/compile: fix TestFormats by using valid formats

is latest change which was tested.

@dmitshur dmitshur reopened this Nov 22, 2018

@dmitshur

This comment has been minimized.

Member

dmitshur commented Nov 22, 2018

@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.)

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 26, 2018

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.

@dmitshur

This comment has been minimized.

Member

dmitshur commented Nov 26, 2018

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.

@dmitshur dmitshur closed this Nov 26, 2018

@bradfitz bradfitz changed the title from cmd/vendor/golang.org/x/tools: vet binary checked in to repo to cmd/vendor/golang.org/x/tools: vet binary checked in to repo, rewrite history Nov 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment