phpfu / puphpet-installer

Provides a composer custom installer that works with `loadsys/puphpet-release` to add a PuPHPet.com vagrant box to a project via composer.
MIT License
2 stars 1 forks source link

Properly handle consuming projects without a .gitignore #10

Closed deizel closed 9 years ago

deizel commented 9 years ago

Found this during my testing, a nice big red error if the consuming project doesn't have a .gitignore file.

 ❯ composer update
Loading composer repositories with package information
Updating dependencies (including require-dev)
  - Installing symfony/filesystem (v2.6.6)
    Loading from cache

  - Installing loadsys/puphpet-release-composer-installer (0.0.4)
    Loading from cache

  - Installing loadsys/puphpet-release (dev-master 0d02e12)
    Cloning 0d02e12455a9f1c6140705ff3bd7930607a68ac7

  [ErrorException]
  file(/private/tmp/puppet-release-test/.gitignore): failed to open stream: No such file or directory

Using PHP's touch() prevents this error from occurring.

beporter commented 9 years ago

I'd like to discuss the behavior here a bit.

If .gitignore file is already present, it's safe to assuming that the consuming project is using git, and thus we should be helpful and add the necessary entries. So far, so good.

However, if no .gitignore is present, the consuming project may be using git, or they could be using Mercurial or Subversion or nothing at all. While I'm content not adding support for exclusion mechanisms used by other version control systems until there is a demand for them, I'm not sure we should presume to add a potentially unnecessary .gitignore file to the consuming project.

There are a few solutions I can think of:

In any case, it's clear that the try/catch block that was intended to already handle the case of a missing .gitignore file is not sufficient, since file() seems to throw an error instead of an exception. I think no matter what we need to clean that up.

Does anybody have any thoughts they'd like to contribute?

justinyost commented 9 years ago

I'm in favor of option 3:

beporter commented 9 years ago

I'm inclined to agree. What do you think, @deizel? Would you be willing to rework your PR? Otherwise we can close this one and handle the change ourselves. Either way we're grateful for you putting some attention towards this.

deizel commented 9 years ago

Sounds like a plan - I've modified the PR to check for a .git/ directory before proceeding.

Note that Git is afraid of .git/, so I've modified the test script to create the directory instead:

$ git add .git/empty
error: Invalid path 'tests/integration/.git/empty'
error: unable to add tests/integration/.git/empty to index
fatal: adding files failed

If you think there are any improvements to make before merging let me know.

beporter commented 9 years ago

Looks pretty nice to me. Thank you!

deizel commented 9 years ago

No problem. :smile: