alirezanet / Husky.Net

Git hooks made easy with Husky.Net internal task runner! 🐶 It brings the dev-dependency concept to the .NET world!
https://alirezanet.github.io/Husky.Net/
MIT License
632 stars 29 forks source link

Support Partially Staged Files #23

Closed benbarth closed 2 years ago

benbarth commented 2 years ago

Hi! 👋🏻

First off -- I love this tool. Thank you for all of your hard work and effort. ❤️

I'd like to incorporate this tool into our development process but can't because of a single issue: Partial stages

For a single file, it is very common for me to stage some changes while leaving other changes unstaged.

      {
         "name": "dotnet-format",
         "group": "pre-commit",
         "command": "dotnet",
         "args": [ "format", "--include", "${staged}" ],
         "include": [ "**/*.cs" ]
      }

I currently use a task similar to the one above ☝🏻. When I commit, the task reformats the entire file (which is fine), but then it stages and commits the entire file including the changes I did not want (or intend) to commit.

Question: Does the include property on the task declare what staged files to "re-stage"?

Potential fix: Can the task runner return an error code if a file that's going to be "re-staged" is in the list of unstaged files?

Again, thank you for this wonderful tool! Let me know if you need any further clarification. Cheers!

alirezanet commented 2 years ago

Hi! 👋🏻 First off -- I love this tool. Thank you for all of your hard work and effort. ❤️

Hi ben, happy to hear that, you're welcome. 💓

I currently use a task similar to the one above ☝🏻. When I commit, the task reformats the entire file (which is fine), but then it stages and commits the entire file including the changes I did not want (or intend) to commit.

hmm Interesting ... good point! I'll look into this soon

Question: Does the include property on the task declare what staged files to "re-stage"?

I didn't fully understand the question but, include is just a filtering layer not related to this problem.

husky by itself doesn't re-stage any files but because it is passing the entire file to the dotnet format, any changes made by dotnet format can cause this problem. I think I should somehow exclude the unstaged parts after executing the task.

thank you for the feedback

benbarth commented 2 years ago

I've done more research and this may not really be a Husky.Net issue. (Although, Husky.Net could probably help resolve the issue.)

These links were VERY helpful: https://www.olioapps.com/blog/automatic-code-formatting/ https://github.com/hallettj/git-format-staged/blob/master/README.md

As a workaround I've added the following tasks. This has mostly resolved the issue. (This is definitely not ideal but seems to work.)

{
   "tasks": [
       {
          "name": "persist unstaged",
          "group": "pre-commit",
          "command": "bash",
          "args": [ "-c", "git", "diff", ">", "unstaged.diff.tmp" ],
          "windows": {
            "command": "cmd",
            "args": [ "/c", "git", "diff", ">", "unstaged.diff.tmp" ]
          }
       },
       {
           "name": "remove unstaged",
           "group": "pre-commit",
           "command": "git",
           "args": [ "restore", "." ]
        },
      {
         "name": "dotnet format",
         "group": "pre-commit",
         "command": "dotnet",
         "args": [ "format", "--include", "${staged}" ]
      },
      {
         "name": "git add",
         "group": "pre-commit",
         "command": "git",
         "args": [ "add", "." ]
      },       {
         "name": "restore unstaged",
         "group": "pre-commit",
         "command": "bash",
         "args": [ "-c", "git", "apply", "unstaged.diff.tmp", "--check", "&&", "git", "apply", "unstaged.diff.tmp", "||", "echo", "Unable to apply unstaged.diff.tmp." ],
         "windows": {
           "command": "cmd",
           "args": [ "/c", "git", "apply", "unstaged.diff.tmp", "--check", "&&", "git", "apply", "unstaged.diff.tmp", "||", "echo", "Unable to apply unstaged.diff.tmp." ]
         }
      }
   ]
}

UPDATE: git diff doesn't include new files. This solution is incomplete.

alirezanet commented 2 years ago

Thanks for sharing the links, Agree this may not be 100% related to husky.net, but I think husky should be able to handle it automatically. I have a few ideas that probably could help to automatically solve this problem in the next versions. (your workaround is very similar to one of them 👌)

niemyjski commented 2 years ago

I literally just ran into this and saw a possible solution here: https://github.com/okonet/lint-staged/issues/62

This feels like somewhat of a blocking issue that users might not be aware of.

alirezanet commented 2 years ago

I literally just ran into this and saw a possible solution here: okonet/lint-staged#62

This feels like somewhat of a blocking issue that users might not be aware of.

Thanks

Looks promising, I will try it today. my problem with git stash currently is the conflicts after git stash pop. I can't find any clean way to solve the conflicts especially when we're using something like dotnet format.

alirezanet commented 2 years ago

Until I find a clean way to solve this problem internally, the bellow workaround using the post-commit hook should do the trick: (this at least doesn't need any extra tasks) e.g

the pre-commit hook:

#!/bin/sh
. "$(dirname "$0")/_/husky.sh"

## stash all un-staged files before running the tasks
git stash -k -q
dotnet husky run --group 'pre-commit'

the post-commit hook:

#!/bin/sh
. "$(dirname "$0")/_/husky.sh"

## restore recently stashed files
git cherry-pick -n -m1 -Xtheirs stash
git reset

please let me know if this solved the problem. I'm still looking for a better way to handle this issue.

niemyjski commented 2 years ago

The fix above solved this issue but completely broke rolling back previous commits (undo in Git Kraken). As such I've had to disable these changes.

alirezanet commented 2 years ago

Do you know what commands Git Kraken is using internally to roll back previous commits? I didn't find any problem using standard git commands e.g git reset --hard HEAD~1.

Although I'm not gonna use the stash internally, that was only a possible workaround, just curious why is not compatible with git Kraken !? (probably is related to the git reset)

niemyjski commented 2 years ago

They allow you to do git reset --soft HEAD~1 (default) and git reset --hard HEAD~1 https://www.gitkraken.com/learn/git/problems/undo-git-commit#:~:text=To%20undo%20a%20Git%20commit%20after%20you%E2%80%99ve%20performed,you%20won%E2%80%99t%20need%20them%20again%20in%20the%20future.

Also make sure you have stagged / unstagged files when you do this as a test case.

alirezanet commented 2 years ago

I released a preview version 0.4.0-preview that includes this feature. testing these kinds of features is really hard and has a lot of edge cases. if you guys confirm that is working I'll release v0.4.

https://www.nuget.org/packages/Husky/0.4.0-preview

dotnet tool update husky --version 0.4.0-preview

My checklist

linter partial commit on-un-staged files on staged files
dotnet-format
csharpier
prettier ⚠️
eslint

about dotnet-format: look's like we can not pass a file to the dotnet-format outside a project/solution. it doesn't support it! or at least I couldn't find a way to do that yet.

thanks for your help

alirezanet commented 2 years ago

A few problems that we found in the first preview version were solved. checkout 0.4.0-preview-2

dotnet tool update husky --version 0.4.0-preview-2
linter partial commit on-un-staged files on staged files
dotnet-format ⚠️
csharpier
prettier
eslint

I'll wait for some feedback, thanks

benbarth commented 2 years ago

The ⚠️ on dotnet-format and lint-staged. What issue is expected?

alirezanet commented 2 years ago

The ⚠️ on dotnet-format and lint-staged. What issue is expected?

When we partially stage a file, that means we have 2 versions of that file. one in the working directory and one in the git object database. In preview-2 husky.net saves the partial version to a temporary cache folder in .husky/_/cache. this way it can send the files to the linters, I did a lot of tests but looks like dotnet-format only works if your file is part of a project/solution. this means dotnet-format doesn't work for partial parts as expected. I changed this to a warning because eventually, you will commit the other part too (working directory changes) that can fix all formatting issues.

benbarth commented 2 years ago

I just tried several scenarios on 0.4.0-preview-2 with this pre-commit hook:

      "command": "dotnet",
      "args": ["format", "Shinydocs.Platform", "--include", "${staged}"]

I'm currently stuck in a scenario where I cannot commit because of the following error message: The directory is not empty. : 'C:\Users\<user>\dev\.husky\_\cache' I tried deleting all of the files in the _cache directory but that did not resolve the issue.

alirezanet commented 2 years ago

Thanks for the feedback. 🙏

I updated a file with some poorly formatted code STAGED and some poorly formatted code UNSTAGED ❌ The staged code was committed without the dotnet format changes.

I'm struggling with this. dotnet-format is not actually like other code formatter or linters. It needs to build the project first which is sad :/.

I'm currently stuck in a scenario where I cannot commit because of the following error message: The directory is not empty. : >'C:\Users\\dev.husky_\cache' I tried deleting all of the files in the _cache directory but that did not resolve the issue.

hmm! Noted. how did it happen? can you give me reproducible examples?

alirezanet commented 2 years ago

Oh nvm, my bad. I found the second problem. 😳

fixed in preview3:

dotnet tool update husky --version 0.4.0-preview-3
benbarth commented 2 years ago

I'm struggling with this. dotnet-format is not actually like other code formatter or linters. It needs to build the project first which is sad :/.

I found a partial solution to this issue. ☝🏻

      "command": "dotnet",
      "args": ["format", "whitespace", "--folder", "--include", "${staged}"]

You CAN format whitespace but cannot format style or format analyzers. 😞

alirezanet commented 2 years ago

I found a partial solution to this issue. ☝🏻

      "command": "dotnet",
      "args": ["format", "whitespace", "--folder", "--include", "${staged}"]

You CAN format whitespace but cannot format style or format analyzers. 😞

I could've moved the temp files(partially staged) next to the original files before running the dotnet-format. this way format style probably works too but it is an ugly solution. 😏😒 also analyzers probably will start throwing errors. (like the duplicate class name .. etc)

benbarth commented 2 years ago

I could've moved the temp files next to the original files before running the dotnet-format. this way format style probably will work too but it is an ugly solution. 😏😒 also analyzers probably will start throwing errors. (like the duplicate class name .. etc)

😬 I was going to suggest a --cache-location flag on husky run but it's not a great solution. I think we'd still need to add the cache directory to the dotnet project. 😬

alirezanet commented 2 years ago

I could've moved the temp files next to the original files before running the dotnet-format. this way format style probably will work too but it is an ugly solution. 😏😒 also analyzers probably will start throwing errors. (like the duplicate class name .. etc)

😬 I was going to suggest a --cache-location flag on husky run but it's not a great solution. I think we'd still need to add the cache directory to the dotnet project. 😬

Yeah, It doesn't solve this. Honestly, I'm not sure how critical is this problem, should I consider implementing this differently or not?

I had two options:

I chose the second approach to avoid conflict problems, now I realized that dotnet-format doesn't behave nicely. 😒

I don't know what is the next step really, suggestions are welcome.

benbarth commented 2 years ago

I agree that the 2nd option is preferable. Hopefully, the dotnet format team can prioritize a fix for this issue. 🤞🏻 🤷🏻

benbarth commented 2 years ago

☝🏻 Looks like this dotnet format limitation is by design. https://github.com/dotnet/format/issues/1292#issuecomment-1024632625

alirezanet commented 2 years ago

Yeah, I noticed, that's because of the analyzer support. also probably ReSharper CLI tools don't work too.

What if I temporarily swap the cache and the workspace files!? it makes sense, if we don't stage a file why do we need to format or analyze it? Not sure yet but, this could solve all problems. I'll give it a shot

UPDATE: Never mind, It also leads to merge conflicts or losing the formatted parts

benbarth commented 2 years ago

@alirezanet It's hacky but a husky run --cache-location=<project>/tmp/ flag will work. When I added <None Include="tmp\**" /> to my project I was able to format the files in that location (even with temporary compilation errors) with dotnet format --include <project>/tmp/<filename>.cs.

alirezanet commented 2 years ago

Considering your suggestion I implemented a better cleaning up process, So by default, it handles temporary files. and we don't need to specify the cache location. Please try preview4 and let me know if it's working as expected. (I didn't fully test yet)

dotnet tool update husky --version 0.4.0-preview-4

PS. If it works correctly for formatting purposees, I think we only need an option to switch to analyzer mode to support dotnet analyzers that usually don't change the code. Not sure yet. (the weird thing is I didn't get any compilation errors with dotnet-format 5.1.250801 !)

alirezanet commented 2 years ago

Fixed in v0.4.0. Thanks for your help