When dest dir is specified and clone breaks for any reason, don't remove the dest dir #1421
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you! However, you seem to have squashed the patches into a single one, without the commit messages, and changing the author attribution... I had the hunch that @peff published the patch series in his fork, too. And I was right... So it would probably be better to rebase Sorry for wasting your time, I think I can do that myself, too. |
Back when this test was written, git-clone could not handle a repository without any commits. These days it works fine, and this comment is out of date. At first glance it seems like we could just drop this code entirely now, but it's necessary for the final test, which was added later. That test corrupts the repository my temporarily removing its objects, which means we need to have some objects to move.
This is an old script which could use some updating before
we add to it:
- use the standard line-breaking:
test_expect_success 'title' '
body
'
- run all code inside test_expect blocks to catch
unexpected failures in setup steps
- use "test_commit -C" instead of manually entering
sub-repo
- use test_when_finished for cleanup steps
- test_path_is_* as appropriate
Two parts of git-clone's setup logic check whether a
directory exists, and they both call stat directly the same
scratch "struct stat" buffer. Let's pull that into a helper,
which has a few advantages:
- it makes the purpose of the stat call more obvious
- it makes it clear that we don't care about the
information in "buf" remaining valid
- if we later decide to make the check more robust (e.g.,
complaining about non-directories), we can do it in one
place
Note that we could just use file_exists() for this, which
has identical code. But we specifically care about
directories, so this future-proofs us against that function
later getting more picky about actual files.
Once upon a time, git-clone would refuse to write into a directory that it did not itself create. The cleanup routines for a failed clone could therefore just remove the git and worktree dirs completely. In 55892d2 (Allow cloning to an existing empty directory, 2009-01-11), we learned to write into an existing directory. Which means that doing: mkdir foo git clone will-fail foo ends up deleting foo. This isn't a huge catastrophe, since by definition foo must be empty. But it's somewhat confusing; we should leave the filesystem as we found it. Because we know that the only directory we'll write into is an empty one, we can handle this case by just passing the KEEP_TOPLEVEL flag to our recursive delete (if we could write into populated directories, we'd have to keep track of what we wrote and what we did not). Note that we need to handle the work-tree and git-dir separately, though, as only one might exist (and the new tests in t5600 cover all cases). Reported-by: Stephan Janssen <[email protected]>
dscho
force-pushed
the
fix-clone-removing-dest
branch
from
e862624 to
b50a218
Compare
|
The interdiff looks like this: diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh
index 5cd94d55582..4a1a912e032 100755
--- a/t/t5600-clone-fail-cleanup.sh
+++ b/t/t5600-clone-fail-cleanup.sh
@@ -89,7 +89,7 @@ test_expect_success 'failed clone into empty leaves directory (separate, git)' '
test_path_is_missing no-wt
'
-test_expect_success 'failed clone into empty leaves directory (separate, git)' '
+test_expect_success 'failed clone into empty leaves directory (separate, wt)' '
mkdir -p empty-wt &&
corrupt_repo &&
test_must_fail git clone --separate-git-dir no-git foo empty-wt &&I currently use VSTS to run the test suite on Windows, Linux and macOS. Once all of them pass, I shall merge. |
dscho
added a commit
to git-for-windows/build-extra
that referenced
this pull request
Jan 5, 2018
When cloning into an existing (empty) directory fails, [Git no longer removes said directory](git-for-windows/git#1421). Signed-off-by: Johannes Schindelin <[email protected]>
dscho
added a commit
that referenced
this pull request
Jan 18, 2018
When dest dir is specified and clone breaks for any reason, don't remove the dest dir
git-for-windows-ci
pushed a commit
that referenced
this pull request
Jan 20, 2018
When dest dir is specified and clone breaks for any reason, don't remove the dest dir
git-for-windows-ci
pushed a commit
that referenced
this pull request
Jan 22, 2018
When dest dir is specified and clone breaks for any reason, don't remove the dest dir
git-for-windows-ci
pushed a commit
that referenced
this pull request
Jan 22, 2018
When dest dir is specified and clone breaks for any reason, don't remove the dest dir
dscho
added a commit
that referenced
this pull request
Jan 22, 2018
When dest dir is specified and clone breaks for any reason, don't remove the dest dir
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(Patch series provided by Jeff King [email protected])