Add line Modification for diffs

[slack-coder]

When source code changes (patches) are proposed to a project, the review process tends to result in iterations on the patch. It is helpful to reviews to show only the changes between the iterations on a patch, the diff of the Patch diff.

Add a new line modification type, similar to the Modification type, which describes a line change between diffs.

[FintanH]

A few things:

1. Why are we writing the serde code directly?
2. Could we put this in another module and maybe re-export through diff?
3. Could we add tests to prove the functionality?

[slack-coder]

A few things:

1. Why are we writing the serde code directly?

Modification appears to do it, I copied from it:

https://github.com/radicle-dev/radicle-git/blob/main/radicle-surf/src/diff.rs#L468-L504

2. Could we put this in another module and maybe re-export through `diff`?

👍

3. Could we add tests to prove the functionality?

👍

[slack-coder]

3. Could we add tests to prove the functionality?

This brings up the question of is radicle-surf a good home for it?

We have the problem of generating data for the test. diff.rs's tests use git2 and the GIT_PLATINUM repository for sourcing data. DDiff's (diff of diffs) are only a git concept through the git-range-diff for -showing- changes. Defining them in surf would be for a similar purpose.

The way moving forward could be to (something like) implement a TryFrom for converting Diff's to DDiff's, and test via loading it from a string, similar to test_none_missing_eof_newline, as a DDiff is a special diff type.

[FintanH]

Modification appears to do it, I copied from it:

https://github.com/radicle-dev/radicle-git/blob/main/radicle-surf/src/diff.rs#L468-L504

Ah yes, that was after much debate around encoding the enum type I believe. Fair enough 👌

[FintanH]

The way moving forward could be to (something like) implement a TryFrom for converting Diff's to DDiff's, and test via loading it from a string, similar to test_none_missing_eof_newline, as a DDiff is a special diff type.

This sounds reasonable. What are the alternatives? What would it look like if it didn't live in surf?

[slack-coder]

One alternative is to build code to diff diffs into Radicle-surf, heavy handed in my opinion. Though Surf is advertised as a way to surf git repositories, and maybe diffing diffs is a way to surf?

If the code lived elsewhere, we would be able to handle DDiffs on the Hunk level thanks to Surf's abstraction. This ends on the file level. DiffContent enforces a certain Hunk type, which makes sense. DiffContent::Plain should contain Hunk<Modification>. Downstream (radicle) would use homegrown versions the FileDiff + Diff types, or wrap Surf's FileDiff type and convert Hunk<Modification> <-> Hunk<DiffModification> internally.

Or allow consumers to define their own DiffContent via a type parameter somewhere?

[FintanH]

One alternative is to build code to diff diffs into Radicle-surf, heavy handed in my opinion. Though Surf is advertised as a way to surf git repositories, and maybe diffing diffs is a way to surf?

Right, I wouldn't say it's a top priority -- but being able to parse a range-diff definitely fits :)

On a side note: can we call this a RangeDiff rather DDiff? Or can range diffs look like something else, e.g. a regular diff?

If the code lived elsewhere, we would be able to handle DDiffs on the Hunk level thanks to Surf's abstraction. This ends on the file level. DiffContent enforces a certain Hunk type, which makes sense. DiffContent::Plain should contain Hunk. Downstream (radicle) would use homegrown versions the FileDiff + Diff types, or wrap Surf's FileDiff type and convert Hunk <-> Hunk internally.

Could we continue with the abstraction in surf and then export Diff and RangeDiff at the top level as concrete types, i.e. with Modification and DiffModification?

[slack-coder]

On a side note: can we call this a RangeDiff rather DDiff? Or can range diffs look like something else, e.g. a regular diff?

They are slightly different, 'collection' vs. 'element'. Range-diff, as per git-range-diff, is about ordering and matching commits between git histories. In code a 'diff of diff' would be Diff. The term is for discussion only :).

Could we continue with the abstraction in surf and then export Diff and RangeDiff at the top level as concrete types, i.e. with Modification and DiffModification?

👍

Or allow consumers to define their own DiffContent via a type parameter somewhere?

Stepping back, how about opening up DiffContent's contents to be defined by downstream?

Surf defines an encapsulating Diff type and its file associated types (FileDiff + diff::Modified). Then there are the modification between files (currently Hunks). Could the FileDiff and diff::Modified types be parameterized? Downstream applications could use this to define custom types for describing differences suited to a format's domain (images, word documents, etc.?).

[FintanH]

They are slightly different, 'collection' vs. 'element'. Range-diff, as per git-range-diff, is about ordering and matching commits between git histories. In code a 'diff of diff' would be Diff. The term is for discussion only :).

Oh so you're essentially getting two diffs and then diff them? Like if you were to save the two diffs as blobs you would then diff those two blobs?

Surf defines an encapsulating Diff type and its file associated types (FileDiff + diff::Modified). Then there are the modification between files (currently Hunks). Could the FileDiff and diff::Modified types be parameterized? Downstream applications could use this to define custom types for describing differences suited to a format's domain (images, word documents, etc.?).

I'm doubtful about the data types being flexible enough to describe diffs other than text files 😅

[slack-coder]

Oh so you're essentially getting two diffs and then diff them? Like if you were to save the two diffs as blobs you would then diff those two blobs?

That's it, but using a diff format for diffs.

I'm doubtful about the data types being flexible enough to describe diffs other than text files sweat_smile

Fair enough.

These types are being included as a Patch into heartwood. Let's see how they go there, we can eventually bubble up parts to radicle-surf as things mature. I'm closing the PR for now.

Thanks for the help 👍