Conversation
Contributor
Reviewer's GuideRefactors Git interactions in shell scripts to use Sequence diagram for the updated fault-tolerant update command flowsequenceDiagram
actor User
participant CLI as dockstarter_cli
participant CmdHandler as run_command
participant UpdateTemplates as update_templates
participant UpdateSelf as update_self
participant Git as git
User->>CLI: run with --update AppBranch TemplatesBranch
CLI->>CmdHandler: run_command --update
CmdHandler->>UpdateTemplates: run_script update_templates TemplatesBranch
Note over UpdateTemplates,CmdHandler: update_templates may fail but is wrapped with || true
UpdateTemplates->>Git: git -C TEMPLATES_PARENT_FOLDER fetch/checkout/reset/pull/gc
UpdateTemplates-->>CmdHandler: success or failure (ignored)
CmdHandler->>UpdateSelf: run_script update_self AppBranch REST_OF_ARGS
Note over UpdateSelf,CmdHandler: update_self may fail but is wrapped with || true
UpdateSelf->>Git: git -C SCRIPTPATH fetch/checkout/reset/pull/gc
UpdateSelf-->>CmdHandler: success or failure (ignored)
CmdHandler-->>CLI: result (last command status)
CLI-->>User: prints update notices and completion status
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
commands_update_self_logicandcommands_update_templates, the newRunAndLog ... git -C ... gc || trueusage will pass'||'and'true'as literal arguments togitrather than short‑circuiting in the shell; consider either dropping|| true(and lettingRunAndLoghandle failures) or wrapping the command in a subshell (e.g.bash -c 'git -C ... gc || true'). - The changes in
run_commandto append|| truetorun_script 'update_templates'andrun_script 'update_self'will now ignore failures from those update routines; please confirm whether this is intentional, and if not, propagate or handle the errors explicitly instead of masking them.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `commands_update_self_logic` and `commands_update_templates`, the new `RunAndLog ... git -C ... gc || true` usage will pass `'||'` and `'true'` as literal arguments to `git` rather than short‑circuiting in the shell; consider either dropping `|| true` (and letting `RunAndLog` handle failures) or wrapping the command in a subshell (e.g. `bash -c 'git -C ... gc || true'`).
- The changes in `run_command` to append `|| true` to `run_script 'update_templates'` and `run_script 'update_self'` will now ignore failures from those update routines; please confirm whether this is intentional, and if not, propagate or handle the errors explicitly instead of masking them.
## Individual Comments
### Comment 1
<location> `.includes/ds_functions.sh:9-10` </location>
<code_context>
- git fetch --quiet --tags &> /dev/null || true
- git symbolic-ref --short HEAD 2> /dev/null || git describe --tags --exact-match 2> /dev/null || git_best_branch "${GitPath}" "${DefaultBranch-}" "${LegacyBranch-}"
- popd &> /dev/null
+ git -C "${GitPath}" fetch --quiet --tags &> /dev/null || true
+ git -C "${GitPath}" symbolic-ref --short HEAD 2> /dev/null || git -C "${GitPath}" describe --tags --exact-match 2> /dev/null || git_best_branch "${GitPath}" "${DefaultBranch-}" "${LegacyBranch-}"
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Loss of explicit fatal behavior when the git path is invalid or inaccessible.
With `pushd "${GitPath}"`, an invalid or inaccessible repo path caused `fatal` and stopped execution. Using `git -C "${GitPath}"` now lets those failures fall through, potentially yielding empty or misleading results instead of a hard failure. If you still want invalid `GitPath` to be a hard error, add an explicit check that `GitPath` is a valid repo (or that `git -C` succeeds) and call `fatal`/return non‑zero when it isn’t.
</issue_to_address>
### Comment 2
<location> `.includes/cmdline.sh:668-669` </location>
<code_context>
local TemplatesBranch="${ParamsArray[1]-}"
- run_script 'update_templates' "${TemplatesBranch-}"
- run_script 'update_self' "${AppBranch-}" "${REST_OF_ARGS_ARRAY[@]}"
+ run_script 'update_templates' "${TemplatesBranch-}" || true
+ run_script 'update_self' "${AppBranch-}" "${REST_OF_ARGS_ARRAY[@]}" || true
result=$?
;;
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `|| true` here forces `result` to always be 0 and hides update failures.
Because of the `|| true`, the last command in each pipeline becomes `true`, so `$?` (and thus `result`) is always 0, regardless of whether `update_templates` or `update_self` failed. Previously, `result` reflected `update_self`’s success/failure. If you want to avoid aborting but still return a meaningful status, capture each exit code before `|| true` and combine them, or at least don’t use `|| true` on the call whose exit code you need to inspect.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+9
to
+10
| git -C "${GitPath}" fetch --quiet --tags &> /dev/null || true | ||
| git -C "${GitPath}" symbolic-ref --short HEAD 2> /dev/null || git -C "${GitPath}" describe --tags --exact-match 2> /dev/null || git_best_branch "${GitPath}" "${DefaultBranch-}" "${LegacyBranch-}" |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Loss of explicit fatal behavior when the git path is invalid or inaccessible.
With pushd "${GitPath}", an invalid or inaccessible repo path caused fatal and stopped execution. Using git -C "${GitPath}" now lets those failures fall through, potentially yielding empty or misleading results instead of a hard failure. If you still want invalid GitPath to be a hard error, add an explicit check that GitPath is a valid repo (or that git -C succeeds) and call fatal/return non‑zero when it isn’t.
Comment on lines
+668
to
+669
| run_script 'update_templates' "${TemplatesBranch-}" || true | ||
| run_script 'update_self' "${AppBranch-}" "${REST_OF_ARGS_ARRAY[@]}" || true |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Using || true here forces result to always be 0 and hides update failures.
Because of the || true, the last command in each pipeline becomes true, so $? (and thus result) is always 0, regardless of whether update_templates or update_self failed. Previously, result reflected update_self’s success/failure. If you want to avoid aborting but still return a meaningful status, capture each exit code before || true and combine them, or at least don’t use || true on the call whose exit code you need to inspect.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
… /
popdPull request
Purpose
Describe the problem or feature in addition to a link to the issues.
Approach
How does this change address the problem?
Open Questions and Pre-Merge TODOs
Check all boxes as they are completed
Learning
Describe the research stage
Links to blog posts, patterns, libraries or addons used to solve this problem
Requirements
Check all boxes as they are completed
Summary by Sourcery
Replace directory-changing git invocations with path-scoped git commands and improve update workflows’ robustness.
Enhancements:
git -C <path>instead ofpushd/popdaround git commands across shared git helper functions.