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

"nothing to commit" error when there is LF/CRLF changes #241

Closed
ZeroRin opened this issue Sep 10, 2022 · 6 comments
Closed

"nothing to commit" error when there is LF/CRLF changes #241

ZeroRin opened this issue Sep 10, 2022 · 6 comments
Labels
bug Something isn't working

Comments

@ZeroRin
Copy link
Contributor

ZeroRin commented Sep 10, 2022

Version of the Action
v4.x.x

Describe the bug
When a file's line break is changed (CRLF->LF or LF->CRLF), running git status will result in an untracked modification (which passes the dirty check of this action).
However, if core.autocrlf is set to true or input, after running git add the change will be fixed and running git status or git commit after that will give you nothing to commit, working tree clean. As a result, the commit step would raise a "nothing to commit" error.

In my own case, I fixed the issue by running _switch_to_branch and _add_files before _git_is_dirty, but I'm not sure whether there will be other subsequences.

@stefanzweifel stefanzweifel added the bug Something isn't working label Sep 11, 2022
@stefanzweifel
Copy link
Owner

I'm sure this bug is annoying, but this seems to be out of scope for this Action to solve.

I don't fully understand what you're doing or how exactly you're encountering this issue. I can't seem to reproduce this error on my macOS machine nor do I know how we should write tests for this.

I've added a note in the "Limitations & Gotchas" section of the README in 18870f2, to inform others about this bug.

If you think this bug affects a lot of people and is worth fixing, please reopen this issue and provide an example project or step by step instructions on how to reproduce this error.

@stefanzweifel stefanzweifel closed this as not planned Won't fix, can't repro, duplicate, stale Sep 17, 2022
@ZeroRin
Copy link
Contributor Author

ZeroRin commented Sep 19, 2022

I created a simple example here
It contains 2 python scripts that rewrite a file to a LF and a CRLF respectively.
While running TestOutput with autocrlf=false would success and create 2 new commits, with autocrlf=true would fail.
I also included a TestOutputFix that uses my own fix of this issue. Not sure whether there will be other sideeffects though.

This bug would affect people who need to set autocrlf to true in their workflow. In my own case the workflow downloads some files with CRLF from another server and I want to change them to LF.

Cannot reopen the issue myself XD

@stefanzweifel
Copy link
Owner

Thanks @ZeroRin for taking the time to put this example repo together.
Will take some time this week to figure out, how we could add a test for this in the action and how we could solve this. Or if it's too much work for the action and is something that has to be solved in "userland".

stefanzweifel added a commit that referenced this issue Sep 28, 2022
This PR adds a test to confirm, that changes to CRLF are not properly detected and that the message "Working tree clean. Nothing to commit." is displayed.

Setting `core.autocrlf` to `true` also has no effect here.

refs #241
@stefanzweifel
Copy link
Owner

stefanzweifel commented Sep 28, 2022

Thanks again @ZeroRin for providing a reproduction repository.
I was able to write a test to verify that git-auto-commit doesn't work as how you expect it to work: b208f78.

However, after investing 2-3 hours into this I was not able to make it work as you might need it. Setting core.autocrlf to true, input or false didn't have any effect. The changes in your fork also didn't yield any result.

Feel free to clone the repository, install the test suite with yarn or npm install and then run the test with yarn test --filter "crlf".
By switching assert_line and refute_line here you would tell the test suite, that it would actually work.

If you find a fix, please open a PR.


For now I'm letting this issue rest.
I deliberately put "for the 80% use case" in the description of this Action, so that I don't invest too much time fixing obscure or niche workflows (no offense).

@ZeroRin
Copy link
Contributor Author

ZeroRin commented Sep 30, 2022

Not familiar with sed command, but as I tried the command it only print the substituted text to terminal without writing back to files.
Also, by redirecting the output to the same file result in an empty file no matter what was in the original file (maybe due to simultaneously read and write), and by redirecting the output to another file I saw raw text ^M at the end of every line, while the line ending is not modified.

The test case should use something like this

    echo -ne "crlf test1\r\n" > "${FAKE_LOCAL_REPOSITORY}"/new-file-2.txt
    echo -ne "crlf test1\n" > "${FAKE_LOCAL_REPOSITORY}"/new-file-3.txt

I tested on my modified version here and the output was as expected (showing failure as I did not edit all assertion properly).
However, my fix broke case 2 with INPUT_STATUS_OPTIONS: --untracked-files=no failed because I add file before the dirty check. (Case 23 failed just because the error is now raised before status check.)

@ZeroRin
Copy link
Contributor Author

ZeroRin commented Sep 30, 2022

Another solution I found is to use git diff instead of git status for dirty check, it will take care of the crlf issue during checking. The problem is that although git diff is more powerful and can mimic almost everything git status can do, their arguments are very different, which may lead to backward-compatibility issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants