> Unlike the original, this always makes two needless structure > assignments for anything that is not a submodule. > > Can we fix that regression before moving forward? > > Even when ce_mode is a gitlink, if .ignore_submodules bit is set, > the two structure assignments between diffopt->flags and orig_flags > become necessary, so the original was already doing extra copies but > we do not have to make it worse. ack > Strange indentation. It is unclear why this bottom 1/3 of the loop > body of run_diff_files() need to be a separate helper function, > while the top 2/3 does not. The resulting loop (below) becomes very > hard to follow because the reader cannot tell when diff_change() is > called and when it is not. > > Overall, I see this change detrimental to diff-lib API at this step > in the series. Later steps may show something more rewarding than > the downsides we see here, hopefully. I'll see if I can come up with a way to rewrite this so it is less confusing. Alternatively, I could remove this refactor.
