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.

Sign up for GitHub

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: incorrect handling of commit --verbose #16376

Closed
mvdan opened this issue Jul 14, 2016 · 7 comments
Closed

x/review/git-codereview: incorrect handling of commit --verbose #16376

mvdan opened this issue Jul 14, 2016 · 7 comments
Labels
FrozenDueToAge
Milestone
Unplanned

Comments

@mvdan
Copy link
Member

mvdan commented Jul 14, 2016 •
edited

  1. What version of Go are you using (go version)?
go version devel +29ed5da Wed Jul 13 21:18:19 2016 +0000 linux/amd64
  1. What operating system and processor architecture are you using (go env)?
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/mvdan/go"
GORACE=""
GOROOT="/home/mvdan/tip"
GOTOOLDIR="/home/mvdan/tip/pkg/tool/linux_amd64"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build418900914=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
  1. What did you do?

Follow https://golang.org/doc/contribute.html to send my first CL.

  1. What did you expect to see?

commit-msg would work as expected, adding Change-Id and leaving the rest.

  1. What did you see instead?

First, the commit-msg hook got confused by this in my .gitconfig:

[commit]
       verbose = true

This is a new option (since 2.9) that spits out the diff at the bottom of the commit message file in every commit that git does. Useful to see what you're committing. The hook, though, included this diff in the final commit message instead of ignoring it. Note that the diff isn't commented out, but it's after the # Everything below will be removed. line.

Once I removed the option from my .gitconfig, that got fixed. But then the Change-Id would still not appear.

I had to follow the git codereview mail instructions, which did give me a hook that actually worked:

remote: ERROR: [8a50cc8] missing Change-Id in commit message footer
remote:
remote: Hint: To automatically insert Change-Id, install the hook:
remote: curl -Lo `git rev-parse --git-dir`/hooks/commit-msg https://gerrit-review.googlesource.com/tools/hooks/commit-msg ; chmod +x `git rev-parse --git-dir`/hooks/commit-msg
@josharian
Copy link
Contributor

I can partially reproduce--I get a Change-Id, but the diff still shows up in the commit message. Not sure what the right fix is. I guess we could teach the hook to look for the line:

# −−−−−−−−−−−−−−−−−−−−−−−− >8 −−−−−−−−−−−−−−−−−−−−−−−−

and cut everything after it.

Maybe there's a better way, but at first glance, I don't really see how hooks are intended to interact with commit --verbose other than by manually reproducing git's behavior.

@josharian
Copy link
Contributor

I'd happily review a CL fixing this if you want to send one.

@josharian josharian added this to the Unplanned milestone Jul 14, 2016
@josharian josharian changed the title review: broken with Git 2.9 x/review/code-review: incorrect handling of commit --verbose Jul 14, 2016
@mvdan
Copy link
Member Author

mvdan commented Jul 14, 2016

I wonder why I'm not getting the Change-Id then. I guess it must be something else that I did wrong.

As for the verbose option, I'll investigate and post a CL if indeed we are supposed to work around this.

@mvdan
Copy link
Member Author

mvdan commented Jul 14, 2016

Just curious, what git version are you using? If someone could test the commit-msg hook on 2.9, it would help me narrow down the Change-Id issue.

@josharian
Copy link
Contributor

I'm using 2.9, but without the config flag. I tested using git commit --verbose, which should have the same effect. (That will be useful when writing tests.) I used a repo that had already the hooks installed, though, which might have been the relevant difference.

@mvdan
Copy link
Member Author

mvdan commented Jul 14, 2016

Removing the hooks and re-running git-codereview hooks it works now, so I have no idea.

As for --verbose, I haven't found any way to make git handle it. The man page seems to imply that other tools need to handle it:

-v, --verbose
    Show unified diff between the HEAD commit and what would be committed at the bottom of the commit message template to help the user describe the commit by reminding
    what changes the commit has. Note that this diff output doesn’t have its lines prefixed with #. This diff will not be a part of the commit message. See the
    commit.verbose configuration variable in git-config(1).

All I could find is other projects doing this line inspection that you proposed. I'll send a CL tomorrow, as I use this option frequently.

@gopherbot
Copy link

CL https://golang.org/cl/25342 mentions this issue.

@mikioh mikioh changed the title x/review/code-review: incorrect handling of commit --verbose x/review/git-codereview: incorrect handling of commit --verbose Aug 1, 2016
camlistorebot pushed a commit to perkeep/perkeep that referenced this issue Mar 6, 2017
@euank
devcam/hook: handle 'verbose' commits
23ef425 
See golang/go#16376, and the related
https://go-review.googlesource.com/#/c/25342/.

Prior to this change, `git commit -v` would result in a nonsensical
commit message.

Change-Id: Ib11de27488b01fccff07b9385f7fa988bc6fe165
@golang golang locked and limited conversation to collaborators Aug 1, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
3 participants
@josharian @mvdan @gopherbot