Add a "branch" or "switch branch" command. #74

Closed
rdunklau opened this Issue Feb 4, 2013 · 15 comments

2 participants

@rdunklau

When working on multiple VCS branches, it is not always pratical to switch from one branch to another, one has to:

  • identify the latest common change
  • revert to that change
  • switch to the branch we want to get (git checkout mybranch)
  • deploy up to the latest change in that branch.

This could be streamlined by the use of a sqitch command that would perform all of the above mentioned steps.

This is related to #25.

You can find a POC implementation here: 7091dfda7a070f3efc5235d2be242ad78cfcce17

This implementation:

  • is hard wired to git (ie, no vcs abstraction layer)
  • uses Git::Wrapper to execute git commands
  • "Refactored" (read: performed an ugly hack on) the plan class so as to be able to use an "in memory file")
  • is probably horrible, since this is my first attempt at writing perl code.
@theory
Owner

Oh man, I love this. Thank you!

A few notes:

  • The Perl is not too bad, nice!
  • This nicely points the way for getting started on an abstracted VCS/SCM implementation
  • I think checkout would be a better command name than branch
  • I really like how you added the in-memory file handle stuff, great hack.

Anyway, now you've got my head buzzing and I need to start thinking about how to integrate something like this. I mean, I think it would be reasonable to go ahead and apply this as a first cut that can be refactored into a VCS abstraction layer the next time we want to do something like this. I don't have the time to work on it right now, even though you now have made me want to quite hard. :-)

Anyway, I can apply this if you would:

  • Rename the command to checkout (if you agree)
  • Remove the hard-coded Git repo directory location. :-) Would . work?
  • Add docs (lib/sqitch-checkout.pod and lib/sqitch-checkout-usage.pod
  • Tests would be nice, too!

Thanks, this is awesome!

David

@rdunklau

Remove the hard-coded Git repo directory location. :-) Would . work?

I don't know what you mean, the git repo has always been hard-coded to "." :-).
I have taken some liberty to rewrite the history, and eliminated the merge commit altogether.

Rename the command to checkout (if you agree)

The name checkout seems to be really git-centric, and seems to confuse many people since the command does so many different things, so I wanted to use another one. But I'm not willing to argue over a name, so if you prefer checkout, so be it !

Add docs (lib/sqitch-checkout.pod and lib/sqitch-checkout-usage.pod
Tests would be nice, too!

I'll see what I can do about that, but I'm afraid I won't have much time these days.

@theory
Owner

I don't know what you mean, the git repo has always been hard-coded to "." :-).

Could have sworn…

I have taken some liberty to rewrite the history, and eliminated the merge commit altogether.

Ah ha, you sneaky devil!

The name checkout seems to be really git-centric, and seems to confuse many people since the command does so many different things, so I wanted to use another one. But I'm not willing to argue over a name, so if you prefer checkout, so be it !

Well, to me, branch means "create a branch". I would be okay with another term, though. Maybe switch? Might look funny, though, sqitch switch. :-)

I'll see what I can do about that, but I'm afraid I won't have much time these days.

You can copy from, say, the deploy or rebase command POD files. Should take much less time than it took to write the code. :-)

@theory
Owner

Maybe sqitch change-branch. Or just cb.

Thinking further on checkout, it may be Git-specific, but since in a system like SVN you wont' be able to change branches at all without changing directories, I think it would work okay. But whatever. Maybe I'll blog it and ask for bike-shedding.

@rdunklau

See the latest patch here: 9092a52
From the previous patch:

  • some documentation has been added, mostly copy pasted from rebase.
  • the checkout command accepts the same options as the rebase command, more or less (log only, set/deploy/revert variables, verify, mode).
  • the file management (in-memory vs plain file) has been cleaned, and should now work as expected when performing subsequents load. This involves removing the plan_fh attribute, and testing the type of the plan_file itself.

Regarding the options, I see that with checkout, there is a growing number of commands that share the same options. This could be refactored to avoid code duplication. I have no idea how to do that in Perl, unfortunately. Maybe some kind of multiple inheritance / mixins ?

This patch does not provide tests for now, as I'm not really sure about how to test the git integration.

My ideas so far, not mutually exclusive:

  • copy a git repo/submodule containing various branches to a temp directory and work from here
  • implement an abstraction layer for the VCS, and use a mock class for this

What do you think ?

@theory
Owner
  • the file management (in-memory vs plain file) has been cleaned, and should now work as expected when performing subsequents load. This involves removing the plan_fh attribute, and testing the type of the plan_file itself.

Oh, I had liked that attribute. What was the issue with subsequence loads?

Regarding the options, I see that with checkout, there is a growing number of commands that share the same options. This could be refactored to avoid code duplication. I have no idea how to do that in Perl, unfortunately. Maybe some kind of multiple inheritance / mixins ?

Yeah, probably a role. Basically, put all the common code into a single class that uses Mouse::Role rather than Mouse (or Mouse). Then load it into each class with the with key word, and the methods will be mixed in.

  • copy a git repo/submodule containing various branches to a temp directory and work from here
  • implement an abstraction layer for the VCS, and use a mock class for this

What do you think ?

I like the first option. We would just have the test skip_all if Git::Wrapper->has_git_in_path returns false.

@rdunklau

Oh, I had liked that attribute. What was the issue with subsequence loads?

The problem was that when calling load a second time (as is done during the tests), the file would be reparsed but not read again. This brought to my attention that if someone sets the plan_file attribute while leaving the plan_fh untouched, it was not clear what should be done.

The plan_file attribute can be either a string to be used as a file name, or a file handle directly.

Yeah, probably a role. Basically, put all the common code into a single class that uses Mouse::Role rather than Mouse (or Mouse). Then load it into each class with the with key word, and the methods will be mixed in.

I'll look into that.

I like the first option. We would just have the test skip_all if Git::Wrapper->has_git_in_path returns false.

Ok, I'll try to implement this.

@theory
Owner

Great, thanks! I think it's getting close; I can probably work on integrating what you come up with in the second half of next week.

@rdunklau

Sorry, I didn't have much time the past couple of weeks, but my repo has now been updated to add some tests.

These are very basic:

  • a git repo is included in t/git/checkout
  • the test clones it to a temp directory
  • the basic functionality is tested against regressions.

The test pass with prove, but not with dzil test: in the latter case, it can't find the git repo to clone.
Would you have any idea why they behave differently ?

Stay tuned for the refactoring part :)

@theory
Owner

Sweet, thanks!

I've been busy too, so no big deal.

@rdunklau

I have begun the work on refactoring the commands configuration.

The command options roles attach "before" method modifiers to the configure method.
Those modifiers will fill a new argument of the configure method, which act as a parameter "accumulator".

It is then the responsibility of each command to merge this accumulator to the parameters they extract from the configuration and the option themselves.

The current patch provides two roles, deploy_variables and revert_variables, which are used in the checkout and rebase commands.

If the proposed approach works for you, other options may be refactored into their own roles.

Minus the issue mentioned before about the difference in behavior when running dzil test and prove, all the test pass.

@theory
Owner

Looks good. You going to submit a pull request for this work?

@theory
Owner

Merged your changes in in a96c853. Thank you very much!

@theory theory closed this Mar 25, 2013
@rdunklau

Thank you for merging and cleaning this. Can't wait to see a release including it !

Just one note on the test involving the git repo: it worked for me when running the tests with prove, but not with dzil test. Anyway, it's probably better to mock this.

@theory
Owner

Weird. Yeah, there are only three methods called, so mocking works very nicely.

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