Re: [PATCH v4 0/5] Fixes for Documentation/MyFirstObjectWalk.txt

[Kyle Lippincott <spectral@google.com>]

On Tue, Mar 26, 2024 at 02:08:35PM +0100, Dirk Gouders wrote:
> The 4th round of this series.
> 
> Chances are that I just waste your time with my attemt [4/5].
> My appologies in advance, should this be the case.
> 
> Recently, there was a discussion [1] on the groff mailing list and I
> guess I couldn't resist to try to practice what I read in the linked
> resources ;-)
> 
> [1] https://lists.gnu.org/archive/html/groff/2024-03/msg00014.html
> 
> Could be that the remaining controversal part of [4/5] should just be
> left untouched, because it is consuming so much time -- I summarized
> all those versions, so that all incarnations can be compared in one
> view:
> 
> * Original:
> 
> 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.
> 
> * v3:
> 
> 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.
> 
> * Junio's suggestion (with minor rearrangement):
> 
> 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 populate the list of filtered
> objects.
> 
> * Kyle's suggestion:
> 
> 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.
> 
> * My new attempt (v4):
> 
> This list of filtered objects may have performance implications,
> however, because despite filtering objects, the possibly much larger
> set of all reachable objects must be processed in order to populate
> that list.

I agree with the issues Junio raised on this phrasing, and trust in Junio's
judgement to get to a clear phrasing :) I'll be unresponsive to email for at
least the next two weeks, so please don't block awaiting my response on any
future rerolls.

> 
> --
> Changes in v4:
> * Used the proper `git show` for references in [1/5] and [3/5]
> * Another attempt to write clear speach in [4/5]
> 
> Changes in v3:
> * Reword the description in [4/5]
> * Add a missing slash in [5/5]
> 
> Changes in v2:
> * Added Emily to Cc in the hope for a review
> * Remove superfluous tags from [1/5] and [3/5]
> * Replace bashism `|&` by `2>&1 |` in [5/5]
> --
> Dirk Gouders (5):
>   MyFirstObjectWalk: use additional arg in config_fn_t
>   MyFirstObjectWalk: fix misspelled "builtins/"
>   MyFirstObjectWalk: fix filtered object walk
>   MyFirstObjectWalk: fix description for counting omitted objects
>   MyFirstObjectWalk: add stderr to pipe processing
> 
>  Documentation/MyFirstObjectWalk.txt | 37 ++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 16 deletions(-)
> 
> Range-diff against v3:
> 1:  0eeb4b78ac ! 1:  102cbc54c4 MyFirstObjectWalk: use additional arg in config_fn_t
>     @@ Metadata
>       ## Commit message ##
>          MyFirstObjectWalk: use additional arg in config_fn_t
>      
>     -    Commit a4e7e317 (config: add ctx arg to config_fn_t) added a fourth
>     -    argument to config_fn_t but did not change relevant function calls
>     -    in Documentation/MyFirstObjectWalk.txt.
>     +    Commit a4e7e317f8 (config: add ctx arg to config_fn_t, 2023-06-28)
>     +    added a fourth argument to config_fn_t but did not change relevant
>     +    function calls in Documentation/MyFirstObjectWalk.txt.
>      
>          Fix those calls and the example git_walken_config() to use
>          that additional argument.
> 2:  3122ae2472 = 2:  5fb7953f31 MyFirstObjectWalk: fix misspelled "builtins/"
> 3:  f21348ab80 ! 3:  b88518df0b MyFirstObjectWalk: fix filtered object walk
>     @@ Metadata
>       ## Commit message ##
>          MyFirstObjectWalk: fix filtered object walk
>      
>     -    Commit f0d2f849 (MyFirstObjectWalk: update recommended usage)
>     -    changed a call of parse_list_objects_filter() in a way that
>     -    probably never worked: parse_list_objects_filter() always needed a
>     -    pointer as its first argument.
>     +    Commit f0d2f84919 (MyFirstObjectWalk: update recommended usage,
>     +    2022-03-09) changed a call of parse_list_objects_filter() in a way
>     +    that probably never worked: parse_list_objects_filter() always needed
>     +    a pointer as its first argument.
>      
>          Fix this by removing the CALLOC_ARRAY and passing the address of
>          rev->filter to parse_list_objects_filter() in accordance to
> 4:  cfa4b9ce50 ! 4:  11510630af MyFirstObjectWalk: fix description for counting omitted objects
>     @@ Documentation/MyFirstObjectWalk.txt: points to the same tree object as its grand
>      -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.
>     ++able to populate an `omitted` list.  This list of filtered objects may have
>     ++performance implications, however, because despite filtering objects, the possibly
>     ++much larger set of all reachable objects must be processed in order to
>     ++populate that list.
>       
>       First, add the `struct oidset` and related items we will use to iterate it:
>       
> 5:  c571abb49d = 5:  8920313ee2 MyFirstObjectWalk: add stderr to pipe processing
> -- 
> 2.43.0
>