W dniu 29.08.2016 o 10:03, Johannes Schindelin pisze:

> This patch series marks the  '4' in the countdown to speed up rebase -i
> by implementing large parts in C. It is based on the `libify-sequencer`
> patch series that I submitted last week.

Which of those got reviewed (and perhaps accepted), and which of those
needs review still?  What is subject of their cover letter?

> The patches in this series merely prepare the sequencer code for the
> next patch series that actually teaches the sequencer to run an
> interactive rebase.
> The reason to split these two patch series is simple: to keep them at a
> sensible size.

That's good.

> The two patch series after that are much smaller: a two-patch "series"
> that switches rebase -i to use the sequencer (except with --root or
> --preserve-merges), and a couple of patches to move several pretty
> expensive script processing steps to C (think: autosquash).

I can understand --preserve-merges, but what is the problem with --root?

> The end game of this patch series is a git-rebase--helper that makes
> rebase -i 5x faster on Windows (according to t/perf/p3404). Travis says
> that even MacOSX and Linux benefit (4x and 3x, respectively).

So do I understand correctly that end goal for *this* series is to move
most of processing to git-rebase--helper, but full builtin-ification
(and retiring git-rebase.sh to contrib/examples/) would have to wait
for later?


I'd like here to summarize the discussion (my review, Dennis review,
Johannes Sixt and Junio comments).

If there are no comments, it means no problems or minor changes.

> Johannes Schindelin (22):
>   sequencer: use static initializers for replay_opts
There is no need for putting zeros in static initializer.  Commit
message expanded.

>   sequencer: use memoized sequencer directory path
>   sequencer: avoid unnecessary indirection
>   sequencer: future-proof remove_sequencer_state()
Leftover unrelated chunk removed.

>   sequencer: allow the sequencer to take custody of malloc()ed data
Is introducing new *_entrust() mechanism (which needs docs, at least
as comments) worth it, instead of just strdup everything and free?
If it is: naming of function parameter + example in commit message.

>   sequencer: release memory that was allocated when reading options
See above.

>   sequencer: future-proof read_populate_todo()
Possibly mention which functions were not future-proofed because
of planned for the subsequent patch full rewrite.

>   sequencer: remove overzealous assumption
Overzealous assumptions, or a worthy check?  Perhaps just remove check
for rebase -i in future commit, and keep test.  Perhaps remove test

>   sequencer: completely revamp the "todo" script parsing
This removes check; it should return if it was worthy.  Some discussion
about eager versus lazy parsing of commits, but IMHO it should be left
for later, if considered worth it.

>   sequencer: avoid completely different messages for different actions
Fix l10n or drop (and not introduce lego translation).

>   sequencer: get rid of the subcommand field
>   sequencer: refactor the code to obtain a short commit name
Explain reason behind this change in the commit mesage.

>   sequencer: remember the onelines when parsing the todo file
Lazy or eager again; "exec", "noop" and --preserve-merges.

>   sequencer: prepare for rebase -i's commit functionality
Add helper function, possibly extract helper function.  Rephrase block

"[PATCH] am: refactor read_author_script()" from Junio.

>   sequencer: introduce a helper to read files written by scripts
Perhaps add why not use open + strbuf_getline to commit message...

>   sequencer: prepare for rebase -i's GPG settings
Possibly fixes bug.  Use *_entrust() or strdup to not leak memory
(and to not crash when freeing memory).

>   sequencer: allow editing the commit message on a case-by-case basis
Enhance the commit message.

>   sequencer: support amending commits
>   sequencer: support cleaning up commit messages
>   sequencer: remember do_recursive_merge()'s return value
>   sequencer: left-trim the lines read from the script
>   sequencer: refactor write_message()
Enhance the commit message.  Quote path in messages while at it.


>  builtin/commit.c                |   2 +-
>  builtin/revert.c                |  42 ++-
>  sequencer.c                     | 573 
> +++++++++++++++++++++++++++-------------
>  sequencer.h                     |  27 +-
>  t/t3510-cherry-pick-sequence.sh |  11 -
>  5 files changed, 428 insertions(+), 227 deletions(-)
> Based-On: libify-sequencer at https://github.com/dscho/git
> Fetch-Base-Via: git fetch https://github.com/dscho/git libify-sequencer
> Published-As: https://github.com/dscho/git/releases/tag/prepare-sequencer-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git prepare-sequencer-v1

An unrelated question: Dscho, how are you generating above lines?

Jakub Narębski

Reply via email to