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:

quoted

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.

quoted

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.

quoted

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.

quoted

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.

quoted

Johannes Schindelin (22):
  sequencer: use static initializers for replay_opts

There is no need for putting zeros in static initializer.  Commit
message expanded.

quoted

  sequencer: use memoized sequencer directory path
  sequencer: avoid unnecessary indirection
  sequencer: future-proof remove_sequencer_state()

Leftover unrelated chunk removed.

quoted

  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.

quoted

  sequencer: release memory that was allocated when reading options

See above.

quoted

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

quoted

  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.

quoted

  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.

quoted

  sequencer: avoid completely different messages for different actions

Fix l10n or drop (and not introduce lego translation).

quoted

  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.

quoted

  sequencer: remember the onelines when parsing the todo file

Lazy or eager again; "exec", "noop" and --preserve-merges.

quoted

  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.

quoted

  sequencer: introduce a helper to read files written by scripts

Perhaps add why not use open + strbuf_getline to commit message...

quoted

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

quoted

  sequencer: allow editing the commit message on a case-by-case basis

Enhance the commit message.

quoted

  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.

quoted

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