[PATCH 1/5] MyFirstObjectWalk: use additional arg in config_fn_t

[PATCH 1/5] MyFirstObjectWalk: use additional arg in config_fn_t

[Dirk Gouders]

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.

Fixes: a4e7e317 (config: add ctx arg to config_fn_t)
Cc: Glen Choo <glencbz@gmail.com>
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

[PATCH 2/5] MyFirstObjectWalk: fix misspelled "builtins/"

[Dirk Gouders]

pack-objects.c resides in builtin/ (not builtins/).

Fix the misspelled directory name.

Signed-off-by: Dirk Gouders <dirk@gouders.net>
---
 Documentation/MyFirstObjectWalk.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
index cceac2df95..c33d22ae99 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -525,7 +525,7 @@ about each one.
 
 We can base our work on an example. `git pack-objects` prepares all kinds of
 objects for packing into a bitmap or packfile. The work we are interested in
-resides in `builtins/pack-objects.c:get_object_list()`; examination of that
+resides in `builtin/pack-objects.c:get_object_list()`; examination of that
 function shows that the all-object walk is being performed by
 `traverse_commit_list()` or `traverse_commit_list_filtered()`. Those two
 functions reside in `list-objects.c`; examining the source shows that, despite
-- 
2.43.0

[PATCH 3/5] MyFirstObjectWalk: fix filtered object walk

[Dirk Gouders]

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.

Fix this by removing the CALLOC_ARRAY and passing the address of
rev->filter to parse_list_objects_filter() in accordance to
such a call in revisions.c, for example.

Fixes: f0d2f849 (MyFirstObjectWalk: update recommended usage)
Cc: Derrick Stolee <stolee@gmail.com>
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 c33d22ae99..a06c712e46 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -734,8 +734,8 @@ walk we've just performed:
 	} else {
 		trace_printf(
 			_("Filtered object walk with filterspec 'tree:1'.\n"));
-		CALLOC_ARRAY(rev->filter, 1);
-		parse_list_objects_filter(rev->filter, "tree:1");
+
+		parse_list_objects_filter(&rev->filter, "tree:1");
 	}
 	traverse_commit_list(rev, walken_show_commit,
 			     walken_show_object, NULL);
-- 
2.43.0

[PATCH 4/5] MyFirstObjectWalk: fix description for counting omitted objects

[Dirk Gouders]

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
+`traverse_commit_list_filtered()` to be able to pass a pointer to the
+oidset defined and initialized above:
 
 ----
 	...
-- 
2.43.0

[PATCH 5/5] MyFirstObjectWalk: add stderr to pipe processing

[Dirk Gouders]

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 using the 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..b96724c4d7 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 |& 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 |& tail -n 10
 ----
 
 The last commit object given should have the same OID as the one we saw at the
-- 
2.43.0

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

[Dirk Gouders]

This series contains fixes for some problems in
Documentation/MyFirstObjectWalk.txt that I noticed while working
through that document.

All patches except [4/5] fix real errors that cause comilation
failures or don't produce the wanted output.
With the fix in [3/5] I'm rather uncertain if it is correct and I
would like to ask for special attention, here.

For this first round I send separate patches in the hope that
simplifies reviews and also, because the reasons for those problems
differ.  Finally, those patches could be merged into one should this be
more appropriate.

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

-- 
2.43.0

Re: [PATCH 5/5] MyFirstObjectWalk: add stderr to pipe processing

[Junio C Hamano]

Dirk Gouders <dirk@gouders.net> writes:

> 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 using the operator '|&' to additionally connect stderr to
> stdin of the latter command.

Isn't |& a bash-ism?

    upstream command >&2 | downstream command

is how you would do the same more portably.

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

[Junio C Hamano]

Dirk Gouders <dirk@gouders.net> writes:

> This series contains fixes for some problems in
> Documentation/MyFirstObjectWalk.txt that I noticed while working
> through that document.
>
> All patches except [4/5] fix real errors that cause comilation
> failures or don't produce the wanted output.
> With the fix in [3/5] I'm rather uncertain if it is correct and I
> would like to ask for special attention, here.
>
> For this first round I send separate patches in the hope that
> simplifies reviews and also, because the reasons for those problems
> differ.  Finally, those patches could be merged into one should this be
> more appropriate.
>
> 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(-)

Emily, if I am not misremembering, this document came from your
direction, so perhaps you have enough familiarity to lend these
patches an eye or two to look them over?

Thanks.

Re: [PATCH 1/5] MyFirstObjectWalk: use additional arg in config_fn_t

[Junio C Hamano]

Dirk Gouders <dirk@gouders.net> writes:

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


> Fixes: a4e7e317 (config: add ctx arg to config_fn_t)
> Cc: Glen Choo <glencbz@gmail.com>

I know what you wanted to convey with these, but in this project we
do not use them.  Both becomes unnecessary once you write the first
part of the log message above ;-)

> 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);
>  }
>  ----

Re: [PATCH 5/5] MyFirstObjectWalk: add stderr to pipe processing

[Dirk Gouders]

Junio C Hamano <gitster@pobox.com> writes:

> Dirk Gouders <dirk@gouders.net> writes:
>
>> 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 using the operator '|&' to additionally connect stderr to
>> stdin of the latter command.
>
> Isn't |& a bash-ism?
>
>     upstream command >&2 | downstream command
>
> is how you would do the same more portably.

Ah yes, it is a bash-ism, thank you.

I guess you meant

        upstream command 2>&1 | downstream command

I will wait a bit (some days) for other comments
and then at least remove this bash-ism plus the wrong tags you also
mentioned -- also thanks for that.

Dirk

Re: [PATCH 5/5] MyFirstObjectWalk: add stderr to pipe processing

[Junio C Hamano]

Dirk Gouders <dirk@gouders.net> writes:

> Ah yes, it is a bash-ism, thank you.
>
> I guess you meant

Yup, thanks, that was a typo.

>         upstream command 2>&1 | downstream command

[PATCH v2 0/5] Fixes for Documentation/MyFirstObjectWalk.txt

[Dirk Gouders]

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

Range-diff against v1:
1:  15b74566e0 ! 1:  babf04295e MyFirstObjectWalk: use additional arg in config_fn_t
    @@ Commit message
         Fix those calls and the example git_walken_config() to use
         that additional argument.
     
    -    Fixes: a4e7e317 (config: add ctx arg to config_fn_t)
    -    Cc: Glen Choo <glencbz@gmail.com>
         Signed-off-by: Dirk Gouders <dirk@gouders.net>
     
      ## Documentation/MyFirstObjectWalk.txt ##
2:  c1ac705840 = 2:  ab0b820df7 MyFirstObjectWalk: fix misspelled "builtins/"
3:  0f67a161ef ! 3:  fac6886af3 MyFirstObjectWalk: fix filtered object walk
    @@ Commit message
         rev->filter to parse_list_objects_filter() in accordance to
         such a call in revisions.c, for example.
     
    -    Fixes: f0d2f849 (MyFirstObjectWalk: update recommended usage)
    -    Cc: Derrick Stolee <stolee@gmail.com>
         Signed-off-by: Dirk Gouders <dirk@gouders.net>
     
      ## Documentation/MyFirstObjectWalk.txt ##
4:  637070dd48 = 4:  33a1845889 MyFirstObjectWalk: fix description for counting omitted objects
5:  a2d30eff21 ! 5:  64c36dbf16 MyFirstObjectWalk: add stderr to pipe processing
    @@ Commit message
         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 using the operator '|&' to additionally connect stderr to
    -    stdin of the latter command.
    +    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: those lines without having to recompile.
      
      ----
     -$ GIT_TRACE=1 ./bin-wrappers/git walken | head -n 10
    -+$ 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
    @@ Documentation/MyFirstObjectWalk.txt: of the first handful:
      ----
      $ make
     -$ GIT_TRACE=1 ./bin-wrappers git walken | tail -n 10
    -+$ 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

[PATCH v2 1/5] MyFirstObjectWalk: use additional arg in config_fn_t

[Dirk Gouders]

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

[PATCH v2 2/5] MyFirstObjectWalk: fix misspelled "builtins/"

[Dirk Gouders]

pack-objects.c resides in builtin/ (not builtins/).

Fix the misspelled directory name.

Signed-off-by: Dirk Gouders <dirk@gouders.net>
---
 Documentation/MyFirstObjectWalk.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
index cceac2df95..c33d22ae99 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -525,7 +525,7 @@ about each one.
 
 We can base our work on an example. `git pack-objects` prepares all kinds of
 objects for packing into a bitmap or packfile. The work we are interested in
-resides in `builtins/pack-objects.c:get_object_list()`; examination of that
+resides in `builtin/pack-objects.c:get_object_list()`; examination of that
 function shows that the all-object walk is being performed by
 `traverse_commit_list()` or `traverse_commit_list_filtered()`. Those two
 functions reside in `list-objects.c`; examining the source shows that, despite
-- 
2.43.0

[PATCH v2 3/5] MyFirstObjectWalk: fix filtered object walk

[Dirk Gouders]

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.

Fix this by removing the CALLOC_ARRAY and passing the address of
rev->filter to parse_list_objects_filter() in accordance to
such a call in revisions.c, for example.

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 c33d22ae99..a06c712e46 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -734,8 +734,8 @@ walk we've just performed:
 	} else {
 		trace_printf(
 			_("Filtered object walk with filterspec 'tree:1'.\n"));
-		CALLOC_ARRAY(rev->filter, 1);
-		parse_list_objects_filter(rev->filter, "tree:1");
+
+		parse_list_objects_filter(&rev->filter, "tree:1");
 	}
 	traverse_commit_list(rev, walken_show_commit,
 			     walken_show_object, NULL);
-- 
2.43.0

[PATCH v2 4/5] MyFirstObjectWalk: fix description for counting omitted objects

[Dirk Gouders]

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
+`traverse_commit_list_filtered()` to be able to pass a pointer to the
+oidset defined and initialized above:
 
 ----
 	...
-- 
2.43.0

[PATCH v2 5/5] MyFirstObjectWalk: add stderr to pipe processing

[Dirk Gouders]

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

Re: [PATCH v2 1/5] MyFirstObjectWalk: use additional arg in config_fn_t

[Kyle Lippincott]

On Tue, Mar 19, 2024 at 12:23:11PM +0100, Dirk Gouders wrote:
> 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(-)

Looks good, thanks.

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

Re: [PATCH v2 5/5] MyFirstObjectWalk: add stderr to pipe processing

[Kyle Lippincott]

On Tue, Mar 19, 2024 at 12:23:15PM +0100, Dirk Gouders wrote:
> 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

I think there's a second issue here: this should be `./bin-wrappers/git`, right?

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

Re: [PATCH v2 5/5] MyFirstObjectWalk: add stderr to pipe processing

[Dirk Gouders]

Kyle Lippincott <spectral@google.com> writes:

> On Tue, Mar 19, 2024 at 12:23:15PM +0100, Dirk Gouders wrote:

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

Oh yes, that is a second issue -- thank you very much for spending the
time to look at this series.

Dirk

Re: [PATCH v2 4/5] MyFirstObjectWalk: fix description for counting omitted objects

[Kyle Lippincott]

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

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

[Kyle Lippincott]

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.

Re: [PATCH v2 4/5] MyFirstObjectWalk: fix description for counting omitted objects

[Dirk Gouders]

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

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

[Dirk Gouders]

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.

Dirk

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

[Junio C Hamano]

Dirk Gouders <dirk@gouders.net> writes:

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

Yeah, I too liked the comments on [4/5].  Thanks for working well
together.

[PATCH v3 0/5] Fixes for Documentation/MyFirstObjectWalk.txt

[Dirk Gouders]

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
1:  4219237868 ! 4:  cfa4b9ce50 MyFirstObjectWalk: fix description for counting omitted objects
    @@ Commit message
         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 ##
    @@ Documentation/MyFirstObjectWalk.txt: points to the same tree object as its grand
     -`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.
    ++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:
      
    @@ Documentation/MyFirstObjectWalk.txt: 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:
    ++Replace the call to `traverse_commit_list()` with
    ++`traverse_commit_list_filtered()` and pass a pointer to the `omitted` oidset
    ++defined and initialized above:
      
      ----
      	...
2:  9b0f0832b7 ! 5:  c571abb49d MyFirstObjectWalk: add stderr to pipe processing
    @@ Commit message
         Fix this by redirecting stderr to stdout prior to the pipe operator
         to additionally connect stderr to stdin of the latter command.
     
    +    Further, while reviewing the above fix, Kyle Lippincott noticed
    +    a second issue with the second of the examples: a missing slash in the
    +    executable path "./bin-wrappers git".
    +
    +    Add the missing slash.
    +
    +    Helped-by: Kyle Lippincott <spectral@google.com>
         Signed-off-by: Dirk Gouders <dirk@gouders.net>
     
      ## Documentation/MyFirstObjectWalk.txt ##
    @@ Documentation/MyFirstObjectWalk.txt: 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
    ++$ 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

[PATCH v3 1/5] MyFirstObjectWalk: use additional arg in config_fn_t

[Dirk Gouders]

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

[PATCH v3 2/5] MyFirstObjectWalk: fix misspelled "builtins/"

[Dirk Gouders]

pack-objects.c resides in builtin/ (not builtins/).

Fix the misspelled directory name.

Signed-off-by: Dirk Gouders <dirk@gouders.net>
---
 Documentation/MyFirstObjectWalk.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
index cceac2df95..c33d22ae99 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -525,7 +525,7 @@ about each one.
 
 We can base our work on an example. `git pack-objects` prepares all kinds of
 objects for packing into a bitmap or packfile. The work we are interested in
-resides in `builtins/pack-objects.c:get_object_list()`; examination of that
+resides in `builtin/pack-objects.c:get_object_list()`; examination of that
 function shows that the all-object walk is being performed by
 `traverse_commit_list()` or `traverse_commit_list_filtered()`. Those two
 functions reside in `list-objects.c`; examining the source shows that, despite
-- 
2.43.0

[PATCH v3 3/5] MyFirstObjectWalk: fix filtered object walk

[Dirk Gouders]

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.

Fix this by removing the CALLOC_ARRAY and passing the address of
rev->filter to parse_list_objects_filter() in accordance to
such a call in revisions.c, for example.

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 c33d22ae99..a06c712e46 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -734,8 +734,8 @@ walk we've just performed:
 	} else {
 		trace_printf(
 			_("Filtered object walk with filterspec 'tree:1'.\n"));
-		CALLOC_ARRAY(rev->filter, 1);
-		parse_list_objects_filter(rev->filter, "tree:1");
+
+		parse_list_objects_filter(&rev->filter, "tree:1");
 	}
 	traverse_commit_list(rev, walken_show_commit,
 			     walken_show_object, NULL);
-- 
2.43.0

[PATCH v3 4/5] MyFirstObjectWalk: fix description for counting omitted objects

[Dirk Gouders]

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

[PATCH v3 5/5] MyFirstObjectWalk: add stderr to pipe processing

[Dirk Gouders]

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.

Further, while reviewing the above fix, Kyle Lippincott noticed
a second issue with the second of the examples: a missing slash in the
executable path "./bin-wrappers git".

Add the missing slash.

Helped-by: Kyle Lippincott <spectral@google.com>
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 811175837c..3d78403c4a 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

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

[Kyle Lippincott]

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!

Re: [PATCH v3 1/5] MyFirstObjectWalk: use additional arg in config_fn_t

[Junio C Hamano]

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.

> 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);
>  }
>  ----

Re: [PATCH v3 4/5] MyFirstObjectWalk: fix description for counting omitted objects

[Junio C Hamano]

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

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

[Junio C Hamano]

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.

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

[Kyle Lippincott]

On Mon, Mar 25, 2024 at 10:50 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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?

On another project, I've had a (separate) test file that just does
what the tutorial says to do, and there's an automatic notice for
"you're touching tutorial-test.sh, make sure you make any required
changes to tutorial.txt as well". I don't know if we have that second
part available to us here, though.

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

Re: [PATCH v3 1/5] MyFirstObjectWalk: use additional arg in config_fn_t

[Dirk Gouders]

Junio C Hamano <gitster@pobox.com> writes:

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

Yes, thanks, will do.
(This means I read SubmittingPatches too fast and have to re-read it to
see if I missed more details.)

Dirk

Re: [PATCH v3 4/5] MyFirstObjectWalk: fix description for counting omitted objects

[Dirk Gouders]

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

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

[Dirk Gouders]

Kyle Lippincott <spectral@google.com> writes:

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

Thank you for looking at it.

Dirk

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

[Dirk Gouders]

Junio C Hamano <gitster@pobox.com> writes:

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

My plan was to also work through MyFirstContribution.txt and test
everything which at least should disclose currently existing issues.

But, besides those tutorial documents, many manual pages also contain
examples and I wonder how these are maintained.

Dirk

Re: [PATCH v3 4/5] MyFirstObjectWalk: fix description for counting omitted objects

[Kyle Lippincott]

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

Re: [PATCH v3 4/5] MyFirstObjectWalk: fix description for counting omitted objects

[Junio C Hamano]

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.

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

[Dirk Gouders]

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

[PATCH v4 1/5] MyFirstObjectWalk: use additional arg in config_fn_t

[Dirk Gouders]

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.

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

[PATCH v4 2/5] MyFirstObjectWalk: fix misspelled "builtins/"

[Dirk Gouders]

pack-objects.c resides in builtin/ (not builtins/).

Fix the misspelled directory name.

Signed-off-by: Dirk Gouders <dirk@gouders.net>
---
 Documentation/MyFirstObjectWalk.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
index cceac2df95..c33d22ae99 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -525,7 +525,7 @@ about each one.
 
 We can base our work on an example. `git pack-objects` prepares all kinds of
 objects for packing into a bitmap or packfile. The work we are interested in
-resides in `builtins/pack-objects.c:get_object_list()`; examination of that
+resides in `builtin/pack-objects.c:get_object_list()`; examination of that
 function shows that the all-object walk is being performed by
 `traverse_commit_list()` or `traverse_commit_list_filtered()`. Those two
 functions reside in `list-objects.c`; examining the source shows that, despite
-- 
2.43.0

[PATCH v4 3/5] MyFirstObjectWalk: fix filtered object walk

[Dirk Gouders]

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
such a call in revisions.c, for example.

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 c33d22ae99..a06c712e46 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -734,8 +734,8 @@ walk we've just performed:
 	} else {
 		trace_printf(
 			_("Filtered object walk with filterspec 'tree:1'.\n"));
-		CALLOC_ARRAY(rev->filter, 1);
-		parse_list_objects_filter(rev->filter, "tree:1");
+
+		parse_list_objects_filter(&rev->filter, "tree:1");
 	}
 	traverse_commit_list(rev, walken_show_commit,
 			     walken_show_object, NULL);
-- 
2.43.0

[PATCH v4 4/5] MyFirstObjectWalk: fix description for counting omitted objects

[Dirk Gouders]

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 | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

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.
 
 First, add the `struct oidset` and related items we will use to iterate it:
 
@@ -778,8 +780,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:
 
 ----
 	...
-- 
2.43.0

[PATCH v4 5/5] MyFirstObjectWalk: add stderr to pipe processing

[Dirk Gouders]

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.

Further, while reviewing the above fix, Kyle Lippincott noticed
a second issue with the second of the examples: a missing slash in the
executable path "./bin-wrappers git".

Add the missing slash.

Helped-by: Kyle Lippincott <spectral@google.com>
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 6901561263..90446c30a4 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -848,7 +848,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
@@ -876,7 +876,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

Re: [PATCH v4 4/5] MyFirstObjectWalk: fix description for counting omitted objects

[Junio C Hamano]

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?

Other than that I had no trouble with this latest round.

Thanks.

Re: [PATCH v4 4/5] MyFirstObjectWalk: fix description for counting omitted objects

[Dirk Gouders]

Junio C Hamano <gitster@pobox.com> writes:

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

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

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

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

This answer has the potential to cause an enhancement request, anyway:

Previously, the document didn't state that
traverse_commit_list_filtered() can be used without asking for a
`omitted` list (and I didn't change that), so the contrasting
in my understanding explicitely is traverse_commit_list()
vs. traverse_commit_list_filtered().

The second of your cases is only included implicitely, for those who
know or can guess they could use NULL as the pointer to `omitted` list.

Thank you for looking at this one more time!

Dirk

> Other than that I had no trouble with this latest round.
>
> Thanks.

Re: [PATCH v4 4/5] MyFirstObjectWalk: fix description for counting omitted objects

[Junio C Hamano]

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.

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

[Kyle Lippincott]

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
> 

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

[Dirk Gouders]

Kyle Lippincott <spectral@google.com> writes:

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

Thank you.

Re: [PATCH v4 4/5] MyFirstObjectWalk: fix description for counting omitted objects

[Dirk Gouders]

Junio C Hamano <gitster@pobox.com> writes:

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

Thank you for the clarification with "implications"
I will fix it.

Dirk

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

[PATCH v5 0/5] Fixes for Documentation/MyFirstObjectWalk.txt

[Dirk Gouders]

The fifth round with the correction that a list itself cannot have
performance implications -- asking for one may have.

---
Changes in v5:
* Rephrase implications that asking for an `omitted` list
  may have in [4/5]

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 v4:
1:  2501fea789 = 1:  292ae67548 MyFirstObjectWalk: use additional arg in config_fn_t
2:  6b336e15d6 = 2:  99284db8c1 MyFirstObjectWalk: fix misspelled "builtins/"
3:  83034594e2 = 3:  e1b4a4c996 MyFirstObjectWalk: fix filtered object walk
4:  f23ff9fd1b ! 4:  e6030f1c0a 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.  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.
    ++able to populate an `omitted` list.  Asking for this list of filtered objects
    ++may cause performance degradations, 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.
      
      First, add the `struct oidset` and related items we will use to iterate it:
      
5:  582cb7d44a = 5:  0a3dbd1452 MyFirstObjectWalk: add stderr to pipe processing
-- 
2.43.0

[PATCH v5 1/5] MyFirstObjectWalk: use additional arg in config_fn_t

[Dirk Gouders]

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.

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

[PATCH v5 2/5] MyFirstObjectWalk: fix misspelled "builtins/"

[Dirk Gouders]

pack-objects.c resides in builtin/ (not builtins/).

Fix the misspelled directory name.

Signed-off-by: Dirk Gouders <dirk@gouders.net>
---
 Documentation/MyFirstObjectWalk.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
index cceac2df95..c33d22ae99 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -525,7 +525,7 @@ about each one.
 
 We can base our work on an example. `git pack-objects` prepares all kinds of
 objects for packing into a bitmap or packfile. The work we are interested in
-resides in `builtins/pack-objects.c:get_object_list()`; examination of that
+resides in `builtin/pack-objects.c:get_object_list()`; examination of that
 function shows that the all-object walk is being performed by
 `traverse_commit_list()` or `traverse_commit_list_filtered()`. Those two
 functions reside in `list-objects.c`; examining the source shows that, despite
-- 
2.43.0

[PATCH v5 3/5] MyFirstObjectWalk: fix filtered object walk

[Dirk Gouders]

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
such a call in revisions.c, for example.

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 c33d22ae99..a06c712e46 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -734,8 +734,8 @@ walk we've just performed:
 	} else {
 		trace_printf(
 			_("Filtered object walk with filterspec 'tree:1'.\n"));
-		CALLOC_ARRAY(rev->filter, 1);
-		parse_list_objects_filter(rev->filter, "tree:1");
+
+		parse_list_objects_filter(&rev->filter, "tree:1");
 	}
 	traverse_commit_list(rev, walken_show_commit,
 			     walken_show_object, NULL);
-- 
2.43.0

[PATCH v5 4/5] MyFirstObjectWalk: fix description for counting omitted objects

[Dirk Gouders]

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 | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
index a06c712e46..e969a3a68a 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.  Asking for this list of filtered objects
+may cause performance degradations, 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.
 
 First, add the `struct oidset` and related items we will use to iterate it:
 
@@ -778,8 +780,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:
 
 ----
 	...
-- 
2.43.0

[PATCH v5 5/5] MyFirstObjectWalk: add stderr to pipe processing

[Dirk Gouders]

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.

Further, while reviewing the above fix, Kyle Lippincott noticed
a second issue with the second of the examples: a missing slash in the
executable path "./bin-wrappers git".

Add the missing slash.

Helped-by: Kyle Lippincott <spectral@google.com>
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 e969a3a68a..dec8afe5b1 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -848,7 +848,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
@@ -876,7 +876,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