* [PATCH v3 1/5] MyFirstObjectWalk: use additional arg in config_fn_t
2024-03-25 12:33 ` [PATCH v3 " Dirk Gouders
@ 2024-03-25 12:33 ` Dirk Gouders
2024-03-25 17:16 ` Junio C Hamano
2024-03-25 12:33 ` [PATCH v3 2/5] MyFirstObjectWalk: fix misspelled "builtins/" Dirk Gouders
` (11 subsequent siblings)
12 siblings, 1 reply; 60+ messages in thread
From: Dirk Gouders @ 2024-03-25 12:33 UTC (permalink / raw)
To: git; +Cc: Dirk Gouders, Junio C Hamano, Emily Shaffer, Kyle Lippincott
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
^ permalink raw reply related [flat|nested] 60+ messages in thread* Re: [PATCH v3 1/5] MyFirstObjectWalk: use additional arg in config_fn_t
2024-03-25 12:33 ` [PATCH v3 1/5] MyFirstObjectWalk: use additional arg in config_fn_t Dirk Gouders
@ 2024-03-25 17:16 ` Junio C Hamano
2024-03-25 19:50 ` Dirk Gouders
0 siblings, 1 reply; 60+ messages in thread
From: Junio C Hamano @ 2024-03-25 17:16 UTC (permalink / raw)
To: Dirk Gouders; +Cc: git, Emily Shaffer, Kyle Lippincott
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);
> }
> ----
^ permalink raw reply [flat|nested] 60+ messages in thread* Re: [PATCH v3 1/5] MyFirstObjectWalk: use additional arg in config_fn_t
2024-03-25 17:16 ` Junio C Hamano
@ 2024-03-25 19:50 ` Dirk Gouders
0 siblings, 0 replies; 60+ messages in thread
From: Dirk Gouders @ 2024-03-25 19:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Emily Shaffer, Kyle Lippincott
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
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v3 2/5] MyFirstObjectWalk: fix misspelled "builtins/"
2024-03-25 12:33 ` [PATCH v3 " Dirk Gouders
2024-03-25 12:33 ` [PATCH v3 1/5] MyFirstObjectWalk: use additional arg in config_fn_t Dirk Gouders
@ 2024-03-25 12:33 ` Dirk Gouders
2024-03-25 12:33 ` [PATCH v3 3/5] MyFirstObjectWalk: fix filtered object walk Dirk Gouders
` (10 subsequent siblings)
12 siblings, 0 replies; 60+ messages in thread
From: Dirk Gouders @ 2024-03-25 12:33 UTC (permalink / raw)
To: git; +Cc: Dirk Gouders, Junio C Hamano, Emily Shaffer, Kyle Lippincott
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
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v3 3/5] MyFirstObjectWalk: fix filtered object walk
2024-03-25 12:33 ` [PATCH v3 " Dirk Gouders
2024-03-25 12:33 ` [PATCH v3 1/5] MyFirstObjectWalk: use additional arg in config_fn_t Dirk Gouders
2024-03-25 12:33 ` [PATCH v3 2/5] MyFirstObjectWalk: fix misspelled "builtins/" Dirk Gouders
@ 2024-03-25 12:33 ` Dirk Gouders
2024-03-25 12:33 ` [PATCH v3 4/5] MyFirstObjectWalk: fix description for counting omitted objects Dirk Gouders
` (9 subsequent siblings)
12 siblings, 0 replies; 60+ messages in thread
From: Dirk Gouders @ 2024-03-25 12:33 UTC (permalink / raw)
To: git; +Cc: Dirk Gouders, Junio C Hamano, Emily Shaffer, Kyle Lippincott
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
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v3 4/5] MyFirstObjectWalk: fix description for counting omitted objects
2024-03-25 12:33 ` [PATCH v3 " Dirk Gouders
` (2 preceding siblings ...)
2024-03-25 12:33 ` [PATCH v3 3/5] MyFirstObjectWalk: fix filtered object walk Dirk Gouders
@ 2024-03-25 12:33 ` Dirk Gouders
2024-03-25 17:25 ` Junio C Hamano
2024-03-25 12:33 ` [PATCH v3 5/5] MyFirstObjectWalk: add stderr to pipe processing Dirk Gouders
` (8 subsequent siblings)
12 siblings, 1 reply; 60+ messages in thread
From: Dirk Gouders @ 2024-03-25 12:33 UTC (permalink / raw)
To: git; +Cc: Dirk Gouders, Junio C Hamano, Emily Shaffer, Kyle Lippincott
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
^ permalink raw reply related [flat|nested] 60+ messages in thread* Re: [PATCH v3 4/5] MyFirstObjectWalk: fix description for counting omitted objects
2024-03-25 12:33 ` [PATCH v3 4/5] MyFirstObjectWalk: fix description for counting omitted objects Dirk Gouders
@ 2024-03-25 17:25 ` Junio C Hamano
2024-03-25 20:07 ` Dirk Gouders
2024-03-25 20:59 ` Kyle Lippincott
0 siblings, 2 replies; 60+ messages in thread
From: Junio C Hamano @ 2024-03-25 17:25 UTC (permalink / raw)
To: Dirk Gouders; +Cc: git, Emily Shaffer, Kyle Lippincott
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:
>
> ----
> ...
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v3 4/5] MyFirstObjectWalk: fix description for counting omitted objects
2024-03-25 17:25 ` Junio C Hamano
@ 2024-03-25 20:07 ` Dirk Gouders
2024-03-25 21:25 ` Junio C Hamano
2024-03-25 20:59 ` Kyle Lippincott
1 sibling, 1 reply; 60+ messages in thread
From: Dirk Gouders @ 2024-03-25 20:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Emily Shaffer, Kyle Lippincott
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
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v3 4/5] MyFirstObjectWalk: fix description for counting omitted objects
2024-03-25 20:07 ` Dirk Gouders
@ 2024-03-25 21:25 ` Junio C Hamano
0 siblings, 0 replies; 60+ messages in thread
From: Junio C Hamano @ 2024-03-25 21:25 UTC (permalink / raw)
To: Dirk Gouders; +Cc: git, Emily Shaffer, Kyle Lippincott
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.
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v3 4/5] MyFirstObjectWalk: fix description for counting omitted objects
2024-03-25 17:25 ` Junio C Hamano
2024-03-25 20:07 ` Dirk Gouders
@ 2024-03-25 20:59 ` Kyle Lippincott
1 sibling, 0 replies; 60+ messages in thread
From: Kyle Lippincott @ 2024-03-25 20:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Dirk Gouders, git, Emily Shaffer
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:
> >
> > ----
> > ...
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v3 5/5] MyFirstObjectWalk: add stderr to pipe processing
2024-03-25 12:33 ` [PATCH v3 " Dirk Gouders
` (3 preceding siblings ...)
2024-03-25 12:33 ` [PATCH v3 4/5] MyFirstObjectWalk: fix description for counting omitted objects Dirk Gouders
@ 2024-03-25 12:33 ` Dirk Gouders
2024-03-25 17:05 ` [PATCH v3 0/5] Fixes for Documentation/MyFirstObjectWalk.txt Kyle Lippincott
` (7 subsequent siblings)
12 siblings, 0 replies; 60+ messages in thread
From: Dirk Gouders @ 2024-03-25 12:33 UTC (permalink / raw)
To: git; +Cc: Dirk Gouders, Junio C Hamano, Emily Shaffer, Kyle Lippincott
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
^ permalink raw reply related [flat|nested] 60+ messages in thread* Re: [PATCH v3 0/5] Fixes for Documentation/MyFirstObjectWalk.txt
2024-03-25 12:33 ` [PATCH v3 " Dirk Gouders
` (4 preceding siblings ...)
2024-03-25 12:33 ` [PATCH v3 5/5] MyFirstObjectWalk: add stderr to pipe processing Dirk Gouders
@ 2024-03-25 17:05 ` Kyle Lippincott
2024-03-25 20:07 ` Dirk Gouders
2024-03-25 17:50 ` Junio C Hamano
` (6 subsequent siblings)
12 siblings, 1 reply; 60+ messages in thread
From: Kyle Lippincott @ 2024-03-25 17:05 UTC (permalink / raw)
To: Dirk Gouders; +Cc: git, Junio C Hamano, Emily Shaffer
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!
^ permalink raw reply [flat|nested] 60+ messages in thread* Re: [PATCH v3 0/5] Fixes for Documentation/MyFirstObjectWalk.txt
2024-03-25 17:05 ` [PATCH v3 0/5] Fixes for Documentation/MyFirstObjectWalk.txt Kyle Lippincott
@ 2024-03-25 20:07 ` Dirk Gouders
0 siblings, 0 replies; 60+ messages in thread
From: Dirk Gouders @ 2024-03-25 20:07 UTC (permalink / raw)
To: Kyle Lippincott; +Cc: git, Junio C Hamano, Emily Shaffer
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
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v3 0/5] Fixes for Documentation/MyFirstObjectWalk.txt
2024-03-25 12:33 ` [PATCH v3 " Dirk Gouders
` (5 preceding siblings ...)
2024-03-25 17:05 ` [PATCH v3 0/5] Fixes for Documentation/MyFirstObjectWalk.txt Kyle Lippincott
@ 2024-03-25 17:50 ` Junio C Hamano
2024-03-25 18:01 ` Kyle Lippincott
2024-03-25 20:22 ` Dirk Gouders
2024-03-26 13:08 ` [PATCH v4 " Dirk Gouders
` (5 subsequent siblings)
12 siblings, 2 replies; 60+ messages in thread
From: Junio C Hamano @ 2024-03-25 17:50 UTC (permalink / raw)
To: Dirk Gouders; +Cc: git, Emily Shaffer, Kyle Lippincott
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.
^ permalink raw reply [flat|nested] 60+ messages in thread* Re: [PATCH v3 0/5] Fixes for Documentation/MyFirstObjectWalk.txt
2024-03-25 17:50 ` Junio C Hamano
@ 2024-03-25 18:01 ` Kyle Lippincott
2024-03-25 20:22 ` Dirk Gouders
1 sibling, 0 replies; 60+ messages in thread
From: Kyle Lippincott @ 2024-03-25 18:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Dirk Gouders, git, Emily Shaffer
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.
>
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v3 0/5] Fixes for Documentation/MyFirstObjectWalk.txt
2024-03-25 17:50 ` Junio C Hamano
2024-03-25 18:01 ` Kyle Lippincott
@ 2024-03-25 20:22 ` Dirk Gouders
1 sibling, 0 replies; 60+ messages in thread
From: Dirk Gouders @ 2024-03-25 20:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Emily Shaffer, Kyle Lippincott
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
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 0/5] Fixes for Documentation/MyFirstObjectWalk.txt
2024-03-25 12:33 ` [PATCH v3 " Dirk Gouders
` (6 preceding siblings ...)
2024-03-25 17:50 ` Junio C Hamano
@ 2024-03-26 13:08 ` Dirk Gouders
2024-03-27 1:04 ` Kyle Lippincott
2024-03-27 11:22 ` [PATCH v5 " Dirk Gouders
2024-03-26 13:08 ` [PATCH v4 1/5] MyFirstObjectWalk: use additional arg in config_fn_t Dirk Gouders
` (4 subsequent siblings)
12 siblings, 2 replies; 60+ messages in thread
From: Dirk Gouders @ 2024-03-26 13:08 UTC (permalink / raw)
To: git; +Cc: Dirk Gouders, Junio C Hamano, Emily Shaffer, Kyle Lippincott
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
^ permalink raw reply [flat|nested] 60+ messages in thread* Re: [PATCH v4 0/5] Fixes for Documentation/MyFirstObjectWalk.txt
2024-03-26 13:08 ` [PATCH v4 " Dirk Gouders
@ 2024-03-27 1:04 ` Kyle Lippincott
2024-03-27 6:25 ` Dirk Gouders
2024-03-27 11:22 ` [PATCH v5 " Dirk Gouders
1 sibling, 1 reply; 60+ messages in thread
From: Kyle Lippincott @ 2024-03-27 1:04 UTC (permalink / raw)
To: Dirk Gouders; +Cc: git, Junio C Hamano, Emily Shaffer
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
>
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 0/5] Fixes for Documentation/MyFirstObjectWalk.txt
2024-03-27 1:04 ` Kyle Lippincott
@ 2024-03-27 6:25 ` Dirk Gouders
0 siblings, 0 replies; 60+ messages in thread
From: Dirk Gouders @ 2024-03-27 6:25 UTC (permalink / raw)
To: Kyle Lippincott; +Cc: git, Junio C Hamano, Emily Shaffer
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.
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v5 0/5] Fixes for Documentation/MyFirstObjectWalk.txt
2024-03-26 13:08 ` [PATCH v4 " Dirk Gouders
2024-03-27 1:04 ` Kyle Lippincott
@ 2024-03-27 11:22 ` Dirk Gouders
2024-03-27 11:22 ` [PATCH v5 1/5] MyFirstObjectWalk: use additional arg in config_fn_t Dirk Gouders
` (4 more replies)
1 sibling, 5 replies; 60+ messages in thread
From: Dirk Gouders @ 2024-03-27 11:22 UTC (permalink / raw)
To: git; +Cc: Dirk Gouders, Junio C Hamano, Emily Shaffer, Kyle Lippincott
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
^ permalink raw reply [flat|nested] 60+ messages in thread* [PATCH v5 1/5] MyFirstObjectWalk: use additional arg in config_fn_t
2024-03-27 11:22 ` [PATCH v5 " Dirk Gouders
@ 2024-03-27 11:22 ` Dirk Gouders
2024-03-27 11:22 ` [PATCH v5 2/5] MyFirstObjectWalk: fix misspelled "builtins/" Dirk Gouders
` (3 subsequent siblings)
4 siblings, 0 replies; 60+ messages in thread
From: Dirk Gouders @ 2024-03-27 11:22 UTC (permalink / raw)
To: git; +Cc: Dirk Gouders, Junio C Hamano, Emily Shaffer, Kyle Lippincott
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
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v5 2/5] MyFirstObjectWalk: fix misspelled "builtins/"
2024-03-27 11:22 ` [PATCH v5 " Dirk Gouders
2024-03-27 11:22 ` [PATCH v5 1/5] MyFirstObjectWalk: use additional arg in config_fn_t Dirk Gouders
@ 2024-03-27 11:22 ` Dirk Gouders
2024-03-27 11:22 ` [PATCH v5 3/5] MyFirstObjectWalk: fix filtered object walk Dirk Gouders
` (2 subsequent siblings)
4 siblings, 0 replies; 60+ messages in thread
From: Dirk Gouders @ 2024-03-27 11:22 UTC (permalink / raw)
To: git; +Cc: Dirk Gouders, Junio C Hamano, Emily Shaffer, Kyle Lippincott
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
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v5 3/5] MyFirstObjectWalk: fix filtered object walk
2024-03-27 11:22 ` [PATCH v5 " Dirk Gouders
2024-03-27 11:22 ` [PATCH v5 1/5] MyFirstObjectWalk: use additional arg in config_fn_t Dirk Gouders
2024-03-27 11:22 ` [PATCH v5 2/5] MyFirstObjectWalk: fix misspelled "builtins/" Dirk Gouders
@ 2024-03-27 11:22 ` Dirk Gouders
2024-03-27 11:22 ` [PATCH v5 4/5] MyFirstObjectWalk: fix description for counting omitted objects Dirk Gouders
2024-03-27 11:22 ` [PATCH v5 5/5] MyFirstObjectWalk: add stderr to pipe processing Dirk Gouders
4 siblings, 0 replies; 60+ messages in thread
From: Dirk Gouders @ 2024-03-27 11:22 UTC (permalink / raw)
To: git; +Cc: Dirk Gouders, Junio C Hamano, Emily Shaffer, Kyle Lippincott
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
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v5 4/5] MyFirstObjectWalk: fix description for counting omitted objects
2024-03-27 11:22 ` [PATCH v5 " Dirk Gouders
` (2 preceding siblings ...)
2024-03-27 11:22 ` [PATCH v5 3/5] MyFirstObjectWalk: fix filtered object walk Dirk Gouders
@ 2024-03-27 11:22 ` Dirk Gouders
2024-03-27 11:22 ` [PATCH v5 5/5] MyFirstObjectWalk: add stderr to pipe processing Dirk Gouders
4 siblings, 0 replies; 60+ messages in thread
From: Dirk Gouders @ 2024-03-27 11:22 UTC (permalink / raw)
To: git; +Cc: Dirk Gouders, Junio C Hamano, Emily Shaffer, Kyle Lippincott
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
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v5 5/5] MyFirstObjectWalk: add stderr to pipe processing
2024-03-27 11:22 ` [PATCH v5 " Dirk Gouders
` (3 preceding siblings ...)
2024-03-27 11:22 ` [PATCH v5 4/5] MyFirstObjectWalk: fix description for counting omitted objects Dirk Gouders
@ 2024-03-27 11:22 ` Dirk Gouders
4 siblings, 0 replies; 60+ messages in thread
From: Dirk Gouders @ 2024-03-27 11:22 UTC (permalink / raw)
To: git; +Cc: Dirk Gouders, Junio C Hamano, Emily Shaffer, Kyle Lippincott
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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 1/5] MyFirstObjectWalk: use additional arg in config_fn_t
2024-03-25 12:33 ` [PATCH v3 " Dirk Gouders
` (7 preceding siblings ...)
2024-03-26 13:08 ` [PATCH v4 " Dirk Gouders
@ 2024-03-26 13:08 ` Dirk Gouders
2024-03-26 13:08 ` [PATCH v4 2/5] MyFirstObjectWalk: fix misspelled "builtins/" Dirk Gouders
` (3 subsequent siblings)
12 siblings, 0 replies; 60+ messages in thread
From: Dirk Gouders @ 2024-03-26 13:08 UTC (permalink / raw)
To: git; +Cc: Dirk Gouders, Junio C Hamano, Emily Shaffer, Kyle Lippincott
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
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 2/5] MyFirstObjectWalk: fix misspelled "builtins/"
2024-03-25 12:33 ` [PATCH v3 " Dirk Gouders
` (8 preceding siblings ...)
2024-03-26 13:08 ` [PATCH v4 1/5] MyFirstObjectWalk: use additional arg in config_fn_t Dirk Gouders
@ 2024-03-26 13:08 ` Dirk Gouders
2024-03-26 13:08 ` [PATCH v4 3/5] MyFirstObjectWalk: fix filtered object walk Dirk Gouders
` (2 subsequent siblings)
12 siblings, 0 replies; 60+ messages in thread
From: Dirk Gouders @ 2024-03-26 13:08 UTC (permalink / raw)
To: git; +Cc: Dirk Gouders, Junio C Hamano, Emily Shaffer, Kyle Lippincott
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
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 3/5] MyFirstObjectWalk: fix filtered object walk
2024-03-25 12:33 ` [PATCH v3 " Dirk Gouders
` (9 preceding siblings ...)
2024-03-26 13:08 ` [PATCH v4 2/5] MyFirstObjectWalk: fix misspelled "builtins/" Dirk Gouders
@ 2024-03-26 13:08 ` Dirk Gouders
2024-03-26 13:08 ` [PATCH v4 4/5] MyFirstObjectWalk: fix description for counting omitted objects Dirk Gouders
2024-03-26 13:08 ` [PATCH v4 5/5] MyFirstObjectWalk: add stderr to pipe processing Dirk Gouders
12 siblings, 0 replies; 60+ messages in thread
From: Dirk Gouders @ 2024-03-26 13:08 UTC (permalink / raw)
To: git; +Cc: Dirk Gouders, Junio C Hamano, Emily Shaffer, Kyle Lippincott
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
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 4/5] MyFirstObjectWalk: fix description for counting omitted objects
2024-03-25 12:33 ` [PATCH v3 " Dirk Gouders
` (10 preceding siblings ...)
2024-03-26 13:08 ` [PATCH v4 3/5] MyFirstObjectWalk: fix filtered object walk Dirk Gouders
@ 2024-03-26 13:08 ` Dirk Gouders
2024-03-26 17:00 ` Junio C Hamano
2024-03-26 13:08 ` [PATCH v4 5/5] MyFirstObjectWalk: add stderr to pipe processing Dirk Gouders
12 siblings, 1 reply; 60+ messages in thread
From: Dirk Gouders @ 2024-03-26 13:08 UTC (permalink / raw)
To: git; +Cc: Dirk Gouders, Junio C Hamano, Emily Shaffer, Kyle Lippincott
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
^ permalink raw reply related [flat|nested] 60+ messages in thread* Re: [PATCH v4 4/5] MyFirstObjectWalk: fix description for counting omitted objects
2024-03-26 13:08 ` [PATCH v4 4/5] MyFirstObjectWalk: fix description for counting omitted objects Dirk Gouders
@ 2024-03-26 17:00 ` Junio C Hamano
2024-03-26 20:09 ` Dirk Gouders
0 siblings, 1 reply; 60+ messages in thread
From: Junio C Hamano @ 2024-03-26 17:00 UTC (permalink / raw)
To: Dirk Gouders; +Cc: git, Emily Shaffer, Kyle Lippincott
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.
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 4/5] MyFirstObjectWalk: fix description for counting omitted objects
2024-03-26 17:00 ` Junio C Hamano
@ 2024-03-26 20:09 ` Dirk Gouders
2024-03-26 20:24 ` Junio C Hamano
0 siblings, 1 reply; 60+ messages in thread
From: Dirk Gouders @ 2024-03-26 20:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Emily Shaffer, Kyle Lippincott
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.
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 4/5] MyFirstObjectWalk: fix description for counting omitted objects
2024-03-26 20:09 ` Dirk Gouders
@ 2024-03-26 20:24 ` Junio C Hamano
2024-03-27 6:30 ` Dirk Gouders
0 siblings, 1 reply; 60+ messages in thread
From: Junio C Hamano @ 2024-03-26 20:24 UTC (permalink / raw)
To: Dirk Gouders; +Cc: git, Emily Shaffer, Kyle Lippincott
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.
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 4/5] MyFirstObjectWalk: fix description for counting omitted objects
2024-03-26 20:24 ` Junio C Hamano
@ 2024-03-27 6:30 ` Dirk Gouders
0 siblings, 0 replies; 60+ messages in thread
From: Dirk Gouders @ 2024-03-27 6:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Emily Shaffer, Kyle Lippincott
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.
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 5/5] MyFirstObjectWalk: add stderr to pipe processing
2024-03-25 12:33 ` [PATCH v3 " Dirk Gouders
` (11 preceding siblings ...)
2024-03-26 13:08 ` [PATCH v4 4/5] MyFirstObjectWalk: fix description for counting omitted objects Dirk Gouders
@ 2024-03-26 13:08 ` Dirk Gouders
12 siblings, 0 replies; 60+ messages in thread
From: Dirk Gouders @ 2024-03-26 13:08 UTC (permalink / raw)
To: git; +Cc: Dirk Gouders, Junio C Hamano, Emily Shaffer, Kyle Lippincott
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
^ permalink raw reply related [flat|nested] 60+ messages in thread