Re: [PATCH 00/30] tree-walk: mostly "mode" to "enum object_type"
On Mon, Mar 8, 2021 at 7:07 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 44 quoted lines
>
> This large series goes on top of my 6 patch series for
> read_tree_recursive() as this one further refactors that function. See
> https://lore.kernel.org/git/20210308022138.28166-1-avarab@gmail.com/
> for that series.
>
> I noticed that since 2014 or so we haven't been doing the fsck checks
> for bad file modes in trees. This series fixes that. I plan to add
> tests etc. for that in another follow-up series.
>
> I wanted to get this out for review sooner than later, particularly
> since the fsck testing will probably get me down another refactoring
> path (fsck testing in general in this area is pretty bad...).
>
> As noted in 30/30 it would have been way easier to simply do an
> isolated fix for that bug by introducing some fsck-specific API for
> raw tree reading.
>
> But I thought the bug was symptomatic of a wider problem in our
> codebase. Namely that we pass around the tree's mode *a lot*.
>
> But almost everything that then deals with the mode doesn't per-se
> care about the mode bits in the tree, but using them to map that mode
> to a tree entry for one of of OBJ_{BLOB,TREE,COMMIT}.
>
> So this is a large refactoring of all users of the widely used
> tree-walk.h API to "enum obj2ect_type", finally in 29/30 I rename the
> field to a scary "raw_mode".
>
> At that point we have just ~30-50 grep hits left for "raw_mode" in the
> codebase (depending on whether we count names in function parameters).
>
> Hopefully being in that state alleviates e.g. Elijah's concerns
> expressed in
> https://lore.kernel.org/git/CABPp-BEdu1PqV5W=FuL0f08iFhGzvzV8oSUybNj4eF0aAwTnAw@mail.gmail.com/
> I agree that doing the equivalent of 30/30 on top of master would be
> way too scary, but once we're at 29/30 I think it's sane.
>
> I tested this in combination with his on-list series to add more
> merge-ort testing:
> https://lore.kernel.org/git/pull.973.git.git.1614905738.gitgitgadget@gmail.com/
>
> I found a regression I'd caused in the merge-ort.c code with those
> tests, fixed here. See the comment in merge-ort.c in 30/30.I'll start reading through this series, but two quick early notes:
- I was worried about touching the tree-walk code message up
performance. Since collect_rename_info() in merge-ort.c takes the
most time (and it's mostly a tree-walk), I was worried this would
regress performance for me. A couple runs of my mega-renames big
rebase testcase suggests performance is not that different, so my
fears look unfounded. (Note: I tested in combination with all my
performance improvements, because otherwise tree-walking would just be
in the noise of overall runtime.)
- There's a textual conflict with this series and my sets of patch
series that finish off the ort implementation[1], but it's just two
lines that are pretty easy to resolve.
[1] https://github.com/gitgitgadget/git/pulls?q=is%3Aopen+is%3Apr+author%3Anewren+Optimization+batch
Show 78 quoted lines
> Ævar Arnfjörð Bjarmason (30):
> diff.c: remove redundant canon_mode() call
> notes & match-trees: use name_entry's "pathlen" member
> cache.h: add a comment to object_type()
> tree-walk.h: add object_type member to name_entry
> tree-walk.c: migrate to using new "object_type" field when possible
> cache.h: have base_name_compare() take "is tree?", not "mode"
> tree-walk.h users: switch object_type(...) to new .object_type
> tree.h: format argument lists of read_tree_recursive() users
> tree.h users: format argument lists in archive.c
> archive: get rid of 'stage' parameter
> tree.h API: make read_tree_fn_t take an "enum object_type"
> tree-walk.h users: migrate "p->mode &&" pattern
> tree-walk.h users: refactor chained "mode" if/else into switch
> tree-walk.h users: migrate miscellaneous "mode" to "object_type"
> merge-tree tests: test for the mode comparison in same_entry()
> merge-ort: correct reference to test in 62fdec17a11
> fsck.c: switch on "object_type" in fsck_walk_tree()
> tree-walk.h users: use temporary variable(s) for "mode"
> tree-walk.h API: formatting changes for subsequent commit
> tree-walk.h API: rename get_tree_entry() to get_tree_entry_mode()
> tree-walk.h API users: use "tmp" for mode in shift_tree_by()
> tree-walk.h API: Add get_tree_entry_type()
> tree-walk.h API: add a get_tree_entry_path() function
> tree-walk.h API: document and format tree_entry_extract()
> tree-entry.h API: rename tree_entry_extract() to
> tree_entry_extract_mode()
> tree-walk.h API: add a tree_entry_extract_all() function
> tree-walk.h API: add a tree_entry_extract_type() function
> tree-walk.h API users: rename "struct name_entry"'s "mode" to
> "raw_mode"
> tree.h API users: rename read_tree_fn_t's "mode" to "raw_mode"
> tree-walk.h API: move canon_mode() back out of decode_tree_entry()
>
> archive.c | 51 +++++++++++++-----------
> blame.c | 9 +++--
> builtin/checkout.c | 7 +++-
> builtin/fast-import.c | 8 ++--
> builtin/grep.c | 6 +--
> builtin/log.c | 7 ++--
> builtin/ls-files.c | 13 +++---
> builtin/ls-tree.c | 18 ++++-----
> builtin/merge-tree.c | 32 +++++++++------
> builtin/mktree.c | 4 +-
> builtin/pack-objects.c | 6 +--
> builtin/reflog.c | 3 +-
> builtin/rm.c | 2 +-
> builtin/update-index.c | 7 +++-
> cache-tree.c | 2 +-
> cache.h | 11 ++++--
> combine-diff.c | 8 ++--
> delta-islands.c | 2 +-
> diff.c | 2 +-
> fsck.c | 23 +++++------
> http-push.c | 6 ++-
> line-log.c | 2 +-
> list-objects.c | 20 +++++++---
> match-trees.c | 52 ++++++++++++------------
> merge-ort.c | 34 ++++++++++------
> merge-recursive.c | 33 ++++++++--------
> notes.c | 15 +++----
> object-name.c | 7 ++--
> pack-bitmap-write.c | 8 ++--
> read-cache.c | 16 ++++----
> revision.c | 12 ++++--
> t/t4300-merge-tree.sh | 44 +++++++++++++++++++++
> tree-diff.c | 44 ++++++++++++---------
> tree-walk.c | 89 +++++++++++++++++++++++++++++++-----------
> tree-walk.h | 67 ++++++++++++++++++++++++++-----
> tree.c | 19 +++++----
> tree.h | 5 ++-
> unpack-trees.c | 30 ++++++++------
> walker.c | 22 ++++++-----
> 39 files changed, 482 insertions(+), 264 deletions(-)
>
> --
> 2.31.0.rc0.126.g04f22c5b82
>
Elijah Newren <newren@gmail.com>