Skip to content

Conversation

@NonLogicalDev
Copy link
Contributor

Follow git's convention of keeping interactive edit files in the .git directory.
Avoid writing .stgit-edit.txt files to the working tree.

…ing directory

Follow git's convention of keeping interactive edit files in the `.git` directory.
Avoid writing .stgit-edit.txt files to the working tree.
Copy link
Collaborator

@jpgrayson jpgrayson left a comment

Choose a reason for hiding this comment

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

This change looks great. Just one concern about the STG_EDIT_IN_CWD environment variable. Remove it or convince me it's needed and we'll get this PR merged.

Comment on lines +343 to +344
// If STG_EDIT_IN_CWD is set, return path as is.
match std::env::var("STG_EDIT_IN_CWD") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there known cases where someone might need to fallback to the old behavior?

If not, I'd prefer we not have this environment variable at all; keep it simple.

Another issue with this env var is that we're only checking its existence, not its value. So if someone set STG_EDIT_IN_CWD=0, for example, it would fallback to the old behavior the same as if they'd set STG_EDIT_IN_CWD=1.

@NonLogicalDev
Copy link
Contributor Author

I would say there is a good reason to keep this variable for a little while, maybe until next big release, I don't personally foresee any issues with doing it this way, but it is a behavior change and it is best to keep these behind feature flags. I am a big believer in Hyrum's Law.

I do believe this should probably be sourced from git config rather than ENV, or both, and totally agree with your comment that we should check the value of the Env for truthyness, not just the mere presense.

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.

2 participants