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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 28/04/17 20:22, Johannes Schindelin wrote:
> Hi Philip,
> 
> On Fri, 28 Apr 2017, Phillip Wood wrote:
> 
>> On 26/04/17 12:59, Johannes Schindelin wrote:
>>
>>> 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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]