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

Hi Jeff,

On Fri, 17 Oct 2025, Jeff King wrote:

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)
}
```

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