Re https://lore.kernel.org/git/20220308080551.18538-1-dyroneteng@xxxxxxxxx/ sorry about the delay in reviewing this. I did look it over & come up with the below almost a week ago, but forgot/didn't have time to turn it into inline comments. I don't think all of these need to be squashed or fixed up into the proposed series, but are just various small issues/questions I came up with while reviewing it. Brief notes below: Ævar Arnfjörð Bjarmason (7): ls-tree tests: add tests for --name-status Optional, but maybe a good idea to add to the series. I noticed the zero --name-status test coverage when testing it. ls-tree tests: exhaustively test fast & slow path for --format Probably a good idea to add to it. I.e. the tests for the --format drifted from the RFC version I had so that we weren't testing the optimized v.s. non-optimized path. ls-tree: remove dead labels Removes dead code in the proposed series (from an earlier iteration?) ls-tree: remove unused "MODE_UNSPECIFIED" The MODE_UNSPECIFIED is also dead code in this series, but... ls-tree: detect and error on --name-only --name-status We should die on --name-status --name-only (two option synonyms), but don't on "master", so. This does make some subsequent code here slightly simpler though... ls-tree: remove FIELD_*, just use MODE_* The main outstanding thing for me when trying to understand the code in this series is why we ended up with FIELD_* defines *and* the MODE_*. This moves to just using MODE_*. I think it makes things easier to understand. I.e. it's one less thing to pass around, and what was in parse_shown_fields() is just embedded in show_tree() now without the indirection. There's also a change there to add an array of format <-> fast path fn mappings, instead of the current if/else if chain with harcdoded strcmp(). ls-tree: split up "fast path" callbacks Maybe a good idea, maybe not. Splits up all the "fast path" callbacks into individual functions, instead of a monolithic show_default(). I think the resulting code is easier to read, since it's clear what data we need to set up for what, but maybe we're off into the weeds here... builtin/ls-tree.c | 279 ++++++++++++++++++++++--------------- t/t3101-ls-tree-dirname.sh | 55 ++++---- t/t3103-ls-tree-misc.sh | 15 +- t/t3104-ls-tree-format.sh | 89 +++++------- 4 files changed, 235 insertions(+), 203 deletions(-) -- 2.35.1.1295.g6b025d3e231
