Conflicts at the end of files that don't end in a newline are not materialized correctly #3968
Comments
|
I think pretending that the file had newlines is my preference. This will only matter if the user edits the conflicts manually, in which case the file was probably meant to end in a newline anyway. There is one alternative on my mind, but I don't like it: The fish shell prints $ echo -n aa
aa⏎
$ # Next command hereWe could in principle use a trick like that (either with |
|
Mercurial adds "\ No newline at end of file" in diffs. We could copy that. I'm fine with either that or adding just a newline as you suggested. |
|
I like the idea of using the existing label syntax; that avoids introducing any more ambiguity with change contents that isn’t already present. The two sides could disagree on the presence of the newline, right? If putting it at the end of the label line attached to the relevant text is too subtle, maybe another label line could be added? |
|
IIUC the main idea in the thread so far is to only render the lack of a newline, but not necessarily be able to operate on it in a principled fashion? Shouldn't the lack of a newline be encoded in the conflict marker syntax itself? It seems syntactically important. Wouldn't you want to preserve the invariant that you can reconstruct any of the file sides directly from the conflicted contents, rather than having to consult an out-of-band store? |
|
UPDATE: I decided to promote this to a separate issue, #3975.
This issue has been nerd-sniping me for a while; in addition to trailing newlines, it's also a matter of encoding conflict markers inside files TODO: find link. I remember discussing it with Martin recently in some PR, haven't found it yet. TLDR: In the shortest term, I'll probably ignore both issues and just add newlines to the end of files. However, I think I finally figured out a syntax that will work, will not require a complicated parser, and even be somewhat readable. Before computing conflicts (and only if a file is conflicted), we'd encode file contents as follows:
Fun, huh? This relies on conflict markers having to start at the beginning of the line. If we ever have a notion of conflicts that happen in the middle of lines, we'd need a different syntax, of course. I think that would be nice, but also would probably need a whole different UI to be usable; I'm not sure it can be very usable in the terminal or as a text file. |
This happens for non-empty files only; the materialization code can handle files and hunks with 0 or more lines, but each line should terminate in a newline. This is an imperfect fix to jj-vcs#3968, a better fix is TODO; see jj-vcs#3975.
This happens for non-empty files only; the materialization code can handle files and hunks with 0 or more lines, but each line should terminate in a newline. This is an imperfect fix to jj-vcs#3968, a better fix is TODO; see jj-vcs#3975.
This happens for non-empty files only; the materialization code can handle files and hunks with 0 or more lines, but each line should terminate in a newline. This is an imperfect fix to jj-vcs#3968, a better fix is TODO; see jj-vcs#3975. This also implements FromIterator for Merge. I could split it into a separate commit, but then it'd have no uses.
This happens for non-empty files only; the materialization code can handle files and hunks with 0 or more lines, but each line should terminate in a newline. This is an imperfect fix to jj-vcs#3968, a better fix is TODO; see jj-vcs#3975. This also implements FromIterator for Merge. I could split it into a separate commit, but then it'd have no uses.
This happens for non-empty files only; the materialization code can handle files and hunks with 0 or more lines, but each line should terminate in a newline. This is an imperfect fix to jj-vcs#3968, a better fix is TODO; see jj-vcs#3975. This also implements FromIterator for Merge. I could split it into a separate commit, but then it'd have no uses.
This happens for non-empty files only; the materialization code can handle files and hunks with 0 or more lines, but each line should terminate in a newline. This is an imperfect fix to jj-vcs#3968, a better fix is TODO; see jj-vcs#3975. This also implements FromIterator for Merge. I could split it into a separate commit, but then it'd have no uses.
|
Somebody asked a few days ago whether this will handle the similar problem with diffs, but then seemed to delete their comment. Shamelessly stealing their example from my email, it was (after minor edits): $ jj diff
Modified regular file .gitignore:
1 1: build
2: ignoreAdded regular file .vscode/settings.json:
1: {
2: "spellright.language": [
3: "en"
4: ],
5: }I don't actually have an answer (it may or may not be part of the same solution, and it should look similar), but we certainly shouldn't forget about that. |
|
Hi, that was me, and I deleted it because I noticed I was a version behind, so I upgraded and 0.21.0 handles it just fine, so it seems like there's no extra work to be done here! |
|
I think @yuja may have fixed it without me noticing. Thank you, Yuya! |
|
It seems that our colored_words diff is currently a bit lossy in this case. This wouldn't often come up, but it might be confusing occasionally. We could add a message (e.g. "Added a newline at the end of file" in green or "Removed a newline at the end of file" in red), perhaps, to clarify when there is a diff in whether there is a newline. I don't know whether I'd get to it anytime soon, though. I also think the 
|
|
Another alternative for the colored words diff would be to do what the fish shell does, and represent a lack of a newline at EOL by the 
I'm not sure how clear this is, but at least there is precedent. (Also, this entire problem is less important than the original problem of malformed diff) |
Yes, maybe we can add some marker or header/footer message. Its output doesn't have to be parsable, so we can freely choose the syntax. |
|
I have another possible solution that I just thought of, but I'm not sure if it's too complicated. Basically, we introduce a new conflict marker Here's an example of what a conflict might look like, where the base and side 1 don't have a newline at the end of the file, but side 2 adds a newline: Or if the base has a newline at the end of the file, but both sides remove it: So basically if any non-empty side/base of the conflict is missing a trailing newline, then we add a newline to every side/base in the conflict before we materialize it, and we add a I think this has some nice benefits:
|
|
I don't have a strong opinion, but I think abusing comment part might be also good, something like Maybe we can also restore the EOL state from the original conflict contents (without encoding it in the conflict marker.) It's unlikely that user wants to modify EOL in conflicts without resolving it. |
How is that different in the
I think it might be nice to modify the conflict marker format in the same line, because we can add any number of metadata parts without needing a corresponding number of extra metadata lines. Some other attributes we could include in this way:
Of course, if we choose to store structured metadata in this way, we should establish a real specification for the format. |
I meant for within the conflicts markers themselves under the Is clearer than this: I think @yuja was suggesting also it could be displayed like this: Which I also think is a bit clearer, but it suffers from the same double-negative (to me, |
Actual Result
If the file is subsequently edited,
jjcannot parse the conflict correctly.Expected Behavior
I'd materialize the conflict as though the files ended in newlines, if we decide this is acceptable. We could perhaps notify the user about this in the labels:
If this is not acceptable, I'm not sure what is best to do.
The text was updated successfully, but these errors were encountered: