Kyle Lippincott <spectral@xxxxxxxxxx> 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@xxxxxxxxxxx> >> --- >> 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
