Conversation
…pect empty overrides
…nstall, and partial failures
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
install.cmd still isn’t safe for “special-char paths”: delayed expansion (!VAR!) will corrupt paths containing !, and unquoted echo lines can break on &/|/redirection characters. The installer also returns success even when configure.sh fails, making partial failures hard to detect programmatically. configure.sh’s new remote-branch detection may emit a branch name that doesn’t exist locally, which can break downstream workflows depending on local branches. The +x defaulting approach in _config.sh is an improvement but currently inconsistent across lint/format variables.
Additional notes (3)
-
Compatibility |
install.cmd:13-13
install.cmdnow relies heavily on delayed expansion (!VAR!) for user-provided paths. That breaks the stated goal of “special-char paths” in at least two common cases: -
Any path containing
!will be corrupted when referenced via!TARGET_DIR!/!CGW_DIR!. -
Several
echo ... !TARGET_DIR!/echo ... !CGW_DIR!lines are unquoted, so meta-chars like&,|,<,>in paths can still split commands and break the installer.
As-is, the hardening is incomplete and will fail on real-world Windows paths (especially those containing & or !).
-
Maintainability |
install.cmd:271-277
The installer exits0even whenconfigure.shfails (you only print a warning). For automation (or users running from a terminal), this makes partial failures look like success, contradicting the “partial failures” hardening goal. -
Maintainability |
scripts/git/_config.sh:98-98
You switched severalCGW_LINT_*/CGW_FORMAT_*defaults to the+x(unset-only) pattern, which is good for respecting explicit empty overrides. However, other related vars (e.g.,CGW_LINT_EXTENSIONS) still use:-, so an explicit empty override won’t be consistently respected across lint/format configuration.
Summary of changes
Summary of changes
-
Line-ending normalization
- Added
.shellcheckrcto.gitattributeswitheol=lf.
- Added
-
Hook local-file protection updates
- Updated
.githooks/pre-commitand.githooks/pre-pushto treatMEMORY.mdas local-only (replacingSESSION_LOG.md). - Adjusted
pre-pushconventional prefix regex strings to use ERE alternation (feat|fix|...) instead of escaped pipes.
- Updated
-
Windows installer hardening (
install.cmd)- Switched many expansions to delayed expansion (
!VAR!), added self-install guard, improvedbashdiscovery (adds Git for WindowsbintoPATH), added UNC/pushdfailure handling, added backup-copy failure warnings, and introduced anEXIT_CODEpath with an:abortlabel.
- Switched many expansions to delayed expansion (
-
Git script configuration improvements
scripts/git/_config.sh: normalized indentation and changed multipleCGW_LINT_*/CGW_FORMAT_*defaults to use the+x(unset-only) pattern so explicit empty overrides are preserved.scripts/git/configure.sh: improved source-branch detection by checkingrefs/remotes/origin/*when local branches are absent.
| pause | ||
| endlocal | ||
| exit /b 0 | ||
| exit /b %EXIT_CODE% |
There was a problem hiding this comment.
EXIT_CODE is set under setlocal, but you call endlocal before exit /b %EXIT_CODE%. After endlocal, EXIT_CODE is no longer defined (restored to the caller environment), so the script will very likely exit with the wrong code (often 0), defeating the new failure signaling.
Suggestion
Preserve the exit code across endlocal by combining it into the same line so %EXIT_CODE% expands before the environment is restored.
pause
endlocal & exit /b %EXIT_CODE%(Alternative if you want to keep separate lines: capture to an outer variable via the classic endlocal & set pattern.)
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this fix.
| _detect_source_branch() { | ||
| local target="$1" | ||
| # Check common source branch names | ||
| # Check common source branch names (local first, then remote tracking) | ||
| for name in development develop dev staging; do | ||
| if git show-ref --verify --quiet "refs/heads/${name}" 2>/dev/null; then | ||
| echo "${name}" | ||
| return 0 | ||
| fi | ||
| if git show-ref --verify --quiet "refs/remotes/origin/${name}" 2>/dev/null; then | ||
| echo "${name}" | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
_detect_source_branch() now returns ${name} when only refs/remotes/origin/${name} exists. If later scripts assume the source branch is a local branch (common for merge workflows), this will cause downstream failures (e.g., git checkout development failing because it doesn’t exist locally).
Suggestion
When you detect only a remote-tracking branch, either:
- Ensure a local tracking branch exists before returning (e.g., create
developmenttrackingorigin/development), or - Return an unambiguous ref (like
origin/development) and update downstream consumers to handle remote refs.
Reply with "@CharlieHelps yes please" if you’d like me to add a commit that creates a local tracking branch when only origin/<name> exists.
…oss endlocal, configure.sh failure exit, CGW_LINT_EXTENSIONS +x, auto-create local tracking branch
|
@CharlieHelps review new commits |
There was a problem hiding this comment.
install.cmd still cannot safely operate on paths containing ! due to global EnableDelayedExpansion, and it currently proceeds rather than failing fast. changelog_generate.sh now mis-parses conventional commits that don’t include a space after the colon (e.g., feat:foo), producing incorrect changelog entries. configure.sh creates local tracking branches for remote-only sources, but || true can hide failures and reintroduce downstream breakage.
Additional notes (1)
- Compatibility |
install.cmd:8-16
EnableDelayedExpansionstill makes this installer unsafe for paths containing!(the value will be mangled anywhere you use!TARGET_DIR!/!CGW_DIR!). Right now you only document the limitation, but the script will proceed and produce hard-to-diagnose partial installs if either path contains!(including when CGW itself lives under such a path). If the goal is “harden against special-char paths”, this needs to either be truly supported (by avoiding delayed expansion for user paths) or be a hard preflight failure.
Summary of changes
Summary of changes
-
Line-ending normalization
- Added
.shellcheckrcto.gitattributeswitheol=lf.
- Added
-
Git hooks: local-only file protection + conventional prefixes
- Updated
.githooks/pre-commitand.githooks/pre-pushto treatMEMORY.mdas local-only (replacingSESSION_LOG.md). - Simplified conventional-prefix regex strings in
.githooks/pre-pushto ERE alternation (feat|fix|...).
- Updated
-
Windows installer hardening (
install.cmd)- Added self-install guard, improved
bashdiscovery (adds common Git for Windows locations toPATH), better handling ofpushdfailure, backup copy warnings, cleanup reporting, and non-zero exit signaling viaEXIT_CODE. - Switched many path expansions/prints to delayed expansion and
echo(.
- Added self-install guard, improved
-
Git script configuration + detection
scripts/git/_config.sh: standardized indentation and switched moreCGW_LINT_*/CGW_FORMAT_*defaults to the unset-only (+x) pattern; also updatedCGW_LINT_EXTENSIONSsimilarly.scripts/git/configure.sh: when onlyorigin/<name>exists for common source branches, create a local tracking branch and return the local name.
-
Maintenance/refactors
- Large 2-space indentation normalization across several
scripts/git/*.shscripts. scripts/git/changelog_generate.sh: refactored commit-subject parsing to use bash parameter expansion instead ofsed.
- Large 2-space indentation normalization across several
| while IFS='|' read -r hash subject _body; do | ||
| [[ -z "${hash}" ]] && continue | ||
|
|
||
| local prefix rest | ||
| if echo "${subject}" | grep -qE "^[a-zA-Z]+:"; then | ||
| prefix="${subject%%:*}" | ||
| rest="${subject#*: }" | ||
| else | ||
| prefix="other" | ||
| rest="${subject}" | ||
| fi |
There was a problem hiding this comment.
The new subject “rest” extraction is incorrect for valid conventional commits that omit the space after the colon (e.g., feat:fix parser or feat:foo). With rest="${subject#*: }", those messages won’t match ": " and you’ll end up emitting the full subject (including the prefix) in the changelog entry, which is noisy and breaks categorization expectations.
Suggestion
Make the prefix-strip tolerant of optional whitespace by stripping up to : and then trimming leading whitespace:
prefix="${subject%%:*}"
rest="${subject#*:}"
# trim leading whitespace (space/tab/etc.)
rest="${rest#"${rest%%[![:space:]]*}"}"This keeps the no-external-process benefit while handling both type: desc and type:desc.
Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this fix (and a small test snippet you can run manually).
1 participant
Changes
Branch
development->main