Skip to content

Conversation

qbisi
Copy link
Contributor

@qbisi qbisi commented Sep 25, 2025

The issue

Consider these two unmatching cases running git config --global --get safe.directory "/home/armbian/build"

  1. "/home/armbian/build/subdir" in global safe directory: return true. While git does not consider "/home/armbian/build" a safe directory.
  2. "/home/armbian/*" in global safe directory: return false. While git does consider "/home/armbian/build" a safe directory.

The cause

git config --get match driectory name with a tab completion, not exactly the directory name or using glob match pattern

The solution

use git -C $1 status to check the safety of the repo.

The benefit

less impurity of possible change on user's ~/.git/config file.

Documentation summary for feature / change

Please delete this section if entry to main documentation is not needed.

If documentation entry is predicted, please provide key elements for further implementation into main documentation and set label to "Needs Documentation". You are welcome to open a PR to documentation or you can leave following information for technical writer:

  • short description (copy / paste of PR title)
  • summary (description relevant for end users)
  • example of usage (how to see this in function)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also note any relevant details for your test configuration.

  • Test A
  • Test B

Checklist:

Please delete options that are not relevant.

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@qbisi qbisi requested a review from a team as a code owner September 25, 2025 20:18
Copy link
Contributor

coderabbitai bot commented Sep 25, 2025

Walkthrough

The function git_ensure_safe_directory changed its safety verification: instead of checking git config for safe.directory, it now runs a git status with -C on the target directory. If that per-directory status fails, it falls back to adding the directory to the global safe.directory using regular_git. The function’s external signature remains unchanged. No new public methods, updates to public method signatures, or public data structures were introduced.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title clearly identifies the affected function and the key change of using git status for safety checks, concisely summarizing the main update in the patch without extraneous detail.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed The pull request description clearly outlines the problem with current safe.directory checks, explains the cause, describes the new status-based solution, and highlights the benefit of avoiding config changes, directly corresponding to the code modifications.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot added 11 Milestone: Fourth quarter release size/small PR with less then 50 lines Needs review Seeking for review Framework Framework components labels Sep 25, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
lib/functions/general/git.sh (2)

57-58: Use the local variable consistently

Minor: use "$git_dir" instead of "$1" for readability and to match the canonicalization above.

-		local git_dir="$1"
-		if [[ -e "$1/.git" ]]; then
+		local git_dir="$1"
+		if [[ -e "$git_dir/.git" ]]; then

59-59: Clarify log message

Message implies “marking all directories as safe,” which isn’t accurate. The proposed change makes it explicit and less misleading.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 9ef5b99 and 21ac546.

📒 Files selected for processing (1)
  • lib/functions/general/git.sh (1 hunks)
🔇 Additional comments (1)
lib/functions/general/git.sh (1)

59-61: Review git_ensure_safe_directory: Canonicalize paths and use rev-parse

  • Always convert git_dir to an absolute path before invoking git -C to avoid “cannot chdir” on relative inputs.
  • Replace the expensive git status call with:
    - git -C "$git_dir" status > /dev/null
    + abs_dir="$(realpath -m "$git_dir")"
    + if ! git -C "$abs_dir" rev-parse --git-dir >/dev/null 2>&1; then
    +   regular_git config --global --add safe.directory "$abs_dir"
    + fi
  • Update the alert message accordingly.

This change prevents relative‐path failures and reduces overhead. [mandato‌ry_refactors_required]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
11 Milestone: Fourth quarter release Framework Framework components Needs review Seeking for review size/small PR with less then 50 lines
Development

Successfully merging this pull request may close these issues.

1 participant