Rewrite .info files inside drush make when checking out from git to include version and project info

To clarify, this is absolutely necessary when drush make is pulling from Git and run without the --working-copy flag, since you neither have the benefit of the d.o packaging script's .info file rewriting from tarballs, not any possibility that git_deploy can work, since the Git data is destroyed before make is done. drush make itself is the only thing that can make update manager work in this case.

However, moshe pointed out in IRC that *always* rewriting the .info files, even if you're doing --working-copy might be annoying, since then you get all these superfluous git diffs in your workspace.

So, we probably want a kill switch for this .info rewriting, and we should consider if that killswitch should always be triggered with --working-copy...

Bump. The issues in #1 are both done and merged into drush's master branch. I *believe* all we'd need to do for this is ensure that if a given project is configured to use the 'git' download type, that we add --gitinfofile to the options used when invoking drush pm-download concurrently. However, poking around the code, it's not obvious how to do that. ;) The patch here is just a placeholder for where I believe the check should happen, but I don't know what to do inside the if clause to actually set the option we need for this particular project's concurrent invocation. There's a lot of mojo going on with propagating options into the concurrent backend and I don't grok all of that.

Maybe this is the wrong place to be trying to define and propagate that option. Perhaps we want to do it inside make_download_git()? In fact, given make_download_git(), I don't really see how the pm download code is being used at all here. Maybe that conversion is still incomplete. In that case, seems like we just need to reuse the code to munge the .info files directly inside make_download_git()?

Anyway, it'd be nice to get this wrapped up soon so that we can allow -dev releases in distros packaged on d.o. I'm happy to work on this if I had some clear direction on where to make this happen.

Thanks,
-Derek

Great, thanks for the clarification. I didn't notice #1375270: Drush Make should use Drush core's native download abilities for git clones. I'll take a stab at this now and see how far I get.

I got pulled into other things, but I came back to this and worked out the attached patch. It's a bit of a hack in some ways, but hopefully I'm making up for it by cleaning up some of the code and logic in other ways. ;) I got some pointers from Moshe on where he wanted various pieces of this code to live, so hopefully this can go in for now and all be revisited once we're really working on #1375270.

Patch summary:
- Added a new shared helper function drush_file_append_data() to includes/filesystem.inc
- Totally refactored, cleaned up, renamed, and moved package_handler_inject_info(). It now lives in commands/pm/pm.drush.inc as drush_pm_inject_info_file_metadata(). Fixed it to only generate the info metadata string once, then iterate over all the .info files using drush_file_append_data() to append that little hunk of .info metadata to each one. Also, instead of taking PM-specific opaque arrays as input and only using certain keys out of them, it now just takes 3 self-documenting parameters for its input.
- make_download_git() now tries to figure out a version string to use based on the .make contents, and uses drush_pm_inject_info_file_metadata() to inject .info data unless the --no-gitinfofile killswitch is set.
- Fixed makeTest.php to set this killswitch on the two git makefile tests that do md5 comparisons.

Some things to discuss about the option/killswitch:

A) Should it be a --no- style killswitch like the other make options, or should it be opt-in like the --gitinfofile option in pm-download?

B) How should it interact with --working-copy?

IMHO, we *must* rewrite the info files if --working-copy isn't defined (unless the user specifically tells us not to). So, since --working-copy defaults to FALSE, I think this needs to default to TRUE instead of being opt-in. And given it needs to be opt-out, seems like a --no is best. At least that was my rationale for writing it this way. ;)

Thanks
-Derek

p.s. With the patch, I'm getting:

% phpunit makeTest.php
...
Time: 45 seconds, Memory: 4.75Mb
OK (16 tests, 31 assertions)

% phpunit pmDownloadTest.php
...
Time: 7 seconds, Memory: 4.75Mb

OK (4 tests, 28 assertions)

Sweet, thanks!

However, I realized there's a problem here:

      // Figure out the proper version string to use based on the .make file.
      // Best case is the .make file author told us directly.
      if (!empty($download['version'])) {
        $version = $download['version'];
      }

Actually, 'version' is a project-level attribute, not a download-level attribute. I.e. in the .make file, you say this:

projects[views][version] = 3.1

Not this:

projects[views][download][version] = 7.x-3.1

This is particularly an issue in combination with #1371306: Add validation to ensure that if a .make file includes a git hash, it also defines a branch. At least for the d.o distro packaging case, if .make authors define a 'revision' (to checkout from a specific git hash) we're requiring they define a version so we can rewrite the .info files appropriately (since a given hash might live on many branches, we don't know exactly what they're intending). However, that's just in the project data, not in the download data, so it's never getting passed to make_download_factory(). :( I'm not 100% sure what to do about this, other than perhaps inside DrushMakeProject::download(), before we call make_download_factory(), we should copy the version info (if we've got it) out of $this->version and put it into $this->download['version'] (or something). While we're at it, that's probably a reasonable place to prepend the core version (e.g. the '7.x' part) so that the version string looks like we expect.

The whole way make handles versions is a bit weird, but a more far-reaching refactoring is obviously way out of scope here.

Thoughts?

Thanks!
-Derek

For consideration, here's what I'm talking about. This actually works, although it makes me feel dirty inside.

Ugh. Maybe #9 isn't the right approach either. Digging into the pm case, I found related badness. See #1267228-73: Drush Make should use Drush core's native download abilities concurrently for more. But, I think #9 has the potential to break the pm case. I believe this is safer.

New patch based on #1267228-74: Drush Make should use Drush core's native download abilities concurrently.

Re-rolled for #1267228-79: Drush Make should use Drush core's native download abilities concurrently

Ugh, sorry. Clearly we need to write some tests for all of these edge cases. But, I found a problem with how we handle certain -dev versions. My regexp was too permissive and when I was looking for just 1.x I was also matching 1.x-dev since I didn't include the $ to ensure that we're hitting the end of the string. This resulted in version strings like '7.x-1.x-dev-dev' in some cases. Now fixed.