| Message ID | 500a8e0a-9fbd-4b7b-b2f2-026a4293bc9f@web.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | diff: report modified binary files as changes in builtin_diff() | expand |
Am 21.09.2024 um 17:09 schrieb René Scharfe: Hi René, > diff --git a/diff.c b/diff.c > index 3be927b073..84a6bb0868 100644 > --- a/diff.c > +++ b/diff.c > @@ -3675,6 +3675,7 @@ static void builtin_diff(const char *name_a, > emit_diff_symbol(o, DIFF_SYMBOL_BINARY_FILES, > sb.buf, sb.len, 0); > strbuf_release(&sb); > + o->found_changes = 1; > goto free_ab_and_return; > } > if (fill_mmfile(o->repo, &mf1, one) < 0 || I poked at the same issue in parallel and had the same fix, but ... > diff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh > index d644310e22..1cea73ef5a 100755 > --- a/t/t4017-diff-retval.sh > +++ b/t/t4017-diff-retval.sh > @@ -145,6 +145,14 @@ test_expect_success 'option errors are not confused by --exit-code' ' > > for option in --exit-code --quiet > do > + test_expect_success "git diff $option returns 1 for changed binary file" " > + test_when_finished 'rm -f .gitattributes' && > + git reset --hard && > + echo a binary >.gitattributes && > + echo 2 >>a && > + test_expect_code 1 git diff $option > + " > + > test_expect_success "git diff $option returns 1 for copied file" " > git reset --hard && > cp a copy && your test is nicer. The patch works here locally. For what it's worth: Reviewed-by: Thomas Braun <thomas.braun@virtuell-zuhause.de> Thomas
Thomas Braun <thomas.braun@virtuell-zuhause.de> writes: > Am 21.09.2024 um 17:09 schrieb René Scharfe: > > Hi René, > >> diff --git a/diff.c b/diff.c >> index 3be927b073..84a6bb0868 100644 >> --- a/diff.c >> +++ b/diff.c >> @@ -3675,6 +3675,7 @@ static void builtin_diff(const char *name_a, >> emit_diff_symbol(o, DIFF_SYMBOL_BINARY_FILES, >> sb.buf, sb.len, 0); >> strbuf_release(&sb); >> + o->found_changes = 1; >> goto free_ab_and_return; >> } >> if (fill_mmfile(o->repo, &mf1, one) < 0 || > > I poked at the same issue in parallel and had the same fix, but ... > >> ... >> test_expect_success "git diff $option returns 1 for copied file" " >> git reset --hard && >> cp a copy && > > your test is nicer. > > The patch works here locally. > > For what it's worth: > > Reviewed-by: Thomas Braun <thomas.braun@virtuell-zuhause.de> > > Thomas Thanks.
diff --git a/diff.c b/diff.c index 3be927b073..84a6bb0868 100644 --- a/diff.c +++ b/diff.c @@ -3675,6 +3675,7 @@ static void builtin_diff(const char *name_a, emit_diff_symbol(o, DIFF_SYMBOL_BINARY_FILES, sb.buf, sb.len, 0); strbuf_release(&sb); + o->found_changes = 1; goto free_ab_and_return; } if (fill_mmfile(o->repo, &mf1, one) < 0 || diff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh index d644310e22..1cea73ef5a 100755 --- a/t/t4017-diff-retval.sh +++ b/t/t4017-diff-retval.sh @@ -145,6 +145,14 @@ test_expect_success 'option errors are not confused by --exit-code' ' for option in --exit-code --quiet do + test_expect_success "git diff $option returns 1 for changed binary file" " + test_when_finished 'rm -f .gitattributes' && + git reset --hard && + echo a binary >.gitattributes && + echo 2 >>a && + test_expect_code 1 git diff $option + " + test_expect_success "git diff $option returns 1 for copied file" " git reset --hard && cp a copy &&
The diff machinery has two ways to detect changes to set the exit code: Just comparing hashes and comparing blob contents. The latter is needed if certain changes have to be ignored, e.g. with --ignore-space-change or --ignore-matching-lines. It's enabled by the diff_options flag diff_from_contents. The code for handling binary files added by 1aaf69e669 (diff: shortcut for diff'ing two binary SHA-1 objects, 2014-08-16) always uses a quick hash-only comparison, even if the slow way is taken. We need it to report a hash difference as a change for the purpose of setting the exit code, though, but it never did. Fix that. d7b97b7185 (diff: let external diffs report that changes are uninteresting, 2024-06-09) set diff_from_contents if external diff programs are allowed. This is the default e.g. for git diff, and so that change exposed the inconsistency much more widely. Reported-by: Kohei Shibata <shiba200712@gmail.com> Signed-off-by: René Scharfe <l.s.r@web.de> --- Thank you for the report! diff.c | 1 + t/t4017-diff-retval.sh | 8 ++++++++ 2 files changed, 9 insertions(+) -- 2.46.0