search

golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
124.37k stars 17.71k forks source link

x/review/git-codereview: Change-Id hook silently doesn't run anymore when codereview.cfg is not present #15616

Open yannk opened 8 years ago

yannk commented 8 years ago

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

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:

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.

  1. https://github.com/golang/review/blob/77ae237af753cd4f4820e67cdf058aea410bccc7/git-codereview/hook.go#L31
bradfitz commented 8 years ago

/cc @josharian @minux @robpike

bradfitz commented 8 years ago

And @adg? Not sure who maintains codereview the most these days.

josharian commented 8 years ago

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.

yannk commented 8 years ago

@josharian Are you referring to codereview.cfg in the first paragraph?

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.

josharian commented 8 years ago

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.

josharian commented 8 years ago

CL 24001

gopherbot commented 8 years ago

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

quite commented 5 years ago

Looks like this has been merged (cherry-picked).