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

package/scripts/postinstall: avoid writing to ~/.gitconfig #17068

Merged
merged 5 commits into from
Apr 11, 2024

Conversation

ZhongRuoyu
Copy link
Member

@ZhongRuoyu ZhongRuoyu commented Apr 10, 2024

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

We can eliminate permission issues by not touching ~/.gitconfig at all.

We pretend that the "global" configs are in the repository itself by overriding XDG_CONFIG_HOME and HOME. For example:

$ XDG_CONFIG_HOME= HOME=$PWD git config --global section.key value
$ XDG_CONFIG_HOME= HOME=$PWD git config --global section.key
value
$ git config --global section.key
$ cat $PWD/.gitconfig
[section]
  key = value

Fixes #17067.

Needs testing as I haven't tried to build a .pkg locally.

We can eliminate permission issues by not touching `~/.gitconfig` at
all.

Fixes #17067.
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Copy link
Member Author

@ZhongRuoyu ZhongRuoyu left a comment

Choose a reason for hiding this comment

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

It fails with fatal: detected dubious ownership in repository at '/usr/local/Homebrew' if I don't remove my existing installation, unfortunately. This is probably the original reason why set_git_safe_directory was introduced:

2024-04-11 01:37:54+08 Ruoyus-Mac package_script_service[2641]: ./postinstall: fatal: detected dubious ownership in repository at '/usr/local/Homebrew'
2024-04-11 01:37:54+08 Ruoyus-Mac package_script_service[2641]: ./postinstall: To add an exception for this directory, call:
2024-04-11 01:37:54+08 Ruoyus-Mac package_script_service[2641]: ./postinstall: 	git config --global --add safe.directory /usr/local/Homebrew
ls -al /usr/local/Homebrew
$ ls -al /usr/local/Homebrew
total 104
drwxr-xr-x  24 ruoyu  admin   768 Apr 11 01:48 .
drwxr-xr-x  14 root   wheel   448 Apr 11 01:47 ..
drwxr-xr-x   5 ruoyu  admin   160 Apr 11 01:47 .devcontainer
-rw-r--r--   1 ruoyu  admin    51 Apr 11 01:47 .dockerignore
-rw-r--r--   1 ruoyu  admin   380 Apr 11 01:47 .editorconfig
drwxr-xr-x  14 root   wheel   448 Apr 11 01:48 .git
drwxr-xr-x  12 ruoyu  admin   384 Apr 11 01:47 .github
-rw-r--r--   1 ruoyu  admin  5867 Apr 11 01:47 .gitignore
-rw-r--r--   1 ruoyu  admin  1342 Apr 11 01:47 .shellcheckrc
drwxr-xr-x   3 ruoyu  admin    96 Apr 11 01:47 .sublime
-rw-r--r--   1 ruoyu  admin    65 Apr 11 01:47 .vale.ini
drwxr-xr-x   4 ruoyu  admin   128 Apr 11 01:47 .vscode
-rw-r--r--   1 ruoyu  admin   207 Apr 11 01:47 CHANGELOG.md
-rw-r--r--   1 ruoyu  admin   829 Apr 11 01:47 CONTRIBUTING.md
-rw-r--r--   1 ruoyu  admin  2984 Apr 11 01:47 Dockerfile
-rw-r--r--   1 ruoyu  admin  1334 Apr 11 01:47 LICENSE.txt
drwxr-xr-x   5 ruoyu  admin   160 Apr 11 01:47 Library
-rw-r--r--   1 ruoyu  admin  8882 Apr 11 01:47 README.md
drwxr-xr-x   3 ruoyu  admin    96 Apr 11 01:47 bin
drwxr-xr-x  11 root   wheel   352 Apr 11 01:12 cache_api
drwxr-xr-x   7 ruoyu  admin   224 Apr 11 01:47 completions
drwxr-xr-x  75 ruoyu  admin  2400 Apr 11 01:47 docs
drwxr-xr-x   4 ruoyu  admin   128 Apr 11 01:47 manpages
drwxr-xr-x   5 ruoyu  admin   160 Apr 11 01:47 package

While the fix is easy, I think there's some value in making sure CI fails in this case.

package/scripts/postinstall Outdated Show resolved Hide resolved
@ZhongRuoyu
Copy link
Member Author

While the fix is easy, I think there's some value in making sure CI fails in this case.

Ah, no wonder why CI didn't fail. The macOS runner image has the following in ~/.gitconfig:

[safe]
        directory = *

https://github.com/actions/runner-images/blob/macos-13/20240405.2/images/macos/scripts/build/install-git.sh#L12

@ZhongRuoyu
Copy link
Member Author

ZhongRuoyu commented Apr 10, 2024

Good news: CI is now able to catch a safe.directory problem!

Bad news: as CI suggests, this unfortunately does not work on macOS 12, whose CLT provide Git 2.37.1. Git only started to respect -c safe.directory in 2.38.0: git/git@6061601.

Will have to think of another way to handle this.

In this attempt we pretend that the "global" configs are in the
repository itself.

    $ XDG_CONFIG_HOME= HOME=$PWD git config --global section.key value
    $ XDG_CONFIG_HOME= HOME=$PWD git config --global section.key
    value
    $ git config --global section.key
    $ cat $PWD/.gitconfig
    [section]
      key = value
@ZhongRuoyu
Copy link
Member Author

Confirmed locally that this is working as expected and ~/.gitconfig is not touched.

@ZhongRuoyu ZhongRuoyu marked this pull request as ready for review April 10, 2024 20:24
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Nice work on the approach here @ZhongRuoyu!

Perhaps an easier option would be overriding HOME for the entire script; it's all run as root anyway so the HOME pointing to the running user is erroneous and unneeded.

package/scripts/postinstall Outdated Show resolved Hide resolved
It is safe to override `HOME` for the entire script as only Git uses it.
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks again @ZhongRuoyu!

@MikeMcQuaid MikeMcQuaid merged commit e8b142d into master Apr 11, 2024
31 checks passed
@MikeMcQuaid MikeMcQuaid deleted the pkg-no-writing-to-gitconfig branch April 11, 2024 14:47
@github-actions github-actions bot added the outdated PR was locked due to age label May 12, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running the macOS installer changes permissions on ~/.gitconfig
2 participants