> On Mar 3, 2021, at 4:29 PM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
>> This patch applies on top of the first one. I added more docstrings since
>> you seem to appreciate it.
> 
> Great!
> 
>> I couldn’t wrap my head around the original tests so I started a new
>> one below.
> 
> That's perfectly fine.
> 
> Earlier you wrote:
>> if you undo-in-region, break the undo chain, then undo-in-region again
>> with undo-only, ulist would be set to t and it breaks at (setq
>> undo-elt (car ulist)).
> 
> I don't see which of the tests corresponds to this.
> Is it this one:
> 
>> +    (should (equal (buffer-string) "abcde")))
>> +  ;; Test undo/redo in region.
>> +  (with-temp-buffer
>> +    (buffer-enable-undo)
>> +    (dolist (x '("a" "b" "c" "d" "e"))
>> +      (insert x)
>> +      (undo-boundary))
>>     (should (equal (buffer-string) "abcde"))
>> -    ))
>> +    (simple-tests--exec '(move-beginning-of-line
>> +                          push-mark-command
>> +                          forward-char
>> +                          forward-char
>> +                          undo))
>> +    (should (equal (buffer-string) "acde"))
>> +    (simple-tests--exec '(undo-only))
>> +    (should (equal (buffer-string) "cde"))
>> +    (simple-tests--exec '(undo-redo))
>> +    (should (equal (buffer-string) "acde"))
>> +    (simple-tests--exec '(undo-redo))
>> +    (should (equal (buffer-string) "abcde"))))
> 
> ?

Yes, that was supposed to be the one. Just to be sure I ran the test with the old version and it didn’t error. Oops! I fixed the test, now it errors on the old version and passes after applying my fix (in the first patch). The test first runs undo in region, then breaks the undo chain (my 2nd patch failed to do that), then runs undo in region again. I’m not sure how to write the comment for that test. Maybe I could write “test for commit xxx” but there is no commit number to refer to right now.

Yuan