Skip to content

Stash unstaged before hook#12

Open
dh-nunes wants to merge 1 commit intorhysd:masterfrom
dh-nunes:fix-stash
Open

Stash unstaged before hook#12
dh-nunes wants to merge 1 commit intorhysd:masterfrom
dh-nunes:fix-stash

Conversation

@dh-nunes
Copy link
Copy Markdown

Runs git stash before any hook operation and traps stash pop for whenever the hook exits. Fixes #3.

@dh-nunes
Copy link
Copy Markdown
Author

Actually forgot pop can fail when overwriting, working on that

@rhysd
Copy link
Copy Markdown
Owner

rhysd commented Nov 3, 2019

I'm sorry for long delay since I focused on a few projects (and some private projects). Let me check this.

build.rs Outdated

set -e

git stash --keep-index --include-untracked --quiet
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I have two points here.

  • I agree with pre-commit hook runs on local files, not on commited ones #3, however, --include-untracked would take time when there is a heavy directory such as node_modules. What is the case untracked file should be stashed?
  • I think showing what hook script does is preferred rather than doing it implicitly. It is important when these lines don't work as we intended. We can analyze what did not work. I prefer to adding echo to show what is being done.
echo '+git stash --keep-index --include-untracked'
git stash --keep-index --include-untracked

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Will do, should the trap echo too?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Yeah, I feel so.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Everything should echo now. Also removed the --quiet

build.rs Outdated
set -e

git stash --keep-index --include-untracked --quiet
trap "git reset --quiet --hard HEAD && git stash pop --index --quiet" EXIT INT TERM
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think this git stash pop will fail when the script generates some untracked file. For example, let's say cargo test generate output.txt. If output.txt already exists, git stash push saves the output.txt, then cargo test will be run for check which generates new output.txt. Finally git stash pop tries to restore output.txt but it already exists. I think we should clean untracked files before git stash pop.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'll respond to the second point above here, since it's the same answer.

The reset --hard is needed to avoid pop conflicts. Since the hard reset would literally wipe the repo we need --include-untracked to save all the files. We would lose all files generated by the hook, true, but I'm not sure this is avoidable

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Since the hard reset would literally wipe the repo we need

I think git reset --hard does not wipe untracked files. Please try the following in some Git repository

# Generate a file
echo hello > hello.txt

# Check hello.txt is untracked
git status

# Run hard reset
git reset --hard HEAD

# hello.txt would still exist

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Damn, my git-fu is failing me! Removed the reset and the --include-untracked. Not sure how to handle the cargo commands generating files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pre-commit hook runs on local files, not on commited ones

2 participants