{"action":"created","issue":{"url":"https://api.github.com/repos/golang/go/issues/15616","repository_url":"https://api.github.com/repos/golang/go","labels_url":"https://api.github.com/repos/golang/go/issues/15616/labels{/name}","comments_url":"https://api.github.com/repos/golang/go/issues/15616/comments","events_url":"https://api.github.com/repos/golang/go/issues/15616/events","html_url":"https://github.com/golang/go/issues/15616","id":153842295,"number":15616,"title":"x/review/git-codereview: Change-Id hook silently doesn't run anymore when codereview.cfg is not present","user":{"login":"yannk","id":40152,"avatar_url":"https://avatars.githubusercontent.com/u/40152?v=3","gravatar_id":"","url":"https://api.github.com/users/yannk","html_url":"https://github.com/yannk","followers_url":"https://api.github.com/users/yannk/followers","following_url":"https://api.github.com/users/yannk/following{/other_user}","gists_url":"https://api.github.com/users/yannk/gists{/gist_id}","starred_url":"https://api.github.com/users/yannk/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/yannk/subscriptions","organizations_url":"https://api.github.com/users/yannk/orgs","repos_url":"https://api.github.com/users/yannk/repos","events_url":"https://api.github.com/users/yannk/events{/privacy}","received_events_url":"https://api.github.com/users/yannk/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/golang/go/labels/NeedsInvestigation","name":"NeedsInvestigation","color":"ededed"}],"state":"open","locked":false,"assignee":{"login":"adg","id":8446613,"avatar_url":"https://avatars.githubusercontent.com/u/8446613?v=3","gravatar_id":"","url":"https://api.github.com/users/adg","html_url":"https://github.com/adg","followers_url":"https://api.github.com/users/adg/followers","following_url":"https://api.github.com/users/adg/following{/other_user}","gists_url":"https://api.github.com/users/adg/gists{/gist_id}","starred_url":"https://api.github.com/users/adg/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/adg/subscriptions","organizations_url":"https://api.github.com/users/adg/orgs","repos_url":"https://api.github.com/users/adg/repos","events_url":"https://api.github.com/users/adg/events{/privacy}","received_events_url":"https://api.github.com/users/adg/received_events","type":"User","site_admin":false},"milestone":{"url":"https://api.github.com/repos/golang/go/milestones/22","html_url":"https://github.com/golang/go/milestones/Unreleased","labels_url":"https://api.github.com/repos/golang/go/milestones/22/labels","id":1067491,"number":22,"title":"Unreleased","description":"Issues that do not affect released Go code and binaries.\r\n","creator":{"login":"rsc","id":104030,"avatar_url":"https://avatars.githubusercontent.com/u/104030?v=3","gravatar_id":"","url":"https://api.github.com/users/rsc","html_url":"https://github.com/rsc","followers_url":"https://api.github.com/users/rsc/followers","following_url":"https://api.github.com/users/rsc/following{/other_user}","gists_url":"https://api.github.com/users/rsc/gists{/gist_id}","starred_url":"https://api.github.com/users/rsc/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rsc/subscriptions","organizations_url":"https://api.github.com/users/rsc/orgs","repos_url":"https://api.github.com/users/rsc/repos","events_url":"https://api.github.com/users/rsc/events{/privacy}","received_events_url":"https://api.github.com/users/rsc/received_events","type":"User","site_admin":false},"open_issues":493,"closed_issues":521,"state":"open","created_at":"2015-04-14T18:48:22Z","updated_at":"2016-06-10T13:47:36Z","due_on":"2099-12-31T05:00:00Z","closed_at":null},"comments":5,"created_at":"2016-05-09T18:44:26Z","updated_at":"2016-06-10T14:36:26Z","closed_at":null,"body":"This is a regression introduced in\r\nhttps://go-review.googlesource.com/#/c/19560/\r\n\r\nBefore that change, if the hooks were installed for a private `foobar.com` Gerrit origin (i.e. not `googlesource.com`), they would all run. After that change, the hook code tries to figure out if Gerrit is in use, and when not, the Change-Id line is silently not added. Unfortunately, the heuristic to determine if Gerrit is in use (`haveGerrit()`) errs on the negative side. Consequently `git-codereview mail` is rejected by Gerrit because of the missing Change-Id. The failure is hard to debug because the hooks seem correctly installed, and it is especially annoying as git-codereview may have removed old working hooks in the past [1] to install its own.\r\n\r\nOne workaround is to create a `codereview.cfg` file in repo's root with a valid gerrit value. But it's not that great because for most installations it should be self-evident what is the Gerrit origin, it feels like a unwarranted burden on a lot of people (see: #15073) to introduce that file in many old and new gerrit repos.\r\n\r\nThe hard problem seems to be the runtime determination of what hooks should be running. Specifically, in the change above, Russ described how he wants the \"issue\" hook to run on non-Gerrit repo, but not the Change-Id one.\r\n\r\nI propose to remove runtime determination of what hooks should run in favor of explicit configuration. i.e. `git-codereview hooks` would evolve to the following:\r\n\r\n* Like current behavior, any git-codereview sub-command would install all the hooks, if and only if a valid Gerrit is detected.\r\n\r\n* Like current behavior, if `git-codereview hooks` is run manually, it would force hooks installation regardless of `haveGerrit()` value. But, unlike current behavior it would also enable all the hook functions: issuerepo, gofmt, change-id. (See below how hooks are enabled).\r\n\r\n* If arguments are passed to `git-codereview hooks`, where arguments could be any sub-set of 'issuerepo', 'gofmt' and 'change-id', then it would only enable those.\r\n\r\nThe git hook script should probably stay a simple exec as it is today, because it is hard to replace and extend in the future. On the other hand, git provides a very powerful `git config` set of commands that git-coderereview could run on behalf of the user to set and get the different expected behaviors. https://go-review.googlesource.com/#/c/19684/ proposes something similar for reading the configuration, although I suggest we take it further and lean on the dedicated git interfaces to do the writing and reading.\r\n\r\nI'm happy to work on a CL if that solution is deemed acceptable.\r\n\r\n1. https://github.com/golang/review/blob/77ae237af753cd4f4820e67cdf058aea410bccc7/git-codereview/hook.go#L31"},"comment":{"url":"https://api.github.com/repos/golang/go/issues/comments/225199447","html_url":"https://github.com/golang/go/issues/15616#issuecomment-225199447","issue_url":"https://api.github.com/repos/golang/go/issues/15616","id":225199447,"user":{"login":"josharian","id":67496,"avatar_url":"https://avatars.githubusercontent.com/u/67496?v=3","gravatar_id":"","url":"https://api.github.com/users/josharian","html_url":"https://github.com/josharian","followers_url":"https://api.github.com/users/josharian/followers","following_url":"https://api.github.com/users/josharian/following{/other_user}","gists_url":"https://api.github.com/users/josharian/gists{/gist_id}","starred_url":"https://api.github.com/users/josharian/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/josharian/subscriptions","organizations_url":"https://api.github.com/users/josharian/orgs","repos_url":"https://api.github.com/users/josharian/repos","events_url":"https://api.github.com/users/josharian/events{/privacy}","received_events_url":"https://api.github.com/users/josharian/received_events","type":"User","site_admin":false},"created_at":"2016-06-10T14:36:26Z","updated_at":"2016-06-10T14:36:26Z","body":"I just ran into this myself. Very frustrating, indeed. I feel like the proposal above is a lot of mechanism. Maybe it's worth it, but the fact that I get mildly confused and tired reading the details and thinking them through is not an auspicious sign.\r\n\r\nIn the meantime, I'm about to mail a change that should make it less painful to use codereview.cfg, which will hopefully help a little.\r\n"}}
{"actor":{"display_login":"josharian"}}