Re: [PATCH v3 4/5] MyFirstObjectWalk: fix description for counting omitted objects

From: Kyle Lippincott <spectral@google.com>
Date: Mar 25, 2024, 20:59 UTC
Message-ID: <CAO_smVhTQS2+QH0--ZtiCKyrFWx9fak0-0mbN-xY=_Ayx8JW=A@mail.gmail.com>
In-Reply-To: <xmqq34semden.fsf@gitster.g>

On Mon, Mar 25, 2024 at 10:25 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 38 quoted lines

>
> Dirk Gouders <dirk@gouders.net> writes:
>
> > Before the changes to count omitted objects, the function
> > traverse_commit_list() was used and its call cannot be changed to pass
> > a pointer to an oidset to record omitted objects.
> >
> > Fix the text to clarify that we now use another traversal function to
> > be able to pass the pointer to the introduced oidset.
> >
> > Helped-by: Kyle Lippincott <spectral@google.com>
> > Signed-off-by: Dirk Gouders <dirk@gouders.net>
> > ---
> >  Documentation/MyFirstObjectWalk.txt | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
> > index a06c712e46..811175837c 100644
> > --- a/Documentation/MyFirstObjectWalk.txt
> > +++ b/Documentation/MyFirstObjectWalk.txt
> > @@ -754,10 +754,11 @@ points to the same tree object as its grandparent.)
> >  === Counting Omitted Objects
> >
> >  We also have the capability to enumerate all objects which were omitted by a
> > -filter, like with `git log --filter=<spec> --filter-print-omitted`. Asking
> > -`traverse_commit_list_filtered()` to populate the `omitted` list means that our
> > -object walk does not perform any better than an unfiltered object walk; all
> > -reachable objects are walked in order to populate the list.
> > +filter, like with `git log --filter=<spec> --filter-print-omitted`. To do this,
> > +change `traverse_commit_list()` to `traverse_commit_list_filtered()`, which is
> > +able to populate an `omitted` list. Note that this means that our object walk
>
> "this means that" could be rephrased in a way a bit more helpful and
> to readers with clarity, perhaps:
>
>         Note that our object walk will not perform any better than
>         an unfiltered walk with this function, because all reachable
>         objects need to be walked in order to ...

This proposed text has a small ambiguity, it can be parsed as:
- Note that (with this function) our object walk will not perform any
better than an unfiltered walk [implying that the function change
itself is the cause of the performance concern]
or
- Note that (our object walk) will not perform any better than an
(unfiltered walk with this function)  [implying that
`traverse_commit_list_filtered` has a filtered and an unfiltered mode
of operation [which it does...]]

The issue is that the name `traverse_commit_list_filtered` is poorly named: `traverse_commit_list` and `traverse_commit_list_filtered` are the exact same function (both support filtering!), it's just that `traverse_commit_list_filtered` is able to announce what was filtered.

Perhaps:

    Note that requesting the list of filtered objects may have
performance implications; all reachable objects will be visited in
order to populate the list of filtered objects.

I'm intentionally being ambiguous about it _definitely_ having performance implications, because it's context dependent. It looks like only the `filter_trees_depth` function actually changes what it visits depending on whether the omits list was specified or not.

Show 22 quoted lines

>
> > +will not perform any better than an unfiltered object walk; all reachable
> > +objects are walked in order to populate the list.
>
> Other than that, looking very good.
>
> Thanks, both.
>
> >  First, add the `struct oidset` and related items we will use to iterate it:
> >
> > @@ -778,8 +779,9 @@ static void walken_object_walk(
> >       ...
> >  ----
> >
> > -Modify the call to `traverse_commit_list_filtered()` to include your `omitted`
> > -object:
> > +Replace the call to `traverse_commit_list()` with
> > +`traverse_commit_list_filtered()` and pass a pointer to the `omitted` oidset
> > +defined and initialized above:
> >
> >  ----
> >       ...