Document binary and source installation#78
Conversation
|
Independent review findings:
|
|
Addressed the posted installation review findings in
Verification:
|
|
Fresh independent review of current head
|
|
Addressed the fresh review concerns in
Verification:
|
|
Fresh review of current head
|
|
Addressed the review concerns in
Verification:
|
cosentinode
left a comment
There was a problem hiding this comment.
Fresh independent review of current head 39e2ed3 found three concerns:
-
Medium - source installs are not actually pinned to the published tag (
docs/installation.md:168,docs/installation.md:196).git clone --branch "v$VERSION"accepts either a branch or a tag, and a same-named branch wins. I reproduced this with distinctrefs/heads/v0.1.0andrefs/tags/v0.1.0: both documented clone forms checked out the branch asheads/v0.1.0. That permits moving/unreleased source with the same package version to pass the later--versioncheck, contradicting lines 159-161. Use an unambiguousrefs/tags/v...fetch/checkout and validate the resulting ref; the validators currently assert the ambiguous command and the Windows git mock ignores its arguments (scripts/test-installation-docs.sh:181-182,scripts/test-installation-docs.ps1:62-63,153-159). -
Medium - Rust 1.85 source-build support is only established for Linux (
docs/installation.md:148-157,.github/workflows/ci.yml:15-29,73-98). The exact 1.85 job builds only the Linux host target. The Windows validator builds with movingstable, while the macOS validator does not compile the workspace at all. Since the documentation applies the 1.85 minimum to Linux, macOS, and Windows source builds, target-specific code/dependencies can exceed the advertised MSRV without PR CI noticing. Pin 1.85 in native platform build/check jobs (or otherwise check every documented source target at that minimum). -
Low - an absent Windows user PATH is persisted with an empty entry (
docs/installation.md:136-140,docs/installation.md:214-218). WhenGetEnvironmentVariable(..., "User")returns null/empty,"$UserPath;$InstallDir"becomes";$InstallDir". Both install methods therefore write a malformed leading empty PATH component for a clean user environment. Build the new value conditionally when no user PATH exists. The validator rewrites the operation to the always-populated process PATH (scripts/test-installation-docs.ps1:19-31,67-68), so this case is not covered.
|
Addressed review concerns in
Verification:
|
|
Fresh independent review of current head
|
|
Addressed both review concerns in
Verification:
|
|
Fresh independent review of current head
|
|
Addressed the Unix selected-version PATH concern in
Verification:
The PowerShell validator was not run locally because |
|
CI follow-up for |
|
Concern: Windows selected-version behavior is not actually tested. |
|
Resolved the Windows selected-version validation concern in The PowerShell validator now rewrites both extracted Windows snippets to synthetic version Verification:
|
|
Concern: persistent Windows PATH order is not actually validated ( |
|
Resolved the persistent Windows PATH ordering concern in
Verification:
|
|
Concern: persistent Windows PATH precedence is not actually validated
Please make the persistent behavior/claim safe for machine-level shadowing (for example, detect and report it) and have the validator compose machine + user PATH in Windows order with an older machine-level fixture. |
|
Resolved the machine-level Windows PATH precedence concern in
Verification:
|
|
Medium - fixed working-directory state makes installation unsafe and breaks the documented retry path. The archive snippets download |
|
Addressed the fixed-working-directory/retry concern from #78 (comment) in commit
Verification:
|
cosentinode
left a comment
There was a problem hiding this comment.
Fresh independent review of current head 41e1a75 found three concerns:
-
Medium - the final version checks do not reliably validate PATH command resolution (
docs/installation.md:70-76,123-129,189-194,268-274,335-340). Bash functions and PowerShell functions/aliases take precedence over executables found throughPATH. A pre-existingbitbygitfunction that printsbitbygit <selected-version>therefore lets these blocks pass even though the bare command never resolved the installed file; a conflicting function instead makes the Windows blocks fail only after persistent user PATH and process PATH were already changed. The document consequently overstates the guarantee atdocs/installation.md:355-358. Resolve and compare the application path itself before the final bare invocation, and test higher-precedence command shadowing plus late-failure state preservation; current fixtures cover only an older executable earlier in PATH (scripts/test-installation-docs.sh:108-111,149-153;scripts/test-installation-docs.ps1:204-221,414-433). -
Low - cleanup failures mask the primary installation result on every platform (
docs/installation.md:48-53,101-106,239-244,195-197,341-343). In each Bash EXIT trap,rm -rfruns underset -e, so if cleanup fails the followingexit "${status}"is never reached and the original failure status is lost. In PowerShell, a terminatingRemove-Itemerror fromfinallysimilarly replaces the exception raised by download, checksum, build, or validation. This makes the actual failure difficult to diagnose, and after a successful Windows PATH mutation it reports failure without distinguishing that installation state was already committed. Preserve the original status/exception while reporting cleanup failure separately, and add forced-cleanup-failure coverage; the validators currently check only successful removal (scripts/test-installation-docs.sh:154-156,219-220,282-293;scripts/test-installation-docs.ps1:186-188,257,315,403-405,466,481). -
Low - successful Unix retries are not PATH-idempotent (
docs/installation.md:70,123,268). Each rerun prepends another identical$HOME/.local/binentry. The validator executes archive and source blocks twice but asserts only that the first entry is correct, so this accumulation passes (scripts/test-installation-docs.sh:142-149,286-293). Deduplicate or avoid prepending when the entry is already present, and assert exactly one entry after retries.
|
Resolved all concerns from #78 (review) in
Verification:
|
|
Fresh independent review of current head
|
|
Resolved both concerns from #78 (comment) in
Verification:
|
|
Fresh independent review of current head
|
|
Fixed the PATHEXT machine-PATH shadow concern in commit
Verification:
|
|
Fresh independent review of current head
|
|
Fixed both concerns in
Verification:
No unresolved risks identified. Not merged. |
cosentinode
left a comment
There was a problem hiding this comment.
Fresh independent review of current head 98c0c3e found two concerns:
-
Medium - source installs fail when Cargo output is configured outside the default
targetdirectory (docs/installation.md:319-327, repeated for Windows at:382-394). Both snippets run Cargo and then unconditionally execute/copy$SourceDir/target/release/bitbygit[.exe].CARGO_TARGET_DIRand the standard Cargobuild.target-dir/build.targetsettings legitimately move that artifact. I reproduced this head withCARGO_TARGET_DIR=/tmp/custom cargo build --locked --release -p bitbygit: the build succeeded,/tmp/custom/release/bitbygitexisted, and the documentedtarget/release/bitbygitpath did not. Pin a private target directory (and native target, if needed) in the snippets or derive the actual Cargo output path. The validators mask this because they pre-place a fixture at the assumed path and mockcargowithout validating arguments or output (scripts/test-installation-docs.sh:343-377;scripts/test-installation-docs.ps1:391-430); the separate CI builds do not execute these source snippets with real Cargo. -
Low - Unix PATH deduplication is still not lossless for newline-containing entries (
docs/installation.md:79-84, repeated at:151-156and:340-345). Unix environment values and path components may legally contain newlines, butread -r -aconsumes only the first line. ForPATH=$'/one:/two\ncontinued:/three:', the documented loop produces only<install>:/one, dropping the rest of the caller PATH. Parse by the colon delimiter without a line-orientedread, and add this case alongside the current empty-entry assertions (scripts/test-installation-docs.sh:146-179,383-423).
|
Addressed both concerns in
Verification:
The pushed PR checks will execute the full PowerShell regression on Windows and the Bash regression on macOS. |
|
Follow-up |
|
Fresh independent review: zero concerns. Reviewed issue #64, the final diff, release workflow contracts, installation snippets and validators, and the effective merge with current |
Summary
Verification
cargo build --locked --release -p bitbygitcargo run -p bitbygit -- --version(bitbygit 0.1.0)git diff --checkFixes #64