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

feat: set the HOME environment variable before invoking a command #32

Merged
merged 1 commit into from
Dec 26, 2023

Conversation

etorreborre
Copy link
Contributor

This should solve #26 where the global .gitignore file cannot be found.

@etorreborre etorreborre mentioned this pull request Dec 25, 2023
Copy link
Owner

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for your contribution 💯

Two things left:

  • Could you add a changelog
  • The HOME code here can be simplified
    • zbg/lib/git.ml

      Lines 22 to 27 in 311e035

      let home_dir = Unix.getenv "HOME" in
      let login =
      Process.proc_stdout
      @@ Printf.sprintf "HOME=%s git config user.login" home_dir
      in
      if String.is_empty login then None else Some login

@chshersh chshersh added the enhancement New feature or request label Dec 26, 2023
@etorreborre etorreborre force-pushed the etorreborre/feat/process-home branch from eb7b1eb to 27e5560 Compare December 26, 2023 13:17
@etorreborre etorreborre requested a review from chshersh December 26, 2023 13:17
@etorreborre
Copy link
Contributor Author

  • Could you add a changelog
  • The HOME code here can be simplified -> Ah, yes of course!

@etorreborre etorreborre force-pushed the etorreborre/feat/process-home branch from 27e5560 to ff31212 Compare December 26, 2023 13:19
@etorreborre etorreborre force-pushed the etorreborre/feat/process-home branch from ff31212 to 5337803 Compare December 26, 2023 13:20
Copy link
Owner

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

Awesome, looks great!

@etorreborre
Copy link
Contributor Author

BTW I forgot to mention that this PR indeed solves #26
Before:
image
After:
image

Copy link
Owner

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

Thanks again!

@chshersh chshersh merged commit 3e51741 into chshersh:main Dec 26, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants