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

Message

Ævar Arnfjörð Bjarmason March 10, 2022, 1:56 p.m. UTC

Re
https://lore.kernel.org/git/[e-mail]/
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(-)

Comments

Teng Long March 17, 2022, 9:51 a.m. UTC | #1

Ævar Arnfjörð Bjarmason wrote on Thu, 10 Mar 2022 14:56:56 +0100

> 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 
> with while reviewing it. Brief notes 
>
> Ævar Arnfjörð Bjarmason (7):
> ...


I looked these commits in "RFC/REVIEW" and I think each one is a good
improvement. So next, to make the squashes or keep it alone  I think
maybe it's like: 


(1) ls-tree tests: add tests for --name-status

    They are new test codes that better than old ones. 

    Action: Keep it individually.

(2) ls-tree tests: exhaustively test fast & slow path for --format

    They are better test codes for the correctness formatting mechanism.

    Action: Squash into commit 'ls-tree: introduce "--format" option'

(3) ls-tree: remove dead labels

    They remove the dead labels, and you mentioned it should be squash into
    the commit which brought them in.
    
    Action: Squash into commit 'ls-tree: slightly refactor `show_tree()`'

(4) ls-tree: remove unused "MODE_UNSPECIFIED"

    As the subject describes, remove unused "MODE_UNSPECIFIED" and make
    "mutx_option" to a better name "mutx_option". It's the prepared commit
    for 'ls-tree: remove FIELD_*, just use MODE_*''

    Action: Squash into commit 'ls-tree: slightly refactor `show_tree()`'

(5) ls-tree: detect and error on --name-only --name-status

    Optimized the incompatible detecting tests codes in "t/t3103-ls-tree-misc.sh"
    and add a new 'MODE_NAME_STATUS'.

    Action: Keep it individually.

(6) ls-tree: remove FIELD_*, just use MODE_*

    Using MODE directlly and make a format-mode mappings for fast-path detection.

    Action: Keep it individually.

(7) ls-tree: split up "fast path" callbacks

    Expand "ls_tree_cmdmode_format" structure for each formats with the specific 
    show function and split up the current show functions name.

    Action: Keep it individually.


This is the way I try to continue this work, please let me know if I understand you wrong.

I look forward to your reply and I will decide the next step based on the reply. If we have
a consistent understanding of the next actions, I will try to make a pull request to your
Git fork first. If there is no problem, I will continue to send patches to the mailing list.

Thanks.

Ævar Arnfjörð Bjarmason March 17, 2022, 10:04 a.m. UTC | #2

On Thu, Mar 17 2022, Teng Long wrote:

> Ævar Arnfjörð Bjarmason wrote on Thu, 10 Mar 2022 14:56:56 +0100
>
>> 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 
>> with while reviewing it. Brief notes 
>>
>> Ævar Arnfjörð Bjarmason (7):
>> ...
>
>
> I looked these commits in "RFC/REVIEW" and I think each one is a good
> improvement. So next, to make the squashes or keep it alone  I think
> maybe it's like: 
>
>
> (1) ls-tree tests: add tests for --name-status
>
>     They are new test codes that better than old ones. 
>
>     Action: Keep it individually.
>
> (2) ls-tree tests: exhaustively test fast & slow path for --format
>
>     They are better test codes for the correctness formatting mechanism.
>
>     Action: Squash into commit 'ls-tree: introduce "--format" option'
>
> (3) ls-tree: remove dead labels
>
>     They remove the dead labels, and you mentioned it should be squash into
>     the commit which brought them in.
>     
>     Action: Squash into commit 'ls-tree: slightly refactor `show_tree()`'
>
> (4) ls-tree: remove unused "MODE_UNSPECIFIED"
>
>     As the subject describes, remove unused "MODE_UNSPECIFIED" and make
>     "mutx_option" to a better name "mutx_option". It's the prepared commit
>     for 'ls-tree: remove FIELD_*, just use MODE_*''
>
>     Action: Squash into commit 'ls-tree: slightly refactor `show_tree()`'
>
> (5) ls-tree: detect and error on --name-only --name-status
>
>     Optimized the incompatible detecting tests codes in "t/t3103-ls-tree-misc.sh"
>     and add a new 'MODE_NAME_STATUS'.
>
>     Action: Keep it individually.
>
> (6) ls-tree: remove FIELD_*, just use MODE_*
>
>     Using MODE directlly and make a format-mode mappings for fast-path detection.
>
>     Action: Keep it individually.
>
> (7) ls-tree: split up "fast path" callbacks
>
>     Expand "ls_tree_cmdmode_format" structure for each formats with the specific 
>     show function and split up the current show functions name.
>
>     Action: Keep it individually.

All sounds good, or rather. I really meant those as "hey maybe it's
useful, you decide what to do with it". So I'm happy with whatever you
picked here :)

> This is the way I try to continue this work, please let me know if I understand you wrong.
>
> I look forward to your reply and I will decide the next step based on the reply. If we have
> a consistent understanding of the next actions, I will try to make a pull request to your
> Git fork first. If there is no problem, I will continue to send patches to the mailing list.

Hi, thanks for looking at it. I think it's better to just post a re-roll
on the list for discussion, with whatever changed you think are
appropriate. Thanks!