| Message ID | xmqq4kkl1atq.fsf@gitster.c.googlers.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d7cd43984a9890034a9210099ddeb874d12d93e4 |
| Headers | show |
| Series | Re* [PATCH] diff: suppress --name-only paths where all hunks are ignored | expand |
On Wed, Dec 16, 2020 at 05:27:13PM -0800, Junio C Hamano wrote: > Johannes Altmanninger <aclopte@gmail.com> writes: > > The diff_from_contents bit means that we have to look at content > > changes to know if a path has changed - modulo ignored hunks. > > But your sentence without " - modulo ignored hunks" says that very > clearly. So perhaps these three words can just go away? My bad, this should rather be The diff_from_contents bit means that we have to look at content changes (modulo ignored hunks) to know if a path has changed. probably dropping the three words makes it even less confusing. > It's not like we were buggy when diff_from_contents bit is in effect > for all output formats, is it? Right, it's useless to set the bit here. > > + opt->color_moved = 0; > > Why is color_moved so special? If we added some other new option, > how would we make sure that the person who is adding that new option > would remember to reset it here? And how does that person decide if > his or her new option needs resetting or not in the first place? I copied color_moved=0 from the DIFF_FORMAT_NO_OUTPUT section of diff_flush(). It happens to work correctly in both places because no diff contents are printed, but yeah it's not robust to future changes. > Also I am not sure if "caching" the file handle to /dev/null is > good idea or not. When will it be closed? Would repeated > invocation of the diff machinery work well with it (think: "git log > -w --name-only") I believe that repeated invocations work fine, because the file is never closed (which may be a problem?). The file could be made nilable if we still need something like that. > This somehow feels wrong. For one thing, RAW is about showing > object names of preimage and postimage, which means it shows the > preimage and postimage are either identical or different, and > options like -w, -I, etc. that inspect and hide some textual > differences should not affect its outcome at all. Yeah, I realized late that -w --raw will have sort of unintuitive semantics. This combination is probably rare but I guess users could set an alias for diff -w. > I am tempted to declare that just like "raw", "name-only" and > "name-status" formats work with byte-for-byte equality OK, I think it's better to keep the current (consistent) behavior. I don't think it's worth to special-case "--name-only". It's easy to read the names from a "diff -I" output anyway. This filter is broken in various ways, but works well enough in practise: diff -I | perl -ne 'print "$1\n" if /\+{3} b\/(.*)/' > and "-w" and friends are ignored just like in "diff --name-status --patch" > the "--patch" option is ignored. For interactive use, it would be more helpful to reject useless options instead of ignoring them. Though ignoring is probably desirable to allow liberal use of these options, I'm not sure. > --- >8 ------ >8 ------ >8 ------ >8 ------ >8 ------ >8 ------ >8 --- > > Subject: [PATCH] diff: correct interaction between --exit-code and -I<pattern> Looks good. > +test_expect_success '-w and --exit-code interact sensibly' ' Maybe 'exit with 0 when all changes are ignored by -w' though either version is fine because I think the intention of the test is already obvious.
Johannes Altmanninger <aclopte@gmail.com> writes: >> +test_expect_success '-w and --exit-code interact sensibly' ' > > Maybe 'exit with 0 when all changes are ignored by -w' though either version > is fine because I think the intention of the test is already obvious. Yeah, 'sensibly' is a zero-bit phrase and the letters are better spent on describing what we deem sensible more clearly. Thanks.
diff --git a/diff.c b/diff.c index 9768d8eab4..69e3bc00ed 100644 --- a/diff.c +++ b/diff.c @@ -4637,7 +4637,8 @@ void diff_setup_done(struct diff_options *options) * inside contents. */ - if ((options->xdl_opts & XDF_WHITESPACE_FLAGS)) + if ((options->xdl_opts & XDF_WHITESPACE_FLAGS) || + options->ignore_regex_nr) options->flags.diff_from_contents = 1; else options->flags.diff_from_contents = 0; diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh index 47f0e2889d..8c574221b2 100755 --- a/t/t4015-diff-whitespace.sh +++ b/t/t4015-diff-whitespace.sh @@ -567,6 +567,30 @@ test_expect_success '--check and --quiet are not exclusive' ' git diff --check --quiet ' +test_expect_success '-w and --exit-code interact sensibly' ' + test_when_finished "git checkout x" && + { + test_seq 15 && + echo " 16" + } >x && + test_must_fail git diff --exit-code && + git diff -w >actual && + test_must_be_empty actual && + git diff -w --exit-code +' + +test_expect_success '-I and --exit-code interact sensibly' ' + test_when_finished "git checkout x" && + { + test_seq 15 && + echo " 16" + } >x && + test_must_fail git diff --exit-code && + git diff -I. >actual && + test_must_be_empty actual && + git diff -I. --exit-code +' + test_expect_success 'check staged with no whitespace errors' ' echo "foo();" >x && git add x &&