mbox series

[0/14] combine-diff cleanups

Message ID 20250109082723.GA2748497@coredump.intra.peff.net (mailing list archive)
Headers show
Series combine-diff cleanups | expand

Message

Jeff King Jan. 9, 2025, 8:27 a.m. UTC
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