Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix over-eager buffer truncation (regression from #98) #129

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

hartwork
Copy link
Collaborator

Regression from #98, may need coordination with #115.

Previously, with high input pressure where full sizeof(input_buf) - 1 bytes could be read, all content would be dropped right after reading, and it could happen with every single read.

Example:

# ./ttyplot < /dev/random

Previously, with high input pressure where full
"sizeof(input_buf) - 1" bytes could be read, all content
would be dropped right after reading, and it could happen
with every single read.

Example:
> # ./ttyplot < /dev/random
@hartwork hartwork requested a review from edgar-bonet November 28, 2023 03:45
@edgar-bonet
Copy link
Collaborator

Since this section is completely rewritten in #115. Fixing this now may not be very useful. And I guess it could just add merge conflicts.

@hartwork
Copy link
Collaborator Author

hartwork commented Nov 28, 2023

Since this section is completely rewritten in #115. Fixing this now may not be very useful. And I guess it could just add merge conflicts.

@edgar-bonet it would give me a chance to clean-up the mess I made earlier regarding truncation, it would separate bugfixes from refactoring and get the fix into development sooner. Toyota would like it 😃

I was hoping that we could merge this (#129) and rebase #115 on top of it after. Conflict resolution is cheap here, it could even be automated, because/assuming that #115 does not have the same problem. Let me demo that with our very case on the shell:

# Start after the first big refactoring commit (that would show conflict)
git checkout -b demo-free-conflict-resolution edgar-bonet-readonly/refactor-input~1

# Merge but keep our side because the refactored version does not have the same issue
EDITOR=true git merge --strategy=ours hartwork-readwrite/fix-buffer-truncation 

# Extract a rebased patch
git diff github-readwrite/fix-buffer-truncation HEAD > rebased.patch

# Pretend to start all over
git reset --hard edgar-bonet-readonly/refactor-input~2

# Apply the rebased version, three commits:
git cherry-pick hartwork-readwrite/fix-buffer-truncation  # 1
patch < rebased.patch
git add ttyplot.c
git commit -C edgar-bonet-readonly/refactor-input~1  # 2
git cherry-pick edgar-bonet-readonly/refactor-input  # 3

# Done

EDIT: There's actually simpler versions to achieve the same. This just was the first version that came to mind that would work.

What do you think?

@hartwork
Copy link
Collaborator Author

hartwork commented Nov 29, 2023

@edgar-bonet PS: For a cleaner version using git commit-tree

# First commit (i.e. start right at <fix-buffer-truncation>)
git checkout -b demo-free-conflict-resolution-2 hartwork-readwrite/fix-buffer-truncation

# Second commit, fake a merge with startegy "theirs" plus remaking original commit metadata
git merge --ff "$(git commit-tree edgar-bonet-readonly/refactor-input~1^{tree} -p HEAD -m replaced-later | tee /dev/stderr)"
EDITOR=true git commit --amend -C edgar-bonet-readonly/refactor-input~1

# Third commit, plain cherry-pick
git cherry-pick edgar-bonet-readonly/refactor-input

@hartwork
Copy link
Collaborator Author

@edgar-bonet thank you, very kind! 🙏

@hartwork hartwork merged commit 226e0fc into tenox7:development Nov 29, 2023
6 checks passed
@hartwork hartwork added this to the 1.6.0 milestone Nov 29, 2023
@hartwork hartwork deleted the fix-buffer-truncation branch November 29, 2023 21:52
@hartwork hartwork changed the title Fix over-eager buffer truncation Fix over-eager buffer truncation (regression from #98) Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants