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

Adjust pre-commit/pre-push hooks to be compatible with git-worktree #17706

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cecemei
Copy link
Contributor

@cecemei cecemei commented Feb 7, 2025

Description

Updates pre-commit/pre-push hooks to work with git-worktree.

git-worktree doesn't have .git folder. The .git in a git worktree directory is a file, e.g. this is the content of the hooks-config worktree in my local env:

gitdir: /Users/cecemei/codespace/oss-druid/.git/worktrees/hooks-config

The content in this gitdir is less compared with a regular git directory.

❯ ls -l /Users/cecemei/codespace/oss-druid/.git/worktrees/hooks-config
total 4560
-rw-r--r--@ 1 cecemei  staff      403 Feb  7 14:40 COMMIT_EDITMSG
-rw-r--r--@ 1 cecemei  staff       29 Feb  7 14:35 HEAD
-rw-r--r--@ 1 cecemei  staff       41 Feb  7 14:35 ORIG_HEAD
-rw-r--r--@ 1 cecemei  staff        6 Feb  7 14:35 commondir
drwxr-xr-x@ 3 cecemei  staff       96 Feb  7 14:35 fsmonitor--daemon
srwxr-xr-x@ 1 cecemei  staff        0 Feb  7 14:35 fsmonitor--daemon.ipc
-rw-r--r--@ 1 cecemei  staff       43 Feb  7 14:35 gitdir
-rw-r--r--@ 1 cecemei  staff  2313883 Feb  7 14:40 index
drwxr-xr-x@ 3 cecemei  staff       96 Feb  7 14:35 logs
❯ ls -l /Users/cecemei/codespace/oss-druid/.git
total 4728
-rw-r--r--@   1 cecemei  staff       99 Jan 24 16:25 COMMIT_EDITMSG
-rw-r--r--@   1 cecemei  staff    14333 Feb  7 13:56 FETCH_HEAD
-rw-r--r--@   1 cecemei  staff       23 Feb  7 13:56 HEAD
-rw-r--r--@   1 cecemei  staff       41 Feb  7 13:57 ORIG_HEAD
-rw-r--r--@   1 cecemei  staff     1623 Feb  7 14:36 config
-rw-r--r--@   1 cecemei  staff       73 Dec 11 11:28 description
drwxr-xr-x@   3 cecemei  staff       96 Jan 26 19:45 fsmonitor--daemon
srwxr-xr-x@   1 cecemei  staff        0 Feb  7 13:56 fsmonitor--daemon.ipc
drwxr-xr-x@   7 cecemei  staff      224 Feb  7 14:40 hooks
-rw-r--r--@   1 cecemei  staff  2313883 Feb  7 14:20 index
drwxr-xr-x@   4 cecemei  staff      128 Feb  6 14:30 info
drwxr-xr-x@   4 cecemei  staff      128 Dec 11 11:34 logs
drwxr-xr-x@ 260 cecemei  staff     8320 Feb  7 14:40 objects
-rw-r--r--@   1 cecemei  staff    62249 Feb  7 13:57 packed-refs
drwxr-xr-x@   6 cecemei  staff      192 Jan 26 20:51 refs
-rw-r--r--@   1 cecemei  staff      174 Jan 23 08:21 sourcetreeconfig
drwxr-xr-x@  13 cecemei  staff      416 Feb  7 14:35 worktrees

The git layout in a worktree doesn't have refs, objects, hooks. The hooks would still be triggered when working in a git-worktree, since it looks up the GIT_COMMON_DIR, which is /Users/cecemei/codespace/oss-druid/.git.

Note that for a regular git directory, GIT_COMMON_DIR is not set, thus git rev-parse --path-format=absolute --git-common-dir just returns GIT_DIR (e.x. /Users/cecemei/codespace/oss-druid/.git).


This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

cp_if_not_exist ${DRUID_ROOT}/hooks/pre-push ${DRUID_ROOT}/.git/hooks/pre-push
cp_if_not_exist ${DRUID_ROOT}/hooks/pre-commits ${DRUID_ROOT}/.git/hooks/pre-commits
cp_if_not_exist ${DRUID_ROOT}/hooks/pre-pushes ${DRUID_ROOT}/.git/hooks/pre-pushes
GIT_COMMON_DIR=$(eval "git -C $DRUID_ROOT rev-parse --path-format=absolute --git-common-dir")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the usage of eval is necessary here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's probably not necessary, but while i was working on this i realized that this would change some files (run-all-in-dir.py, pre-commit, pre-push), and our script does cp_if_not_exist and throws if file is different from the ones in .git dir, so maybe we'd need to add a force flag to installing hook script to overwrite those. that might cause minor disruption to ppl's workflow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cript does cp_if_not_exist and throws if file is different from the ones in .git dir, so maybe we'd need to add a force flag to installing hook script to overwrite those.

that would be what a plain cp does - but I doubt the initial intention was that.
I don't think these scripts change often; but people may modify it locally...

I think cp_if_not_exist should have nothing to do with the usage of eval

there is nothing in this which would make eval necessary; please remove it.

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

Successfully merging this pull request may close these issues.

2 participants