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.

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: hooks does not re-install hooks #16777

Closed
broady opened this issue Aug 17, 2016 · 9 comments
Closed

x/review/git-codereview: hooks does not re-install hooks #16777

broady opened this issue Aug 17, 2016 · 9 comments
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. Suggested Issues that may be good for new contributors looking for work to do.
Milestone

Comments

@broady
Copy link
Member

broady commented Aug 17, 2016

When calling git-codereview hooks, it shouldn't skip over hook files that already exist.

My GOROOT had the default Gerrit hooks instead of git-codereview's hooks.

installHook probably needs a flag to skip this block of code:

        // If hook file exists, assume it is okay.
        _, err := os.Stat(filename)

Or, we could use file size as a heuristic to re-install the hooks, since we're already statting the file.

@quentinmit quentinmit changed the title git-codereview: hooks does not re-install hooks x/review: hooks does not re-install hooks Sep 6, 2016
@quentinmit
Copy link
Contributor

I don't think overwriting the hooks is a good idea either; the user might have their own hooks that they're not expecting to be overwritten. Perhaps a "-f" to overwrite hooks, and a warning if -f is not supplied?

@quentinmit quentinmit added this to the Unreleased milestone Sep 6, 2016
@agnivade
Copy link
Contributor

@josharian for decision

@agnivade agnivade added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 24, 2019
@josharian
Copy link
Contributor

We shouldn't overwrite existing hooks. That much seems clear.

I'd rather not add more flags and I'd rather not have the tool delete anything; this is rarely used functionality, I suspect, which means the opportunity for latent bugs and data loss is higher.

Perhaps when there are existing hooks, we could print a message like:

There is already a hooks file at %v.
To install git-codereview hooks, delete that file and re-run 'git-codereview hooks'.

Thoughts?

@agnivade
Copy link
Contributor

SGTM.

@agnivade agnivade added help wanted NeedsFix The path to resolution is known, but the work has not been done. Suggested Issues that may be good for new contributors looking for work to do. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jun 29, 2019
@gopherbot
Copy link

Change https://golang.org/cl/184417 mentions this issue: git-codereview: log message when non-gerrit hooks were detected

@gopherbot
Copy link

Change https://golang.org/cl/377034 mentions this issue: review: update installHook

@BerndCzech
Copy link

Hi,
I was looking for my first issue to contribute.

It looks like even the last CL was not merged. How can I see what is left to do?

@dmitshur dmitshur changed the title x/review: hooks does not re-install hooks x/review/git-codereview: hooks does not re-install hooks Sep 6, 2023
@dmitshur
Copy link
Contributor

dmitshur commented Sep 6, 2023

@BerndCzech Thanks for looking to contribute to Go. There is indeed an existing CL here, and a next step before it can be submitted is for it to be reviewed. If you're interested, please see https://go.dev/doc/contribute#reviews and https://go.dev/doc/contribute#download in particular. It would help if you can test out the code change to make sure it works as expected, and leave comments if you find opportunities to improve it. I plan to also leave some review comments on CL 377034 soon. Thanks.

@BerndCzech
Copy link

ty. I could make it run on my machine - I was not able to add new Review comments but I hope I could add some value to yours.

If the former contributor does not respond anymore would I create a new CL or takeover his?

r5d added a commit to r5d/review that referenced this issue Sep 12, 2023
Display warning message when a hook is already installed and is
different from the one installed by git-codereview.

Improves upon CL 184417.

Fixes golang/go#16777

Change-Id: I7579a3e86572e8b74f92317973e7cc7094b3942d
r5d added a commit to r5d/review that referenced this issue Sep 13, 2023
Display warning message when a hook is already installed and is
different from the one installed by git-codereview.

Improves upon CL 184417.

Fixes golang/go#16777

Change-Id: I7579a3e86572e8b74f92317973e7cc7094b3942d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. Suggested Issues that may be good for new contributors looking for work to do.
Projects
None yet
Development

No branches or pull requests

7 participants