-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
x/review/git-codereview: Change-Id hook silently doesn't run anymore when codereview.cfg is not present #15616
Comments
/cc @josharian @minux @robpike |
And @adg? Not sure who maintains codereview the most these days. |
I originally wanted to use git config for the config files. There were two issues. (1) Russ wanted key: value instead. (2) There was no obvious way to check in repo-level git config in a way that git config would automatically respect, and there were objections to the filename I chose. Rather than squabble/bikeshed, I acceded, and I'm not sure that reopening that particular discussion has much value. As for having hooks accept an argument to install just a subset of hooks that seems ok. What happens if hook A is installed and hook B is requested for installation? Does A stay or go? I'm not really the maintainer here, though, so this is just me kibitzing. |
@josharian Are you referring to I'd think that the hooks command would set the config values to what's given in argument, replacing whatever was set in the past. |
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. In 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. |
CL 24001 |
CL https://golang.org/cl/24001 mentions this issue. |
Loading codereview.cfg from origin/master effictively means that users have to ask permission to use git-codereview. With this change, users can opt to add the file, .gitignore it, and forget about it. Looking in origin/master also makes it hard to try out git-codereview on a project without having to commit to it. Instead of requiring codereview.cfg be checked in anywhere, just look on the filesystem for it. I can't figure out why I ever thought doing otherwise was a good idea. There are some other related fixes floating around, which might also be good to put in, but this is seems like a good stop-gap, since it is a minimal change; codereview.cfg continues to work for anyone currently using it. Related: golang/go#15616 Related: golang/go#15073 Change-Id: I1e377819f8eb8c8fecf9f022459551a3e8b93d48 Reviewed-on: https://go-review.googlesource.com/24001 Reviewed-by: Andrew Gerrand <adg@golang.org>
Looks like this has been merged (cherry-picked). |
This is a regression introduced in
https://go-review.googlesource.com/#/c/19560/
Before that change, if the hooks were installed for a private
foobar.com
Gerrit origin (i.e. notgooglesource.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. Consequentlygit-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.One 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.The 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.
I 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:git-codereview hooks
is run manually, it would force hooks installation regardless ofhaveGerrit()
value. But, unlike current behavior it would also enable all the hook functions: issuerepo, gofmt, change-id. (See below how hooks are enabled).git-codereview hooks
, where arguments could be any sub-set of 'issuerepo', 'gofmt' and 'change-id', then it would only enable those.The 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.I'm happy to work on a CL if that solution is deemed acceptable.
The text was updated successfully, but these errors were encountered: