Skip to content
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

Jump to bottom

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

Open
dwbuiten opened this issue Jun 22, 2018 · 9 comments
Open

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

dwbuiten opened this issue Jun 22, 2018 · 9 comments
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@dwbuiten
Copy link
Contributor

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.
@gopherbot gopherbot added this to the Unreleased milestone Jun 22, 2018
@ALTree
Copy link
Member

ALTree commented Jun 22, 2018 •
edited

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.

@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 22, 2018
@dwbuiten
Copy link
Contributor Author

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
Copy link
Contributor

CC @rasky

@rasky
Copy link
Member

rasky commented Jun 22, 2018

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
Copy link
Contributor Author

@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
Copy link
Member

ALTree commented Jun 22, 2018

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
Copy link
Member

rasky commented Jun 23, 2018 •
edited

@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
Copy link
Member

rasky commented Jun 24, 2018

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
Copy link
Contributor Author

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

@FiloSottile FiloSottile changed the title x/codereview: Documentation and tool behavior don't match for 'git codereview change' x/review/git-codereview: Documentation and tool behavior don't match for 'git codereview change' Jul 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@rasky @dwbuiten @ianlancetaylor @ALTree @gopherbot