v2, 5 partsMyFirstObjectWalk: 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.
> 
> Fix those calls and the example git_walken_config() to use
> that additional argument.
> 
> Signed-off-by: Dirk Gouders <dirk@gouders.net>
> ---
>  Documentation/MyFirstObjectWalk.txt | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)

> 
> diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
> index c68cdb11b9..cceac2df95 100644
> --- a/Documentation/MyFirstObjectWalk.txt
> +++ b/Documentation/MyFirstObjectWalk.txt
> @@ -210,13 +210,14 @@ We'll also need to include the `config.h` header:
>  
>  ...
>  
> -static int git_walken_config(const char *var, const char *value, void *cb)
> +static int git_walken_config(const char *var, const char *value,
> +			     const struct config_context *ctx, void *cb)
>  {
>  	/*
>  	 * For now, we don't have any custom configuration, so fall back to
>  	 * the default config.
>  	 */
> -	return git_default_config(var, value, cb);
> +	return git_default_config(var, value, ctx, cb);
>  }
>  ----
>  
> @@ -389,10 +390,11 @@ modifying `rev_info.grep_filter`, which is a `struct grep_opt`.
>  First some setup. Add `grep_config()` to `git_walken_config()`:
>  
>  ----
> -static int git_walken_config(const char *var, const char *value, void *cb)
> +static int git_walken_config(const char *var, const char *value,
> +			     const struct config_context *ctx, void *cb)
>  {
> -	grep_config(var, value, cb);
> -	return git_default_config(var, value, cb);
> +	grep_config(var, value, ctx, cb);
> +	return git_default_config(var, value, ctx, cb);
>  }
>  ----
>  
> -- 
> 2.43.0
> 
> 

> 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.

>  
>  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

> +oidset defined and initialized above:
>  
>  ----
>  	...
> -- 
> 2.43.0
> 
> 

> 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:

> In the last chapter of this document, pipes are used in commands to
> filter out the first/last trace messages.  But according to git(1),
> trace messages are sent to stderr if GIT_TRACE is set to '1', so those
> commands do not produce the described results.
> 
> Fix this by redirecting stderr to stdout prior to the pipe operator
> to additionally connect stderr to stdin of the latter command.
> 
> Signed-off-by: Dirk Gouders <dirk@gouders.net>
> ---
>  Documentation/MyFirstObjectWalk.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
> index 981dbf917b..2e6ae4d7fc 100644
> --- a/Documentation/MyFirstObjectWalk.txt
> +++ b/Documentation/MyFirstObjectWalk.txt
> @@ -847,7 +847,7 @@ those lines without having to recompile.
>  With only that change, run again (but save yourself some scrollback):
>  
>  ----
> -$ GIT_TRACE=1 ./bin-wrappers/git walken | head -n 10
> +$ GIT_TRACE=1 ./bin-wrappers/git walken 2>&1 | head -n 10
>  ----
>  
>  Take a look at the top commit with `git show` and the object ID you printed; it
> @@ -875,7 +875,7 @@ of the first handful:
>  
>  ----
>  $ make
> -$ GIT_TRACE=1 ./bin-wrappers git walken | tail -n 10
> +$ GIT_TRACE=1 ./bin-wrappers git walken 2>&1 | tail -n 10

>  ----
>  
>  The last commit object given should have the same OID as the one we saw at the
> -- 
> 2.43.0
> 
> 

>> -$ GIT_TRACE=1 ./bin-wrappers/git walken | head -n 10
>> +$ GIT_TRACE=1 ./bin-wrappers/git walken 2>&1 | head -n 10
>>  ----
>>  
>>  Take a look at the top commit with `git show` and the object ID you printed; it
>> @@ -875,7 +875,7 @@ of the first handful:
>>  
>>  ----
>>  $ make
>> -$ GIT_TRACE=1 ./bin-wrappers git walken | tail -n 10
>> +$ GIT_TRACE=1 ./bin-wrappers git walken 2>&1 | tail -n 10
>
> I think there's a second issue here: this should be `./bin-wrappers/git`, right?

> The second spin for this series.
> ---
> Changes since v1:
> * 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 | 36 ++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 16 deletions(-)

> On Tue, Mar 19, 2024 at 12:23:10PM +0100, Dirk Gouders wrote:
>> The second spin for this series.
>> ---
>> Changes since v1:
>> * 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 | 36 ++++++++++++++++-------------
>>  1 file changed, 20 insertions(+), 16 deletions(-)
>
> Aside from the small comments on 4 and 5, series looks good to me, thanks for
> working on this.

> Kyle Lippincott <spectral@google.com> writes:
>
>> On Tue, Mar 19, 2024 at 12:23:10PM +0100, Dirk Gouders wrote:
>>> The second spin for this series.
>>> ---
>>> Changes since v1:
>>> * 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 | 36 ++++++++++++++++-------------
>>>  1 file changed, 20 insertions(+), 16 deletions(-)
>>
>> Aside from the small comments on 4 and 5, series looks good to me, thanks for
>> working on this.
>
> Thanks for the review -- especially for the detailed explanation and
> suggestions on 4.

> 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

>  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:
>
>> 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 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?

>
> 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:
> >
> >  ----
> >       ...

> 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.
>
> Signed-off-by: Dirk Gouders <dirk@gouders.net>
> ---
>  Documentation/MyFirstObjectWalk.txt | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
> index c68cdb11b9..cceac2df95 100644
> --- a/Documentation/MyFirstObjectWalk.txt
> +++ b/Documentation/MyFirstObjectWalk.txt
> @@ -210,13 +210,14 @@ We'll also need to include the `config.h` header:
>  
>  ...
>  
> -static int git_walken_config(const char *var, const char *value, void *cb)
> +static int git_walken_config(const char *var, const char *value,
> +			     const struct config_context *ctx, void *cb)
>  {
>  	/*
>  	 * For now, we don't have any custom configuration, so fall back to
>  	 * the default config.
>  	 */
> -	return git_default_config(var, value, cb);
> +	return git_default_config(var, value, ctx, cb);
>  }
>  ----
>  
> @@ -389,10 +390,11 @@ modifying `rev_info.grep_filter`, which is a `struct grep_opt`.
>  First some setup. Add `grep_config()` to `git_walken_config()`:
>  
>  ----
> -static int git_walken_config(const char *var, const char *value, void *cb)
> +static int git_walken_config(const char *var, const char *value,
> +			     const struct config_context *ctx, void *cb)
>  {
> -	grep_config(var, value, cb);
> -	return git_default_config(var, value, cb);
> +	grep_config(var, value, ctx, cb);
> +	return git_default_config(var, value, ctx, cb);
>  }
>  ----

> Dirk Gouders <dirk@gouders.net> writes:
>
>> Commit a4e7e317 (config: add ctx arg to config_fn_t) added a fourth
>
> In your next topic, use "git show -s --pretty=reference a4e7e317" to
> show "a4e7e317f8 (config: add ctx arg to config_fn_t, 2023-06-28)"
> with dates.  It makes it easier to see how long what is being fixed
> is broken, giving reviewers a sense of urgency for a fix.  It is not
> necessary to reroll this commit only to update the reference, though.

>
> The 3rd iteration for this series.
>
> I tried to credit Kyle's suggestions for 4 and 5 with Helped-by tags and
> hope it was adequate to do so.  Actually, at least #4 was a lot more
> than a Helped-by, I would say...
> ---
> 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 | 36 ++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 16 deletions(-)
>
> Range-diff against v2:
> -:  ---------- > 1:  0eeb4b78ac MyFirstObjectWalk: use additional arg in config_fn_t
> -:  ---------- > 2:  3122ae2472 MyFirstObjectWalk: fix misspelled "builtins/"
> -:  ---------- > 3:  f21348ab80 MyFirstObjectWalk: fix filtered object walk

> On Mon, Mar 25, 2024 at 6:19 AM Dirk Gouders <dirk@gouders.net> wrote:
>>
>> The 3rd iteration for this series.
>>
>> I tried to credit Kyle's suggestions for 4 and 5 with Helped-by tags and
>> hope it was adequate to do so.  Actually, at least #4 was a lot more
>> than a Helped-by, I would say...
>> ---
>> 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 | 36 ++++++++++++++++-------------
>>  1 file changed, 20 insertions(+), 16 deletions(-)
>>
>> Range-diff against v2:
>> -:  ---------- > 1:  0eeb4b78ac MyFirstObjectWalk: use additional arg in config_fn_t
>> -:  ---------- > 2:  3122ae2472 MyFirstObjectWalk: fix misspelled "builtins/"
>> -:  ---------- > 3:  f21348ab80 MyFirstObjectWalk: fix filtered object walk
>
> Looks good, thanks again!

> The 3rd iteration for this series.
>
> I tried to credit Kyle's suggestions for 4 and 5 with Helped-by tags and
> hope it was adequate to do so.  Actually, at least #4 was a lot more
> than a Helped-by, I would say...

>
> Dirk Gouders <dirk@gouders.net> writes:
>
> > The 3rd iteration for this series.
> >
> > I tried to credit Kyle's suggestions for 4 and 5 with Helped-by tags and
> > hope it was adequate to do so.  Actually, at least #4 was a lot more
> > than a Helped-by, I would say...
>
> It seemed adequate, at least to me, but I'll leave the final say up
> to Kyle.
>
> I left a few comments but overall the series is looking much nicer.
> Thanks for working on it (and thanks for reviewing and helping,
> Kyle).
>
> This is an unrelated tangent, but I wonder if we can come up with a
> way to find breakages coming from API updates to these "tutorial"
> documents.  The original "user-manual" also shares the same issue,
> and the issue may be deeper there as it also needs to catch up with
> end-user facing UI updates.  In any case, we somehow ended up with
> two more "tutorial"-ish documents (MyFirstContribution.txt is the
> other one) that somebody needs to keep an eye on.
>
> Ideally if we can have automated tests, it would be nice.  Perhaps
> sprinkling some special instruction in comments that is hidden from
> AsciiDoc mark-up to help our custom program to assemble the bits
> into the state of the tutorial program that the readers should be
> arriving at at different points in the tutorial document, and make
> sure they compile, link, and test well?

> Or "follow one of our three
> tutorial documents to the letter to see if they need adjusting, and
> come up with a set of patches to adjust them" can be listed as one
> of the microproject ideas?  I'll leave a #leftoverbits mark here, but
> what I want to see discussed (and eventually implemented) is not the
> clean-up itself (which can go stale over time) but the strategy to
> keep the "tutorial" material up-to-date.
>
> THanks.
>

> Dirk Gouders <dirk@gouders.net> writes:
>
>> The 3rd iteration for this series.
>>
>> I tried to credit Kyle's suggestions for 4 and 5 with Helped-by tags and
>> hope it was adequate to do so.  Actually, at least #4 was a lot more
>> than a Helped-by, I would say...
>
> It seemed adequate, at least to me, but I'll leave the final say up
> to Kyle.
>
> I left a few comments but overall the series is looking much nicer.
> Thanks for working on it (and thanks for reviewing and helping,
> Kyle).
>
> This is an unrelated tangent, but I wonder if we can come up with a
> way to find breakages coming from API updates to these "tutorial"
> documents.  The original "user-manual" also shares the same issue,
> and the issue may be deeper there as it also needs to catch up with
> end-user facing UI updates.  In any case, we somehow ended up with
> two more "tutorial"-ish documents (MyFirstContribution.txt is the
> other one) that somebody needs to keep an eye on.

> 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.

> 
> --
> 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
> 

> 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.

> diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
> index a06c712e46..6901561263 100644
> --- a/Documentation/MyFirstObjectWalk.txt
> +++ b/Documentation/MyFirstObjectWalk.txt
> @@ -754,10 +754,12 @@ 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.  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.

> Dirk Gouders <dirk@gouders.net> writes:
>
>> diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
>> index a06c712e46..6901561263 100644
>> --- a/Documentation/MyFirstObjectWalk.txt
>> +++ b/Documentation/MyFirstObjectWalk.txt
>> @@ -754,10 +754,12 @@ 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.  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.
>
> It may be just me not reading what is obvious to everybody else
> clearly, in which case I am happy to take the above text as-is, but
> the updated text that says a "list" may have "performance
> implications" reads a bit odd.  It would be understandable if you
> said "asking for list of filtered objects may have", though.

> Are you contrasting a call to traverse_commit_list() and
> traverse_commit_list_filtered() and discussing their relative
> performance?  
>
> Of are you contrasting a call to traverse_commit_list_filtered()
> with and without the omitted parameter, and saying that a call with
> omitted parameter asks the machinery to do more work so it has to
> cost more?

> Oh yes, you are right (as far as I can say): I would change this to
> something like:
>
> "Asking for this list of filtered objects may cause performance
> implications, however, because in this case, despite filtering objects,
> the possibly much larger set of all reachable objects must be processed
> in order to populate that list."

> Dirk Gouders <dirk@gouders.net> writes:
>
>> Oh yes, you are right (as far as I can say): I would change this to
>> something like:
>>
>> "Asking for this list of filtered objects may cause performance
>> implications, however, because in this case, despite filtering objects,
>> the possibly much larger set of all reachable objects must be processed
>> in order to populate that list."
>
> Better, but the verb "cause" applied to "performance implications"
> feels funny.  It may "have" implications.  Alternatively, it may
> "cause" degradations.  As implications can be both positive or
> negative, it would be better to say "cause performancedegradations"
> when you know if it is negative.

>> (Later in the document, it is suggested to do timing with the two
>> versions, which kind of follows up on the performance impact that is
>> focused on, here.  So, this doesn't remain an unresolved detail.)
>
> Great.
>
> Thanks.