Summary

Strip symlink mode information from files on Git-style Mercurial diffs.

Review Request #12061 — Created Feb. 17, 2022 and submitted Feb. 28, 2022, 9:55 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Git-style Mercurial diffs, like normal Git diffs, contain symlink mode

information. We've hit issues in the past with the server applying

patches containing symlink modes.
Since a Git-style diff otherwise represents a symlink change as a

standard file change, with the content being the old/new target names,

we addressed this problem by stripping out the symlink mode and treating

it as a normal file when patching for display in the diff viewer.
This is by no means a perfect solution (that is in development, with

initial steps having been done in Review Board 4.0.5), but it addresses

the issue in Git.
This change brings that fix over to Mercurial when working with

Git-style diffs. The code in Git has been split out into a common

function that both can call, simplifying maintenance and allowing both

to benefit from any future bug fixes.

Testing Done

Unit tests pass.

Tested with a customer who hit this problem. Verified that they were able to
review symlinks in Mercurial.

Commits

Summary	ID
Strip symlink mode information from files on Git-style Mercurial diffs. Git-style Mercurial diffs, like normal Git diffs, contain symlink mode information. We've hit issues in the past with the server applying patches containing symlink modes. Since a Git-style diff otherwise represents a symlink change as a standard file change, with the content being the old/new target names, we addressed this problem by stripping out the symlink mode and treating it as a normal file when patching for display in the diff viewer. This is by no means a perfect solution (that is in development, with initial steps having been done in Review Board 4.0.5), but it addresses the issue in Git. This change brings that fix over to Mercurial when working with Git-style diffs. The code in Git has been split out into a common function that both can call, simplifying maintenance and allowing both to benefit from any future bug fixes.	b0f0b5324db02a8d20e78b2394f02b213be68794

Summary

Strip symlink mode information from files on Git-style Mercurial diffs.

Git-style Mercurial diffs, like normal Git diffs, contain symlink mode information. We've hit issues in the past with the server applying patches containing symlink modes. Since a Git-style diff otherwise represents a symlink change as a standard file change, with the content being the old/new target names, we addressed this problem by stripping out the symlink mode and treating it as a normal file when patching for display in the diff viewer. This is by no means a perfect solution (that is in development, with initial steps having been done in Review Board 4.0.5), but it addresses the issue in Git. This change brings that fix over to Mercurial when working with Git-style diffs. The code in Git has been split out into a common function that both can call, simplifying maintenance and allowing both to benefit from any future bug fixes.

b0f0b5324db02a8d20e78b2394f02b213be68794

Issues

Description	From	Last Updated
F401 'stat' imported but unused	reviewbot	Feb. 21, 2022, 7:11 p.m.
How does the performance of this particular usage compare to just using b'diff -r' in data?	david	Feb. 28, 2022, 6:21 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/scmtools/hg.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F401 'stat' imported but unused
```

Change Summary:

Removed an unused import.

Commits:

	Summary	ID
	Strip symlink mode information from files on Git-style Mercurial diffs. Git-style Mercurial diffs, like normal Git diffs, contain symlink mode information. We've hit issues in the past with the server applying patches containing symlink modes. Since a Git-style diff otherwise represents a symlink change as a standard file change, with the content being the old/new target names, we addressed this problem by stripping out the symlink mode and treating it as a normal file when patching for display in the diff viewer. This is by no means a perfect solution (that is in development, with initial steps having been done in Review Board 4.0.5), but it addresses the issue in Git. This change brings that fix over to Mercurial when working with Git-style diffs. The code in Git has been split out into a common function that both can call, simplifying maintenance and allowing both to benefit from any future bug fixes.	6aba5b5ff4de5218e0c19cb69b479c95d686d51d
	Strip symlink mode information from files on Git-style Mercurial diffs. Git-style Mercurial diffs, like normal Git diffs, contain symlink mode information. We've hit issues in the past with the server applying patches containing symlink modes. Since a Git-style diff otherwise represents a symlink change as a standard file change, with the content being the old/new target names, we addressed this problem by stripping out the symlink mode and treating it as a normal file when patching for display in the diff viewer. This is by no means a perfect solution (that is in development, with initial steps having been done in Review Board 4.0.5), but it addresses the issue in Git. This change brings that fix over to Mercurial when working with Git-style diffs. The code in Git has been split out into a common function that both can call, simplifying maintenance and allowing both to benefit from any future bug fixes.	b0f0b5324db02a8d20e78b2394f02b213be68794

Diff:

Revision 2 (+722 -48)

Show changes

	reviewboard/scmtools/git.py
	reviewboard/scmtools/hg.py
	reviewboard/scmtools/tests/test_git.py
	reviewboard/scmtools/tests/test_hg.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/scmtools/hg.py (Diff revision 2)

The issue has been dropped. Show all issues

How does the performance of this particular usage compare to just using b'diff -r' in data?

chipx86 Feb. 28, 2022, 6:21 p.m.

The important bit is the ordering. diff --git must be before a diff -r for it to be a git-style diff, and not after (it might be testing data within a diff).

And that's the bigger problem. It doesn't account for whether things are at the beginning of a line, or data within a diff.

This logic is not new, though, just moved. I don't feel comfortable redoing it for this change.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-4.0.x (c567608)