Refactoring git-rebase.sh and git-rebase--interactive.sh

(Resending as plain text. Sorry about the spam to the guys on the CC list.)

Hi,

I have now been using Git for something like 18 months, and I think it's about time that I try to contribute.

So, after adding some features to git-rebase.sh (which I will send separate mails about), I realized I would have to add them to git-rebase--interactive.sh as well. Rather than doing that, I would prefer to first extract the common parts of these scripts and add the features in only one place. Since this is the first time I do anything on Git, I will need a lot of advice.

My main goal is to extract the commonalities in command line parsing and interpretation as well as validation (of command line and repository state, and running the pre-rebase hook).

First of all, do you agree that this should be done and is now a good time to do it (I'm thinking mostly about conflicts with other ongoing efforts)? While at GitTogether, I talked briefly to Thomas Rast about doing this, and he mentioned that resurrecting the git sequencer might be a better idea. However, I *think* much of what I was thinking about doing involves code that is run before the git sequencer is called. I wouldn't mind working on the git sequencer afterwards, unless Christian Couder or someone else is currently working on it.

While I did put 'refactoring' in the subject line, this is not strictly true for I would like to do. There are currently quite some functional differences between 'git rebase' and 'git rebase -i' (not just the obvious one). While refactoring the code, it would be natural to remove some of these differences, because I would guess that most of them are not intentional. The following are some of the differences I have found so far.

1. Several different error messages. For example, 'cannot rebase: you
   have unstaged changes' versus 'Working tree is dirty'.
2. Different order of validation of command line and current state. For
   example, if the working tree is dirty and the upstream is invalid,
   interactive rebase will report the dirty working tree, while non-
   interactive rebase will report the invalid upstream.
3. Different set of supported flags. For example, interactive rebase
   does not support the '--stat' flag (and rebase.stat configuration)
   and various versions of '--strategy'.

The above are just some examples, and it is probably most efficient to decide which behavior to pick in each case while reviewing patches (provided, of course, that you think it is worthwhile to extract the shared functionality).

/Martin

Am 11/2/2010 13:33, schrieb Martin von Zweigbergk:

Differences in the option handling is my favorite gripe with git-rebase, too. I suggest to write a command line processor, git-rebase.sh, that sets shell variables from options that it collects from various sources, then dispatches to one of git-rebase--interactive.sh, git-rebase--merge.sh, or git-rebase--am.sh (the latter two would be stripped-down copies of the current git-rebase.sh).

-- Hannes

On Tuesday 02 November 2010 13:33:07 Martin von Zweigbergk wrote:

Great!

Now that GTAC (http://www.gtac.biz) is over, I plan to work on options --continue, --abort and --skip for git cherry-pick/revert. After that I hope to be able to refactor the code so that in the end common code is used by cherry-pick/revert and rebase.

And I agree that what you want to do does not conflict with my plan. On the contrary it might help in the end. Go for it!

Thanks, Christian.

Perfect! I liked Johannes's proposal and started working on it yesterday. It's looking good so far.

/Martin

Hi Christian,

On Wed, Nov 03, 2010 at 04:24:32AM +0100, Christian Couder wrote:

Sounds like "sequencer is coming back", great news :)

I don't know if you would like the idea enough, but something I often think would be good to have (and which could be useful for cherry-pick and other commands in need of a sequencer), would be more flexibility. The thing I find myself lacking most often, is the possibility to change my mind on an already-edited commit (ie, go back after --continue), the alternatives I can see today being:

- keeping a note on what to do on next pass (but may be more work in case
  of conflicts with further commits)
- fast-forward --continue'ing to keep curent changes and add new ones in
  next pass (same restriction)
- --abort'ing the rebase and starting it again, possibly fetching the
  changes from previous run via HEAD's reflog (not very handy either)
- checkout back to where you want to re-amend and cherry-pick those you
  already passed, essentially redoing an interactive rebase by hand

If we could go back to previous commit, while keeping changes done to the current one (say, --previous), or reverting to the original one (say, --revert). In the same way, continuing until another previously-unforeseen commit without the need to edit the todo file would be nice to have (eg. --next).

While I'm at it, another somewhat loosely option I have thought of would be to seed the todo file with "edit" commands instead of "pick", to make it possible to validate a series of patches one by one before sending. That could be generalized for running a test script automatically, that is inserting "x whatever" between all pick's - and my 1st idea would boil down to inserting arg-less "edit" or "x false" instead. Maybe some --stepcmd=<command> flag ?

On Thu, Nov 4, 2010 at 3:15 PM, Yann Dirson <ydirson@free.fr> wrote:

I've long wanted something like --next. That would save me a lot of multi-pass rebases.

Yes, --stepcmd would be *really* useful for testing a patch sequence or adding forgotten signed-off by's.

Hi Yann,

On Thu, Nov 4, 2010 at 10:15 PM, Yann Dirson <ydirson@free.fr> wrote:

Yeah, but don't expect something soon because I am quite busy these days.

If you want them, feel free to work on those options without waiting for a sequencer.

Thanks, Christian.

I have now removed all the command line processing from git-rebase--interactive.sh. I think the next step will be to align the order of the validation steps between the two script files. When that is done, I will remove one step at a time from git-rebase--interactive.sh while simultaneously moving down the call to it from git-rebase.sh.

To avoid going off in the wrong direction, I would appreciate your input
on the order of the validation steps. I was thinking about having them
in this order:
1. Check command line
2. a. If --continue, --skip or --abort requested, rebase-apply/ or
      rebase-merge/ must exist. (What if -i is also passed and
	  rebase-apply/ exists?)
   b. Otherwise, rebase-apply/ or rebase-merge/ must not exist
3. Check that references, such as upstream and onto, are valid
4. Check that working tree is clean
5. Run pre-rebase hook

What do you think?

rebase -i currently checks that GIT_COMMITTER_IDENT is set. Should that be checked and if so, should it only be checked if the rebase is interactive?

/Martin

This actually hints at a more general question, namely whether command line or saved options should be used when continuing a rebase. For example, if an interactive rebase has been started with a certain merge strategy, should that merge strategy be used throughout the rebase, or should whatever is passed on the command line when continuing be used instead? Does it depend on which option we are talking about?

As far as I can understand from the code, it seems like non-interactive rebase currently does not store e.g. the merge strategy, but allows it to be passed on the command line together with '--continue' (but only if passed before). Interactive rebase, OTOH, does store the option when the rebase is initated and does not allow it to be overriden on the command line. I have not tested either of them, so I may very well be wrong.

Whatever the current behavior is, how do you think it *should* behave?