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

[Dirk Gouders <dirk@gouders.net>]

Kyle Lippincott <spectral@google.com> writes:

> On Tue, Mar 19, 2024 at 12:23:14PM +0100, Dirk Gouders wrote:
>> 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.
>> 
>> 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..981dbf917b 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`. We
>> +can ask `traverse_commit_list_filtered()` to populate the `omitted`
>> +list which 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.
>
> The way the original was phrased makes it sound to me like "Doing <stuff> via
> <mechanismA> is potentially slow.", and I expect a counter-proposal of using
> mechanismB to resolve that. The rewrite partially avoids that, but I think could
> take it further to really drive home that this is a consequence of using this
> new function, and is not a failing we will be proposing a solution for:

Yes, I had similar thoughts.

>  We can ask `traverse_commit_list_filtered()` to populate the `omitted` list.
> +Note that this means that our object walk will not perform any better than
>  an unfiltered object walk; all reachable objects are walked in order
>  to populate the list.
>
> Since that first sentence is now shorter, we could also add a bit more nuance to
> it, calling out that we're going to switch which function we're using earlier
> (and technically redundantly, but I think that's fine); something like the
> following:
>
>  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
> +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
> +will not perform any better than an unfiltered object walk; all reachable
> +objects are walked in order to populate the list.
>
> Feel free to wordsmith any of my proposed text, and I apologize that these are
> just me typing in something that looks "patch like" in my mail client, not
> properly formatted patches. I think what you have is already an improvement,
> though, so if you think my proposed text is too verbose, I'm fine with what you
> have.

Thank you for your suggestion.  To me, this fits much better and I will
use it should no further improvements being asked for.

>>  
>>  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:
>> +You need to replace the call to `traverse_commit_list()` to
>
> If my proposal to introduce the point that we're switching which function we use
> in the earlier diff hunk is accepted, there's a small nit here: saying "You need
> to" would feel (very slightly) awkward, since we already mentioned that it was
> necessary to accomplish the goal. If we accept the previous proposal, we may
> want to change this to remove the "You need to", and just state something like
> "Replace the call..."
>
> Regardless, I think saying "replace the call to A _with_ B" (instead of "A _to_
> B") reads slightly better. I don't know if that's just a personal
> preference/dialect though.

When I wrote that "You need to" it felt semi-optimal even to me
non-native speaker, but I didn't exactly know what to do with it.  So,
I'm very glad you are helping me to do all that better.

>> +`traverse_commit_list_filtered()` to be able to pass a pointer to the
>
> If we remove the "You need to", then we should probably rephrase this to more
> of an instruction, changing "to be able to" to "and".
>
> Something like this:
>
> -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:

Sounds way better and I'd use it.

Thanks again,

Dirk