Make diff behave like diff(1); report consistent behaviors #628
Conversation
Signed-off-by: egibs <[email protected]>
pkg/render/terminal.go
Outdated
| @@ -319,6 +321,13 @@ func renderFileSummary(_ context.Context, fr *malcontent.FileReport, w io.Writer | |||
| content = fmt.Sprintf("%s%s%s %s %s", prefix, indent, bullet, rest, desc) | |||
| e = "" | |||
| } | |||
|
|
|||
| if b.NoDiff { | |||
| prefix = "~~~" | |||
There was a problem hiding this comment.
To increase familiarity with folks who review diffs from diff or GitHub, can we leave NoDiff lines without a prefix or special appearance? I like the disambiguation that ~~~ gives, but since it isn't used elsewhere, I think it will raise more questions than answers.
There was a problem hiding this comment.
Updated in 4d0921c (#628).
pkg/malcontent/malcontent.go
Outdated
| @@ -59,6 +59,7 @@ type Behavior struct { | |||
|
|
|||
| DiffAdded bool `json:",omitempty" yaml:",omitempty"` | |||
| DiffRemoved bool `json:",omitempty" yaml:",omitempty"` | |||
| NoDiff bool `json:",omitempty" yaml:",omitempty"` | |||
There was a problem hiding this comment.
Is NoDiff any different than an absence of DiffAdded or DiffRemoved? If it isn't, I'd recommend just leaving the public structure as is.
If we had good support for enums, this would be a perfect place to have one, since it could make for a confusing situation where all 3 bools are unset or set.
There was a problem hiding this comment.
It's not -- it's equivalent to both b.DiffRemoved and b.DiffAdded being false, so we can use that condition to check for behaviors that didn't change. I'll push that fix.
There was a problem hiding this comment.
Updated in 4d0921c (#628).
…haviors Signed-off-by: egibs <[email protected]>
Signed-off-by: egibs <[email protected]>
|
Diffing archives is exhibiting inconsistent behavior so I need to fix that. Edit: updated in |
Signed-off-by: egibs <[email protected]>
Signed-off-by: egibs <[email protected]>
|
This is huge - thank you! |
|
Will merge in a bit. Working on one last bug. |
Signed-off-by: egibs <[email protected]>
| @@ -190,7 +217,13 @@ func Diff(ctx context.Context, c malcontent.Config) (*malcontent.Report, error) | |||
| if srcFile != nil && destFile != nil { | |||
| formatSrc := displayPath(srcBase, srcFile.Path) | |||
| formatDest := displayPath(destBase, destFile.Path) | |||
| handleFile(ctx, c, srcFile, destFile, fmt.Sprintf("%s -> %s", formatSrc, formatDest), d) | |||
| if scoreFile(srcFile, destFile) { | |||
There was a problem hiding this comment.
This was the last remaining bug. Previously, we were hitting handleFile for the files we calculate moves for, so they would just show up as changed rather than moved. Now, we'll handle those files correctly via inferMoves while sending every other file to handleFile.
I also simplified the inferMove code path significantly.
With the clarification about diff(1) behavior in #599, I wanted to get something written up to address the current implementation gap.
This PR overhauls diff and tries to mimic what
diff(1)does --When diffing directories, the source file report is first compared to the destination report to identify matching files, followed by files only present in the source path. Afterward, the opposite is done to identify files that exist only in the destination path.
The
processSrc,processDest, andfileDestinationfunctions were confusing and I thinkhandleFiledoes everything we need for "modified" files. Otherwise, we're just directly adding reports to the Added/Removed map.I also started tracking consistent behaviors across modified files (originally called existing I think?) and I also updated the renderers to account for the new behaviors. Depending on the format, consistent behaviors will show with no
+or-. In the terminal, consistent behaviors will show up as cyan; the updated diff test data also contains these behaviors.Examples:
Two directories:
Two relative directories:
Two unrelated files:
Two unrelated files in the same parent:
Moving further down the directory structure:
Two directories that share a file of the same name:
Consistent archive diffs: