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

Fix issue 518 #547

Merged
merged 3 commits into from
Nov 20, 2014
Merged

Fix issue 518 #547

merged 3 commits into from
Nov 20, 2014

Conversation

ceh
Copy link
Contributor

@ceh ceh commented Nov 6, 2014

Don't walk the destination path when it is a subdirectory of the source.

Fixes issue 518.

@sethvargo
Copy link
Contributor

Hey @ceh. Thank you for the PR. This looks like it will definitely fix #518! Do you think you could write a test case for this? It should be fairly easy to recreate the previous scenario and assert the recursion does not happen. Let me know if you need some help.

@sethvargo sethvargo added the core label Nov 19, 2014
@ceh
Copy link
Contributor Author

ceh commented Nov 20, 2014

There's two commits in the PR. Did you see 82e4bab "command/init: test for issue 518"?

@@ -100,3 +100,45 @@ func TestInit_noArgs(t *testing.T) {
t.Fatalf("bad: \n%s", ui.OutputWriter.String())
}
}

// https://github.com/hashicorp/terraform/issues/518
func TestIssue518(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's name this something more semantic like "TestInit_dstInSrc". We can still link to the issue, but it's useful to have semantic names for these types of things (imagine if GitHub is down for example)

@sethvargo
Copy link
Contributor

@ceh weird. I definitely didn't see that yesterday 😄. I made one small comment around test naming, but otherwise 👍

@ceh
Copy link
Contributor Author

ceh commented Nov 20, 2014

You might have missed it due to that #518 only shows a76290f.

On a side note, do you prefer pull requests to be squashed into a single commit?

sethvargo added a commit that referenced this pull request Nov 20, 2014
@sethvargo sethvargo merged commit 94e1eac into hashicorp:master Nov 20, 2014
@sethvargo
Copy link
Contributor

This is fine 😄
Thanks!

@ceh ceh deleted the issue-518 branch November 20, 2014 18:40
@ghost ghost locked and limited conversation to collaborators May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants