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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to forced checkout and untracked files #4260

Merged
merged 4 commits into from Jun 11, 2017

Conversation

ethomson
Copy link
Member

@ethomson ethomson commented Jun 8, 2017

A riff on #4129, with the changes discussed therein. Opening a new PR to make sure that CI is happy before I merge it. 馃槃

@ethomson ethomson mentioned this pull request Jun 8, 2017
@ethomson
Copy link
Member Author

ethomson commented Jun 8, 2017

Hrmf, looks like I did something stupid here. 馃憖

@pks-t
Copy link
Member

pks-t commented Jun 9, 2017

I cannot investigate right now, got no Windows VM handy -- I could do so next week, probably, if you've not gotten around to do it by then.

By the way -- feel free to drop the commit containing my previous solution.

pks-t and others added 3 commits June 10, 2017 19:16
If the `GIT_CHECKOUT_FORCE` flag is given to any of the `git_checkout`
invocations, we remove files which were previously staged. But while
doing so, we unfortunately also remove unstaged files in a directory
which contains at least one staged file, resulting in potential data
loss.

This commit adds two tests to verify behavior.
When deleting a directory during checkout, do not simply delete the
directory, since there may be untracked files.  Instead, go into
the iterator and examine each file.

In the original code (the code with the faulty assumption), we look to
see if there's an index entry beneath the directory that we want to
remove.   Eg, it looks to see if we have a workdir entry foo and an
index entry foo/bar.txt. If this is not the case, then the working
directory must have precious files in that directory. This part is okay.
The part that's not okay is if there is an index entry foo/bar.txt. It
just blows away the whole damned directory.

That's not cool.

Instead, by simply pushing the directory itself onto the stack and
iterating each entry, we will deal with the files one by one - whether
they're in the index (and can be force removed) or not (and are
precious).

The original code was a bad optimization, assuming that we didn't need
to git_iterator_advance_into if there was any index entry in the folder.
That's wrong - we could have optimized this iff all folder entries are
in the index.

Instead, we need to simply dig into the directory and analyze its
entries.
Only ignore `EBUSY` from `rmdir` when the `GIT_RMDIR_SKIP_NONEMPTY` bit
is set.
@ethomson
Copy link
Member Author

Previous to this change, we would try to seize the opportunity to efficiently remove directories when we thought that we wanted to remove all of the files inside them. But we could not effectively do that, and risked data loss. This removes that optimization, and makes us process the items inside the directory. By moving the directory deletion into standard call path, we clean up directories that are made newly empty (as a result of deleting the files inside them).

This change illustrated another failure: the option GIT_RMDIR_SKIP_NONEMPTY allows one to skip rmdir failures when the directory is in-use (on Windows). This option was tested in the optimized delete-the-whole-directory-at-once case but never tested the common remove-the-directory-after-all-its-files-are-gone case. The latter always enabled this setting, so it would not fail when it could not remove a directory due to EBUSY. Moving the checkout logic to always use the latter showed this failure.

So the last commit teaches the rmdir case to fail when EBUSY and GIT_RMDIR_SKIP_NONEMPTY is omitted. This brings us in line with the promises made by the documentation and should not be a regression.

Callers on Windows should probably be using GIT_CHECKOUT_SKIP_LOCKED_DIRECTORIES.

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.

None yet

2 participants