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. |
adg
added
the
NeedsInvestigation
|
@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.comGerrit 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 mailis 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.cfgfile 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 hookswould evolve to the following:git-codereview hooksis 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 configset 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: