check: Use --text when potentially diffing generated files #947
Conversation
The diffs generated from running `tools/check build_runner` were not detected. See comment: zulip#904 (comment) We normally exclude diffs from generated files through our `.gitattributes` configurations. This happens to `git --quiet -- [<path>...]` too. -a/--text allows us to include those files regardless. Signed-off-by: Zixuan James Li <[email protected]>
PIG208
added
a-tools
|
LGTM, marking for Greg's review. |
chrisbobbe
added
integration review
Normally in scripts I prefer using long options, to make the meaning more explicit for the reader. But this particular option `--text` on `git diff` has a pretty confusing name: it sounds like it potentially means "only look at text files", which is the opposite of what it turns out to mean. Plus its meaning really isn't about "text" (vs "binary") at all: the files we set the `-diff` attribute on are mostly just as textual as any others, just boring for diffs because they're generated. So use the short name `-a` instead. That suggests "all", which is exactly the right idea. Also expand the comment a bit.
|
Aha, that diagnosis makes sense! Thanks @PIG208 for investigating and fixing this. One thing that likely misled me, when I wrote the In particular the diffs to our So that would have caused the existing version of this code to work fine for me on those files, even while it didn't in CI or for other people locally. The changes all look good; thanks for the nice commit message. Merging, with an added NFC commit on top. |
The diffs generated from running
tools/check build_runnerwere not detected. See comment: #904 (comment)We normally exclude diffs from generated files through our
.gitattributesconfigurations. This happens togit --quiet -- [<path>...]too. -a/--text allows us to include those files regardless.