[06/11] MyFirstObjectWalk: update recommended usage

Commit Message

Derrick Stolee Feb. 23, 2022, 5:55 p.m. UTC

From: Derrick Stolee <[e-mail]>

The previous change consolidated traverse_commit_list() and
traverse_commit_list_filtered(). This allows us to simplify the
recommended usage in MyFirstObjectWalk.txt to use this new set of
values.

While here, add some clarification on the difference between the two
methods.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 Documentation/MyFirstObjectWalk.txt | 44 +++++++++++------------------
 1 file changed, 16 insertions(+), 28 deletions(-)

Comments

Junio C Hamano March 4, 2022, 10:33 p.m. UTC | #1

"Derrick Stolee via GitGitGadget" <[e-mail]> writes:

> From: Derrick Stolee <[e-mail]>
>
> The previous change consolidated traverse_commit_list() and
> traverse_commit_list_filtered(). This allows us to simplify the
> recommended usage in MyFirstObjectWalk.txt to use this new set of
> values.
>
> While here, add some clarification on the difference between the two
> methods.
>
> Signed-off-by: Derrick Stolee <[e-mail]>
> ---
>  Documentation/MyFirstObjectWalk.txt | 44 +++++++++++------------------
>  1 file changed, 16 insertions(+), 28 deletions(-)

Nice simplification.

>
> diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
> index ca267941f3e..8ec83185b8a 100644
> --- a/Documentation/MyFirstObjectWalk.txt
> +++ b/Documentation/MyFirstObjectWalk.txt
> @@ -522,24 +522,25 @@ 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
>  the name, these functions traverse all kinds of objects. Let's have a look at
> -the arguments to `traverse_commit_list_filtered()`, which are a superset of the
> -arguments to the unfiltered version.
> +the arguments to `traverse_commit_list()`.
>  
> -- `struct list_objects_filter_options *filter_options`: This is a struct which
> -  stores a filter-spec as outlined in `Documentation/rev-list-options.txt`.
> -- `struct rev_info *revs`: This is the `rev_info` used for the walk.
> +- `struct rev_info *revs`: This is the `rev_info` used for the walk. It
> +  includes a `filter` member which contains information for how to filter
> +  the object list.

Perhaps,

    "When its `filter` member is not NULL, it contains ..."

implying that it is valid for `filter` member to be NULL and none of
the following things will happen in such a case.

>  For now, we are not going to track the omitted objects, so we'll replace those
>  parameters with `NULL`. For the sake of simplicity, we'll add a simple
> -build-time branch to use our filter or not. Replace the line calling
> +build-time branch to use our filter or not. Preface the line calling

Good eyes.

>  `traverse_commit_list()` with the following, which will remind us which kind of
>  walk we've just performed:
>  
> @@ -733,19 +723,17 @@ walk we've just performed:
>  	if (0) {
>  		/* Unfiltered: */
>  		trace_printf(_("Unfiltered object walk.\n"));
> -		traverse_commit_list(rev, walken_show_commit,
> -				walken_show_object, NULL);
>  	} else {
>  		trace_printf(
>  			_("Filtered object walk with filterspec 'tree:1'.\n"));
> -		parse_list_objects_filter(&filter_options, "tree:1");
> -
> -		traverse_commit_list_filtered(&filter_options, rev,
> -			walken_show_commit, walken_show_object, NULL, NULL);
> +		CALLOC_ARRAY(rev->filter, 1);
> +		parse_list_objects_filter(rev->filter, "tree:1");
>  	}
> +	traverse_commit_list(rev, walken_show_commit,
> +			     walken_show_object, NULL);
>  ----
>  
> -`struct list_objects_filter_options` is usually built directly from a command
> +The `rev->filter` member is usually built directly from a command
>  line argument, so the module provides an easy way to build one from a string.
>  Even though we aren't taking user input right now, we can still build one with
>  a hardcoded string using `parse_list_objects_filter()`.
> @@ -784,7 +772,7 @@ object:
>  ----
>  	...
>  
> -		traverse_commit_list_filtered(&filter_options, rev,
> +		traverse_commit_list_filtered(rev,
>  			walken_show_commit, walken_show_object, NULL, &omitted);
>  
>  	...

Derrick Stolee March 7, 2022, 2:05 p.m. UTC | #2

On 3/4/2022 5:33 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <[e-mail]> writes:

>>  the name, these functions traverse all kinds of objects. Let's have a look at
>> -the arguments to `traverse_commit_list_filtered()`, which are a superset of the
>> -arguments to the unfiltered version.
>> +the arguments to `traverse_commit_list()`.
>>  
>> -- `struct list_objects_filter_options *filter_options`: This is a struct which
>> -  stores a filter-spec as outlined in `Documentation/rev-list-options.txt`.
>> -- `struct rev_info *revs`: This is the `rev_info` used for the walk.
>> +- `struct rev_info *revs`: This is the `rev_info` used for the walk. It
>> +  includes a `filter` member which contains information for how to filter
>> +  the object list.
> 
> Perhaps,
> 
>     "When its `filter` member is not NULL, it contains ..."
> 
> implying that it is valid for `filter` member to be NULL and none of
> the following things will happen in such a case.

Definitely room for improvement here. I got hung up on the two uses of
"it" so here is another attempt:

 If its `filter` member is not `NULL`, then `filter` contains
 information for how to filter the object list.

What do you think?

Thanks,
-Stolee

Junio C Hamano March 7, 2022, 4:47 p.m. UTC | #3

Derrick Stolee <[e-mail]> writes:

> Definitely room for improvement here. I got hung up on the two uses of
> "it" so here is another attempt:
>
>  If its `filter` member is not `NULL`, then `filter` contains
>  information for how to filter the object list.
>
> What do you think?

Sure.  I won't get hung up on three uses of filter here ;-)

The above reads well.

Thanks.

Patch

diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
index ca267941f3e..8ec83185b8a 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -522,24 +522,25 @@  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
 the name, these functions traverse all kinds of objects. Let's have a look at
-the arguments to `traverse_commit_list_filtered()`, which are a superset of the
-arguments to the unfiltered version.
+the arguments to `traverse_commit_list()`.
 
-- `struct list_objects_filter_options *filter_options`: This is a struct which
-  stores a filter-spec as outlined in `Documentation/rev-list-options.txt`.
-- `struct rev_info *revs`: This is the `rev_info` used for the walk.
+- `struct rev_info *revs`: This is the `rev_info` used for the walk. It
+  includes a `filter` member which contains information for how to filter
+  the object list.
 - `show_commit_fn show_commit`: A callback which will be used to handle each
   individual commit object.
 - `show_object_fn show_object`: A callback which will be used to handle each
   non-commit object (so each blob, tree, or tag).
 - `void *show_data`: A context buffer which is passed in turn to `show_commit`
   and `show_object`.
+
+In addition, `traverse_commit_list_filtered()` has an additional paramter:
+
 - `struct oidset *omitted`: A linked-list of object IDs which the provided
   filter caused to be omitted.
 
-It looks like this `traverse_commit_list_filtered()` uses callbacks we provide
-instead of needing us to call it repeatedly ourselves. Cool! Let's add the
-callbacks first.
+It looks like these methods use callbacks we provide instead of needing us
+to call it repeatedly ourselves. Cool! Let's add the callbacks first.
 
 For the sake of this tutorial, we'll simply keep track of how many of each kind
 of object we find. At file scope in `builtin/walken.c` add the following
@@ -712,20 +713,9 @@  help understand. In our case, that means we omit trees and blobs not directly
 referenced by `HEAD` or `HEAD`'s history, because we begin the walk with only
 `HEAD` in the `pending` list.)
 
-First, we'll need to `#include "list-objects-filter-options.h"` and set up the
-`struct list_objects_filter_options` at the top of the function.
-
-----
-static void walken_object_walk(struct rev_info *rev)
-{
-	struct list_objects_filter_options filter_options = { 0 };
-
-	...
-----
-
 For now, we are not going to track the omitted objects, so we'll replace those
 parameters with `NULL`. For the sake of simplicity, we'll add a simple
-build-time branch to use our filter or not. Replace the line calling
+build-time branch to use our filter or not. Preface the line calling
 `traverse_commit_list()` with the following, which will remind us which kind of
 walk we've just performed:
 
@@ -733,19 +723,17 @@  walk we've just performed:
 	if (0) {
 		/* Unfiltered: */
 		trace_printf(_("Unfiltered object walk.\n"));
-		traverse_commit_list(rev, walken_show_commit,
-				walken_show_object, NULL);
 	} else {
 		trace_printf(
 			_("Filtered object walk with filterspec 'tree:1'.\n"));
-		parse_list_objects_filter(&filter_options, "tree:1");
-
-		traverse_commit_list_filtered(&filter_options, rev,
-			walken_show_commit, walken_show_object, NULL, NULL);
+		CALLOC_ARRAY(rev->filter, 1);
+		parse_list_objects_filter(rev->filter, "tree:1");
 	}
+	traverse_commit_list(rev, walken_show_commit,
+			     walken_show_object, NULL);
 ----
 
-`struct list_objects_filter_options` is usually built directly from a command
+The `rev->filter` member is usually built directly from a command
 line argument, so the module provides an easy way to build one from a string.
 Even though we aren't taking user input right now, we can still build one with
 a hardcoded string using `parse_list_objects_filter()`.
@@ -784,7 +772,7 @@  object:
 ----
 	...
 
-		traverse_commit_list_filtered(&filter_options, rev,
+		traverse_commit_list_filtered(rev,
 			walken_show_commit, walken_show_object, NULL, &omitted);
 
 	...