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 Hexa editor display of binary files #11747

Merged
merged 2 commits into from
May 24, 2024

Conversation

pmiossec
Copy link
Member

@pmiossec pmiossec commented May 22, 2024

by:

  • reading file and blob with a better encoding
  • not touching/altering blob content (i.e not breaking content by reencoding)
  • not converting in ASCII before doing the Hexa editor display (it breaks char > 0x80)

As a result, display is exactly the same obtained by using an external Hexa Editor:

  • Hexa value are accurate
  • string display is the same (except some cheating done by hexa editor
    for example with char 0x99 displayed as ™)
  • File size is accurate

Kind of followup to #11727 as it was not always working well because beginning of some files (ex: jpegs) were badly altered when reading blob content.

Screenshots

Before

In commit window

  • before:

image

  • After:

image

Goal (Hex editor):

image

Recover jpeg blob:

  • before:
    image

  • after (because content is not corrupted, type of file is well recognized):

image

Test methodology

  • Manual

Test environment(s)

  • Git Extensions 33.33.33
  • Build 561d355
  • Git 2.45.0.windows.1
  • Microsoft Windows NT 10.0.22631.0
  • .NET 8.0.1
  • DPI 96dpi (no scaling)
  • Portable: False
  • Microsoft.WindowsDesktop.App Versions
    Microsoft.WindowsDesktop.App 6.0.26 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
    Microsoft.WindowsDesktop.App 7.0.15 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
    Microsoft.WindowsDesktop.App 8.0.1 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Merge strategy

I agree that the maintainer squash merge this PR (if the commit message is clear).


✒️ I contribute this code under The Developer Certificate of Origin.

Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch!
Looks sensible, have not run

GitUI/CommandsDialogs/FormVerify.cs Outdated Show resolved Hide resolved
GitUI/CommandsDialogs/FormVerify.cs Outdated Show resolved Hide resolved
GitUI/Editor/FileViewer.cs Outdated Show resolved Hide resolved
Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pending mstv comments
have not run

@pmiossec
Copy link
Member Author

I have added a little refactoring.

Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits

GitUI/Editor/FileViewer.cs Outdated Show resolved Hide resolved
GitUI/Editor/FileViewer.cs Outdated Show resolved Hide resolved
.AppendLine();

double mb = text.Length / (1024d * 1024);
if (mb > 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (mb > 1)
if (mb >= 0.1)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (mb > 1)
if (text.Length > (1024 * 1024) / 10)

GitUI/Editor/FileViewer.cs Outdated Show resolved Hide resolved
Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending mstv approval

@pmiossec pmiossec force-pushed the fix_binaries_display branch 2 times, most recently from 9f53da7 to 5a3e1fd Compare May 23, 2024 22:50
by:
* reading file and blob with a better encoding
* not touching/altering *blob* content (i.e not breaking content by reencoding)
* not converting in ASCII before doing the Hexa editor display (it breaks char > 0x80)

As a result, display is **exactly** the same obtained by using an external Hexa Editor:
* Hexa value are accurate
* string display is the same
(except some cheating done by hexa editor
for example with char 0x99 displayed as ™)
* DRY
* add display in MB (more human friendly) & translated
Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have not run

@pmiossec pmiossec merged commit fbebe3f into gitextensions:master May 24, 2024
4 checks passed
@pmiossec pmiossec deleted the fix_binaries_display branch May 24, 2024 17:23
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.

None yet

3 participants