desktop / desktop/desktop

Incorrect content in partial commits in respect of identical lines at insertion boundaries

Open
#12,604 0 comments 0 reactions 0 assignees View on GitHub
investigation-needed
Dominant language
TypeScript
Stars
21.8k
Forks
10.5k
Avg merge
1d 15h
Merged PRs (30d)
25

Description

### Describe the bug

In some situations when doing a commit of portions of a textual file, the file as committed is not correct. If there are some identical lines (for instance, blank lines) both above and below an insertion, the number of these lines above versus below the insertion may differ both from what is shown and selected in GitHub Desktop’s Changes view, and what is possible based on the file’s contents in the work tree. The total number of boundary lines will be correct, but the number of them above versus below the insertion may be incorrect (for instance, 5+6 instead of 3+8), such that some boundary lines are effectively moved across the insertion.

The committed file does not match the one that would be committed by instead using `git add --patch` and making the same line selections in the configured text editor.

### Version & OS

GitHub Desktop Version 2.9.0 (x64). Windows 7 SP1. This test case was initially found in 2.8.0.

### Steps to reproduce the behavior

I’ve tried to make a simple reproduction but the issue seems to be sensitive to either unrelated content or some aspect of the hunk pattern that I can’t isolate. As such, I have to attach files similar to the actual case in which I encountered this. The issue is not sensitive to exact line content but instead to the pattern of lines, so I was able to sanitize and condense the document by replacing lines with their SHA-1 hashes.

Since the pattern of blank lines in the original document was significant, I have replaced the SHA-1 for a blank line with a special string that makes it easier to recognize. The fact that the lines were blank is not itself significant, because the issue reproduces regardless of what these lines actually contain, as long as they are all the same. But the issue is most likely to appear in real life on blank lines.

In the following steps and discussions, I list line content by the first four characters, which are unique here.

Mentioned files are in: [problem-text.zip](https://github.com/desktop/desktop/files/6811417/problem-text.zip)

1. Place the file `f1.txt` into the work tree of an empty repository as `f.txt` and commit it.

2. Delete `f.txt`, place `f2.txt` into the work tree, and rename it to `f.txt`.

3. Run GitHub Desktop, pick the repository, and look at changes to the file.

4. On the first hunk, `@@ -112,6 +112,28 @@`, select *all but* the first two insert lines (start at the `daa4` line).

5. On the second hunk, `@@ -183,7 +205,64 @@`, select *all but* the last three insert lines (stop before the `a99c` line).

6. On the second hunk, observe that the edits leave three `####` lines between the `6450` line and the `2746` line. To confirm this, you can expand the hunk upwards in the display such that it becomes `@@ -163,27 +185,84 @@`. Expanding the hunk upwards might not be possible unless you first fiddle with expanding other hunks later in the file — this may be a separate bug. But note that the outcome is the same regardless of whether any hunks are expanded or not. You can also confirm that there should be three `####` lines without having to expand any hunks, by checking the line numbers in an external editor. Also observe that there should be eight `####` lines committed between the `5f38` line and the `5dd8` line. The remaining steps list hunks before any expansions.

7. Select the whole third hunk, `@@ -278,14 +357,9 @@`.

8. On the fourth hunk, `@@ -306,7 +380,9 @@`, select the whole hunk *except for* the four insert lines at the end of the file, which start with the `e090` line.

9. Commit. The committed file is incorrect. (The resultant work tree diff will therefore now include some unexpected changes that would undo the errors if committed — specifically, the deletions of committed lines 206 and 207 and the insertions of work tree lines 266 and 267 — in addition to the expected changes from insertions left unselected in the above steps.)

### Expected behavior

The committed file should have three `####` lines between the `6450` line and the `2746` line, and eight `####` lines between the `5f38` line and the `5dd8` line.

This is what you actually do get if instead of committing via GHD, you use `git add --patch` and select the same lines as given in the reproduction steps above.

### Actual behavior

There will be five `####` lines between the `6450` line and the `2746` line in the committed file, instead of three, and six `####` lines between the `5f38` line and the `5dd8` line instead of eight.

Basically, the block inserted by the second hunk has been shifted down by two `####` lines compared to how it appeared in GHD before doing the commit, and how it appears in the work tree.

### Screenshots

This is what you see in Changes view (assuming you expand the hunk to see relevant context) after making the work tree changes in `f2.txt` and after change selection, but before committing:

![Changes view before commit - before.png](https://user-images.githubusercontent.com/84051370/125512689-6bddb02c-aa96-4f63-b3de-de3ca307a542.png)

After committing, this is what you see in the commit in History view, with the hunk expanded to see relevant context:

![History view of commit - commit.png](https://user-images.githubusercontent.com/84051370/125512858-f30699e8-fcb3-4590-8546-440806cb09b4.png)

In both of these screen shots, the overlaid pairs of yellow and blue arrows are placed on specific content lines and mark the “boundary” regions of interest, where the number of `####` lines is at issue. For each of the arrow pairs, the total number of context and selected lines between the arrows in the “Staging” picture should match the total number of context and insertion lines between the same arrows in the “Commit” picture. They do not.

### Logs

n/a

### Additional context

If instead of running GitHub Desktop, you examine the output of `git diff`, you can observe that its output is the same as the hunk patterns GHD shows. Likewise, `git add --patch` presents the same hunk patterns.

In the reproducing case, the second hunk shows line 230 in `f2.txt`, the `####` line between `e618` and `3494`, as a context line, even though it is in the middle of the inserted portion. This is not entirely unexpected in of itself, as Git and GitHub Desktop both generally prove fairly bad at delineating context intelligently, and the resulting patches should still be correct even if not very readable. However, in trying to find simpler reproductions of the present bug, I observed that it would either not reproduce or not do so in as clear a fashion if this spurious context line was instead a `+` line. *Simplifications in unrelated areas of the document, dozens of lines away from any of the changed hunks, often caused this line to flip to a* `+` *line.*

I encountered a similar situation a while back with Git itself, where Git was producing bogus merge conflicts when both sides of the merge had changed in the same way. It turned out to be because changes to unrelated parts of the document led to alterations in how the relevant hunk’s diff was laid out in terms of the ordering of `+` and `-` lines. This led to patches that were *equivalent* but differed *textually*, which triggered merge conflicts because Git was simply comparing *patch text*. I was not able to determine why the hunk layouts were changing. To be clear, this is not the same problem as the present bug, but it complicates reproduction and could have some relationship to it.

A probably important observation is that selecting the *whole* first hunk in step 4 avoids the error. But the exact number of lines left unselected on the first hunk doesn’t appear to change the number of `####` lines by which the second hunk’s insertion gets shifted, at least not with 3 or 4 lines instead of 2 lines.

#### Impact

On a number of prior occasions I have had the feeling that GHD did something wonky regarding partial committing, but wasn’t sure exactly. So I think this is maybe not as rare of an occurrence as the finickiness of reproduction would suggest. (But I do partial committing a lot.) In any case, any lapse of correctness in commits is a serious issue.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.