| Message ID | cfa4b9ce503e98035d3ce09b0c9e00bcfb6ff70a.1711368499.git.dirk@gouders.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fixes for Documentation/MyFirstObjectWalk.txt | expand |
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 ... > +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: > > ---- > ...
Junio C Hamano <gitster@pobox.com> writes: > 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 ... Would it be OK to rearrange it even more? To me, the above raises the new question "How do I use traverse_commit_list_filtered() to do an unfiltered walk?": Note that our object walk with this function will not perform any better than the previous unfiltered walk, because all reachable objects need to be walked in order to ... Dirk
On Mon, Mar 25, 2024 at 10:25 AM Junio C Hamano <gitster@pobox.com> wrote: > > 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. > > > +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: > > > > ---- > > ...
Dirk Gouders <dirk@gouders.net> writes: >> "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 ... > > Would it be OK to rearrange it even more? Sure. We are in the business of clarifying this document, so making it easier to read is very much welcomed.
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 +will 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: +Replace the call to `traverse_commit_list()` with +`traverse_commit_list_filtered()` and pass a pointer to the `omitted` 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. 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(-)