Skip to content
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

feat: add git-continue #1176

Merged
merged 4 commits into from
Nov 28, 2024
Merged

Conversation

oikarinen
Copy link
Contributor

Also refactor git-abort to parse action from the called file name.

There's a promise for this in #865 but that was a long time ago.

Also refactor `git-abort` to parse action from the called file name.

There's a promise for this in tj#865 but that was a long time ago.
Copy link
Collaborator

@hyperupcall hyperupcall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good, I have a few suggestions


action=$(discover_action "$(basename "$0")")
op=$(discover_op)
validate_op "$op"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am reading the code correctly, if validate_op fails, then the script does not exit early (and the git command will erroneously execute) because errexit is not set. (the same is the case with discover_action). I think errexit should be set at the top?

bin/git-abort Outdated
f=${f/-/_}
test -f "${gitdir}/${f}_HEAD" && fcnt=1$fcnt && opfound=$i
done
set -xuo pipefail
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
set -xuo pipefail
set -euo pipefail

Recommend setting errexit (for the reasons mentioned below). And unsetting xtrace because xtrace is not set for any other scripts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey thanks for the catch, looks like copilot had a clitch that i didn't notice 🥹

fi
}

function discover_action() {
Copy link
Collaborator

@hyperupcall hyperupcall Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a huge fan of this function (feel like there is a better way to go about this), but I'm good with this for now.

@oikarinen oikarinen force-pushed the oikarinen-git-continue branch from 27ac89b to 0b381f4 Compare November 18, 2024 17:19
@oikarinen oikarinen force-pushed the oikarinen-git-continue branch from 0b381f4 to e2224c9 Compare November 22, 2024 19:10
tests/helper.py Outdated
from git import Repo, GitCommandError
from testpath import MockCommand, modified_env
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were not used, sorry for whitespace change for the file that ruff caused.

address review comments, fix the script, remove unnecessary testpath
import.
@oikarinen oikarinen force-pushed the oikarinen-git-continue branch from e2224c9 to d359df9 Compare November 22, 2024 20:15
@hyperupcall
Copy link
Collaborator

Thanks for the changes, LGTM. @spacewander what do you think?

@spacewander spacewander merged commit 2f4b57d into tj:main Nov 28, 2024
5 checks passed
@spacewander
Copy link
Collaborator

Merged. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants