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

Support Partially Staged Files #23

Closed
benbarth opened this issue Jan 20, 2022 · 26 comments
Closed

Support Partially Staged Files #23

benbarth opened this issue Jan 20, 2022 · 26 comments
Assignees
Labels
enhancement New feature or request feature request Request for a feature

Comments

@benbarth
Copy link

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 alirezanet self-assigned this Jan 20, 2022
@alirezanet alirezanet added good first issue Good for newcomers enhancement New feature or request labels Jan 20, 2022
@alirezanet
Copy link
Owner

alirezanet commented Jan 20, 2022

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

@alirezanet alirezanet changed the title Re-staging should fail on partial stages Support Partially Staged Files Jan 21, 2022
@alirezanet alirezanet added bug Something isn't working and removed good first issue Good for newcomers labels Jan 21, 2022
@benbarth
Copy link
Author

benbarth commented Jan 21, 2022

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
Copy link
Owner

alirezanet commented Jan 21, 2022

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
Copy link

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

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

@alirezanet
Copy link
Owner

alirezanet commented Jan 22, 2022

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
Copy link
Owner

alirezanet commented Jan 22, 2022

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.

@alirezanet alirezanet added help wanted Extra attention is needed and removed bug Something isn't working labels Jan 22, 2022
@niemyjski
Copy link

niemyjski commented Jan 25, 2022

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
Copy link
Owner

alirezanet commented Jan 25, 2022

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)

@alirezanet alirezanet removed the help wanted Extra attention is needed label Jan 26, 2022
@niemyjski
Copy link

niemyjski commented Jan 26, 2022

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 alirezanet added the feature request Request for a feature label Jan 27, 2022
alirezanet added a commit that referenced this issue Jan 27, 2022
@alirezanet
Copy link
Owner

alirezanet commented Jan 27, 2022

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 alirezanet added the help wanted Extra attention is needed label Jan 27, 2022
@alirezanet
Copy link
Owner

alirezanet commented Jan 28, 2022

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
Copy link
Author

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

@alirezanet
Copy link
Owner

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
Copy link
Author

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 added a poorly formatted file STAGED and it formatted it correctly ✔️
  • I added a poorly formatted file UNSTAGED and it did not attempt to format it ✔️
  • 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 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
Copy link
Owner

alirezanet commented Jan 28, 2022

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<user>\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
Copy link
Owner

alirezanet commented Jan 28, 2022

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

fixed in preview3:

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

@benbarth
Copy link
Author

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
Copy link
Owner

alirezanet commented Jan 28, 2022

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
Copy link
Author

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
Copy link
Owner

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:

  • Using patch files and merging changes (that could cause unsolvable conflicts)
  • Using git object index database and passing two different files to the linters

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
Copy link
Author

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

@benbarth
Copy link
Author

☝🏻 Looks like this dotnet format limitation is by design. dotnet/format#1292 (comment)

@alirezanet

This comment has been minimized.

@benbarth
Copy link
Author

benbarth commented Jan 29, 2022

@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 added a commit that referenced this issue Jan 29, 2022
@alirezanet
Copy link
Owner

alirezanet commented Jan 29, 2022

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
Copy link
Owner

Fixed in v0.4.0.
Thanks for your help

@alirezanet alirezanet removed the help wanted Extra attention is needed label Mar 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature request Request for a feature
Projects
None yet
Development

No branches or pull requests

3 participants