x/review/git-codereview: Documentation and tool behavior don't match for 'git codereview change'

[dwbuiten]

Please answer these questions before submitting your issue. Thanks!

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

go version go1.10.3 linux/amd64

What version of git-codereview are you using?

golang/review@3faf270

Does this issue reproduce with the latest release?

Yes.

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

GOBIN=""
GOCACHE="/home/vimeo/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/vimeo/godep"
GORACE=""
GOROOT="/home/vimeo/go"
GOTMPDIR=""
GOTOOLDIR="/home/vimeo/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build902163312=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Cloned the x/image repo as per its README:

$ git clone https://go.googlesource.com/image $GOPATH/src/golang.org/x/image
$ cd $GOPATH/src/golang.org/x/image

Signed up for and setup cookies for Go's Gerrit system (I had already set it up since previosly contributing, technically.)

Installed git-codereview as per the documentation:

$ go get -u golang.org/x/review/git-codereview

Created a new branch for work, and did my changes, as per the documentation:

$ git checkout -b newbranch
$ vim whatever.go
$ git add whatever.go
$ git codereview change

What did you expect to see?

I expected git codereview change to add a Change-Id: line to the commit messages.

What did you see instead?

It did not add a Change-Id: to the commit message.

Workaround / extra information which could be important

If I do not follow the documentation, and instead of using git checkout -b newbranch, I use git codereview change newbranch, it does add the Change-Id: line as required. But this does not match what the official documentation on golang.org says to do. It is also worth noting this occurs on the x/imagerepo; I haven't tried the officialgo` repo.

[ALTree]

I've always used git codereview change <branchname> when working on a new patch and AFAIK that was the suggested way... not sure how that git checkout -b newbranch suggestion made its way in the documentation, but I'm fairly sure it wasn't there the last year.

I think it was added in the recent rewrite of the contribution guide. It may be useful to investigate why it was changed.

[dwbuiten]

Looks like it was added in the huge rewrite commit: a3d8326

It doesn't seem any reason was included in the commit messages or in the review comments, so I'm not sure it explains much.

[ianlancetaylor]

CC @rasky

[rasky]

Works for me. You don't see the Change-Id line when $EDITOR first opens, but it's visible immediately after (with git show). Can somebody else try?

[dwbuiten]

@rasky It isn't there for git show after, either, though, for me, though. Did you test on a non-golang/go repository like x/image?

[ALTree]

I think the suggestion should be reverted to use codereview anyway. The rest of the workflow in the guide uses it and I don't think there's really any reason to avoid it for the 1st step. If you add the git aliases (which I believe most regular contributors will end up doing), git change <name> is also faster to type than git checkout -b <name>.

[rasky]

@dwbuiten no: I did on golang/go. So it works for me and it doesn't for you. Can anybody else please test so that we understand what it is going on?

@ALTree: I disagree that it should be reverted. The contribute guide isn't meant to be a comprehensive reference for experienced or regular contributors; it is a tutorial for newcomers. The less concepts we force them to learn to send the first patch, the better. Creating a branch for a patch is part of a standard workflow that most (all) git users will know; asking them to use git codereview change is not, it affects the working copy in ways that are not even 100% clear to me (see the fact that I don't understand why this issue exists for @dwbuiten), and I don't want to explain the users in that very initial moment that git codereview change <branchname> is similar to git checkout -b <branchname> but not-really-similar.

[rasky]

I think I now understand what it is going on: in each repo, the hooks are installed as side-effect on the first codereview command. So if you run change, you get the hooks installed as side effect before the commit is made. After you install the hooks, checkout -b works as well for the described flow. That's unfortunate as it looks like it's not possible to teach mail as first command. I'll think of this.

[dwbuiten]

@rasky That explains a lot, like why some others couldn't reproduce. Thanks!

I guess there's always the ugly route of something like git codereview init, but that's pretty... meh-ish.