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

Ensure down migrations work with GIT_WORK_TREE #12

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Contributor

mal commented Dec 12, 2012

tl;dr: this patch enables hooks to run with a pwd within, but deeper than the root of the containing git repo.


So I found this at the same time as the GIT_DIR thing, but it was seemingly unrelated hence a separate PR.

The problem
... started with changing the pwd inside the hook, such that it was no longer the repository root. In order for the git commands inside hookup to still work GIT_WORK_TREE had to be exported as the repository root.

The difficulty is that git diff <sha1> -- migrations/* returns output that's relative to the repository root, rather than pwd.

~/repo/subdir$ git diff --name-status 53ddb9... beb5097... -- migrations/*
M       subdir/migrations/mig1.rb

When it comes time to checkout that future migration so it can be reverted, something like this happens:

~/repo/subdir$ git co 53ddb9... -- subdir/migrations/mig1.rb
error: pathspec 'subdir/migrations/mig1.rb' did not match any file(s) known to git.

The solution
... is to join the repository root to the diff output to create an absolute path, and then convert that path relative to pwd

absPath = File.join("/path/to/repo", "subdir/migrations/mig1.rb") #=> "/path/to/repo/subdir/migrations/mig1.rb"
path = Pathname.new(absPath).relative_path_from( ... pwd ... ) #=> "migrations/mig1.rb"

This new relative path can then be used to checkout the future migration and revert it.

@mal mal referenced this pull request Dec 13, 2012

Closed

Ignore GIT_DIR while in gemspec #11

Contributor

mal commented Jan 4, 2013

@tpope any pointers on this? Happy to make any changes you deem necessary.

Owner

tpope commented Jan 4, 2013

I'm confused about how this works and haven't had a chance to dive in. Why isn't the repository root the current working directory? I thought this was a universal truth.

Contributor

mal commented Jan 4, 2013

The code effectively rebases the migrations path from a "repository root" relative path, into a pwd relative path, such that git commands run from pwd still find the files referenced.

As for the why; it's mostly down to the volume and breadth of the databases involved, and some logic in the hook so as to only migrate databases that are important to the branch being checked out.

Owner

tpope commented Jan 4, 2013

I mean I don't understand why Dir.getwd doesn't always equal git rev-parse --top-level. I take it this has something to do with GIT_WORK_TREE but I don't understand how it fits in.

Contributor

mal commented Jan 4, 2013

Ahh, sorry. The GIT_WORK_TREE setting is needed so that when hookup runs git diff --name-status #{old} #{new} -- #{schema} it gets the correct results, despite running from a subdirectory. Without this hookup never sees a change in schema.rb and as result never attempts to run any migrations.

Contributor

mal commented Jan 10, 2013

As to the reason for pwd not being the repo root in the hook; that's because the hook had to be augmented to change directory to where the Gemfile and Rakefile are (they're not in the repo root, not my call sadly).

Owner

tpope commented Jan 11, 2013

Okay, now that makes sense. I feel like it would be better to keep the directory the root, and pass in the subdirectory hookup -C the/app (-C being the same way Ruby itself does it). What do you think?

Contributor

mal commented Jan 11, 2013

That makes sense; I gave it a try here: https://github.com/mal/hookup/compare/feature/chdir-option. If it looks good to you I'll file a seperate PR and we can get rid of this one.

Only problem I have using it is that when I use bundler exec hookup from the hook, it doesn't find subdir/Gemfile. I can set the BUNDLE_GEMFILE=subdir/Gemfile environment variable which works; however then when hookup tries to use bundler exec rake (inside subdir) it ends up looking for subdir/subdir/Gemfile.

I think that will be solved once a release is cut and I can resume using a direct call to hookup in the hook, but it's worth noting in case it comes up in future (or you can think of a nice solution).

Owner

tpope commented Jan 12, 2013

Oh hmm, I didn't consider the bundle exec hookup case. I think I'm okay with that.

Does -Cdirectory allow a space after -C? If so, I think this is good to go.

Contributor

mal commented Jan 12, 2013

I copied the -Cdirectory style from the ruby binary, but I had to write a small test case to convince myself of the same query you raised:

require 'optparse'
opts = OptionParser.new
opts.on('-Cdirectory', 'test option') do |dir|
  puts dir
end
opts.parse ['-C', 'some/directory/name/using/space']
#=> some/directory/name/using/space

It works as expected, so I'm closing this PR and I'll open a new one from the other branch.

@mal mal closed this Jan 12, 2013

@mal mal deleted the mal:feature/support-git-work-tree branch Jan 13, 2013

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