Fix issue with bad characters in file paths

[tobyjwebb]

This PR fixes an issue where the app would be left in an inconsistent state when a filename in the repo is present with strange characters in it (for example "\n").

To reproduce the issue, run the following command from a bash terminal inside a repo that is being watched by GitButler:

touch oh_no$'\n'.txt

Should file names contain \n in them? Probably not. Can they? It would seem so. If they can, it would be better that the app doesn't error out.

Note that I haven't tested all the code that this PR modifies, and there may be some path.display() which needs replacing - the places I searched for were:

• Any struct which is Serialized with serde and contains a PathBuf
• Most places where file_path.display() where being used were changed

[Qix-]

I'm not fully understanding what the rust portions of this PR are fixing. Could you perhaps explain a bit why a custom serializer that wraps JSON's default Path formatting is necessary here?

As for the handling in the frontend, we should definitely be checking which platform we're running on and switching the regex based off that, not assuming. That's really the only 'proper' solution here since any character except for / and \0 is a valid pathname character on *nix.

[tobyjwebb]

@Qix- Sure:

The problem was that when the files in the repo contain \n characters (I imagine other characters could also be problematic, but \n was what was creating an issue for me), when the meta/ownership file is populated, we would end up with something like this:

ohno
.txt:1-17-fcb6d34f0431c0b8aff8066dc9a17cfd-1711990013953

Which would then cause the UI to break:

What this PR achieves is that the file path is escaped:

ohno\n.txt:1-17-fcb6d34f0431c0b8aff8066dc9a17cfd-1711990013953

and so the ownership lines can be properly parsed back and understood by the UI (and backend).

[edit] - lemme check how this behaves in Windows...

[Qix-]

Thanks, makes sense. Would it be possible if we go one step further and add a Tauri function that returns the path separator for the platform, and incorporate that into the path splitting function?

[Qix-]

By the way, changes should probably be rebased on top of #3405 which is going to be merged here shortly to unblock some of the other changes we want to make this week. It's just a rename with minimal other changes so it should rebase cleanly.

Just wanted to give you a heads up.

[tobyjwebb]

By the way, changes should probably be rebased on top of #3405 which is going to be merged here shortly to unblock some of the other changes we want to make this week. It's just a rename with minimal other changes so it should rebase cleanly.

Updated my branch (again).

[krlvi]

@Qix- since the code moved quite a bit, would you mind doing the rebase / conflicts on this one? I can also attempt to do it tomorrow morning as I am catching up after my vacation

[mtsgrd]

Is this pr still relevant?

[tobyjwebb]

Is this pr still relevant?

With the latest stable release:

• The bad chars are not leaving the repo in a bad state (which is an improvement)
• Unfortunately, when trying to commit the changes, I'm getting this error:

I've also tested with the latest master (with the same result) as of the following commit:

Merge: 31eeb9f32 788958ee0
Author: Kiril Videlov <[email protected]>
Date:   Sun Jun 9 10:08:20 2024 +0200

    Merge pull request #4046 from Byron/various-fixes
    
    various fixes

As this is taking a while to get accepted, and it is a pretty edge-case issue (I only discovered the bug because I was using a different buggy app), it might be better to close this PR and open a low-priority issue instead. Thoughts?

[mtsgrd]

Hey, sorry for leaving this PR without a follow-up. @krlvi @Byron what do we want to do here? I just tested this with touch oh_no$'\n'.txt and can confirm it breaks ownership assignment since it introduces an unescaped newline into the ownership string.

[Byron]

I think the most recent version will not write invalid values anymore, but hard-reject files with newlines in them, as to prevent corrupting on-disk state. This should already be an improvement, even though ultimately it would be better if these paths would be escaped.

My recommendation for this PR is to close it and retry - everything changed by now.