Skip to content

Don't warn for restricted characters when unsafe.allowUnsafeCustomBinary is set - #1194

Closed
devtechedge wants to merge 1 commit into
steveukx:mainfrom
devtechedge:fix/allow-unsafe-custom-binary-no-warn
Closed

devtechedge wants to merge 1 commit into
steveukx:mainfrom
devtechedge:fix/allow-unsafe-custom-binary-no-warn

Conversation

@devtechedge

Copy link
Copy Markdown

When a custom binary path contains restricted characters and unsafe: { allowUnsafeCustomBinary: true } is set, construction succeeds but still prints "Invalid value supplied for custom binary, restricted characters must be removed or supply the unsafe.allowUnsafeCustomBinary option" to console.warn. Applications that create an instance per repo or workspace see the same warning several times at startup even though every git call succeeds.

This change skips the warning on the opted-in path. Supplying unsafe.allowUnsafeCustomBinary is the acknowledgement, so warning again per construction is noise. Restricted characters without the option continue to throw with the same error as before. This follows the first option suggested in the issue: do not warn when the flag is true.

Fixes #1190

Testing:

  • Added a unit test asserting console.warn is never called and construction succeeds when the option is set.
  • Added a unit test asserting restricted characters without the option still throw and do not warn.
  • Before the change, the new no-warn test fails against the untouched source; after the change, the full unit suite passes. The six integration suites that fail locally fail identically on the base commit of a Windows checkout and are unrelated to this change.
  • Added a changeset.

@changeset-bot

changeset-bot Bot commented Sep 13, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 2179c19

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
simple-git Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@devtechedge

Copy link
Copy Markdown
Author

Hi - polite check-in on the custom-binary warn skip whenever bandwidth allows.

Even with unsafe.allowUnsafeCustomBinary set, simple-git still printed the restricted-character console.warn on every simpleGit() construction, which is noisy for apps that create one instance per repo (Fixes #1190).

simple-git/src/lib/plugins/custom-binary.plugin.ts now throws only when the flag is absent and never warns on the opted-in path; the throw path stays the same.

simple-git/test/unit/plugins/plugin.binary.spec.ts adds two unit tests (no warn + command queued when opted in; throw without the flag and no warn), proven fail-before/pass-after by stashing the source alone, and a patch changeset is included.

Local unit suite was green after the change; GitHub Actions CI and Lint are still in action_required pending first-contribution approval, with Snyk already green.

Glad to adjust wording, drop the changeset shape, or align with any preferred option from #1190 if that would help.

Whenever it fits is fine - still ready from my end.

@steveukx steveukx mentioned this pull request Sep 22, 2026
Merged
@steveukx

Copy link
Copy Markdown
Owner

Hello, thank you for the PR and for the associated issue.

This change is going to be rolled into the v4 release #1193 which should be released within the week.

https://github.com/steveukx/git-js/pull/1193/changes#diff-d7b3b57ffb8cfe6c169f5e520df2a14e212be575e58298067ec1e062292d948bR27

Thanks

@devtechedge

Copy link
Copy Markdown
Author

Thanks for the update, Steve.

Glad the fix fits into the v4 release, and the detailed release notes there look great.

Happy to adjust anything on my end if it helps the rollup.

@steveukx steveukx added the more-info-needed More information is required in order to investigate label Sep 24, 2026
devtechedge added a commit to devtechedge/oss-contributions that referenced this pull request Sep 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

more-info-needed More information is required in order to investigate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

console.warn on every instance when allowUnsafeCustomBinary is opted in — noise for multi-instance apps

2 participants