Re: [PATCH 00/22] Prepare the sequencer for the upcoming rebase -i patches
- From
Jakub Narębski <jnareb@gmail.com>
- Date
- Sep 2, 2016, 11:41 UTC
- Message-ID
- <ced4a190-6a79-e608-ca0b-3815267c5f93@gmail.com>
- In-Reply-To
- <cover.1472457609.git.johannes.schindelin@gmx.de>
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?
Show 7 quoted lines
> > 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.
Show 5 quoted lines
> > 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.
Show 5 quoted lines
> 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
Show 12 quoted lines
> > 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