Fix resetting selected lines for renamed files in index #10252
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,9 +28,9 @@ public static class PatchManager | |
| } | ||
| } | ||
|
|
||
| public static byte[]? GetSelectedLinesAsPatch(string text, int selectionPosition, int selectionLength, bool isIndex, Encoding fileContentEncoding, bool reset, bool isNewFile, bool isRenamed) | ||
| { | ||
| IReadOnlyList<Chunk>? selectedChunks = GetSelectedChunks(text, selectionPosition, selectionLength, out string header); | ||
|
|
||
| if (selectedChunks is null || header is null) | ||
| { | ||
|
|
@@ -43,9 +43,15 @@ public static class PatchManager | |
| header = CorrectHeaderForNewFile(header); | ||
| } | ||
|
|
||
| // if file is renamed and selected lines are being reset from index then the patch undoes the rename too | ||
| if (isIndex && isRenamed && reset) | ||
| { | ||
| header = CorrectHeaderForRenamedFile(header); | ||
| } | ||
|
|
||
| string? body = ToIndexPatch(selectedChunks, isIndex, isWholeFile: false); | ||
|
|
||
| if (body is null) | ||
| { | ||
| return null; | ||
| } | ||
|
|
@@ -82,6 +88,72 @@ private static string CorrectHeaderForNewFile(string header) | |
| return sb.ToString(); | ||
| } | ||
|
|
||
| private static string CorrectHeaderForRenamedFile(string header) | ||
| { | ||
| // Expected input: | ||
| // | ||
| // diff --git a/original.txt b/original2.txt | ||
| // similarity index 88% | ||
| // rename from original.txt | ||
| // rename to original2.txt | ||
| // index 0e05069..d4029ea 100644 | ||
| // --- a/original.txt | ||
| // +++ b/original2.txt | ||
|
|
||
| // Expected output: | ||
| // | ||
| // diff --git a/original2.txt b/original2.txt | ||
| // index 0e05069..d4029ea 100644 | ||
| // --- a/original2.txt | ||
| // +++ b/original2.txt | ||
|
|
||
| string[] headerLines = header.Split(new[] { '\n' }); | ||
| string? oldNameWithPrefix = null; | ||
| string? newName = null; | ||
| foreach (string line in headerLines) | ||
| { | ||
| if (line.StartsWith("+++")) | ||
| { | ||
| // Takes the "original2.txt" part from patch file line: +++ b/original2.txt | ||
| newName = line[6..]; | ||
| } | ||
| else if (line.StartsWith("---")) | ||
| { | ||
| // Takes the "a/original.txt" part from patch file line: --- a/original.txt | ||
| oldNameWithPrefix = line[4..]; | ||
| } | ||
| } | ||
|
|
||
| StringBuilder sb = new(); | ||
|
|
||
| for (int i = 0; i < headerLines.Length; i++) | ||
| { | ||
| string line = headerLines[i]; | ||
| if (line.StartsWith("--- ")) | ||
| { | ||
| line = $"--- a/{newName}"; | ||
| } | ||
| else if (line.Contains($" {oldNameWithPrefix} ")) | ||
| { | ||
| line = line.Replace($" {oldNameWithPrefix} ", $" a/{newName} "); | ||
| } | ||
| else if (line.StartsWith("rename from ") || line.StartsWith("rename to ") || line.StartsWith("similarity index ")) | ||
|
There was a problem hiding this comment. Are any of these strings ever translated in Git? There was a problem hiding this comment. Since these are part of a diff format used by git and are called extended headers I think it's safe to assume that that wouldn't happen. At least documentation doesn't mention anything of that sort. There was a problem hiding this comment. It is described as a format, it is not even a 'requested' format, certainly not a porcelain interface. There was a problem hiding this comment. I'd like to push back if I may. Translation argument sounds almost like saying that translating browsers would mean translating HTTP Headers they send (custom ones, not covered by the standard). |
||
| { | ||
| // Note: this logic depends on git not localizing patch file format | ||
| continue; | ||
| } | ||
|
|
||
| if (i != 0) | ||
| { | ||
| sb.Append('\n'); | ||
| } | ||
|
|
||
| sb.Append(line); | ||
| } | ||
|
|
||
| return sb.ToString(); | ||
| } | ||
|
|
||
| public static byte[]? GetSelectedLinesAsNewPatch(GitModule module, string newFileName, string text, int selectionPosition, int selectionLength, Encoding fileContentEncoding, bool reset, byte[] filePreamble, string? treeGuid) | ||
| { | ||
| var selectedChunks = FromNewFile(module, text, selectionPosition, selectionLength, reset, filePreamble, fileContentEncoding); | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| using NUnit.Framework; | ||
|
|
||
| [SetUpFixture] | ||
| public class GlobalSetupOnce | ||
|
There was a problem hiding this comment. From the NUnit docs Since |
||
| { | ||
| [OneTimeSetUp] | ||
| public void RunBeforeAnyTests() | ||
| { | ||
| Application.EnableVisualStyles(); | ||
| Application.SetCompatibleTextRenderingDefault(false); | ||
| } | ||
|
|
||
| [OneTimeTearDown] | ||
| public void RunAfterAnyTests() | ||
| { | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a risky check. I believe that a/, b/ is forced right now in GE, but Git could change the output format any time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose that would be a serious breaking change (and not just for GE). Also git documentation mentions these prefixes explicitly https://git-scm.com/docs/diff-format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be changed with diff.mnemonicprefix=true. It was forced to false some time ago so a/b is likely stable.
But number of spaces etc are not porcelain and can be changed without notice.
It could be different in existing Git versions too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you approve of using regex here? Something like:
^---\s+(?<a>.+)/(?<aName>.+)$and^\+\+\+\s+(?<b>.+)/(?<bName>.+)$to capture a prefix used and a file name as well as make it more resilient to spacing.