[RFC/REVIEW 0/7] fixups/suggestions/musings for tl/ls-tree-oid-only

Re https://lore.kernel.org/git/20220308080551.18538-1-dyroneteng@gmail.com/ 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(-)