Re: [PATCH 0/7] Move the read_tree() function to its only user

From: Elijah Newren <newren@gmail.com>
Date: Mar 6, 2021, 22:06 UTC
Message-ID: <CABPp-BHTPmW+uwHum7p0PsWbMpM2uGX7CP=KoZ+W3PRzxjQxWw@mail.gmail.com>
In-Reply-To: <20210306193458.20633-1-avarab@gmail.com>

On Sat, Mar 6, 2021 at 11:35 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 17 quoted lines

>
> This is a small cleanup series to remove move the last user of
> read_tree() over to read_tree_recursive(), and while we're at it
> adjust the API of read_tree_fn_t to its current use-case.
>
> I found out after writing this that there had been a FIXME comment
> (never made it into git.git) about this from mid-2009:
> https://lore.kernel.org/git/1240014568-3675-1-git-send-email-pclouds@gmail.com/
>
> Ævar Arnfjörð Bjarmason (7):
>   tree.c API: move read_tree() into builtin/ls-files.c
>   ls-files: don't needlessly pass around stage variable
>   ls-files: remove cache juggling + sorting
>   merge-ort: move cmp_cache_name_compare() from tree.c
>   ls-files: refactor read_one_entry_quick() to use a strbuf
>   tree.h API: remove support for starting at prefix != ""
>   tree.h API: remove "stage" parameter from read_tree_recursive()

I read over all 7 patches.  For the most part, they look good.

Some of the wording in the commit message for patch 6 seemed confusing and could use some touch-ups; it may also be worth documenting when the code stopped having any callers with a base other than "".

For patch 3 (the actual removal of read_tree()), either I'm misunderstanding something, or the changes look problematic and unsafe. I'm hoping it's the former.