Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Sanity-check :clean-targets #1458

Closed
cemerick opened this Issue Feb 24, 2014 · 7 comments

Comments

4 participants
Collaborator

cemerick commented Feb 24, 2014

Right now, you can put anything in :clean-targets, including clearly absurd things like "..", "/", and so on.

I'd like to suggest that we do some minimal sanity-checking before deleting anything. As a (hopefully) uncontroversial straw-man, perhaps :clean-targets should be limited to:

  • paths within the current project root
  • paths that aren't within known source roots

FWIW, context.

Discuss?

Owner

technomancy commented Feb 24, 2014

Seems pretty reasonable; happy to take a patch for this.

Contributor

cpmcdaniel commented Feb 25, 2014

I'll take a stab at this. Should it complain loudly, or ignore the invalid targets?

Owner

technomancy commented Feb 25, 2014

Thanks! It should definitely just abort.

cpmcdaniel added a commit to cpmcdaniel/leiningen that referenced this issue Feb 26, 2014

cpmcdaniel added a commit to cpmcdaniel/leiningen that referenced this issue Feb 28, 2014

Contributor

cpmcdaniel commented Mar 1, 2014

About ready to submit a pull. Some tests are failing in the repl namespace, but I don't think this is my doing:

lein test :only leiningen.test.repl/test-connect-string

FAIL in (test-connect-string) (repl.clj:82)
expected: (re-find #"Port is required" (lthelper/abort-msg connect-string {} []))
actual: (not (re-find #"Port is required" ""))

lein test :only leiningen.test.repl/test-connect-string

FAIL in (test-connect-string) (repl.clj:82)
expected: (is (re-find #"Port is required" (lthelper/abort-msg connect-string {} [])))
actual: nil

I'll spend a little bit of time confirming this (checking out an old rev and making sure the same tests fail) before I submit the pull.

cpmcdaniel added a commit to cpmcdaniel/leiningen that referenced this issue Mar 1, 2014

Contributor

cpmcdaniel commented Mar 1, 2014

I've confirmed that the two test failures are from before I touched the code. Sending the pull request...

cpmcdaniel added a commit to cpmcdaniel/leiningen that referenced this issue Mar 3, 2014

cpmcdaniel added a commit to cpmcdaniel/leiningen that referenced this issue Mar 4, 2014

Support for overriding :clean-targets sanity checking (issue #1458).
Also made unit tests safer and faster using with-redefs to mock out
calls to delete-file-recursively.
Better error messages when a path is being protected.

technomancy added a commit that referenced this issue Mar 4, 2014

Merge pull request #1461 from cpmcdaniel/master
Fix for #1458: clean-targets sanity checks.
Contributor

cpmcdaniel commented Mar 6, 2014

Can this be closed now?

Collaborator

hypirion commented Mar 6, 2014

Right, this should be closed, as #1461 is merged in.

@hypirion hypirion closed this Mar 6, 2014

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