ENB: PR #4631: lessons learned

[Edward K. Ream]

​PR #4631 fixes a worthy bug. I'm about to merge it.

This Engineering Note post discusses the mistakes I made creating the PR. These mistakes have taught me important lessons. See the summary if you are in a hurry :-)

For the last several days my inner critic has been screaming at me. The critic wasn't wrong. I chose a highly inefficient plan for fixing the bug.

Background

PR #4627 introduced the bug. It escaped detection because it seemed to affect only cut/copy/paste in the Completion pane. This pane shows all tab completions. It's rare to want to copy text from it. By great good fortune, however, I did discover the bug soon after merging the PR.

The bug seemingly arises from what git bisect labels the first bad commit: rev  cf2d872. I say seemingly because it's possible that later commits also played a part. I still don't know for sure.

My first strategy was to merge the later commits into the first bad commit, but that didn't work well. There were hard-to-resolve conflicts during the merge, and the bug persisted after several attempts at resolving the conflicts.

Panic

At this point I felt stuck, but I didn't then realize how important it was to choose my strategy carefully. In my ignorance, I chose to revert all the files in PR #4627 to their versions in the 6.8.8 branch (the same as master). Yes, that did indeed "fix" the problem, but those reversions clobbered a lot of work!

In retrospect, my urgent priorities should have been:

- Revert as few files as possible.

- Revert as few lines of code as possible.

Here's how.

Step one

I could have reverted just the most likely culprit file: qt_text.py, creating a separate branch for this experiment. Other files may have had to change as a result, but it shouldn't have taken too much work to get a workable executable.

There would have been no guarantee that this single reversion would have worked. If it didn't, I might have tried to revert just one other file.

Step two

After isolating the culprit files, instead of actually reverting those files, I could have reverted only the actual code changes in the changed files. There would be no need to revert annotations!

I would then create another branch in which to try reverting only the executable code. If that branch worked, my task would be essentially complete. For sure, I would have narrowed the possible culprits to a comparatively few lines of code.

Summary

This ENB confronts serious mistakes in strategy. As a result, PR #4631 reverts way too much code! If I had it to do over again, I would have tried to find the minimal number of culprit files and then focus on minimizing the number of executable lines in those files.

There is never any need to panic. git ensures that no code (and no checkpoint) ever disappears.

It's way too late to undo my mistakes. I'm stuck with PR #4631 as it is. I'll release this PR today, but issue #4634 suggests undoing still more reversions.

​All comments, questions, and suggestions are welcome.

​Edward

[Edward K. Ream]

On Friday, April 24, 2026 at 2:21:35 PM UTC-5 Edward K. Ream wrote:

> It's way too late to undo my mistakes. I'm stuck with PR #4631 as it is.

Utter nonsense! On second thought, I shall (must!?) follow the plan described in this thread! Doing so will be (a little bit) easier if I don't merge the PR.

Starting from scratch has important benefits:

- Much smaller diffs.

- A good chance of finding the (small!) number of lines that created the bug.

- Retaining a lot of work that PR #4631 clobbers, thereby eliminating the need for the "reconciliation" issue #4634.

- I get to reinforce a new bug-fixing pattern in the old bean.

True, new PRs must restore the "good parts" of PR 4631:

- Fix a bug in test_leoserver.py.

- Add a Qt unit test.

- Reformat (in another PR) import statements as in the PR.

Summary

It's been stressful having my critic scream at me for almost a week. Now I can make amends :-)

Rethinking and redoing the bug fix is well worth doing. Yes, I'll redo significant work, but doing so will drastically reduce the size of a crucial PR. Another lesson learned: avoid huge PRs!

Edward

[Edward K. Ream]

On Saturday, April 25, 2026 at 6:27:57 AM UTC-5 Edward K. Ream wrote:

> Rethinking and redoing the bug fix is well worth doing. Yes, I'll redo significant work, but...

Aha! Examining the diffs between the master and kr-4626-completion-tab branches brought my attention to an earlier PR, #4624. And sure enough, the bug exists in the  ekr-4623-log-pane-keys branch! So it looks like  PR #4624,  not  PR #4627, is the real culprit.

This changes everything. It looks like I've been looking in the wrong places for the bug. I'm not sure why git bisect mislead me, but that hardly matters. It's time to look at the diffs between the master and ekr-4623-log-pane-keys branches!

Edward

[Edward K. Ream]

On Saturday, April 25, 2026 at 8:12:05 AM UTC-5 Edward K. Ream wrote:

>> Rethinking and redoing the bug fix is well worth doing. Yes, I'll redo significant work, but...

Heh, I've done work to discover that (for this bug) a diff-based approach is too dangerous.

My inner critic is now satisfied that the brute force approach, PR #4631, is best. The PR's great virtue is that it does indeed fix the bug. I've reopened that PR and also its follow-on issue, #4634. All remaining work is routine.

Edward