Re: [PATCH 00/22] Prepare the sequencer for the upcoming rebase -i patches
Hi Kuba, On Fri, 2 Sep 2016, Jakub Narębski wrote: > 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? Most of the patch series I sent before last week got accepted. Only one got rejected, IIRC, and replaced by a better solution (3727318 (Merge branch 'jk/test-send-sh-x-trace-elsewhere', 2016-05-17)). The patch series I submitted as part of my rebase--helper work that were accepted: b232439 (Merge branch 'js/t3404-typofix', 2016-05-17) 7b02771 (Merge branch 'js/perf-rebase-i', 2016-05-23) 3437017 (Merge branch 'js/perf-on-apple', 2016-07-06) 62e5e83 (Merge branch 'js/find-commit-subject-ignore-leading-blanks', 2016-07-11) c510926 (Merge branch 'js/sign-empty-commit-fix', 2016-07-13) 6c35952 (Merge branch 'js/t3404-grammo-fix', 2016-07-13) 63641fb (Merge branch 'js/log-to-diffopt-file', 2016-07-19) 3d55eea (Merge branch 'js/am-call-theirs-theirs-in-fallback-3way', 2016-07-19) c97268c (Merge branch 'js/rebase-i-tests', 2016-07-28) 1a5f1a3 (Merge branch 'js/am-3-merge-recursive-direct', 2016-08-10) You will note that I broke out a couple of patch series that do not strictly have anything to do with the rebase--helper, such as perf-on-apple. Nevertheless, they were part of a 99-strong patch series that was my initial working rebase--helper, which I have used ever since to perform all of my interactive rebases. There are still a couple of patch series in flight. Let me list them by the tags created by my mail-patch-series.sh script: https://github.com/dscho/git/releases/tag/libify-sequencer-v2 https://github.com/dscho/git/releases/tag/require-clean-work-tree-v1 https://github.com/dscho/git/releases/tag/prepare-sequencer-v1 https://github.com/dscho/git/releases/tag/sequencer-i-v1 https://github.com/dscho/git/releases/tag/rebase--helper-v1 These tags all contain links to the cover letter as stored on public-inbox.org, identified by the Message-ID. Please note that the first four of this batch of five already saw substantial work-after-review, thanks in part to your helpful comments. You may appreciate the fact that a link of the form https://github.com/dscho/git/compare/libify-sequencer-v2...libify-sequencer shows you where I am at, although it cannot give you a real interdiff because I rebased to a newer version of upstream/master in the meantime. Finally, there is one last patch series that I did not yet submit: the 'rebase-i-extra' patch series. However, as I continuously update the overall 'interactive-rebase' branch thicket (and have done so since the very beginning of my work on the rebase--helper), it is relatively easy to see what is left: https://github.com/dscho/git/compare/rebase--helper...interactive-rebase BTW thanks for making me dig out all of this information (it did take a while to uncover it...), as I am so totally going to use that in a blog post. > > The reason to split these two patch series is simple: to keep them at a > > sensible size. > > That's good. Thanks. I really try to be sensible with other people's time. Even more so after being so offended by the talk at the most recent Git Merge that stated that some people deliberately waste contributors' time because they value their own time so much more. I am *really* offended by that. As a maintainer of Git for Windows, I do everything in my power to strike a sensible balance between how much time I spend on improving the software and how much time I ask others to do so. > > 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 problem with --root is that it *creates* an initial commit. It is empty, and will be amended. It would most likely not be a lot of work, but I really wanted this work to be incremental, focusing on the most important aspects first. In fact, I do hope that somebody with the need for --root will take the baton and run with it. > > 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? Oh yes! Retiring git-rebase.sh is *far, far, far* in the future. We really missed the boat a
Re: [PATCH 00/22] Prepare the sequencer for the upcoming rebase -i patches
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
Re: [PATCH 00/22] Prepare the sequencer for the upcoming rebase -i patches
Hi Dennis, On Mon, 29 Aug 2016, Dennis Kaarsemaker wrote: > On ma, 2016-08-29 at 10:03 +0200, Johannes Schindelin wrote: > > > Therefore I would be most grateful for every in-depth review. > > Tried to do that, but could come up only with a few nits. I think the > approach is sensible. Thank you for the review! Ciao, Dscho
Re: [PATCH 00/22] Prepare the sequencer for the upcoming rebase -i patches
On ma, 2016-08-29 at 10:03 +0200, Johannes Schindelin wrote: > Therefore I would be most grateful for every in-depth review. Tried to do that, but could come up only with a few nits. I think the approach is sensible. D.
