-
Notifications
You must be signed in to change notification settings - Fork 1
LT Rebase Merge: Deal with failure better, Shell Check fixes #23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: mainline
Are you sure you want to change the base?
Conversation
If for whatever reason the execution fails its annoying to figure out what the remaining commands need to be this prefills them out so if it fails its easy to pick up upon failure. Future work: Self Healing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances lt_rebase_merge.sh
by improving quoting for branch variables, adding a preflight block to list expected git commands, and cleaning up the PR branch before renaming.
- Wraps all branch variable expansions in quotes to satisfy ShellCheck and handle edge cases.
- Adds an echo block showing each git command that will run, for easier manual recovery.
- Deletes the PR branch remotely before proceeding with branch renames and tagging.
Comments suppressed due to low confidence (1)
lt_rebase_merge.sh:66
- It may be useful to add automated tests or a dry-run mode to verify that the preflight echo block lists the correct commands.
echo "Commands expected to run (This is in the even it fails during the run)"
# merge PR branch into next branch | ||
set +x | ||
|
||
echo "Commands expected to run (This is in the even it fails during the run)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a typo in this message: "even" should be "event".
echo "Commands expected to run (This is in the even it fails during the run)" | |
echo "Commands expected to run (This is in the event it fails during the run)" |
Copilot uses AI. Check for mistakes.
# merge PR branch into next branch | ||
set +x | ||
|
||
echo "Commands expected to run (This is in the even it fails during the run)" | ||
echo "git checkout $NEXT_BRANCH" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider quoting the variable in the echo to match the actual command and prevent word splitting if branch names contain spaces, e.g.: echo "git checkout \"${NEXT_BRANCH}\""
.
echo "git checkout $NEXT_BRANCH" | |
echo "git checkout \"${NEXT_BRANCH}\"" |
Copilot uses AI. Check for mistakes.
git checkout "${NEXT_BRANCH}" | ||
if [ $? -ne 0 ]; then | ||
echo "Failed to checkout ${NEXT_BRANCH}." | ||
exit 1 | ||
fi | ||
|
||
echo "git pull origin $NEXT_BRANCH" | ||
git pull origin $NEXT_BRANCH | ||
git pull origin "${NEXT_BRANCH}" | ||
if [ $? -ne 0 ]; then | ||
echo "Failed to pull ${NEXT_BRANCH}." | ||
exit 1 | ||
fi | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The repeated pattern of echo
+ command + if [ $? -ne 0 ]
could be refactored into a helper function to reduce duplication and improve readability.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for the typo pointed out by copilot, LGTM 🥌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚤
Due to changes I made to the rules I accidentally blocked our process here and the
lt_rebase_merge.sh
script failed mid way so I had to hand redo it. It was possible to do this from the shell output but it was less straight forward than I'd have wished and for other users.Now we're adding the intended actions prior to starting the script so users can copy and paste.
Ideally in the future this will be self healing state machine.