Re: [PATCH v2 04/13] fast-rebase: write conflict state to working tree, index, and HEAD

[Derrick Stolee <stolee@gmail.com>]

On 5/17/2021 11:42 PM, Elijah Newren wrote:
> On Mon, May 17, 2021 at 6:32 AM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 5/3/21 10:12 PM, Elijah Newren via GitGitGadget wrote:
>>> From: Elijah Newren <newren@gmail.com>
>>>
>>> Previously, when fast-rebase hit a conflict, it simply aborted and left
>>> HEAD, the index, and the working tree where they were before the
>>> operation started.  While fast-rebase does not support restarting from a
>>> conflicted state, write the conflicted state out anyway as it gives us a
>>> way to see what the conflicts are and write tests that check for them.
>>>
>>> This will be important in the upcoming commits, because sequencer.c is
>>> only superficially integrated with merge-ort.c; in particular, it calls
>>> merge_switch_to_result() after EACH merge instead of only calling it at
>>> the end of all the sequence of merges (or when a conflict is hit).  This
>>> not only causes needless updates to the working copy and index, but also
>>> causes all intermediate data to be freed and tossed, preventing caching
>>> information from one merge to the next.  However, integrating
>>> sequencer.c more deeply with merge-ort.c is a big task, and making this
>>> small extension to fast-rebase.c provides us with a simple way to test
>>> the edge and corner cases that we want to make sure continue working.
>>
>> This seems a noble goal. I'm worried about the ramifications of such
>> a change, specifically about the state an automated process would be in
>> after hitting a conflict.
> 
> Why would an automated process be using test-helper code?  Note that
> this is code from t/helper/test-fast-rebase.c.
...
>> So perhaps this could use a test case to demonstrate the change in
>> behavior? Such a test would want to be created before this commit, then
>> the behavior change is provided as an edit to the test in this commit.
>>
>> But maybe I'm misunderstanding the change here and such a test is
>> inappropriate.
> 
> If this weren't code under t/helper/, I'd agree whole-heartedly with
> the suggestion.

You're right. Ignore me.

-Stolee