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

Don't commit files when only LF/CRLF changes #265

Merged
merged 6 commits into from Dec 1, 2022

Conversation

ZeroRin
Copy link
Contributor

@ZeroRin ZeroRin commented Nov 30, 2022

Previously I tried to solve this by switching the order for add and status, which results in some side effects.
Here I tried to solve the issue by adding a diff check right before commit so other behaviors are not affected.

Note that the testcase for crlf in the original repo does not change line ending properly, so I also updated the test. The original action fails on this test while the modified version passes all the tests.

Related to #241.

@stefanzweifel
Copy link
Owner

Thanks for the PR @ZeroRin. I will take a closer look over the next few days.

Big thanks for also fixing the test case.

Update test name to make it clear that the Action no longer fails to detect CRLF changes.
Copy link
Owner

@stefanzweifel stefanzweifel left a comment

Choose a reason for hiding this comment

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

Updated the name of the test and added some comment for future me.

@stefanzweifel
Copy link
Owner

@ZeroRin I have one last question, but I struggle to formulate it correctly:

The changes made in this PR … does this represented the behaviour one would expect?

If I understood everything correctly, the Action now no longer creates a commit, if CRLF changes/differs.
Basically all I want to know is why we need to make this change.

@ZeroRin
Copy link
Contributor Author

ZeroRin commented Nov 30, 2022

@ZeroRin I have one last question, but I struggle to formulate it correctly:

The changes made in this PR … does this represented the behaviour one would expect?

If I understood everything correctly, the Action now no longer creates a commit, if CRLF changes/differs. Basically all I want to know is why we need to make this change.

setting autocrlf means that one intentionally want all line endings in the repository to be LF, and ask git to manage it automatically. switching line ending style of a local file should not be considered as a change.

Actually I feel the current pipeline is a bit weird. We decide whether to do a commit at the very beginning, and hoping the following add stage does not break it. I believe that the dirty check should just be done right before the commit stage.

just come up with another example that would break the original action:

touch "temp"
git add .
rm "temp"

Here I created a repo with an unstaged change that reverts the staged change. The repo is considered dirty during the beginning check, but after git add it sudennly becomes clean, and would raise an error during commit stage.
In contrast, I cannot think of a situation that we have to check dirtiness before add.

@stefanzweifel
Copy link
Owner

Thanks @ZeroRin. Gonna merge this PR now and tag a new version.

just come up with another example that would break the original action:

That example wouldn't work with this Action. There is no way for users to delete stuff between "stage files" and "create and push files". (Maybe through git-hooks, but that seems far fetched)

Anyway. :D
Thanks again for you contribution!

@stefanzweifel stefanzweifel merged commit 3ea6ae1 into stefanzweifel:master Dec 1, 2022
@stefanzweifel stefanzweifel changed the title Fix "nothing to commit" error with LF/CRLF changes #241 Don't commit files when only LF/CRLF changes Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants