Skip to content

[rush] rush git-hooks edge case #3816

Closed
#4132
#3873
Closed
[rush] rush git-hooks edge case#3816
#4132
#3873
Labels
bugSomething isn't working as intendedeffort: easyProbably a quick fix. Want to contribute? :-)help wantedIf you're looking to contribute, this issue is a good place to start!

Description

@chengcyber
Cheng Liu (chengcyber)
opened on Dec 6, 2022

Summary

Background: our team is migrating husky to rush git-hooks, and we found an edge case with current rush git hooks implementation. So, this PR reports the edge case and proposes a alternative git hooks implementation.

Repro steps

Assuming the repo maintainer wants to add a "rush prettier" to "pre-commit" git hooks. So the maintainer follows the official doc:

  1. Add a rush prettier custom command
  2. Use rush prettier in common/git-hooks/pre-commit.

Ok. Push and merge it to the main branch. Everything seems fine now.

Actually, users may run into an issue that the following issue throws:

rush: error: argument "<command>": Invalid choice: prettier (choose from ....

Details

Let's say we got the following git branches with their commit history:

  1. branch main: A -> B(rush prettier + pre-commit)
  2. branch feat-1: B -> C (checkout from main when the latest commit is B)
  3. branch feat-2: A -> D (checkout from main when the latest commit is A)

And B is the commit for rush prettier in pre-commit.

Now. Users checkout to feat-1, rush update to sync the git hooks to .git folder and correctly run pre-commit with rush prettier
And then, checkout to feat-2, modify a readme file, and run git commit -m 'xxx', which triggers pre-commit hooks. BOOM, there is no rush prettier defined in branch feat-2.

Possible solution

Instead of syncing files from common/git-hooks to .git/hooks, how about setting core by running git config core.hooksPath common/git-hooks

Similar logic in Husky at https://github.com/typicode/husky/blob/9efb720c165ffcfec774b4584aa61f59dae774c7/src/index.ts#L49

Standard questions

Please answer these questions to help us investigate your issue more quickly:

Question Answer
@microsoft/rush globally installed version? 5.86.0
rushVersion from rush.json? 5.86.0
useWorkspaces from rush.json? Yes
Operating system? Mac
Would you consider contributing a PR? Yes
Node.js version (node -v)? 14

Activity

  1. Cheng Liu (chengcyber)
    changed the title [-][rush] [/-] [+][rush] rush git-hooks edge case[/+] on Dec 6, 2022

  2. stekycz commented on Dec 16, 2022

    @stekycz
    Martin Štekl (stekycz)
    on Dec 16, 2022
    Contributor

    We have the same case in our repo. We have solved it by using script like the following to refer to actual implementation. This file lives in common/git-hooks/pre-commit.

    #!/bin/bash

SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
# Assume the script is installed in .git/hooks
SCRIPT_IMPLEMENTATION_PATH="$SCRIPT_DIR/../../common/scripts/pre-commit-implementation.sh"

if [[ -f "$SCRIPT_IMPLEMENTATION_PATH" ]]; then
  exec "$SCRIPT_IMPLEMENTATION_PATH"
else
  echo "The pre-commit Git hook does not exist in your version of the repo. Either remove '.git/hooks/pre-commit' file in case this feature is not used anymore in main branch. Otherwise, updating your branch to current mainstream will solve the issue." >&2
fi

    I am sure it can be improved and generalized. However, I would expect this layer to be handled by Rush.js instead of making all Rush.js user do it on their own.

  3. Faithfinder commented on Dec 19, 2022

    @Faithfinder
    Dmitrii Kartashev (Faithfinder)
    on Dec 19, 2022
    Contributor

    Relevant: #3004

    Personally, I don't like the idea of tools configuring each other. Rush configuring git feels wrong.

  4. iclanton commented on Dec 21, 2022

    @iclanton
    Ian Clanton-Thuon (iclanton)
    on Dec 21, 2022
    Member

    Martin Štekl (@stekycz) - That seems like a good approach. It feels safer than overriding core.hooksPath because that would blow away hooks not handled by Rush.

    Would you or Cheng Liu (@chengcyber) be interested in putting that together? It probably wouldn't be too difficult.

  5. Ian Clanton-Thuon (iclanton)
    added
    bugSomething isn't working as intended
    help wantedIf you're looking to contribute, this issue is a good place to start!
    effort: easyProbably a quick fix. Want to contribute? :-)
    on Dec 21, 2022

  6. stekycz commented on Jan 5, 2023

    @stekycz
    Martin Štekl (stekycz)
    on Jan 5, 2023
    Contributor

    Ian Clanton-Thuon (@iclanton) I have just created the PR

  7. chengcyber commented on Jan 11, 2023

    @chengcyber
    Cheng Liu (chengcyber)
    on Jan 11, 2023
    ContributorAuthor

    I think the solution from Martin Štekl (@stekycz) covers the case when a new git hook file introduced in Monorepo. but there are cases that the repo maintainer introduces a new rush command in the existing git hook(such as pre-commit).

    Like the edge case i described in the issue content, the error actually is rush: error: argument "<command>": Invalid choice: prettier (choose from ..... User syncs .git/hooks/pre-commit from the master branch, and later he/she checkout to the branch where rush prettier is not introduced. In this branch, running git commit throws the error.

  8. stekycz commented on Jan 11, 2023

    @stekycz
    Martin Štekl (stekycz)
    on Jan 11, 2023
    Contributor

    Cheng Liu (@chengcyber) I do not think that is a problem. The script installed to yours .git/hooks/pre-commit will refer the script in your repo version. Therefore, when you change a branch in which the prettier is missing, it should be missing in the hook script as well.

  9. chengcyber commented on Jan 12, 2023

    @chengcyber
    Cheng Liu (chengcyber)
    on Jan 12, 2023
    ContributorAuthor

    The script installed to yours .git/hooks/pre-commit will refer the script in your repo version

    That's not always true. because developer needs to run rush update or rush install to sync the script into .git/hooks/pre-commit.

    What if one checkout to the branch and modify readme file, so he/she doesn't need to run rush update to install deps.

  10. stekycz commented on Jan 12, 2023

    @stekycz
    Martin Štekl (stekycz)
    on Jan 12, 2023
    Contributor

    Cheng Liu (@chengcyber) You will not need to run rush install or rush update anymore to get updated Git hooks. The Git hook script installed in .git/hooks is the dummy one. So it remains the same among all revisions no matter what (possibly except update of the script template in Rush.js itself). It will execute the target script only. The target script will change accordingly to Git revision.

  11. chengcyber commented on Jan 12, 2023

    @chengcyber
    Cheng Liu (chengcyber)
    on Jan 12, 2023
    ContributorAuthor

    Cheng Liu (@chengcyber) You will not need to run rush install or rush update anymore to get updated Git hooks. The Git hook script installed in .git/hooks is the dummy one. So it remains the same among all revisions no matter what (possibly except update of the script template in Rush.js itself). It will execute the target script only. The target script will change accordingly to Git revision.

    Yes. I check your proposal again. and i misunderstood it. You solution cover my edge case 👍

  12. David Michon (dmichon-msft)
    closed this as completedin #3873on Jan 23, 2023
  13. Ian Clanton-Thuon (iclanton)
    added this to Bug Triageon Aug 15, 2023
  14. Ian Clanton-Thuon (iclanton)
    moved this to Closed in Bug Triageon Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working as intendedeffort: easyProbably a quick fix. Want to contribute? :-)help wantedIf you're looking to contribute, this issue is a good place to start!

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions