Re: [PATCH 00/22] Prepare the sequencer for the upcoming rebase -i patches

["Jakub Narębski" <jnareb@gmail.com>]

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

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

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


HTH

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