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