Re: [PATCH v3 1/9] rebase -i: generate the script via rebase--helper

On 28/04/17 20:22, Johannes Schindelin wrote:

Hi Philip,

On Fri, 28 Apr 2017, Phillip Wood wrote:

quoted

On 26/04/17 12:59, Johannes Schindelin wrote:

quoted

The first step of an interactive rebase is to generate the so-called
"todo script", to be stored in the state directory as
"git-rebase-todo" and to be edited by the user.

Originally, we adjusted the output of `git log <options>` using a
simple sed script. Over the course of the years, the code became more
complicated. We now use shell scripting to edit the output of `git
log` conditionally, depending whether to keep "empty" commits (i.e.
commits that do not change any files).

On platforms where shell scripting is not native, this can be a
serious drag. And it opens the door for incompatibilities between
platforms when it comes to shell scripting or to Unix-y commands.

Let's just re-implement the todo script generation in plain C, using
the revision machinery directly.

This is substantially faster, improving the speed relative to the
shell script version of the interactive rebase from 2x to 3x on
Windows.

This changes the behaviour of git -c rebase.instructionFormat= rebase -i
The shell version treats the rebase.instructionFormat being unset or set
to the empty string as equivalent. This version generates a todo list
with lines like 'pick <abbrev sha1>' rather than 'pick <abbrev sha1>
<subject>'

I only picked this up because I have a script that does 'git -c
rebase.instructionFormat= rebase -i' with a custom sequence editor. I
can easily add '%s' in the appropriate place but I thought I'd point it
out in case other people are affected by the change.

While I would argue that the C version is more correct, it would be
backwards-incompatible.

I was going to make a point about resetting config variables to their
default value on the command line but Junio beat me to it.

So I changed it.

That's great, thanks

BTW in the future you could help me a *lot* by providing a patch that adds
a test case to our test suite that not only demonstrates what exactly goes
wrong, but also will help prevent future regressions.

I'll bear that in mind, it does assume that reporters have a good
understanding of the test suite layout and helper functions though. Is
there a particular reason you put the test case in the autosquash tests?
I wouldn't have thought of doing that.

Thanks again

Phillip