| Message ID | 33a18458891259565e553ab39301108ce642d02f.1710840596.git.dirk@gouders.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fixes for Documentation/MyFirstObjectWalk.txt | expand |
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: 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. > > 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. > +`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: > +oidset defined and initialized above: > > ---- > ... > -- > 2.43.0 > >
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
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. 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 +`traverse_commit_list_filtered()` to be able to pass a pointer to the +oidset defined and initialized above: ---- ...
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(-)