[PATCH 0/14] combine-diff cleanups
- From
Jeff King <peff@peff.net>
- Date
- Jan 9, 2025, 08:27 UTC
- Message-ID
- <20250109082723.GA2748497@coredump.intra.peff.net>
- In-Reply-To
- <20250104033210.GA892381@coredump.intra.peff.net>
Since Wink successfully nerd-sniped me into digging into the combine-diff code, and since I had such a hard time figuring out some of its logic, I spent a little time trying to put that puzzling to good use to make it more readable.
Aside from a minor leak fix in the first patch, I didn't find any bugs. So arguably this whole thing could be discarded as churn. But I hope at least some of it is worthwhile, and I tried to order it to keep the less controversial bits near the top.
The series can be split into a few sections:
[01/14]: run_diff_files(): delay allocation of combine_diff_path [02/14]: combine-diff: add combine_diff_path_new() [03/14]: tree-diff: clear parent array in path_appendnew() [04/14]: combine-diff: use pointer for parent paths [05/14]: diff: add a comment about combine_diff_path.parent.path [06/14]: run_diff_files(): de-mystify the size of combine_diff_path struct
These first six clean up most of the allocation and initialization
confusion that started this thread. They can't go all the way
because of the scariness in path_appendnew().[07/14]: tree-diff: drop path_appendnew() alloc optimization [08/14]: tree-diff: pass whole path string to path_appendnew() [09/14]: tree-diff: inline path_appendnew() [10/14]: combine-diff: drop public declaration of combine_diff_path_size()
And these ones take it further, but at the cost of losing an
optimization in patch 07. I don't think it was doing much (and I
gave some timings there). But it's a judgement call on whether the
cleaner code is worthwhile.[11/14]: tree-diff: drop list-tail argument to diff_tree_paths() [12/14]: tree-diff: use the name "tail" to refer to list tail [13/14]: tree-diff: simplify emit_path() list management [14/14]: tree-diff: make list tail-passing more explicit
And these last four fix some confusion I had while reading the
functions. I think they _could_ be done independent of 7-14,
but there'd be some kinks to work out in emit_path(). The final one is probably a matter of taste, and I'm not sure if
people find it easier to understand than the original or not. If
not, it can easily be dropped.combine-diff.c | 80 +++++++++++++------------- diff-lib.c | 36 ++++-------- diff.h | 18 ++++-- tree-diff.c | 152 ++++++++++++------------------------------------- 4 files changed, 102 insertions(+), 184 deletions(-)
-Peff