topgit icon indicating copy to clipboard operation
topgit copied to clipboard

needs rebase support

Open aspiers opened this issue 10 years ago • 17 comments
trafficstars

As mentioned in #38, topgit's third stated design tenet is a worthy goal:

(iii) TopGit is specifically designed to work in a distributed environment. You can have several instances of TopGit-aware repositories and smoothly keep them all up-to-date and transfer your changes between them.

However this focus seems to have resulted in a lack of focus for the opposite case, i.e. where your topic branches are mainly private. In this case, when a topic A changes, it makes more sense to rewrite the history of the other topics depending on that topic A, i.e. to update them via rebase rather than merge, since merge muddies the commit history unnecessarily.

In fact rebase sometimes even makes more sense in the use case of distributed, public topic branches too - as long as other consumers of those topics are suitably warned in advance of the expectation that history will be rewritten.

aspiers avatar Feb 09 '15 13:02 aspiers

+1000

lkraav avatar Feb 09 '15 13:02 lkraav

According to the original author, rebase support is explicitly excluded from the design :-(

aspiers avatar Feb 09 '15 13:02 aspiers

Yes, I think it would be a nice tool for keeping one's private history graspable.

I wonder whether tg export can do something similar:

To better understand the function of tg export, consider this dependency structure:

(where each of the branches may have a hefty history). Then

master$ tg export for-linus

will create this commit structure on the branch 'for-linus'

I'm not quite sure from the docs what would happen to the "hefty" history. Perhaps, it would be re-written in a nice form...

imz avatar Feb 18 '15 14:02 imz

Perhaps, this issue has a not so complex solution: the idea would be to substitute "git rebase BASE" for "git merge BASE" in tg-update for this option.

imz avatar Feb 18 '15 14:02 imz

I don't want to have to rely on tg export because that introduces an unnecessary extra step into the workflow. If I create a feature or bugfix topic branch, I want to be able to immediately push it to github when it's ready for creating a pull request from.

Yes, it might be easy to adapt the existing code to do rebase instead of merge. However we need to preserve the existing merge functionality, so I think a new tg rebase command would be the way to go.

aspiers avatar Feb 18 '15 15:02 aspiers

I don't think that tg export is really a big extra step compared to what you suggest: you have tg rebase as an "extra" step.

Only I'm not sure that tg export provides something like rebasing all branches cleanly. I had such impression from the README, but haven't done any experiments yet.

imz avatar Feb 19 '15 10:02 imz

I don't think that tg export is really a big extra step compared to what you suggest

I didn't say it was a big extra step. But it's an extra step, and that's unacceptable considering how often I would need to do it.

you have tg rebase as an "extra" step.

Nope, rebasing is a different part of the workflow to submitting pull requests.

aspiers avatar Feb 19 '15 11:02 aspiers

I hacked tg-update to make it posisble to rebase (for the brave users). My work is in https://github.com/imz/topgit/compare/tg_update-custom-merge?expand=1.

But now, as I was pushing this, I had a thought about a possible issue I had not thought when implementing it:

What happens if git rebase stops to ask the user to resolve conflicts?

Well, thinking a bit about this issue, it appears to me that everything needed to handle this case must already be in the code, because git merge could also stop to ask to resolve conflicts. The user simply completes the operation, and the new HEAD ref is OK. (As for recursive updates, the code to handle this in subshells is also already there!)

Testing it in a simple case, it works! (Read my small changes to understand it better.)

$ tg update --this-with 'git rebase'
tg: Updating base with tg_rename changes...
Updating ee884ee..8436705
Fast-forward
README                            |  3 ++-
 tg-rename.sh => tg-rename-leaf.sh | 44 +++++++++++++++++++++++++++++++++++---------
 2 files changed, 37 insertions(+), 10 deletions(-)
 rename tg-rename.sh => tg-rename-leaf.sh (56%)
tg: Updating tg_rebase against new base (with the cmd given by TG_MERGE, by default: git merge...
First, rewinding head to replay your work on top of it...
Applying: (tg) create
Applying: (docs) A plan for what tg-rebase-leaf.sh should do.
Applying: A plan to implement it with a hack in tg update.
$ 

imz avatar Feb 24 '15 11:02 imz

The branch with my work got renamed: https://github.com/imz/topgit/compare/custom-merge?expand=1.

I've discovered that not only tg update needs support for using a custom merge command, but also tg create (implemented), and tg depend add (not yet taken into account in my patches). (Hence the new name for the branch.)

  • I'm quite happy now with tg update --this-with 'git rebase --preserve-merges', which allows me not to clutter the branches I'm currently working on with numerous merges!
  • I haven't yet tried to use tg {update,create} --this-with 'git merge -s ours -m "merge -s ours"' extensively (untested). (As wanted in https://github.com/greenrd/topgit/issues/42, shouldn't bother those who are interested in rebasing only.)
  • I haven't tried (not that I remember) the recursive rebase with TG_MERGE='git rebase --preserve-merges' tg update [--all].
  • tg depend add should also get support for the custom merge (not implemented).

Please try it out.

imz avatar Feb 25 '15 17:02 imz

As an aside, now I have a wish to beautify some places in the history of my TopGit branches, where merges were done before rebasing was available to me with this mechanism (so having no other alternative and being in a hurry, I used the standard merge of tg update).

(That's not appropriate when a new feature was added in a dependency, because an extra merge symbolizes that the code worked without the new feature before the merge, and now uses the feature after the merge. Or if that stuff got "released" to the public already. But it's wanted to get rid of the merges if I simply amended something in the dependency.)

Probably, quite possible for fresh merges--probably, by manually moving the refs, and manually rebasing the relevant pieces of history.

Don't know yet what kind of automation would be of help in this task.

imz avatar Feb 25 '15 17:02 imz

I've reviewed the commits. One extra piece of feedback is that I'm not convinced that unifying merge and rebase operations under the same UI makes sense. I'm sure it makes the implementation of rebase support easier, but it provides an ugly UX. It would be more intuitive if the user could just type tg rebase. But it raises the question: would it ever make sense to switch handling of one topic branch from merge to rebase, or vice-versa?

aspiers avatar Feb 28 '15 16:02 aspiers

Would it ever make sense to switch handling of one topic branch from merge to rebase, or vice-versa?

Ah, I should have read your last comment first, where you explain that this does make sense :)

aspiers avatar Feb 28 '15 16:02 aspiers

I am hoping to work on this as part of my Hack Week.

I just noticed that @deepfire already made an independent attempt to add rebase support ...

aspiers avatar Apr 13 '15 15:04 aspiers

@aspiers, I have actually been using this code, and it proved to be useful -- until I've hit another peculiarity of topgit, which made me abandon it.

Namely, I have failed to figure out how to deal with completely merged branches -- AFAIUI, the "annihilated" branches ought to be automagic, but weren't, and I hadn't time to figure out why.

Perhaps my additions broke it.

deepfire avatar Apr 13 '15 15:04 deepfire

Thanks for the info. I might get a chance to take a look at your changes.

aspiers avatar Apr 14 '15 16:04 aspiers

I almost haven't been using this code since then, but today I gave it another try in a practical situation, and have noticed an unwanted ugly thing as a result of my implementation of rebase with TG_MERGE='git rebase -p' (perhaps, unwanted only in this specific case, but not in some other similar cases -- different scenarios with different intentions can make thinking about the correct behavior difficult).

I have rebased several interdependent branches: some of them on top of other ones; and in the "middle level" of these layers of branches there was an "octupus" branch merging several patch branches with commonly useful patches. Let's call it master_imz. Then I use it as a base for my feature branches.

         /-- fixA -\                /- featureQ
master -<           >- master_imz -<
         \-- fixB -/                \- featureR

And the ugly thing was that fixA and fixB rebased cleanly, but the top-base for master_imz merged the old top-base for it, and hence--when looking at the overall structure of the branches-- master_imz itself had THREE ancestors (not immediate, but the old pre-rebase history was included). Including the pre-rebase history wasn't my wish in this case.

The same happened with the top-bases for featureQ and featureR: they were not re-written, but rather they remained and merged the new updated branches into them.

So, I should think more about how to treat the top-bases cleanly in this case...

(In the pre-rebase state, the top-base for master_imz was a merge of two branches fixA and fixB, and hence there is no clear indication whether there is some content in that commit that shouldn't be lost.)

imz avatar Jul 03 '15 12:07 imz

The problem with the top-bases vs rebase is difficult to tackle without having a specification of the semantics of a top-base.

I suspect its purpose is to be a merge of all the dependencies of a branch.

If so, when doing a rebase (of a dependency) we should re-create cleanly the merge with the new rebased branch.

So under this view, a rebase of a TopGit branch must trigger the re-writing of all the top-bases which use it. Something not done now, correct? Now the updates can be "lazy", until you are interested in a specific branch.

Well, this can be post-poned.

But now I come to the following conclusion: when rebasing a topgit branch, its top-base should be recreated. Rebasing means re-writing the history, so re-writing the top-base is not bad in this case.

Now, I believe this is the main point in rebase support for TopGit, and it's not implemented in my patches.

imz avatar Jul 03 '15 13:07 imz