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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sep 2, 2016, 13:56 UTC
Message-ID: <alpine.DEB.2.20.1609021432070.129229@virtualbox>
In-Reply-To: <ced4a190-6a79-e608-ca0b-3815267c5f93@gmail.com>

Hi Kuba,

On Fri, 2 Sep 2016, Jakub Narębski wrote:

Show 8 quoted lines

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

Show 6 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 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.

Show 8 quoted lines

> > 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 *looooong* time ago to turn this from a hacky shell script into a proper C builtin.

There is so much more to do before git-rebase.sh can be retired.

For starters, git-rebase.sh is actually just a glorified command-line option parser and front-end to git-rebase--am.sh, git-rebase--interactive.sh and git-rebase--merge.sh.

To retire it, those three shell scripts need to be *completely* built-in first.

(Actually, for the --preserve-merges case, I could imagine that we simply refactor it into its own shell script and call that from a builtin git-rebase, until we retire --preserve-merges, but that's a couple of years down the road.)

So the first goal would be to retire git-rebase--interactive.sh. For that to happen, --root needs to be supported first. Then the --preserve-merges stuff needs to be refactored into its own shell script. And then the command-line option parsing needs to be moved to rebase--helper, too. And *then* git-rebase--interactive.sh can be retired.

As I stated earlier, my hope is that the rebase--helper work is only an initial step, opening the door for other contributors to tackle independent parts of making git-rebase a builtin.

Show 6 quoted lines

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

Please keep in mind that my current state (local, and pushed to my GitHub repository) has advanced substantially. I am reluctant to send it out yet because I still need to send out rebase-i-extra first, so that it gets *some* visibility before v2.10.0.

Show 21 quoted lines

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

Note that this commit is about read_populate_todo(), not about save_todo(). So I do not think that we should mention anything in this commit's message about other functions that may be rewritten instead of being future-proofed..

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

As mentioned earlier, I bit the bullet and reimplemented that logic. Mostly to fend off more comments in this direction.

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

Again, it was reintroduced. The test to check for the overzealous assumption was not removed, and it passes, to prove that I did it right.

Show 32 quoted lines

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

Apart from the l10n issues, I think I addressed them all locally, and pushed the result out to my GitHub repository, although I plan to send out additional iterations only after releasing Git for Windows v2.10.0.

Show 6 quoted lines

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

It's the `mail-patch-series.sh` script, in conjunction with setting the config variable mail.publishremoteto to point to my GitHub remote. You can find the `mail-patch-series.sh` script here:

	https://github.com/dscho/mail-patch-series

I probably forgot to adjust the README to reflect the most recent changes (such as the `--basedon` feature)... PRs welcome [*1*].

Ciao, Dscho

Footnote *1*: https://raw.githubusercontent.com/dscho/images/master/i-can-haz-pull-request.png