feat: ability to show diff when failing on changes#1227
feat: ability to show diff when failing on changes#1227mrexox merged 2 commits intoevilmartians:masterfrom
Conversation
scop
force-pushed
the
feat/fail-on-changes-diff
branch
4 times, most recently
from
45ab893 to
361f71f
Compare
scop
force-pushed
the
feat/fail-on-changes-diff
branch
from
361f71f to
d57cdd8
Compare
| return nil | ||
| } | ||
|
|
||
| func (g *guard) changesetDiff(changesetAfter map[string]string) { |
There was a problem hiding this comment.
I don't particularly like the interface of the method here, I think I would have rather had it return the diff and an error, and checked g.failOnChangesDiff at the call site. However exposing more ifs in after() runs afoul with the nestif linter. But nevermind, I can live with this :)
| if !g.failOnChangesDiff { | ||
| return | ||
| } | ||
| changed := make([]string, 0, len(g.changesetBefore)) |
There was a problem hiding this comment.
We don't currently meet the test coverage target. Calculating changed could be extracted to another function and tests added for it if seen better that way to catch up some. That calculation isn't that complex though, fine with me either way, let me know.
| if diff, err := g.git.Git.BatchedCmd(diffCmd, changed); err != nil { | ||
| log.Warnf("Couldn't diff changed files: %s", err) | ||
| } else { | ||
| log.Println(diff) |
There was a problem hiding this comment.
As long as we are outputting the diff content from here (and not returning as a string as mentioned in an earlier comment above), we could just let the git diff output out directly. Capturing and re-printing does not add value here, just some memory consumption. BatchedCmd and Cmd lack the ability to do that at the moment though -- should we add that and make use of it here, or do you think this is fine for now?
mrexox
left a comment
mrexox
left a comment
There was a problem hiding this comment.
I like this feature! What do you think about always showing diff on fail changes? If needed it may be controlled by output setting, but I think in most cases you'd like to see what has changed.
I can work on diff styling later to display it not as the command output, but as lefthook output (with leading | ).
As noted in #1171, talking about the default: "i.e. not show the diff if not in CI, because one can easily figure out locally if need be, and the diff output may wreak havoc in the tool doing the commit/showing its output" For example, if an IDE shows the Lefthook output in a pop-up dialog, or something like https://git-cola.github.io shows it, having the diff in it may become quite a mess. (Even the usual "fancy" Lefthook output we have tends to do that.) And the diff may be huge, etc etc.
FWIW, I would personally prefer it to be shown as possibly colorized but otherwise unstyled plain diff. |
2 participants
Closes #1171
Draft: untested.Tested now.Context
See above issue for discussion.
Changes
Add a cmdline flag and config option for controlling the behavior, default to on in CI.