Skip to content

Conversation

Byron
Copy link
Collaborator

@Byron Byron commented Sep 22, 2025

This should work as we transform to a normalised form before diffing. However, I think the reason for the change ultimately was Windows, where crlf/lf differences might make diffs light up unnecessarily.

Maybe there will be more polish in that regard. Unfortunately it's also not clear anymore why stripping was deemed so very necessary in the first place.

Tasks

  • "Design" around it for now
  • frontend needs to differentiate between files and no diff being shown - can infer
    • it seems that "generate" for the commit message will think the file was deleted. I thought it should think that 'nothing changed'.

Future Tasks

  • send newlines to the frontend at least so it can determine what changed
  • somehow strip newlines in the frontend, but visualise changes somehow?

Copy link

vercel bot commented Sep 22, 2025

@Byron is attempting to deploy a commit to the GitButler Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions github-actions bot added the rust Pull requests that update Rust code label Sep 22, 2025
@Byron Byron marked this pull request as ready for review September 22, 2025 12:50
@Byron
Copy link
Collaborator Author

Byron commented Sep 22, 2025

With this fix, newlines are relevant in diffs.

Screen.Recording.2025-09-22.at.14.50.59.mov

They could benefit from special rendering in the frontend as well.

It's unclear if there are corner-cases on Windows that were bypassed previously, maybe stripping newlines was never necessary though or it stopped being necessary over time due to improvements in other places.

CC @krlvi as this might need some testing on Windows. We don't seem to have a breaking test though so but I am not sure they cover everything.

@Byron Byron requested a review from mtsgrd September 22, 2025 13:19
@Byron
Copy link
Collaborator Author

Byron commented Sep 25, 2025

Here is my comment from Discord for a second life:


Can we fix the 'line change diff shows up empty' issue in post 😅?
Right now, changing just the \n at the end of the file by adding or removing it creates empty diffs. Fixing this is more involved and probably won't be possible to do nicely without more frontend awareness, and it's a rabbit hole I'd like to defer going down to focus on apply/unapply.
So here is my MVP improvement for this: The UI detects if it gets empty hunks, and instead shows something along the lines of 'line separators changed', or 'newline was added or removed from the file'. The latter is a very common reason for empty diffs.
CC @mtsgrd

This picture would have to be changed to something else, to buy time until we can tackle the actual problem properly.

Screenshot_2025-09-23_at_04 43 54

@mtsgrd
Copy link
Contributor

mtsgrd commented Sep 25, 2025

Ok, if it's a deep rabbit hole then let's try and manage this in the front end. I won't be able to get to this in the next few days, but I can try and make it so if there are no flags changes, and the file is modified, then we switch the message to something about newlines.

@Byron
Copy link
Collaborator Author

Byron commented Sep 26, 2025

Thank you, appreciated!
I just want to prioritise 'apply/unapply' now and be more aggressive in my pursuit - it's hanging around for too long and I want something to be usable (even if not complete) ASAP.

This *should* work as we transform to a normalised form before diffing.
However, I think the reason for the change ultimately was Windows, where
crlf/lf differences might make diffs light up unnecessarily.

Maybe there will be more polish in that regard.
Unfortunately it's also not clear anymore why stripping was deemed so very
necessary in the first place.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants