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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 11176 bytes --]

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

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

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

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

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