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

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

[Martin von Zweigbergk]

(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

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

[Johannes Sixt]

Am 11/2/2010 13:33, schrieb Martin von Zweigbergk:
> There are currently quite some
> functional differences between 'git rebase' and 'git rebase -i' (not
> just the obvious one).

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

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

[Christian Couder]

On Tuesday 02 November 2010 13:33:07 Martin von Zweigbergk wrote:
> (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.

Great!

> 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.

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.

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

[Martin von Zweigbergk]

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

/Martin

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

[Yann Dirson]

Hi Christian,

On Wed, Nov 03, 2010 at 04:24:32AM +0100, Christian Couder wrote:
> 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.

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 ?

-- 
Yann

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

[Pat Notz]

On Thu, Nov 4, 2010 at 3:15 PM, Yann Dirson <ydirson@free.fr> wrote:
> Hi Christian,
>
> On Wed, Nov 03, 2010 at 04:24:32AM +0100, Christian Couder wrote:
>> 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.
>
> 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).

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

> 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 ?

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

> --
> Yann
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

[Christian Couder]

Hi Yann,

On Thu, Nov 4, 2010 at 10:15 PM, Yann Dirson <ydirson@free.fr> wrote:
> Hi Christian,
>
> On Wed, Nov 03, 2010 at 04:24:32AM +0100, Christian Couder wrote:
>> 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.
>
> Sounds like "sequencer is coming back", great news :)

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

> 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 ?

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

Thanks,
Christian.

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

[Martin von Zweigbergk]

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

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

[Martin von Zweigbergk]

> 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

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?