Regression in `git diff --quiet HEAD` when a new file is staged

Regression in `git diff --quiet HEAD` when a new file is staged

[Jake Zimmerman]

In git v2.51.1, `git diff --quiet HEAD` will actually print something
if the diff output includes a new, staged file.

## To reproduce

    ❯ mkdir foo
    ❯ cd foo
    ❯ git init .
    Initialized empty Git repository in /Users/jez/foo/.git/
    ❯ gc --allow-empty -m "Initial empty commit"
    [master (root-commit) 858966f] Initial empty commit
    ❯ touch foo.txt
    ❯ git add foo.txt
    ❯ git diff --quiet HEAD

On git v2.51.0, the output of the last command is empty.
On git v2.51.1, the output of the last command is this:

    diff --git a/foo.txt b/foo.txt
    new file mode 100644
    index 0000000..e69de29

## Expected behavior

The stated docs for `--quiet`: "Disable all output of the program," so
I expect there to be no output, like in older versions.

## Likely cause

I ran a git bisect and isolated this commit:

b55e6d36ebce69136559add8fffd1a65df231518
( https://github.com/git/git/commit/e1d3d61a45bfdc5031d2066c0e4505ebd8145777 )

"diff: ensure consistent diff behavior with ignore options"

Re: Regression in `git diff --quiet HEAD` when a new file is staged

[Jeff King]

On Thu, Oct 16, 2025 at 05:09:07PM -0700, Jake Zimmerman wrote:

> In git v2.51.1, `git diff --quiet HEAD` will actually print something
> if the diff output includes a new, staged file.
> [...]
> I ran a git bisect and isolated this commit:
> b55e6d36ebce69136559add8fffd1a65df231518

Yikes, that is a pretty bad regression. I'm rather surprised that this
wasn't covered in the test suite. t4035 does set this situation up, but
it checks with git-diff-tree, not git-diff. I initially thought that was
because diff defaults to "--patch" output and diff-tree does not, but
even "diff-tree --patch" does not show the bug. Weird. Maybe it has to
do with running diffcore bits?

I see that the author of b55e6d36eb (diff: ensure consistent diff
behavior with ignore options, 2025-08-08) posted this patch earlier
today:

  https://lore.kernel.org/git/pull.2071.git.git.1760671049113.gitgitgadget@gmail.com/

which seems to fix it, but there's no mention there of this thread. And
the included test is still using "-I", where there is clearly collateral
damage even for people who are not using "-I" at all. So I'm not sure if
it's coincidence, or meant to be a fix. ;)

Looking at that patch, my biggest concern is: are we missing other spots
that need to special-case the dry_run setting? Because it's a regression
in a maint release, I'm tempted to say we should do the dumbest possible
thing that covers all cases and just revert this hunk from the original
patch, like:

diff --git a/diff.c b/diff.c
index 87fa16b730..687206f353 100644
--- a/diff.c
+++ b/diff.c
@@ -6890,6 +6890,15 @@ void diff_flush(struct diff_options *options)
 	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
 	    options->flags.exit_with_status &&
 	    options->flags.diff_from_contents) {
+		/*
+		 * run diff_flush_patch for the exit status. setting
+		 * options->file to /dev/null should be safe, because we
+		 * aren't supposed to produce any output anyway.
+		 */
+		diff_free_file(options);
+		options->file = xfopen("/dev/null", "w");
+		options->close_file = 1;
+		options->color_moved = 0;
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
 			if (check_pair_status(p))

That would catch the bug here, as well as any others lurking. And it
converts any missing dry_run from correctness problems (we definitely
will not produce extra output) into optimization problems (we might emit
data we do not need, but we can fix those separately). At least for the
normal code paths. I think without those extra fixes the problems that
b55e6d36eb tried to fix for "-I" would still be observable, but at least
its fixes could not regress the other code paths.

-Peff

[PATCH] diff: restore redirection to /dev/null for diff_from_contents

[Jeff King]

On Fri, Oct 17, 2025 at 03:51:53AM -0400, Jeff King wrote:

> On Thu, Oct 16, 2025 at 05:09:07PM -0700, Jake Zimmerman wrote:
> 
> > In git v2.51.1, `git diff --quiet HEAD` will actually print something
> > if the diff output includes a new, staged file.
> > [...]
> > I ran a git bisect and isolated this commit:
> > b55e6d36ebce69136559add8fffd1a65df231518
> 
> Yikes, that is a pretty bad regression. I'm rather surprised that this
> wasn't covered in the test suite. t4035 does set this situation up, but
> it checks with git-diff-tree, not git-diff. I initially thought that was
> because diff defaults to "--patch" output and diff-tree does not, but
> even "diff-tree --patch" does not show the bug. Weird. Maybe it has to
> do with running diffcore bits?

Ah, I see. It is because porcelain diff has --ext-diff turned on by
default. And that triggers content-level diffs due to this bit in
diff_setup_done():

          /*
           * External diffs could declare non-identical contents equal
           * (think diff --ignore-space-change).
           */
          if (options->flags.allow_external && options->flags.exit_with_status)
                  options->flags.diff_from_contents = 1;

The really gross part, of course, is that this triggers even when you do
not have any external diff commands defined, because we don't find out
about them until flushing individual pairs!

I suspect that things could be improved there. Once we're in
run_diff_cmd() and realize that no, we don't have have an external diff
command, I think we still run the actual diff anyway, not realizing that
we are only here on a contingency that is not true. So we produce the
diff and throw it away, but could return early. But again, that's an
optimization issue, not a correctness one (and it has been that way for
many years, so perhaps nobody cares too much).

I also suspect that textconv should get the same treatment (you could
define a textconv that turns two distinct binary blobs into an identical
text, so we should trigger a content diff for that). But again, it has
been that way for years.

> Looking at that patch, my biggest concern is: are we missing other spots
> that need to special-case the dry_run setting? Because it's a regression
> in a maint release, I'm tempted to say we should do the dumbest possible
> thing that covers all cases and just revert this hunk from the original
> patch, like:

Here it is with a commit message and test, in case that is helpful.

-- >8 --
Subject: [PATCH] diff: restore redirection to /dev/null for diff_from_contents

In --quiet mode, since we produce only an exit code for "something was
changed" and no actual output, we can often get by with just a
tree-level diff. However, certain options require us to actually look at
the file contents (e.g., if we are ignoring whitespace changes). We have
a flag "diff_from_contents" for that, and if it is set we call
diff_flush() on each path.

To avoid producing any output (since we were asked to be --quiet), we
traditionally just redirected the output to /dev/null. That changed in
b55e6d36eb (diff: ensure consistent diff behavior with ignore options,
2025-08-08), which replaced that with a "dry_run" flag. In theory, with
dry_run set, we should produce no output. But it carries a risk of
regression: if we forget to respect dry_run in any of the output paths,
we'll accidentally produce output.

And indeed, there is at least one such regression in that commit, as it
covered only the case where we actually call into xdiff, and not
creation or deletion diffs, where we manually generate the headers. We
even test this case in t4035, but only with diff-tree, which does not
show the bug by default because it does not require diff_from_contents.
But git-diff does, because it allows external diff programs by default
(so we must dig into each diff filepair to decide if it requires running
an external diff that may declare two distinct blobs to actually be the
same).

We should fix all of those code paths to respect dry_run correctly, but
in the meantime we can protect ourselves more fully by restoring the
redirection to /dev/null. This gives us an extra layer of protection
against regressions dues to other code paths we've missed.

Though the original issue was reported with "git diff" (and due to its
default of --ext-diff), I've used "diff-tree -w" in the new test. It
triggers the same issue, but I think the fact that "-w" implies
diff_from_contents is a bit more obvious, and fits in with the rest of
t4035.

Reported-by: Jake Zimmerman <jake@zimmerman.io>
Signed-off-by: Jeff King <peff@peff.net>
---
I didn't test, but I also wondered if this might be necessary to avoid
actual external diff programs from spewing to stdout. Looking at
run_external_diff(), we do:

  int quiet = !(o->output_format & DIFF_FORMAT_PATCH);
  [...]
  cmd.no_stdout = quiet;

so I _think_ it should be OK even without this patch. But again, I like
the extra layer of protection here.

 diff.c                | 9 +++++++++
 t/t4035-diff-quiet.sh | 4 ++++
 2 files changed, 13 insertions(+)

diff --git a/diff.c b/diff.c
index 87fa16b730..687206f353 100644
--- a/diff.c
+++ b/diff.c
@@ -6890,6 +6890,15 @@ void diff_flush(struct diff_options *options)
 	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
 	    options->flags.exit_with_status &&
 	    options->flags.diff_from_contents) {
+		/*
+		 * run diff_flush_patch for the exit status. setting
+		 * options->file to /dev/null should be safe, because we
+		 * aren't supposed to produce any output anyway.
+		 */
+		diff_free_file(options);
+		options->file = xfopen("/dev/null", "w");
+		options->close_file = 1;
+		options->color_moved = 0;
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
 			if (check_pair_status(p))
diff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh
index 0352bf81a9..35eaf0855f 100755
--- a/t/t4035-diff-quiet.sh
+++ b/t/t4035-diff-quiet.sh
@@ -50,6 +50,10 @@ test_expect_success 'git diff-tree HEAD HEAD' '
 	test_expect_code 0 git diff-tree --quiet HEAD HEAD >cnt &&
 	test_line_count = 0 cnt
 '
+test_expect_success 'git diff-tree -w HEAD^ HEAD' '
+	test_expect_code 1 git diff-tree --quiet -w HEAD^ HEAD >cnt &&
+	test_line_count = 0 cnt
+'
 test_expect_success 'git diff-files' '
 	test_expect_code 0 git diff-files --quiet >cnt &&
 	test_line_count = 0 cnt
-- 
2.51.1.685.g6bf3278fbc

Re: [PATCH] diff: restore redirection to /dev/null for diff_from_contents

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Looking at that patch, my biggest concern is: are we missing other spots
>> that need to special-case the dry_run setting? Because it's a regression
>> in a maint release, I'm tempted to say we should do the dumbest possible
>> thing that covers all cases and just revert this hunk from the original
>> patch, like:
>
> Here it is with a commit message and test, in case that is helpful.
>
> -- >8 --
> Subject: [PATCH] diff: restore redirection to /dev/null for diff_from_contents
> ...
> I didn't test, but I also wondered if this might be necessary to avoid
> actual external diff programs from spewing to stdout. Looking at
> run_external_diff(), we do:
>
>   int quiet = !(o->output_format & DIFF_FORMAT_PATCH);
>   [...]
>   cmd.no_stdout = quiet;
>
> so I _think_ it should be OK even without this patch. But again, I like
> the extra layer of protection here.

I do like this direction, in addition I really do appreciate your
thought above on optimizing ext-diff and textconv away when they are
not necessary (obviously outside the scope of the regression fix).

I also wonder if we want to get rid of the new code related to the
"dry-run" mode that we no longer have to use.  But as a regression
fix that wants to be minimum, I think this patch stops at the right
place.

Thanks.


>  diff.c                | 9 +++++++++
>  t/t4035-diff-quiet.sh | 4 ++++
>  2 files changed, 13 insertions(+)
>
> diff --git a/diff.c b/diff.c
> index 87fa16b730..687206f353 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -6890,6 +6890,15 @@ void diff_flush(struct diff_options *options)
>  	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
>  	    options->flags.exit_with_status &&
>  	    options->flags.diff_from_contents) {
> +		/*
> +		 * run diff_flush_patch for the exit status. setting
> +		 * options->file to /dev/null should be safe, because we
> +		 * aren't supposed to produce any output anyway.
> +		 */
> +		diff_free_file(options);
> +		options->file = xfopen("/dev/null", "w");
> +		options->close_file = 1;
> +		options->color_moved = 0;
>  		for (i = 0; i < q->nr; i++) {
>  			struct diff_filepair *p = q->queue[i];
>  			if (check_pair_status(p))
> diff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh
> index 0352bf81a9..35eaf0855f 100755
> --- a/t/t4035-diff-quiet.sh
> +++ b/t/t4035-diff-quiet.sh
> @@ -50,6 +50,10 @@ test_expect_success 'git diff-tree HEAD HEAD' '
>  	test_expect_code 0 git diff-tree --quiet HEAD HEAD >cnt &&
>  	test_line_count = 0 cnt
>  '
> +test_expect_success 'git diff-tree -w HEAD^ HEAD' '
> +	test_expect_code 1 git diff-tree --quiet -w HEAD^ HEAD >cnt &&
> +	test_line_count = 0 cnt
> +'
>  test_expect_success 'git diff-files' '
>  	test_expect_code 0 git diff-files --quiet >cnt &&
>  	test_line_count = 0 cnt

Re: [PATCH] diff: restore redirection to /dev/null for diff_from_contents

[Johannes Schindelin]

Hi Jeff,

On Fri, 17 Oct 2025, Jeff King wrote:

> diff --git a/diff.c b/diff.c
> index 87fa16b730..687206f353 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -6890,6 +6890,15 @@ void diff_flush(struct diff_options *options)
>  	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
>  	    options->flags.exit_with_status &&
>  	    options->flags.diff_from_contents) {
> +		/*
> +		 * run diff_flush_patch for the exit status. setting
> +		 * options->file to /dev/null should be safe, because we
> +		 * aren't supposed to produce any output anyway.
> +		 */
> +		diff_free_file(options);
> +		options->file = xfopen("/dev/null", "w");
> +		options->close_file = 1;
> +		options->color_moved = 0;

I do not see any discussion about the `color_moved` line in
https://lore.kernel.org/git/20250808033019.78817-1-yldhome2d2@gmail.com/#r,
nor here.

Since you re-add it, I consider at least a little bit of reasonsing in
order, e.g. why this is necessary, and if it is necessary, why isn't
`options->use_color` forced to 0 also?

Taking a step back to see the 100ft view, I can understand why you want
that "extra level of protection" here. An even more important thing, that
is missing, is a plan to avoid the need for this protection.

Given that you're still on GitHub's payroll if the hallway rumors are
correct, I am quite a bit puzzled that you did not immediately reach for
CodeQL (which is a GitHub-sponsored technology, after all) to get clarity
on the code paths that would make this exra "layer of protection" still
necessary, and thereby provide said plan.

I started an AI-assisted brainstorm session and ended up with this query
(which is neither as concise nor as comprehensible as I would have liked,
but at least it does the job of finding the `run_diff_cmd()` code path
that I also find, and no other code path, and in v4 of Lidong Yan's patch,
it finds no remaining code path):

```codeql
/**
 * @name Potential file write during a dry run
 * @description Traces paths where `diff_options->dry_run` is set to non-zero
 * and the corresponding `diff_options->file` is later used.
 * @kind path-problem
 * @problem.severity warning
 * @id cpp/potential-dry-run-file-write
 * @tags correctness
 */

import cpp
import semmle.code.cpp.dataflow.new.DataFlow
import semmle.code.cpp.controlflow.IRGuards as IRGuards
import semmle.code.cpp.exprs.LogicalOperation
import semmle.code.cpp.exprs.ComparisonOperation
import semmle.code.cpp.exprs.Literal

/** Holds when `assign` sets `dry_run` to a non-zero literal. */
predicate setsDryRunNonZero(AssignExpr assign, FieldAccess dryRunField) {
  dryRunField.getTarget().hasName("dry_run") and
  assign.getLValue() = dryRunField and
  assign.getOperator() = "=" and
  isNonZeroLiteralExpr(assign.getRValue())
}

/** True when `expr` is literally zero (allowing common suffixes). */
predicate isZeroLiteralExpr(Expr expr) {
  exists(Literal lit |
    expr = lit and
    lit.getValueText().regexpMatch("(?i)\\s*0[uUlL]*\\s*")
  )
}

/** True when `expr` is a literal that is definitely non-zero. */
predicate isNonZeroLiteralExpr(Expr expr) {
  exists(Literal lit |
    expr = lit and
    not lit.getValueText().regexpMatch("(?i)\\s*0[uUlL]*\\s*")
  )
}

/** Holds if `access` uses the `file` member of a diff_options instance. */
predicate usesFileField(FieldAccess access) {
  access.getTarget().hasName("file")
}

/** True when an `if (options->dry_run)` immediately returns. */
predicate earlyReturnOnDryRun(FieldAccess fa) {
  fa.getTarget().hasName("dry_run") and
  exists(IfStmt ifStmt, ReturnStmt ret |
    ret = ifStmt.getThen() and
    ifStmt.getCondition() = fa and
    not exists(Stmt elseStmt | elseStmt = ifStmt.getElse())
  )
}

/** Data-flow configuration tracking diff options pointers while dry-run is enabled. */
module DryRunConfig implements DataFlow::ConfigSig {
  /** Sources: the `diff_options *` pointer whose `dry_run` field is set to non-zero. */
  predicate isSource(DataFlow::Node source) {
    exists(AssignExpr assign, FieldAccess dryRunField |
      setsDryRunNonZero(assign, dryRunField) and
      source.asExpr() = dryRunField.getQualifier()
    )
  }

  /** Sinks: any dereference of the `file` field through that diff options pointer. */
  predicate isSink(DataFlow::Node sink) {
    exists(FieldAccess access |
      usesFileField(access) and
      sink.asExpr() = access.getQualifier()
    )
  }

  /** Barriers: proofs that `dry_run` is zero or explicit resets back to zero. */
  predicate isBarrier(DataFlow::Node barrier) {
    exists(IRGuards::GuardCondition guard, FieldAccess fa |
      fa.getTarget().hasName("dry_run") and
      guard.getAChild*() = fa and
      barrier.asExpr() = fa.getQualifier() and
      safeDryRunCheck(guard, fa)
    )
    or
    exists(AssignExpr assign, FieldAccess fa |
      fa.getTarget().hasName("dry_run") and
      assign.getLValue() = fa and
      assign.getOperator() = "=" and
      barrier.asExpr() = fa.getQualifier() and
      isZeroLiteralExpr(assign.getRValue())
    )
    or
    exists(FieldAccess fa |
      earlyReturnOnDryRun(fa) and
      barrier.asExpr() = fa.getQualifier()
    )
  }

  /** Holds if `guard` ensures that `dry_run` evaluates to zero/false. */
  additional predicate safeDryRunCheck(IRGuards::GuardCondition guard, FieldAccess fa) {
    exists(NotExpr notExpr |
      guard.getAChild*() = notExpr and
      notExpr.getOperand() = fa
    )
    or
    exists(EQExpr eqExpr |
      guard.getAChild*() = eqExpr and
      (
        eqExpr.getLeftOperand() = fa and
        isZeroLiteralExpr(eqExpr.getRightOperand())
        or
        eqExpr.getRightOperand() = fa and
        isZeroLiteralExpr(eqExpr.getLeftOperand())
      )
    )
  }
}

/** Execute the configured global data-flow analysis. */
module DryRunFlow = DataFlow::Global<DryRunConfig>;

from DryRunFlow::PathNode source, DryRunFlow::PathNode sink,
  AssignExpr srcAssign, FieldAccess dryRunAccess, FieldAccess fileAccess
where
  DryRunFlow::flowPath(source, sink) and
  setsDryRunNonZero(srcAssign, dryRunAccess) and
  source.getNode().asExpr() = dryRunAccess.getQualifier() and
  usesFileField(fileAccess) and
  sink.getNode().asExpr() = fileAccess.getQualifier()
select fileAccess, source, sink,
  "`diff_options->file` used while `dry_run` forced non-zero at $@ and consumed here at $@.",
  srcAssign, "dry_run assignment",
  fileAccess, "file field use"

query predicate edges(DryRunFlow::PathNode edgeSource, DryRunFlow::PathNode edgeSink,
  string edgeKind, string edgeText) {
  DryRunFlow::PathGraph::edges(edgeSource, edgeSink, edgeKind, edgeText)
}
```

>  		for (i = 0; i < q->nr; i++) {
>  			struct diff_filepair *p = q->queue[i];
>  			if (check_pair_status(p))
> diff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh
> index 0352bf81a9..35eaf0855f 100755
> --- a/t/t4035-diff-quiet.sh
> +++ b/t/t4035-diff-quiet.sh
> @@ -50,6 +50,10 @@ test_expect_success 'git diff-tree HEAD HEAD' '
>  	test_expect_code 0 git diff-tree --quiet HEAD HEAD >cnt &&
>  	test_line_count = 0 cnt
>  '
> +test_expect_success 'git diff-tree -w HEAD^ HEAD' '
> +	test_expect_code 1 git diff-tree --quiet -w HEAD^ HEAD >cnt &&
> +	test_line_count = 0 cnt

I understand that you imitate the surrounding code, but there is
`test_must_be_empty` now, which has the huge advantage of documenting
intention much better than requiring the line count to be zero (and I wish
that there was a comprehensive roadmap and planning in general to avoid,
or at least clean up, the vast amount of style inconsistencies in Git,
preferably via automation so that no human being is burdened with _that_
cognitive load).

Ciao,
Johannes

> +'
>  test_expect_success 'git diff-files' '
>  	test_expect_code 0 git diff-files --quiet >cnt &&
>  	test_line_count = 0 cnt
> -- 
> 2.51.1.685.g6bf3278fbc
> 
> 
> 

Re: [PATCH] diff: restore redirection to /dev/null for diff_from_contents

[Jeff King]

On Sun, Oct 19, 2025 at 11:09:28PM +0200, Johannes Schindelin wrote:

> > @@ -6890,6 +6890,15 @@ void diff_flush(struct diff_options *options)
> >  	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
> >  	    options->flags.exit_with_status &&
> >  	    options->flags.diff_from_contents) {
> > +		/*
> > +		 * run diff_flush_patch for the exit status. setting
> > +		 * options->file to /dev/null should be safe, because we
> > +		 * aren't supposed to produce any output anyway.
> > +		 */
> > +		diff_free_file(options);
> > +		options->file = xfopen("/dev/null", "w");
> > +		options->close_file = 1;
> > +		options->color_moved = 0;
> 
> I do not see any discussion about the `color_moved` line in
> https://lore.kernel.org/git/20250808033019.78817-1-yldhome2d2@gmail.com/#r,
> nor here.
> 
> Since you re-add it, I consider at least a little bit of reasonsing in
> order, e.g. why this is necessary, and if it is necessary, why isn't
> `options->use_color` forced to 0 also?

My patch is a revert of the hunk from b55e6d36eb which caused the
"--quiet" regression, and hence includes that line. Perhaps I could have
made that more clear in the commit message.

I don't think use_color is related here. There's no clue in the commit
message which added that color_moved line (it was just the commit which
added the color_moved feature in the first place). But knowing the code,
I'd guess that it is not about trying to avoid producing color (which
is, after all, just going to go to /dev/null anyway) but rather avoiding
the computation to detect moved lines, since nobody will see them.

So probably (but I did not do any experimenting) the code produces the
correct output with or without color_moved. But it is also probably
wasting some extra CPU since b55e6d36eb. In a world with a dry_run flag,
it probably would make sense to skip the color_moved feature when
dry_run is set.

> Taking a step back to see the 100ft view, I can understand why you want
> that "extra level of protection" here. An even more important thing, that
> is missing, is a plan to avoid the need for this protection.

Sure. The goal of my patch was not to fix the dry-run feature. It was to
do the release engineering to undo the "--quiet" regression in the
simplest and least error-prone way possible. One way to do that is to
just revert b55e6d36eb entirely, add a new test covering the regression,
and then try again on top (perhaps on master this time). But I did the
more selective revert to reduce the back-and-forth noise of dropping the
dry_run code and then adding it back, which I thought gave the original
author a better base to work from.

Whether that /dev/null redirection survives once we are confident that
dry_run is hitting all of the code paths is up for debate.

I take it that you would prefer to try to fix dry_run in place on
'maint'. I think that can work, too. It's just not how I would do it
(not because I think this particular case is so hard, but because as a
general release engineering principle I prefer to fix regressions by
backing out changes rather than piling more changes on top). I am OK if
you want to go the other way, though.

> Given that you're still on GitHub's payroll if the hallway rumors are
> correct, I am quite a bit puzzled that you did not immediately reach for
> CodeQL (which is a GitHub-sponsored technology, after all) to get clarity
> on the code paths that would make this exra "layer of protection" still
> necessary, and thereby provide said plan.

There is no need to be puzzled. I have never actually used CodeQL at
all, beyond analyzing some of the false positives I've seen it report.
And the fact that GitHub sponsors my work on git.git is not really
relevant to how I go about that work.

> I started an AI-assisted brainstorm session and ended up with this query
> (which is neither as concise nor as comprehensible as I would have liked,
> but at least it does the job of finding the `run_diff_cmd()` code path
> that I also find, and no other code path, and in v4 of Lidong Yan's patch,
> it finds no remaining code path):

Neat, though it is very hard for me to quickly assess whether that
CodeQL block is doing the right thing. Your idea of manually tracing the
paths that touch opts->file seemed much simpler to me (and I think came
up with similar results).

-Peff

Re: Regression in `git diff --quiet HEAD` when a new file is staged

[Johannes Schindelin]

Hi Jeff,

On Fri, 17 Oct 2025, Jeff King wrote:

> On Thu, Oct 16, 2025 at 05:09:07PM -0700, Jake Zimmerman wrote:
> 
> > In git v2.51.1, `git diff --quiet HEAD` will actually print something
> > if the diff output includes a new, staged file.
> > [...]
> > I ran a git bisect and isolated this commit:
> > b55e6d36ebce69136559add8fffd1a65df231518
> 
> Yikes, that is a pretty bad regression. I'm rather surprised that this
> wasn't covered in the test suite. t4035 does set this situation up, but
> it checks with git-diff-tree, not git-diff. I initially thought that was
> because diff defaults to "--patch" output and diff-tree does not, but
> even "diff-tree --patch" does not show the bug. Weird. Maybe it has to
> do with running diffcore bits?
> 
> I see that the author of b55e6d36eb (diff: ensure consistent diff
> behavior with ignore options, 2025-08-08) posted this patch earlier
> today:
> 
>   https://lore.kernel.org/git/pull.2071.git.git.1760671049113.gitgitgadget@gmail.com/
> 
> which seems to fix it, but there's no mention there of this thread.

The fix predates the thread, that's why.

The reason why it "seems to fix it" is this: The `git diff --quiet HEAD`
call enters this code block
(https://github.com/git-for-windows/git/blob/rebase-to-v2.51.1/diff.c#L6876-L6886):

```c
	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
	    options->flags.exit_with_status &&
	    options->flags.diff_from_contents) {
		for (i = 0; i < q->nr; i++) {
			struct diff_filepair *p = q->queue[i];
			if (check_pair_status(p))
				diff_flush_patch_quietly(p, options);
			if (options->found_changes)
				break;
		}
	}
```

Specifically, the `diff_flush_patch_quietly()` function is called, which sets the `dry_run` flag. Later on, the `emit_diff_symbol_from_struct()` function is entered. Here is the call stack:

```
#0  emit_diff_symbol_from_struct (o=0x5ff480, eds=0x5fe910) at diff.c:1355
#1  0x00007ff7c3b275fe in emit_diff_symbol (o=0x5ff480, s=DIFF_SYMBOL_HEADER,
    line=0x3561a010380 "\033[1mdiff --git a/file b/file\033[m\n\033[1mnew file mode 100644\033[m\n\033[1mindex 0000000..e69de29\033[m\n", len=90, flags=0) at diff.c:1597
#2  0x00007ff7c3b2d602 in builtin_diff (name_a=0x3561a0702a0 "file", name_b=0x3561a0702a0 "file", one=0x3561a070240,
    two=0x3561a0702b0, xfrm_msg=0x3561a1a0500 "\033[1mindex 0000000..e69de29\033[m\n", must_show_header=1, o=0x5ff480,
    complete_rewrite=0) at diff.c:3723
#3  0x00007ff7c3b2fdc6 in run_diff_cmd (pgm=0x0, name=0x3561a0702a0 "file", other=0x0, attr_path=0x3561a0702a0 "file",
    one=0x3561a070240, two=0x3561a0702b0, msg=0x5febf0, o=0x5ff480, p=0x3561a0220c0) at diff.c:4617
#4  0x00007ff7c3b302af in run_diff (p=0x3561a0220c0, o=0x5ff480) at diff.c:4711
#5  0x00007ff7c3b353b0 in diff_flush_patch (p=0x3561a0220c0, o=0x5ff480) at diff.c:6172
#6  0x00007ff7c3b35413 in diff_flush_patch_quietly (p=0x3561a0220c0, o=0x5ff480) at diff.c:6184
#7  0x00007ff7c3b372ec in diff_flush (options=0x5ff480) at diff.c:6882
#8  0x00007ff7c3b2134f in run_diff_index (revs=0x5feed0, option=0) at diff-lib.c:643
#9  0x00007ff7c39d8427 in builtin_diff_index (revs=0x5feed0, argc=1, argv=0x3561a0202a0) at builtin/diff.c:170
#10 0x00007ff7c39d9487 in cmd_diff (argc=1, argv=0x3561a0202a0, prefix=0x0, repo=0x0) at builtin/diff.c:633
#11 0x00007ff7c39932f0 in run_builtin (p=0x7ff7c3d46368 <commands+840>, argc=3, argv=0x3561a0202a0,
    repo=0x7ff7c3e742c0 <the_repo>) at git.c:506
#12 0x00007ff7c3993849 in handle_builtin (args=0x5ffd70) at git.c:778
#13 0x00007ff7c3993b04 in run_argv (args=0x5ffd70) at git.c:861
#14 0x00007ff7c3993f56 in cmd_main (argc=3, argv=0x3561a0300e0) at git.c:983
#15 0x00007ff7c3ab0a7e in main (argc=7, argv=0x3561a0300c0) at common-main.c:9
```

The `if (o->dry_run) return;` guard introduced in the fix from
https://lore.kernel.org/git/pull.2071.git.git.1760671049113.gitgitgadget@gmail.com/
will then suppress the output, as desired.

> Looking at that patch, my biggest concern is: are we missing other spots
> that need to special-case the dry_run setting?

That's an excellent concern to have, seeing as bugs love like company.

A comparatively deeper analysis shows that the `o->file` attribute is used
in these functions that are not guarded by the early return introduced in
the proposed fix:

- show_numstat()
- gather_dirstat()
- checkdiff_consume()
- builtin_checkdiff()
- run_diff_cmd() (unmerged paths)
- diff_flush_raw()
- flush_one_pair() (DIFF_FORMAT_NAME)

Of these, I think the only concerning one is the one in `run_diff_cmd()`.

Ciao,
Johannes

Re: Regression in `git diff --quiet HEAD` when a new file is staged

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Looking at that patch, my biggest concern is: are we missing other spots
> that need to special-case the dry_run setting? Because it's a regression
> in a maint release, I'm tempted to say we should do the dumbest possible
> thing that covers all cases and just revert this hunk from the original
> patch, like:
>
> diff --git a/diff.c b/diff.c
> index 87fa16b730..687206f353 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -6890,6 +6890,15 @@ void diff_flush(struct diff_options *options)
>  	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
>  	    options->flags.exit_with_status &&
>  	    options->flags.diff_from_contents) {
> +		/*
> +		 * run diff_flush_patch for the exit status. setting
> +		 * options->file to /dev/null should be safe, because we
> +		 * aren't supposed to produce any output anyway.
> +		 */
> +		diff_free_file(options);
> +		options->file = xfopen("/dev/null", "w");
> +		options->close_file = 1;
> +		options->color_moved = 0;
>  		for (i = 0; i < q->nr; i++) {
>  			struct diff_filepair *p = q->queue[i];
>  			if (check_pair_status(p))
>
> That would catch the bug here, as well as any others lurking. And it
> converts any missing dry_run from correctness problems (we definitely
> will not produce extra output) into optimization problems (we might emit
> data we do not need, but we can fix those separately). At least for the
> normal code paths. I think without those extra fixes the problems that
> b55e6d36eb tried to fix for "-I" would still be observable, but at least
> its fixes could not regress the other code paths.

Ahh.  I like this "stupid but cannot be incorrect" version even
better than the original one that introduced the "dry run" mode.

But once we go in that direction, do we still need the dry-run
machinery with diff_flush_patch_quietly() helper function?

Re: Regression in `git diff --quiet HEAD` when a new file is staged

[Lidong Yan]

Junio C Hamano <gitster@pobox.com> writes:
> 
> Jeff King <peff@peff.net> writes:
> 
>> Looking at that patch, my biggest concern is: are we missing other spots
>> that need to special-case the dry_run setting? Because it's a regression
>> in a maint release, I'm tempted to say we should do the dumbest possible
>> thing that covers all cases and just revert this hunk from the original
>> patch, like:
>> 
>> diff --git a/diff.c b/diff.c
>> index 87fa16b730..687206f353 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -6890,6 +6890,15 @@ void diff_flush(struct diff_options *options)
>> if (output_format & DIFF_FORMAT_NO_OUTPUT &&
>>   options->flags.exit_with_status &&
>>   options->flags.diff_from_contents) {
>> + /*
>> + * run diff_flush_patch for the exit status. setting
>> + * options->file to /dev/null should be safe, because we
>> + * aren't supposed to produce any output anyway.
>> + */
>> + diff_free_file(options);
>> + options->file = xfopen("/dev/null", "w");
>> + options->close_file = 1;
>> + options->color_moved = 0;
>> for (i = 0; i < q->nr; i++) {
>> struct diff_filepair *p = q->queue[i];
>> if (check_pair_status(p))
>> 
>> That would catch the bug here, as well as any others lurking. And it
>> converts any missing dry_run from correctness problems (we definitely
>> will not produce extra output) into optimization problems (we might emit
>> data we do not need, but we can fix those separately). At least for the
>> normal code paths. I think without those extra fixes the problems that
>> b55e6d36eb tried to fix for "-I" would still be observable, but at least
>> its fixes could not regress the other code paths.
> 
> Ahh.  I like this "stupid but cannot be incorrect" version even
> better than the original one that introduced the "dry run" mode.
> 
> But once we go in that direction, do we still need the dry-run
> machinery with diff_flush_patch_quietly() helper function?

I believe we can move Peff’s code from diff_flush() to diff_flush_patch_quiet().
However, I'm unsure whether we should remove the dry-run logic. In dry-run
mode, we would halt as early as possible in xdl_diff by using quick_consume().

Re: Regression in `git diff --quiet HEAD` when a new file is staged

[Jeff King]

On Sat, Oct 18, 2025 at 09:04:40AM +0800, Lidong Yan wrote:

> I believe we can move Peff’s code from diff_flush() to diff_flush_patch_quiet().
> However, I'm unsure whether we should remove the dry-run logic. In dry-run
> mode, we would halt as early as possible in xdl_diff by using quick_consume().

Yeah, exactly.

I am OK to put the /dev/null code into diff_flush_patch_quiet(). That
would give all callers the same belt-and-suspenders protection.

The patch I posted put it where it was because that's where it was prior
to b55e6d36eb. It is essentially a revert (because I only wanted one
hunk I didn't call "git revert", but rather did a reversed patch
application).

But after that revert, I think it would be reasonable to move the code
on top (with the justification that it is helping the other caller of
the _quiet function).

-Peff

Re: Regression in `git diff --quiet HEAD` when a new file is staged

[Jeff King]

On Fri, Oct 17, 2025 at 10:45:12AM -0700, Junio C Hamano wrote:

> > diff --git a/diff.c b/diff.c
> > index 87fa16b730..687206f353 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -6890,6 +6890,15 @@ void diff_flush(struct diff_options *options)
> >  	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
> >  	    options->flags.exit_with_status &&
> >  	    options->flags.diff_from_contents) {
> > +		/*
> > +		 * run diff_flush_patch for the exit status. setting
> > +		 * options->file to /dev/null should be safe, because we
> > +		 * aren't supposed to produce any output anyway.
> > +		 */
> > +		diff_free_file(options);
> > +		options->file = xfopen("/dev/null", "w");
> > +		options->close_file = 1;
> > +		options->color_moved = 0;
> >  		for (i = 0; i < q->nr; i++) {
> >  			struct diff_filepair *p = q->queue[i];
> >  			if (check_pair_status(p))
> >
> > That would catch the bug here, as well as any others lurking. And it
> > converts any missing dry_run from correctness problems (we definitely
> > will not produce extra output) into optimization problems (we might emit
> > data we do not need, but we can fix those separately). At least for the
> > normal code paths. I think without those extra fixes the problems that
> > b55e6d36eb tried to fix for "-I" would still be observable, but at least
> > its fixes could not regress the other code paths.
> 
> Ahh.  I like this "stupid but cannot be incorrect" version even
> better than the original one that introduced the "dry run" mode.
> 
> But once we go in that direction, do we still need the dry-run
> machinery with diff_flush_patch_quietly() helper function?

I'm not sure which of these you mean:

  - Do we still need to call diff_flush_patch_quietly() directly below
    the hunk above, in diff_flush()?

    The answer is no, we do not need to (just like we did not before
    b55e6d36eb). But I think it is worth doing so still, because the
    low-level code may be able to use the flag to do things more
    efficiently.

  - Do we still need the dry-run code at all?

    My impression is yes, because there are other code paths which do
    the dry-run thing and need it for correctness.

    If I understand the motivation of b55e6d36eb, it really has multiple
    parts:

      1. Add a dry-run mode to the diff code.

      2. Use that dry-run mode for handling -I with name-status, etc.

      3. Since we now have dry-run mode, convert diff_flush()'s
	 /dev/null for --quiet mode to use it.

    The goal was really part (2). And any bugs in (1) would show up
    there, but they couldn't actually be regressions, but rather just an
    incomplete fix for (2). But by doing part (3), now bugs in (1) are
    regressions for --quiet. Hence my suggestion to undo just that part,
    and then do fixes for (1) separately.

    Or did you just mean: can we just go to a world where the _quietly()
    function just redirects /dev/null rather than worrying about dry-run
    at all? That is certainly an option, though I do think there is room
    for more efficiency with dry-run. So I think I prefer the
    belt-and-suspenders of "redirect to /dev/null just in case we miss a
    spot, but also tell the low-level code nobody is looking at the
    output".

-Peff

Re: Regression in `git diff --quiet HEAD` when a new file is staged

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'm not sure which of these you mean:
>
>   - Do we still need to call diff_flush_patch_quietly() directly below
>     the hunk above, in diff_flush()?
>
>   - Do we still need the dry-run code at all?

Both.  We do not have to call flush_quietly() and can call the real
thing with output disabled.  The dry-run bit was only added to
implement the flush_quietly() variant.  If we lose the only caller
to flush_quietly(), all of the supporting infrastructure can go.

It concentrates only on the regression-fix aspect of the changes.
Going forward, my preference is:

 * Apply your patch.  This is the base of the fix for 'maint' and
   all branches.

 * As Lidong updates dry-run code by adding more "ah we are in
   dry-run, so we should stop at the first change and se should be
   silent" fixes, we can queue them on the 'master' front for the
   preparation for a better future.  Note that the 'master' front
   would contain your "In from_contents modes, run flush_quietly()
   with output redirected to /dev/null".

 * Once we regain enough confidence for dry-run with the above
   effort, we mark your "why not redirect to /dev/null for extra
   protection?" code with NEEDSWORK comment to be removed after a
   thorough code audit to ensure that dry-run is now sound.

And I do not mind if the NEEDSWORK comment stay there for extended
period of time.

Thanks.

Re: Regression in `git diff --quiet HEAD` when a new file is staged

[Jeff King]

On Sat, Oct 18, 2025 at 08:23:13AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I'm not sure which of these you mean:
> >
> >   - Do we still need to call diff_flush_patch_quietly() directly below
> >     the hunk above, in diff_flush()?
> >
> >   - Do we still need the dry-run code at all?
> 
> Both.  We do not have to call flush_quietly() and can call the real
> thing with output disabled.  The dry-run bit was only added to
> implement the flush_quietly() variant.  If we lose the only caller
> to flush_quietly(), all of the supporting infrastructure can go.

It's not the only caller, though. b55e6d36eb added another earlier in
diff_flush(), to handle --name-status, etc (which was its original
goal). That code possibly remains broken, even with my patch, and
would wait either on Lidong's dry-run fixes, or lifting the /dev/null
into the flush_quietly() function.

> It concentrates only on the regression-fix aspect of the changes.
> Going forward, my preference is:
> 
>  * Apply your patch.  This is the base of the fix for 'maint' and
>    all branches.
> 
>  * As Lidong updates dry-run code by adding more "ah we are in
>    dry-run, so we should stop at the first change and se should be
>    silent" fixes, we can queue them on the 'master' front for the
>    preparation for a better future.  Note that the 'master' front
>    would contain your "In from_contents modes, run flush_quietly()
>    with output redirected to /dev/null".
> 
>  * Once we regain enough confidence for dry-run with the above
>    effort, we mark your "why not redirect to /dev/null for extra
>    protection?" code with NEEDSWORK comment to be removed after a
>    thorough code audit to ensure that dry-run is now sound.
> 
> And I do not mind if the NEEDSWORK comment stay there for extended
> period of time.

Yeah, that matched my thinking exactly.

But thinking on it more, I think the regression is slightly bigger than
I originally counted. My view was that:

  - the attempt to fix "-I" was incomplete but did not make anything
    worse there

  - that attempt also broke "--quiet"

So we should first un-break "--quiet" as simply as possible, and then
try to make the fix for "-I" more complete as a separate step. But I
think "-I" may actually have regressed, too, since it is subject to
printing the extra bogus output when trying to decide if the
content-diff is applicable, which it did not do before.

So really, the regression fix should probably cover both of them (which
it would if we move the /dev/null redirection into the flush_quietly()
variant).

-Peff

Re: Regression in `git diff --quiet HEAD` when a new file is staged

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Both.  We do not have to call flush_quietly() and can call the real
>> thing with output disabled.  The dry-run bit was only added to
>> implement the flush_quietly() variant.  If we lose the only caller
>> to flush_quietly(), all of the supporting infrastructure can go.
>
> It's not the only caller, though. b55e6d36eb added another earlier in
> diff_flush(), to handle --name-status, etc (which was its original
> goal). That code possibly remains broken, even with my patch, and
> would wait either on Lidong's dry-run fixes, or lifting the /dev/null
> into the flush_quietly() function.

Ah, OK.  That makes sense.

> So really, the regression fix should probably cover both of them (which
> it would if we move the /dev/null redirection into the flush_quietly()
> variant).

Do you mean something like this on top of your patch for 'maint',
and the latest from Lidong to the 'master' front, then?

Having calls to this helper in two loops in one function looks a bit
awkward but the conditions to enter these two loops are mutually
exclusive, so it is not like we can remember the result of the calls
we make in the first loop and reuse in the second loop, so this
probably is the best we can do.

--- >8 ---
Subject: diff: fix "-w -I<regex> --quiet"

An earlier fix made sure we stay quiet during "dry run" patch output
taken for the purpose of choosing which filepairs should be shown,
but the same helper function needs to be made silent when we iterate
over the diff-queue to compute the exit status.

 diff.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git c/diff.c w/diff.c
index 9b8d658b9e..1492ae108f 100644
--- c/diff.c
+++ w/diff.c
@@ -6172,6 +6172,8 @@ static void diff_flush_patch(struct diff_filepair *p, struct diff_options *o)
 	run_diff(p, o);
 }
 
+static void diff_free_file(struct diff_options *options);
+
 /* return 1 if any change is found; otherwise, return 0 */
 static int diff_flush_patch_quietly(struct diff_filepair *p, struct diff_options *o)
 {
@@ -6179,6 +6181,15 @@ static int diff_flush_patch_quietly(struct diff_filepair *p, struct diff_options
 	int saved_found_changes = o->found_changes;
 	int ret;
 
+	/*
+	 * run diff_flush_patch for the exit status. setting
+	 * options->file to /dev/null should be safe, because we
+	 * aren't supposed to produce any output anyway.
+	 */
+	diff_free_file(o);
+	o->file = xfopen("/dev/null", "w");
+	o->close_file = 1;
+	o->color_moved = 0;
 	o->dry_run = 1;
 	o->found_changes = 0;
 	diff_flush_patch(p, o);
@@ -6876,15 +6887,6 @@ void diff_flush(struct diff_options *options)
 	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
 	    options->flags.exit_with_status &&
 	    options->flags.diff_from_contents) {
-		/*
-		 * run diff_flush_patch for the exit status. setting
-		 * options->file to /dev/null should be safe, because we
-		 * aren't supposed to produce any output anyway.
-		 */
-		diff_free_file(options);
-		options->file = xfopen("/dev/null", "w");
-		options->close_file = 1;
-		options->color_moved = 0;
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
 			if (check_pair_status(p))

Re: Regression in `git diff --quiet HEAD` when a new file is staged

[Lidong Yan]

Junio C Hamano <gitster@pobox.com> writes:
> 
> /* return 1 if any change is found; otherwise, return 0 */
> static int diff_flush_patch_quietly(struct diff_filepair *p, struct diff_options *o)
> {
> @@ -6179,6 +6181,15 @@ static int diff_flush_patch_quietly(struct diff_filepair *p, struct diff_options
> int saved_found_changes = o->found_changes;
> int ret;
> 
> + /*
> + * run diff_flush_patch for the exit status. setting
> + * options->file to /dev/null should be safe, because we
> + * aren't supposed to produce any output anyway.
> + */
> + diff_free_file(o);
> + o->file = xfopen("/dev/null", "w");
> + o->close_file = 1;
> + o->color_moved = 0;
> o->dry_run = 1;
> o->found_changes = 0;
> diff_flush_patch(p, o);
> 

This would make everything going to "/dev/null" after the flush_quietly() call.
I think we need to restore o->file.

Thanks
Lidong

Re: Regression in `git diff --quiet HEAD` when a new file is staged

[Jeff King]

On Wed, Oct 22, 2025 at 12:46:55PM +0800, Lidong Yan wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> > 
> > /* return 1 if any change is found; otherwise, return 0 */
> > static int diff_flush_patch_quietly(struct diff_filepair *p, struct diff_options *o)
> > {
> > @@ -6179,6 +6181,15 @@ static int diff_flush_patch_quietly(struct diff_filepair *p, struct diff_options
> > int saved_found_changes = o->found_changes;
> > int ret;
> > 
> > + /*
> > + * run diff_flush_patch for the exit status. setting
> > + * options->file to /dev/null should be safe, because we
> > + * aren't supposed to produce any output anyway.
> > + */
> > + diff_free_file(o);
> > + o->file = xfopen("/dev/null", "w");
> > + o->close_file = 1;
> > + o->color_moved = 0;
> > o->dry_run = 1;
> > o->found_changes = 0;
> > diff_flush_patch(p, o);
> > 
> 
> This would make everything going to "/dev/null" after the flush_quietly() call.
> I think we need to restore o->file.

We probably also need to restore o->color_moved, too.

In the long run (and this is the kind of cleanup I was hoping you'd work
on for 'master'), we probably could drop that line entirely and just
skip running the moved-line detection when dry_run is set. Assuming it
even runs at all. From a quick look at the code, it looks like we only
do color-moved handling via diff_flush_patch_all_file_pairs(), so it
wouldn't trigger at all for the cases that do individual calls to
diff_flush_patch_quietly()?

-Peff

Re: Regression in `git diff --quiet HEAD` when a new file is staged

[Lidong Yan]

Jeff King <peff@peff.net> writes:
> 
> We probably also need to restore o->color_moved, too.
> 
> In the long run (and this is the kind of cleanup I was hoping you'd work
> on for 'master'), we probably could drop that line entirely and just
> skip running the moved-line detection when dry_run is set. Assuming it
> even runs at all. From a quick look at the code, it looks like we only
> do color-moved handling via diff_flush_patch_all_file_pairs(), so it
> wouldn't trigger at all for the cases that do individual calls to
> diff_flush_patch_quietly()?
> 

Sounds interesting, I’d like to dig into this ‘color_moved’ option and see
if we can optimize some code path in dry-run mode.

Thanks,
Lidong

Re: Regression in `git diff --quiet HEAD` when a new file is staged

[Junio C Hamano]

Lidong Yan <yldhome2d2@gmail.com> writes:

>> + diff_free_file(o);
>> + o->file = xfopen("/dev/null", "w");
>> + o->close_file = 1;
>> + o->color_moved = 0;
>> o->dry_run = 1;
>> o->found_changes = 0;
>> diff_flush_patch(p, o);
>> 
>
> This would make everything going to "/dev/null" after the flush_quietly() call.
> I think we need to restore o->file.

Ah, true, the original location was only for NO_OUTPUT but the other
caller to the diff_flush_patch_quietly() helper does deal with other
cases as well.

Thanks.

Re: Regression in `git diff --quiet HEAD` when a new file is staged

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Lidong Yan <yldhome2d2@gmail.com> writes:
>
>>> + diff_free_file(o);
>>> + o->file = xfopen("/dev/null", "w");
>>> + o->close_file = 1;
>>> + o->color_moved = 0;
>>> o->dry_run = 1;
>>> o->found_changes = 0;
>>> diff_flush_patch(p, o);
>>> 
>>
>> This would make everything going to "/dev/null" after the flush_quietly() call.
>> I think we need to restore o->file.
>
> Ah, true, the original location was only for NO_OUTPUT but the other
> caller to the diff_flush_patch_quietly() helper does deal with other
> cases as well.

Now it turns out to be rather ugly, having to go back and forth on a
few members of the diff_options structure.  I suspect there are
members other than color_moved that would not affect the outcome
(like --word-diff and --color-words) that cost us without giving any
benefit in this context that we may want to disable, but that would
make it even uglier.

I am having second thoughts on this approach to move the redirection
to patch_quietly(), which means for N-path change, we end up /dev/null
redirection N times.  We have two callers, so we may be better off
having the redirection around the loops that contain these callers?

I dunno.


 diff.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git c/diff.c w/diff.c
index 9b8d658b9e..d28f69e5ce 100644
--- c/diff.c
+++ w/diff.c
@@ -6177,14 +6177,28 @@ static int diff_flush_patch_quietly(struct diff_filepair *p, struct diff_options
 {
 	int saved_dry_run = o->dry_run;
 	int saved_found_changes = o->found_changes;
+	int saved_color_moved = o->color_moved;
+	FILE *saved_file = o->file;
 	int ret;
 
+	/*
+	 * Do the dry-run check while sending output to /dev/null and
+	 * extra computation like color_moved that would not change
+	 * the final outcome disabled.
+	 */
+	o->file = xfopen("/dev/null", "w");
+	o->color_moved = 0;
 	o->dry_run = 1;
 	o->found_changes = 0;
+
 	diff_flush_patch(p, o);
 	ret = o->found_changes;
+	fclose((o->file);
+
 	o->dry_run = saved_dry_run;
 	o->found_changes |= saved_found_changes;
+	o->color_moved = saved_color_moved;
+	o->file = saved_file;
 	return ret;
 }
 
@@ -6876,15 +6890,6 @@ void diff_flush(struct diff_options *options)
 	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
 	    options->flags.exit_with_status &&
 	    options->flags.diff_from_contents) {
-		/*
-		 * run diff_flush_patch for the exit status. setting
-		 * options->file to /dev/null should be safe, because we
-		 * aren't supposed to produce any output anyway.
-		 */
-		diff_free_file(options);
-		options->file = xfopen("/dev/null", "w");
-		options->close_file = 1;
-		options->color_moved = 0;
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
 			if (check_pair_status(p))

Re: Regression in `git diff --quiet HEAD` when a new file is staged

[Jeff King]

On Tue, Oct 21, 2025 at 07:38:03AM -0700, Junio C Hamano wrote:

> > So really, the regression fix should probably cover both of them (which
> > it would if we move the /dev/null redirection into the flush_quietly()
> > variant).
> 
> Do you mean something like this on top of your patch for 'maint',
> and the latest from Lidong to the 'master' front, then?

Yep, exactly (though with the "o->file" restoration that Lidong
pointed out).

> Having calls to this helper in two loops in one function looks a bit
> awkward but the conditions to enter these two loops are mutually
> exclusive, so it is not like we can remember the result of the calls
> we make in the first loop and reuse in the second loop, so this
> probably is the best we can do.

Yeah. I suspect there is some formulation along the lines of: if we have
diff_from_contents set but are not looking at a content-level diff, then
up-front in diff_flush() we should quietly flush each to find out what
is changed and what is not. But the loop for NAME_STATUS, etc, needs to
know _which_ pairs still had changes (whereas --quiet only cares about
whether there were any changes at all). So we'd have to store that
somewhere.

And of course the chance of regressing some unconsidered corner case is
high. Definitely not something we should entertain while doing another
regression fix. ;)

-Peff

Re: Regression in `git diff --quiet HEAD` when a new file is staged

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Oct 21, 2025 at 07:38:03AM -0700, Junio C Hamano wrote:
>
>> > So really, the regression fix should probably cover both of them (which
>> > it would if we move the /dev/null redirection into the flush_quietly()
>> > variant).
>> 
>> Do you mean something like this on top of your patch for 'maint',
>> and the latest from Lidong to the 'master' front, then?
>
> Yep, exactly (though with the "o->file" restoration that Lidong
> pointed out).
>
>> Having calls to this helper in two loops in one function looks a bit
>> awkward but the conditions to enter these two loops are mutually
>> exclusive, so it is not like we can remember the result of the calls
>> we make in the first loop and reuse in the second loop, so this
>> probably is the best we can do.
>
> Yeah. I suspect there is some formulation along the lines of: if we have
> diff_from_contents set but are not looking at a content-level diff, then
> up-front in diff_flush() we should quietly flush each to find out what
> is changed and what is not. But the loop for NAME_STATUS, etc, needs to
> know _which_ pairs still had changes (whereas --quiet only cares about
> whether there were any changes at all). So we'd have to store that
> somewhere.
>
> And of course the chance of regressing some unconsidered corner case is
> high. Definitely not something we should entertain while doing another
> regression fix. ;)

Of course.  The "redirect inside flush_quietly()" change by itself
is turning out to be tricky enough for the other caller of the
helper.

Here is what I have on top of your patch right now, after ditching
the idea to move the redirect to flush_quietly() because it would
mean redirecting N times for a N-path patch, but one thing that is
frustrating is that I cannot come up with a scenario or test in
which it makes a difference to this other caller if we forget to
restore o->file member.

diff --git c/diff.c i/diff.c
index 9b8d658b9e..ceb57d1ef8 100644
--- c/diff.c
+++ i/diff.c
@@ -6814,6 +6814,16 @@ void diff_flush(struct diff_options *options)
 			     DIFF_FORMAT_NAME |
 			     DIFF_FORMAT_NAME_STATUS |
 			     DIFF_FORMAT_CHECKDIFF)) {
+		/*
+		 * make sure diff_Flush_patch_quietly() to be silent.
+		 */
+		FILE *saved_file = options->file;
+		int saved_color_moved = options->color_moved;
+
+		if (options->flags.diff_from_contents) {
+			options->file = xfopen("/dev/null", "w");
+			options->color_moved = 0;
+		}
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
 
@@ -6826,6 +6836,11 @@ void diff_flush(struct diff_options *options)
 
 			flush_one_pair(p, options);
 		}
+		if (options->flags.diff_from_contents) {
+			fclose(options->file);
+			options->file = saved_file;
+			options->color_moved = saved_color_moved;
+		}
 		separator++;
 	}
 

Re: Regression in `git diff --quiet HEAD` when a new file is staged

[Jeff King]

On Wed, Oct 22, 2025 at 09:48:37AM -0700, Junio C Hamano wrote:

> Here is what I have on top of your patch right now, after ditching
> the idea to move the redirect to flush_quietly() because it would
> mean redirecting N times for a N-path patch, but one thing that is
> frustrating is that I cannot come up with a scenario or test in
> which it makes a difference to this other caller if we forget to
> restore o->file member.

Isn't it just running "git show -w --name-status" at all? If I take the
patch you showed below and drop the restoration, like so:

  diff --git a/diff.c b/diff.c
  index ceb57d1ef8..d402f960a9 100644
  --- a/diff.c
  +++ b/diff.c
  @@ -6836,11 +6836,6 @@ void diff_flush(struct diff_options *options)
   
   			flush_one_pair(p, options);
   		}
  -		if (options->flags.diff_from_contents) {
  -			fclose(options->file);
  -			options->file = saved_file;
  -			options->color_moved = saved_color_moved;
  -		}
   		separator++;
   	}
   

and then do:

  git init
  echo content >file
  git add file
  git commit -m file
  git show -w --name-status

then we do not show anything. We redirect to /dev/null to run
diff_flush_patch_quietly() and find that it does indeed have changes to
show (despite -w). But when we try to show the name-status output via
flush_one_pair(), we are still redirected to /dev/null.

But wait! That bug is already there in what you have queued in
jc/diff-from-contents-fix, even without my change!

That is because you are trying to redirect to /dev/null once at the
beginning of the loop. But the loop is effectively:

  for each pair
    check for content changes with diff_flush_patch_quietly();
    output actual pair data with flush_one_pair();

We want the redirection to /dev/null for the first part of the loop
body, but not the second. So you have to do the redirection inside the
loop.

I agree that opening /dev/null over and over is silly. But we can reuse
the same filehandle for each one. I.e., like:

diff --git a/diff.c b/diff.c
index dac3ea9e01..e903afcf04 100644
--- a/diff.c
+++ b/diff.c
@@ -6835,11 +6835,11 @@ void diff_flush(struct diff_options *options)
 		/*
 		 * make sure diff_Flush_patch_quietly() to be silent.
 		 */
-		FILE *saved_file = options->file;
+		FILE *dev_null = NULL;
 		int saved_color_moved = options->color_moved;
 
 		if (options->flags.diff_from_contents) {
-			options->file = xfopen("/dev/null", "w");
+			dev_null = xfopen("/dev/null", "w");
 			options->color_moved = 0;
 		}
 		for (i = 0; i < q->nr; i++) {
@@ -6848,15 +6848,20 @@ void diff_flush(struct diff_options *options)
 			if (!check_pair_status(p))
 				continue;
 
-			if (options->flags.diff_from_contents &&
-			    !diff_flush_patch_quietly(p, options))
-				continue;
+			if (options->flags.diff_from_contents) {
+				FILE *saved_file = options->file;
+				int r;
+				options->file = dev_null;
+				r = diff_flush_patch_quietly(p, options);
+				options->file = saved_file;
+				if (!r)
+					continue;
+			}
 
 			flush_one_pair(p, options);
 		}
 		if (options->flags.diff_from_contents) {
-			fclose(options->file);
-			options->file = saved_file;
+			fclose(dev_null);
 			options->color_moved = saved_color_moved;
 		}
 		separator++;

You could even imagine diff_flush_patch_quietly() saving the /dev/null
descriptor in a static variable and effectively leaking it (or if we
want to be more structured, cached inside the diff_options struct). And
then the callers do not have to worry about it at all.

And of course this all explains your confusion with Lidong's t4013 test
that started failing. It should generate three lines, because they are
the actual --raw lines. Once the bug in jc/diff-from-contents-fix is
fixed as above, they come back. And running it with the test fixup you
have queued on ly/diff-name-only-with-diff-from-content yields a failure
with:

  'actual' is not empty, it contains:
  :100644 000000 e69de29 0000000 D	file1
  :100644 000000 e69de29 0000000 D	file2
  :000000 100644 0000000 0000000 U	file3

-Peff

Re: Regression in `git diff --quiet HEAD` when a new file is staged

[Jeff King]

On Thu, Oct 23, 2025 at 08:01:01AM -0400, Jeff King wrote:

> You could even imagine diff_flush_patch_quietly() saving the /dev/null
> descriptor in a static variable and effectively leaking it (or if we
> want to be more structured, cached inside the diff_options struct). And
> then the callers do not have to worry about it at all.

Something like this (on top of jk/diff-from-contents-fix, replacing what
you have in jc/diff-from-contents/fix):

diff --git a/diff.c b/diff.c
index 9b8d658b9e..c9d3aaeb0f 100644
--- a/diff.c
+++ b/diff.c
@@ -6175,14 +6175,34 @@ static void diff_flush_patch(struct diff_filepair *p, struct diff_options *o)
 /* return 1 if any change is found; otherwise, return 0 */
 static int diff_flush_patch_quietly(struct diff_filepair *p, struct diff_options *o)
 {
+	static FILE *dev_null;
+	FILE *saved_file = o->file;
+	int saved_color_moved = o->color_moved;
 	int saved_dry_run = o->dry_run;
 	int saved_found_changes = o->found_changes;
 	int ret;
 
+	/*
+	 * As an extra precaution against code sending output to o->file even
+	 * when o->dry_run is set, redirect to /dev/null.
+	 *
+	 * We cache the /dev/null filehandle forever, effectively leaking it.
+	 * Gross, but it's O(1) gross-ness. A better solution would perhaps be
+	 * stuffing it into o->cached_dev_null or something, and freeing it
+	 * with the rest of the diff options.
+	 */
+	if (!dev_null)
+		dev_null = xfopen("/dev/null", "w");
+
+	o->file = dev_null;
+	/* TODO check if this is actually doing anything! */
+	o->color_moved = 0;
 	o->dry_run = 1;
 	o->found_changes = 0;
 	diff_flush_patch(p, o);
 	ret = o->found_changes;
+	o->file = saved_file;
+	o->color_moved = saved_color_moved;
 	o->dry_run = saved_dry_run;
 	o->found_changes |= saved_found_changes;
 	return ret;
@@ -6876,15 +6896,6 @@ void diff_flush(struct diff_options *options)
 	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
 	    options->flags.exit_with_status &&
 	    options->flags.diff_from_contents) {
-		/*
-		 * run diff_flush_patch for the exit status. setting
-		 * options->file to /dev/null should be safe, because we
-		 * aren't supposed to produce any output anyway.
-		 */
-		diff_free_file(options);
-		options->file = xfopen("/dev/null", "w");
-		options->close_file = 1;
-		options->color_moved = 0;
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
 			if (check_pair_status(p))

And you can see the difference with the tests Lidong added in t4013, or
just with this simple sequence:

  git init
  echo content >file
  git add file
  git commit -m foo
  git.compile show -w --name-status

Without either the /dev/null redirection above (or the actual dry_run
fixes), you get a bogus "diff --git" header in the output.

So mulling over that for a moment...if we are going to teach all code
paths that look at o->file to check o->dry_run, why do we need a
/dev/null redirection at all? Can't we just set o->file to NULL, and
that is the clue that we do not want output?

I know that is more intricate, and not what we want to do for the
immediate regression fix. But in the long run it makes more sense to me.
We get rid of the extra flag, and any code that does the wrong thing (by
trying to write to o->file) will blow up horribly with a segfault rather
than quietly produce wrong output.

-Peff

Re: Regression in `git diff --quiet HEAD` when a new file is staged

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> That is because you are trying to redirect to /dev/null once at the
> beginning of the loop. But the loop is effectively:
>
>   for each pair
>     check for content changes with diff_flush_patch_quietly();
>     output actual pair data with flush_one_pair();
>
> We want the redirection to /dev/null for the first part of the loop
> body, but not the second. So you have to do the redirection inside the
> loop.

Yeah, my bad.  Lidong noticed the same thing.

> I agree that opening /dev/null over and over is silly. But we can reuse
> the same filehandle for each one. I.e., like:
>
> diff --git a/diff.c b/diff.c
> index dac3ea9e01..e903afcf04 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -6835,11 +6835,11 @@ void diff_flush(struct diff_options *options)
>  		/*
>  		 * make sure diff_Flush_patch_quietly() to be silent.
>  		 */
> -		FILE *saved_file = options->file;
> +		FILE *dev_null = NULL;
>  		int saved_color_moved = options->color_moved;
>  
>  		if (options->flags.diff_from_contents) {
> -			options->file = xfopen("/dev/null", "w");
> +			dev_null = xfopen("/dev/null", "w");
>  			options->color_moved = 0;
>  		}
>  		for (i = 0; i < q->nr; i++) {
> @@ -6848,15 +6848,20 @@ void diff_flush(struct diff_options *options)
>  			if (!check_pair_status(p))
>  				continue;
>  
> -			if (options->flags.diff_from_contents &&
> -			    !diff_flush_patch_quietly(p, options))
> -				continue;
> +			if (options->flags.diff_from_contents) {
> +				FILE *saved_file = options->file;
> +				int r;
> +				options->file = dev_null;
> +				r = diff_flush_patch_quietly(p, options);
> +				options->file = saved_file;
> +				if (!r)
> +					continue;
> +			}
>  
>  			flush_one_pair(p, options);
>  		}
>  		if (options->flags.diff_from_contents) {
> -			fclose(options->file);
> -			options->file = saved_file;
> +			fclose(dev_null);
>  			options->color_moved = saved_color_moved;
>  		}
>  		separator++;
>
> You could even imagine diff_flush_patch_quietly() saving the /dev/null
> descriptor in a static variable and effectively leaking it (or if we
> want to be more structured, cached inside the diff_options struct). And
> then the callers do not have to worry about it at all.

That would be bigger change than a regression fix warrants, so let's
leave it out, but let me use the above to replace my botched
attempt.

Thanks, both of you.

Re: Regression in `git diff --quiet HEAD` when a new file is staged

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> So really, the regression fix should probably cover both of them (which
>> it would if we move the /dev/null redirection into the flush_quietly()
>> variant).

So, here is what I ended up with.  Instead of redirect many times in
the loop, dealing with the two callers would be simpler and less
error prone.  If we ever have the third caller, that is where we
should consider refactoring this even more into a separate
abstraction.

This goes on top of your patch and intend to go to 'maint'.

----- >8 -----
Subject: [PATCH] diff: make sure the other caller of diff_flush_patch_quietly() is silent

Earlier, we added is a protection for the loop that computes "git
diff --quiet -w" to ensure calls to the diff_flush_patch_quietly()
helper stays quiet.  Do the same for another loop that deals with
options like "--name-status" to make calls to the same helper.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/diff.c b/diff.c
index 9b8d658b9e..ceb57d1ef8 100644
--- a/diff.c
+++ b/diff.c
@@ -6814,6 +6814,16 @@ void diff_flush(struct diff_options *options)
 			     DIFF_FORMAT_NAME |
 			     DIFF_FORMAT_NAME_STATUS |
 			     DIFF_FORMAT_CHECKDIFF)) {
+		/*
+		 * make sure diff_Flush_patch_quietly() to be silent.
+		 */
+		FILE *saved_file = options->file;
+		int saved_color_moved = options->color_moved;
+
+		if (options->flags.diff_from_contents) {
+			options->file = xfopen("/dev/null", "w");
+			options->color_moved = 0;
+		}
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
 
@@ -6826,6 +6836,11 @@ void diff_flush(struct diff_options *options)
 
 			flush_one_pair(p, options);
 		}
+		if (options->flags.diff_from_contents) {
+			fclose(options->file);
+			options->file = saved_file;
+			options->color_moved = saved_color_moved;
+		}
 		separator++;
 	}
 
-- 
2.51.1-633-gaa2b1236d0

Re: Regression in `git diff --quiet HEAD` when a new file is staged

[Lidong Yan]

Junio C Hamano <gitster@pobox.com> writes:
> 
> ----- >8 -----
> Subject: [PATCH] diff: make sure the other caller of diff_flush_patch_quietly() is silent
> 
> Earlier, we added is a protection for the loop that computes "git
> diff --quiet -w" to ensure calls to the diff_flush_patch_quietly()
> helper stays quiet.  Do the same for another loop that deals with
> options like "--name-status" to make calls to the same helper.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
> 
> diff --git a/diff.c b/diff.c
> index 9b8d658b9e..ceb57d1ef8 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -6814,6 +6814,16 @@ void diff_flush(struct diff_options *options)
>     DIFF_FORMAT_NAME |
>     DIFF_FORMAT_NAME_STATUS |
>     DIFF_FORMAT_CHECKDIFF)) {
> + /*
> + * make sure diff_Flush_patch_quietly() to be silent.
> + */
> + FILE *saved_file = options->file;
> + int saved_color_moved = options->color_moved;
> +
> + if (options->flags.diff_from_contents) {
> + options->file = xfopen("/dev/null", "w");
> + options->color_moved = 0;
> + }
> for (i = 0; i < q->nr; i++) {
> struct diff_filepair *p = q->queue[i];
> 
> @@ -6826,6 +6836,11 @@ void diff_flush(struct diff_options *options)
> 
> flush_one_pair(p, options);
> }
> + if (options->flags.diff_from_contents) {
> + fclose(options->file);
> + options->file = saved_file;
> + options->color_moved = saved_color_moved;
> + }
> separator++;
> }
> 
> -- 
> 2.51.1-633-gaa2b1236d0
> 

Do you think we should make a new ‘going to be flushed’ queue
and flush them out of ‘quiet’ loop would be a good idea? I think we
shouldn’t discard output of flush_one_pair().

Thanks,
Lidong

Re: Regression in `git diff --quiet HEAD` when a new file is staged

[Junio C Hamano]

Lidong Yan <yldhome2d2@gmail.com> writes:

> Do you think we should make a new ‘going to be flushed’ queue
> and flush them out of ‘quiet’ loop would be a good idea? I think we
> shouldn’t discard output of flush_one_pair().

Thanks for catching my sillyness. 

Peff caught the same thing but in each iteration of this loop we do
the "diff -p >/dev/null" to decide if we do "diff
--(raw|name-only|...)" for the path, so redirecting the whole thing
would break it big time.