cmd/go: vcs.* info can be inaccurate for Mercurial

[mharbison72]

What version of Go are you using (go version)?

$ go1.20.2 version
go version go1.20.2 windows/amd64

Does this issue reproduce with the latest release?

I didn't try, but looking at the current code, yes.

What operating system and processor architecture are you using (go env)?

go env Output

$ go1.20.2 env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\Matt\AppData\Local\go-build
set GOENV=C:\Users\Matt\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\Matt\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\Matt\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Users\Matt\sdk\go1.20.2
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Users\Matt\sdk\go1.20.2\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.20.2
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=NUL
set GOWORK=
set CGO_CFLAGS=-O2 -g
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-O2 -g
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=C:\Users\Matt\AppData\Local\Temp\go-build3808080945=/tmp/go-build -gno-record-gcc-switches

What did you do?

I ran go1.20.2.exe version -m on my executable.

What did you expect to see?

        build   vcs=hg
        build   vcs.revision=76cef81bcff371c88d277f17c712ecf22b8c83e7
        build   vcs.time=2023-10-12T23:09:25Z
        build   vcs.modified=true

What did you see instead?

        build   vcs=hg
        build   vcs.revision=82be5bac456b8fbde0268380e336fe2fbd1294e7
        build   vcs.time=2023-09-28T04:18:18Z
        build   vcs.modified=true

It looks like 76cef81 introduced a regression when it switched from identify -i (which produces information about the current checkout) to log -l1 (which prints info about whatever the topmost commit is, and may be a totally different branch). [1] From the commit comment there, it sounds like this particular change was to get the full commit hash too. The fix here is to simply add a -r. argument to the log command to look at the current commit, and that will fix both vcs.time and vcs.revision.

Typically, the topmost commit is whatever the developer is working on, but it won't be if updating to an old commit, or pulling changes from the server. While these might be edge cases, creating a tag creates a new commit, and CI typically updates to the commit that was tagged. That means this info will be wrong for any tagged builds (which is how I noticed this).

There are two other correctness issues here:

• The status command should have a -S argument, so that it recurses into any subrepos when checking for modified files.[2]

• HGPLAIN=1 should be set in the environment when executing any commands, to prevent unexpected command output changes.[3] For example, I have ui.tweakdefaults=True in my configuration, so at the end of hg status listing any file changes, it can print info about the current state of the repo, like this:

# The repository is in an unfinished *bisect* state.

# To mark the changeset good:    hg bisect --good
# To mark the changeset bad:     hg bisect --bad
# To abort:                      hg bisect --reset

Given the current code of checking for any status output, that would incorrectly flag the build as modified.

[1] 76cef81#diff-50574a97a14d32e7cc56ad1ba6de8c554e491c48a3718e599cfc8cc0ab2858e4R166
[2] https://github.com/golang/go/blame/cc13161a0d62fc1deab019996c17a7da1ff6a7f1/src/cmd/go/internal/vcs/vcs.go#L211
[3] https://repo.mercurial-scm.org/hg/file/6.5.2/mercurial/helptext/scripting.txt#l45

[bcmills]

@mharbison72, given that you seem to understand the problems here, would you like to send a fix?

(I'm not sure that I have a solid enough understanding of the Mercurial command line to do it properly — especially to write a regression test that would correctly reproduce these bugs.)

[mharbison72]

@mharbison72, given that you seem to understand the problems here, would you like to send a fix?

(I'm not sure that I have a solid enough understanding of the Mercurial command line to do it properly — especially to write a regression test that would correctly reproduce these bugs.)

I'd be happy too, but I haven't contributed here before. I'm reading the contributing doc and need to get setup first.

I'm not sure where you are in the release cycle- is there a particular branch I should target to get these fixes backported? Also, where are the tests I should add to? I see src/cmd/go/internal/vcs/vcs_test.go and src/cmd/go/testdata/vcstest/hg which I assume is what you mean, but I'm not sure how to run them. Is the typical workflow to just push to a PR, and let tests run on GH?

[bcmills]

I'm not sure where you are in the release cycle- is there a particular branch I should target to get these fixes backported?

All fixes should land on the master branch first; then we can decide about backports.

Also, where are the tests I should add to?

src/cmd/go/testdata/script — see in particular src/cmd/go/testdata/script/version_buildvcs_hg.txt.

They are run as subtests of cmd/go.TestScript, so you can invoke them individually like:

$ go test cmd/go -run=TestScript/version_buildvcs_hg

[mharbison72]

Thanks for the pointers @bcmills. I'm probably missing something obvious, but the tests seem to sail right over the changes I'm making to version_buildvcs_hg.txt when I run the test command in verbose mode, with caching disabled:

$ time GOFLAGS="-count=1" go test -v cmd/go -run=TestScript/version_buildvcs_hg

Also, if I understand how the scripts work, they aren't currently testing the revision or time value at all, other than that it's present:

 stdout '^\tbuild\tvcs.revision='
 stdout '^\tbuild\tvcs.time='
 stdout '^\tbuild\tvcs.modified=false$'

I can see why that was done- the current time and username are part of the commit hash, so they aren't fixed from run to run. They can be set to specific values in the test environment like in the Mercurial test setup if you want to go that far (and go.exe doesn't clear them somewhere), but I don't want to leave you with a bunch of complexity. Without it though, there's really no way to add regression tests for this.

[mharbison72]

I figured out my problem- I was running the global go instead of the local one to run the tests.

LMK if the more elaborate revision matching in the tests is what you had in mind, or if you meant something else.

[gopherbot]

Change https://go.dev/cl/535377 mentions this issue: cmd/go: fix the accuracy of Mercurial vcs.* stamped data