stefanzweifel / git-auto-commit-action

Automatically commit and push changed files back to GitHub with this GitHub Action for the 80% use case.
MIT License
1.96k stars 226 forks source link

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

Closed ZeroRin closed 1 year ago

ZeroRin commented 1 year ago

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 commented 1 year ago

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 https://github.com/stefanzweifel/git-auto-commit-action/commit/18870f2286c2860e1e6c802fe8f7d5f382b903d7, 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.

ZeroRin commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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: https://github.com/stefanzweifel/git-auto-commit-action/commit/b208f78c106ef46799e92c989427512a5259f056.

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 commented 1 year ago

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 commented 1 year ago

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.