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