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

Show a hint for how to disable pre-push hook in the error message #18

Open
huan opened this issue Nov 8, 2021 · 7 comments
Open

Show a hint for how to disable pre-push hook in the error message #18

huan opened this issue Nov 8, 2021 · 7 comments

Comments

@huan
Copy link
Member

huan commented Nov 8, 2021

RxJS has a very nice error message to let users know how to skip the pre-push hook, so do us.

husky > commit-msg hook failed (add --no-verify to bypass)

$ git commit -m 'fixx(fromEvent): clean fromEvent.ts code to make sure its clean'
husky > pre-commit (node v16.3.0)
✔ Preparing...
✔ Running tasks...
✔ Applying modifications...
✔ Cleaning up... 
husky > commit-msg (node v16.3.0)
INVALID COMMIT MSG: "fixx" is not allowed type ! Valid types are: feat, fix, docs, style, refactor, perf, test, build, ci, chore, revert
fixx(fromEvent): clean fromEvent.ts code to make sure its clean
husky > commit-msg hook failed (add --no-verify to bypass)

CC @lucifer1004

@binsee
Copy link
Member

binsee commented Apr 8, 2022

I read the code of git, and the running process of git push hook is roughly as follows:

int cmd_push(int argc, const char **argv, const char *prefix){
  // ...
	rc = do_push(flags, push_options, remote);
  // ...
  return rc;
}


int do_push(int flags, const struct string_list *push_options, struct remote *remote) {
  // ...
  if (push_with_options(transport, push_refspec, flags))
    errs++;
  // ...
	return !!errs;
}


int push_with_options(){
  // ...
	err = transport_push(the_repository, transport, rs, flags, &reject_reasons);
  // ...
	if (err != 0) {
		fprintf(stderr, "%s", push_get_color(PUSH_COLOR_ERROR));
		error(_("failed to push some refs to '%s'"), anon_url);
		fprintf(stderr, "%s", push_get_color(PUSH_COLOR_RESET));
	}
  // ...
}


int transport_push(){
  // ...
  if (run_pre_push_hook(transport, remote_refs))
          return -1;
  // push  submodules
  // ...
}

int run_pre_push_hook() {
  // run hook
  return ret;
}

In push_with_options (),if return is err for call transport_push(), print error msg.
So, we want do not show error msg, must return 0 for call transport_push(), and must return 0 for call run_pre_push_hook(), and return 0 for run pre-push script file.

I think we can change git push command args, like run git push --dry-run {retome} {branch}, run git push {retome} {branch} in pre-push script file, and return 0 in pre-push script file.

pre-push
This hook is called by git-push[1] and can be used to prevent a push from taking place.

The design purpose of the pre-push hook is to choose whether to block the push, not to change the push content, so using the --dry-run parameter is more suitable for our scenario.

@huan
Copy link
Member Author

huan commented Apr 8, 2022

The git push hook sometimes will confuse our first-time contributors to the Wechaty community.

So I think when there's any push action has been blocked, we can show a message like the following:

The push has been blocked because XXX ...
If you think this should not be blocked, you can skip the push hook by setting the below environment variable when running git push:
   $ NO_HOOK=1 git push

Learn more from our issue: https://github.com/Chatie/git-scripts/issues/18

@binsee
Copy link
Member

binsee commented Apr 8, 2022

test > git checkout -b test2
Switched to a new branch 'test2'
test2 > git push --dry-run

> @chatie/[email protected] lint
> npm run lint:es && npm run lint:ts


> @chatie/[email protected] lint:es
> eslint bin/**/*.ts src/**/*.ts tests/**/*.ts


> @chatie/[email protected] lint:ts
> tsc --noEmit

v0.7.2
remote: 
remote: Create a pull request for 'test2' on GitHub by visiting:        
remote:      https://github.com/binsee/chatie-git-scripts/pull/new/test2        
remote: 
To github.com:binsee/chatie-git-scripts.git
 * [new branch]      test2 -> test2

____ _ _        ____            _
/ ___(_) |_     |  _ \ _   _ ___| |__
| |  _| | __|    | |_) | | | / __| '_ \
| |_| | | |_     |  __/| |_| \__ \ | | |
\____|_|\__|    |_|    \__,_|___/_| |_|

____                              _ _
/ ___| _   _  ___ ___ ___  ___  __| | |
\___ \| | | |/ __/ __/ _ \/ _ \/ _^ | |
___) | |_| | (_| (_|  __/  __/ (_| |_|
|____/ \__,_|\___\___\___|\___|\__,_(_)






 ### Npm version bumped and pushed by inner push inside hook pre-push ###"
 -- vvvvvv outer push will be canceled, don't worry, not bug :) vvvvvv --"



Failed to exec pre-push hook script
error: failed to push some refs to '[email protected]:binsee/chatie-git-scripts.git'

test2 > git push --dry-run
Everything up-to-date

Now git push will not be blocked, because in the pre-push hook, we append the refs parameter to the new git push, so the push should always succeed.

What bothers everyone is that when you push for the first time, because there is no remote refs information, it is blocked, so you need to use NO_HOOK=1 to create refs information. And now the mechanism has been changed, so the problem doesn't occur anymore.

The only problem is that the outer git push cannot really push. In the hook, the version will be bumped first, which will add a new commit id, and running git push in the child process will push the latest commit id. At this time, the outer git push where the hook is located is still the old commit id, and an error will be reported when pushing to the server.

So the hook must return non-zero to prevent the outer git push from actually pushing, but this will throw an error. This prompt cannot be skipped unless the code of the git project is modified.

We can use the --dry-run parameter externally to make the outer git push skip the actual push, and then the hook returns 0.

Also we need a unique return value, when this return value is found in hook/pre-push, instead of showing an error message, show Please ignore the next line of red warning from git.

@binsee
Copy link
Member

binsee commented Apr 12, 2022

We now have a better reading experience.

There is only one question left.

I hope git can add new features, when the hook exit code is a specific value, no error is displayed, and it will immediately use 0 as the exit code and end the run.

@huan
Copy link
Member Author

huan commented Apr 13, 2022

Awesome, the output message layout has been fixed after merging your PR.

Thank you very much for helping the community to improve the developing experiences!

@binsee
Copy link
Member

binsee commented Apr 14, 2022

Hope to release a new version to npm soon.

I plan to upgrade this dependency for multiple projects in the community to solve the push blocking problem.

@huan
Copy link
Member Author

huan commented Apr 14, 2022

I have just released the new version.

Sorry for I have forgotten this repo has no DevOps configured.

Please let me know if there's anything I missed.

Thanks!

@huan huan changed the title Show a hit for how to disable pre-push hook in the error message Show a hint for how to disable pre-push hook in the error message Apr 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants