diff: symlinks with external diff tools produce invalid directory structures

[fowles]

Config.toml:

[ui.diff]
tool = "ls"

[merge-tools.ls]
diff-args = ["-la", "$left", "$right"]

Setup dir

$ ln -s .gitignore .ignore

Behold

jj diff --tool ls
.../left:
total 0
drwxr-xr-x  2 beaker  staff   64 Jun 25 15:22 .
drwxr-xr-x  6 beaker  staff  192 Jun 25 15:22 ..

.../right:
total 0
drwxr-xr-x  3 beaker  staff   96 Jun 25 15:22 .
drwxr-xr-x  6 beaker  staff  192 Jun 25 15:22 ..
lrwxr-xr-x  1 beaker  staff   10 Jun 25 15:22 .ignore -> .gitignore

[ilyagr]

I'm not sure I'm following, I may have missed the point. Assuming .gitignore doesn't exist, this behavior seems like what I would expect. Even if it does exist, including it does not seem essential for correctness (as long as it didn't change), though it might be a nice convenience.

[fowles]

It does exist in the repo, just not in the tiny directory set up for the diff tool. The tiny directory is setup with a dangling symlink

[ilyagr]

In my mind, this is not too bad. It feels similar to how ln -s abra cadabra works regardless of whether abra exists.

OTOH, it might be a nice convenience feature to include .gitignore in the tiny directory, at least I can't think of a reason not to. We just have to make sure it gets included in both left and right.

[yuja]

The current external diff interface expects the tool can diff symlinks (rather than files targeted by the symlinks.)

Maybe we could add extra logic to copy symlink targets if existed in the repo, but I don't think it's useful. I think it's wrong for diff backend of VCS to follow symlinks, and we can't catch target changes if symlink is unchanged.

[ilyagr]

we can't catch target changes if symlink is unchanged.

This is a fair point: if we make the change and the users get used to the targets showing up when their symlinks change, would they expect the symlinks to show up when the targets change? If so, that would be bad, as we can't efficiently accomplish that, and doing too much in that direction seems to take us further afield of what a diff tool's job is.

AFAIU, the best way to understand symlinks on UNIX is as "a file with symlink attribute set that, by convention, usually contains a path", and there are no guarantees about what if anything that path points to. With the major exception of opening a file for reading, most Unix utils seem to treat symlinks that way most of the time.

[fowles]

rather than make a dangling symlink, we could make a text file where the text is the target of the symlink (or something similar)

[ilyagr]

Yes, we could. I think this is closely related with supporting filesystems that don't understand symlinks (e.g. FAT32 or Windows without Developer Mode on), #2 . If that's implemented (actually, it already is partially at least, I don't remember to what degree: #2939), we could support diff tools that don't understand symlinks in the same way.

[yuja]

rather than make a dangling symlink, we could make a text file where the text is the target of the symlink (or something similar)

That could be an opt-in feature. I'm against making it the default because diff backend can show file type and mode changes if supported.

[fowles]

For what its worth, writing the target of the symlink to a file is precisely what git does for diffing symlinks.

[martinvonz]

For what its worth, writing the target of the symlink to a file is precisely what git does for diffing symlinks.

It's what jj does too. For example:

FYI, the tiny directories given to the diff editor are actually implemented as very sparse working copies. If we want to write symlinks as files there, we would presumably implement it by adding an intermediate step where create new trees with the relevant symlinks replaced by files, and then we do the reverse transformation for those files after snapshotting the temporary working copies. I'm far from convinced it's worth it, though.

[fowles]

What do you mean by the reverse transformation? Are those directories read/write in some fashion?

[martinvonz]

What do you mean by the reverse transformation? Are those directories read/write in some fashion?

Oh, I didn't realize this was thread was just about viewing diffs. They are readonly in jj diff --tool, I think, but (IIRC) it's largely the same code as what powers all the interactive commands, like jj split, jj diffedit, jj squash -i, etc. The tiny sparse working copies are snapshotted just like normal working copies after we let the user edit them.

We probably want the same behavior w.r.t symlinks from jj diff --tool as in jj diffedit.

[fowles]

I was lightly planning on mucking with those directories to make copy tracking work correctly too.

The other alternative is to make the tool thing not always do directory mode but to let it do a file-by-file mode

[yuja]

For what its worth, writing the target of the symlink to a file is precisely what git does for diffing symlinks.

That might be because external diff interface of git is file-oriented? jj does dir-diff by default, and there's no file-diffs support yet.

Anyway, I'm not against adding a config knob to materialize symlink as plain file.

[fowles]

I think I am going to need file-diff for my next adventure anyway, so I might implement that instead

[fowles]

#3982 implements file-by-file mode since that is a pre-req for better copy tracing anyway