[RFC,00/11] rebase -i run without forking rebase--interactive
mbox series

Message ID 20190319190317.6632-1-phillip.wood123@gmail.com
Headers show
Series
  • rebase -i run without forking rebase--interactive
Related show

Message

Phillip Wood March 19, 2019, 7:03 p.m. UTC
From: Phillip Wood <phillip.wood@dunelm.org.uk>

When the builtin rebase starts an interactive rebase it parses the
options and then repackages them and forks `rebase--interactive`. This
series refactors rebase--interactive so that interactive rebases can
be started by the builtin rebase without forking. My motivation was to
make it easier to debug the sequencer but this should help future
maintainability.

This series involves some code movement so viewing the diffs with
--color-moved is recommended.

These patches are based on a merge of master [e902e9bcae ("The second
batch", 2019-03-11)] and ag/sequencer-reduce-rewriting-todo ed35d18841
("rebase--interactive: move transform_todo_file()", 2019-03-05). They
can be fetched from the tag rebase-i-no-fork/rfc at
https://github.com/phillipwood/git.git

Phillip Wood (11):
  sequencer: always discard index after checkout
  rebase: rename write_basic_state()
  rebase: use OPT_RERERE_AUTOUPDATE()
  rebase -i: combine rebase--interactive.c with rebase.c
  rebase -i: remove duplication
  rebase -i: use struct commit when parsing options
  rebase -i: use struct object_id for squash_onto
  rebase -i: use struct rebase_options to parse args
  rebase -i: use struct rebase_options in do_interactive_rebase()
  rebase: use a common action enum
  rebase -i: run without forking rebase--interactive

 Makefile                      |   1 -
 builtin/rebase--interactive.c | 377 --------------------
 builtin/rebase.c              | 625 ++++++++++++++++++++++++++--------
 parse-options-cb.c            |  34 ++
 parse-options.h               |   4 +
 sequencer.c                   |  42 ++-
 sequencer.h                   |   7 +-
 7 files changed, 556 insertions(+), 534 deletions(-)
 delete mode 100644 builtin/rebase--interactive.c

Comments

Josh Steadmon March 20, 2019, 8:50 p.m. UTC | #1
On 2019.03.19 19:03, Phillip Wood wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> When the builtin rebase starts an interactive rebase it parses the
> options and then repackages them and forks `rebase--interactive`. This
> series refactors rebase--interactive so that interactive rebases can
> be started by the builtin rebase without forking. My motivation was to
> make it easier to debug the sequencer but this should help future
> maintainability.
> 
> This series involves some code movement so viewing the diffs with
> --color-moved is recommended.
> 
> These patches are based on a merge of master [e902e9bcae ("The second
> batch", 2019-03-11)] and ag/sequencer-reduce-rewriting-todo ed35d18841
> ("rebase--interactive: move transform_todo_file()", 2019-03-05). They
> can be fetched from the tag rebase-i-no-fork/rfc at
> https://github.com/phillipwood/git.git
> 
> Phillip Wood (11):
>   sequencer: always discard index after checkout
>   rebase: rename write_basic_state()
>   rebase: use OPT_RERERE_AUTOUPDATE()
>   rebase -i: combine rebase--interactive.c with rebase.c
>   rebase -i: remove duplication
>   rebase -i: use struct commit when parsing options
>   rebase -i: use struct object_id for squash_onto
>   rebase -i: use struct rebase_options to parse args
>   rebase -i: use struct rebase_options in do_interactive_rebase()
>   rebase: use a common action enum
>   rebase -i: run without forking rebase--interactive

Although I'm not very familiar with the rebase code, this series looks
good to me.

Reviewed-by: Josh Steadmon <steadmon@google.com>
Ævar Arnfjörð Bjarmason March 20, 2019, 11:05 p.m. UTC | #2
On Tue, Mar 19 2019, Phillip Wood wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When the builtin rebase starts an interactive rebase it parses the
> options and then repackages them and forks `rebase--interactive`. This
> series refactors rebase--interactive so that interactive rebases can
> be started by the builtin rebase without forking. My motivation was to
> make it easier to debug the sequencer but this should help future
> maintainability.
>
> This series involves some code movement so viewing the diffs with
> --color-moved is recommended.
>
> These patches are based on a merge of master [e902e9bcae ("The second
> batch", 2019-03-11)] and ag/sequencer-reduce-rewriting-todo ed35d18841
> ("rebase--interactive: move transform_todo_file()", 2019-03-05). They
> can be fetched from the tag rebase-i-no-fork/rfc at
> https://github.com/phillipwood/git.git

Just a that the t/perf/*rebase* numbers look much better with this. I
don't have these in front of me anymore, but over 10 runs with -O3 one
of those long-runnings test was 30% faster.

Another one (rebase -i) went from 0.02 to 0.01 sec, with that short
amount of time I wonder (but didn't dig) if the test itself is broken...
Junio C Hamano March 21, 2019, 1:44 a.m. UTC | #3
Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When the builtin rebase starts an interactive rebase it parses the
> options and then repackages them and forks `rebase--interactive`. This
> series refactors rebase--interactive so that interactive rebases can
> be started by the builtin rebase without forking. My motivation was to
> make it easier to debug the sequencer but this should help future
> maintainability.
>
> This series involves some code movement so viewing the diffs with
> --color-moved is recommended.
>
> These patches are based on a merge of master [e902e9bcae ("The second
> batch", 2019-03-11)] and ag/sequencer-reduce-rewriting-todo ed35d18841
> ("rebase--interactive: move transform_todo_file()", 2019-03-05). They
> can be fetched from the tag rebase-i-no-fork/rfc at
> https://github.com/phillipwood/git.git

Exciting.

>
> Phillip Wood (11):
>   sequencer: always discard index after checkout
>   rebase: rename write_basic_state()
>   rebase: use OPT_RERERE_AUTOUPDATE()
>   rebase -i: combine rebase--interactive.c with rebase.c
>   rebase -i: remove duplication
>   rebase -i: use struct commit when parsing options
>   rebase -i: use struct object_id for squash_onto
>   rebase -i: use struct rebase_options to parse args
>   rebase -i: use struct rebase_options in do_interactive_rebase()
>   rebase: use a common action enum
>   rebase -i: run without forking rebase--interactive
>
>  Makefile                      |   1 -
>  builtin/rebase--interactive.c | 377 --------------------
>  builtin/rebase.c              | 625 ++++++++++++++++++++++++++--------
>  parse-options-cb.c            |  34 ++
>  parse-options.h               |   4 +
>  sequencer.c                   |  42 ++-
>  sequencer.h                   |   7 +-
>  7 files changed, 556 insertions(+), 534 deletions(-)
>  delete mode 100644 builtin/rebase--interactive.c
Phillip Wood March 21, 2019, 2:40 p.m. UTC | #4
On 20/03/2019 23:05, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Mar 19 2019, Phillip Wood wrote:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> When the builtin rebase starts an interactive rebase it parses the
>> options and then repackages them and forks `rebase--interactive`. This
>> series refactors rebase--interactive so that interactive rebases can
>> be started by the builtin rebase without forking. My motivation was to
>> make it easier to debug the sequencer but this should help future
>> maintainability.
>>
>> This series involves some code movement so viewing the diffs with
>> --color-moved is recommended.
>>
>> These patches are based on a merge of master [e902e9bcae ("The second
>> batch", 2019-03-11)] and ag/sequencer-reduce-rewriting-todo ed35d18841
>> ("rebase--interactive: move transform_todo_file()", 2019-03-05). They
>> can be fetched from the tag rebase-i-no-fork/rfc at
>> https://github.com/phillipwood/git.git
> 
> Just a that the t/perf/*rebase* numbers look much better with this. I
> don't have these in front of me anymore, but over 10 runs with -O3 one
> of those long-runnings test was 30% faster.

That's surprising I wouldn't expect that much change as the time taken 
to fork shouldn't be that significant compared to the overall running 
time. I'll make a note to have a look at those tests.

> Another one (rebase -i) went from 0.02 to 0.01 sec, with that short
> amount of time I wonder (but didn't dig) if the test itself is broken...

Yeah, that sounds suspicious

Thanks for sharing those results

Phillip