* [PATCH v3 1/5] bundle API: start writing API documentation
2021-08-26 14:05 ` [PATCH v3 0/5] " Ævar Arnfjörð Bjarmason
@ 2021-08-26 14:05 ` Ævar Arnfjörð Bjarmason
2021-08-26 14:05 ` [PATCH v3 2/5] strvec: add a strvec_pushvec() Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
5 siblings, 0 replies; 42+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-08-26 14:05 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Emily Shaffer, Derrick Stolee,
Ævar Arnfjörð Bjarmason
There are no other API docs in bundle.h, but this is at least a
start. We'll add a parameter to this function in a subsequent commit,
but let's start by documenting it.
The "/**" comment (as opposed to "/*") signifies the start of API
documentation. See [1] and bdfdaa4978d (strbuf.h: integrate
api-strbuf.txt documentation, 2015-01-16) and 6afbbdda333 (strbuf.h:
unify documentation comments beginnings, 2015-01-16) for a discussion
of that convention.
1. https://lore.kernel.org/git/874kbeecfu.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
bundle.h | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/bundle.h b/bundle.h
index 1927d8cd6a4..84a6df1b65d 100644
--- a/bundle.h
+++ b/bundle.h
@@ -27,6 +27,13 @@ int create_bundle(struct repository *r, const char *path,
int version);
int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);
#define BUNDLE_VERBOSE 1
+
+/**
+ * Unbundle after reading the header with read_bundle_header().
+ *
+ * We'll invoke "git index-pack --stdin --fix-thin" for you on the
+ * provided `bundle_fd` from read_bundle_header().
+ */
int unbundle(struct repository *r, struct bundle_header *header,
int bundle_fd, int flags);
int list_bundle_refs(struct bundle_header *header,
--
2.33.0.733.ga72a4f1c2e1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v3 2/5] strvec: add a strvec_pushvec()
2021-08-26 14:05 ` [PATCH v3 0/5] " Ævar Arnfjörð Bjarmason
2021-08-26 14:05 ` [PATCH v3 1/5] bundle API: start writing API documentation Ævar Arnfjörð Bjarmason
@ 2021-08-26 14:05 ` Ævar Arnfjörð Bjarmason
2021-08-28 1:23 ` Junio C Hamano
2021-08-26 14:05 ` [PATCH v3 3/5] bundle API: change "flags" to be "extra_index_pack_args" Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
5 siblings, 1 reply; 42+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-08-26 14:05 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Emily Shaffer, Derrick Stolee,
Ævar Arnfjörð Bjarmason
Add a strvec_pushvec() function to concatenate two "struct strvec *"
together, and modify code added in 50d89ad6542 (submodule: use
argv_array instead of hand-building arrays, 2012-09-01) to use it. In
a subsequent commit we'll gain another API user.
This could also have been named strvec_concat()[1], but I opted to
make its name consistent with the strbuf_addbuf() function instead. We
only name these sorts of functions *_concat() in one instance:
parse_options_concat().
1. http://lore.kernel.org/git/30620e13-4509-1905-7644-9962b6adf9c5@gmail.com
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
strvec.c | 8 ++++++++
strvec.h | 7 +++++++
submodule.c | 4 +---
3 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/strvec.c b/strvec.c
index 61a76ce6cb9..54ed8427c13 100644
--- a/strvec.c
+++ b/strvec.c
@@ -56,6 +56,14 @@ void strvec_pushv(struct strvec *array, const char **items)
strvec_push(array, *items);
}
+void strvec_pushvec(struct strvec *array, const struct strvec *items)
+{
+ int i;
+
+ for (i = 0; i < items->nr; i++)
+ strvec_push(array, items->v[i]);
+}
+
void strvec_pop(struct strvec *array)
{
if (!array->nr)
diff --git a/strvec.h b/strvec.h
index fdcad75b45b..9003bde2b7d 100644
--- a/strvec.h
+++ b/strvec.h
@@ -62,6 +62,13 @@ void strvec_pushl(struct strvec *, ...);
/* Push a null-terminated array of strings onto the end of the array. */
void strvec_pushv(struct strvec *, const char **);
+/**
+ * Push the contents of another "struct strvec *" onto the end of the
+ * array. Like strvec_pushv(), this is a convenience wrapper that
+ * calls strvec_push() in a loop.
+ */
+void strvec_pushvec(struct strvec *, const struct strvec *);
+
/**
* Remove the final element from the array. If there are no
* elements in the array, do nothing.
diff --git a/submodule.c b/submodule.c
index 8e611fe1dbf..84b5f5dc0e0 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1606,7 +1606,6 @@ int fetch_populated_submodules(struct repository *r,
int default_option,
int quiet, int max_parallel_jobs)
{
- int i;
struct submodule_parallel_fetch spf = SPF_INIT;
spf.r = r;
@@ -1622,8 +1621,7 @@ int fetch_populated_submodules(struct repository *r,
die(_("index file corrupt"));
strvec_push(&spf.args, "fetch");
- for (i = 0; i < options->nr; i++)
- strvec_push(&spf.args, options->v[i]);
+ strvec_pushvec(&spf.args, options);
strvec_push(&spf.args, "--recurse-submodules-default");
/* default value, "--submodule-prefix" and its value are added later */
--
2.33.0.733.ga72a4f1c2e1
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v3 2/5] strvec: add a strvec_pushvec()
2021-08-26 14:05 ` [PATCH v3 2/5] strvec: add a strvec_pushvec() Ævar Arnfjörð Bjarmason
@ 2021-08-28 1:23 ` Junio C Hamano
2021-08-28 1:29 ` Junio C Hamano
0 siblings, 1 reply; 42+ messages in thread
From: Junio C Hamano @ 2021-08-28 1:23 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Emily Shaffer, Derrick Stolee
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Add a strvec_pushvec() function to concatenate two "struct strvec *"
> together, and modify code added in 50d89ad6542 (submodule: use
> argv_array instead of hand-building arrays, 2012-09-01) to use it. In
> a subsequent commit we'll gain another API user.
>
> This could also have been named strvec_concat()[1], but I opted to
> make its name consistent with the strbuf_addbuf() function instead. We
> only name these sorts of functions *_concat() in one instance:
> parse_options_concat().
FWIW pushvec() is a much better name than concat() for this.
concat(A, B) could be an operation that is non-destructive for both
A and B that returns a new vector C. I would imagine that it would
be how, if not majority of, but at least a nontrivial percentage of,
readers expect a function called concat() to behave. A better name
for destructive version would probably be append(), if there is no
other constraints on naming.
The name pushvec(A, B) makes it clear, thanks to similarity with
other push*(A, ...) variants, that A is mutated using the other
arguments. The name is much more clear with less chance of
misunderstanding.
> +void strvec_pushvec(struct strvec *array, const struct strvec *items)
> +{
> + int i;
> +
> + for (i = 0; i < items->nr; i++)
> + strvec_push(array, items->v[i]);
> +}
This implementation is not wrong per-se, but is somewhat
disappointing. When items->nr is large, especially relative to the
original array->alloc, it would incur unnecessary reallocations that
we can easily avoid by pre-sizing the array before pushing the
elements of items from it.
In the original code that became the first user of this helper, it
may not have made much difference, but now it is becoming a more
generally reusable API function, we should care.
Other than that, looks quite straight-forward.
Thanks.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v3 2/5] strvec: add a strvec_pushvec()
2021-08-28 1:23 ` Junio C Hamano
@ 2021-08-28 1:29 ` Junio C Hamano
2021-08-28 4:12 ` Jeff King
0 siblings, 1 reply; 42+ messages in thread
From: Junio C Hamano @ 2021-08-28 1:29 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Emily Shaffer, Derrick Stolee
Junio C Hamano <gitster@pobox.com> writes:
>> +void strvec_pushvec(struct strvec *array, const struct strvec *items)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < items->nr; i++)
>> + strvec_push(array, items->v[i]);
>> +}
>
> This implementation is not wrong per-se, but is somewhat
> disappointing. When items->nr is large, especially relative to the
> original array->alloc, it would incur unnecessary reallocations that
> we can easily avoid by pre-sizing the array before pushing the
> elements of items from it.
>
> In the original code that became the first user of this helper, it
> may not have made much difference, but now it is becoming a more
> generally reusable API function, we should care.
And if we do not care, you can rewrite the code that became the
first user of this helper to instead call strvec_pushv() on the
items->v array that is guaranteed to be NULL terminated, without
inventing this new helper.
I think I am fine with either way.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v3 2/5] strvec: add a strvec_pushvec()
2021-08-28 1:29 ` Junio C Hamano
@ 2021-08-28 4:12 ` Jeff King
2021-08-29 23:54 ` Junio C Hamano
2021-09-02 23:19 ` Ævar Arnfjörð Bjarmason
0 siblings, 2 replies; 42+ messages in thread
From: Jeff King @ 2021-08-28 4:12 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ævar Arnfjörð Bjarmason, git, Emily Shaffer,
Derrick Stolee
On Fri, Aug 27, 2021 at 06:29:30PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> >> +void strvec_pushvec(struct strvec *array, const struct strvec *items)
> >> +{
> >> + int i;
> >> +
> >> + for (i = 0; i < items->nr; i++)
> >> + strvec_push(array, items->v[i]);
> >> +}
> >
> > This implementation is not wrong per-se, but is somewhat
> > disappointing. When items->nr is large, especially relative to the
> > original array->alloc, it would incur unnecessary reallocations that
> > we can easily avoid by pre-sizing the array before pushing the
> > elements of items from it.
> >
> > In the original code that became the first user of this helper, it
> > may not have made much difference, but now it is becoming a more
> > generally reusable API function, we should care.
>
> And if we do not care, you can rewrite the code that became the
> first user of this helper to instead call strvec_pushv() on the
> items->v array that is guaranteed to be NULL terminated, without
> inventing this new helper.
I came here to say that. ;)
I do not mind using pushv() directly, or a pushvec() that is a
convenience wrapper for pushv(). Even better if that wrapper is smart
enough to pre-allocate based on items->nr, as you mentioned, but that
can also come later.
One thing that did surprise me: the use of "int" here for iterating,
rather than size_t. But it seems that strvec is already storing ints,
which is an accident!
I think we'd want the patch below. It can be applied independently
(though if we do take the index-iterating version of Ævar's patch, I
think it should switch to size_t).
-- >8 --
Subject: [PATCH] strvec: use size_t to store nr and alloc
We converted argv_array (which later became strvec) to use size_t in
819f0e76b1 (argv-array: use size_t for count and alloc, 2020-07-28) in
order to avoid the possibility of integer overflow. But later, commit
d70a9eb611 (strvec: rename struct fields, 2020-07-28) accidentally
converted these back to ints!
Those two commits were part of the same patch series. I'm pretty sure
what happened is that they were originally written in the opposite order
and then cleaned up and re-ordered during an interactive rebase. And
when resolving the inevitable conflict, I mistakenly took the "rename"
patch completely, accidentally dropping the type change.
We can correct it now; better late than never.
Signed-off-by: Jeff King <peff@peff.net>
---
strvec.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/strvec.h b/strvec.h
index fdcad75b45..6b3cbd6758 100644
--- a/strvec.h
+++ b/strvec.h
@@ -29,8 +29,8 @@ extern const char *empty_strvec[];
*/
struct strvec {
const char **v;
- int nr;
- int alloc;
+ size_t nr;
+ size_t alloc;
};
#define STRVEC_INIT { empty_strvec, 0, 0 }
--
2.33.0.399.g06f2549587
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v3 2/5] strvec: add a strvec_pushvec()
2021-08-28 4:12 ` Jeff King
@ 2021-08-29 23:54 ` Junio C Hamano
2021-08-30 17:30 ` Derrick Stolee
2021-09-02 23:19 ` Ævar Arnfjörð Bjarmason
1 sibling, 1 reply; 42+ messages in thread
From: Junio C Hamano @ 2021-08-29 23:54 UTC (permalink / raw)
To: Jeff King
Cc: Ævar Arnfjörð Bjarmason, git, Emily Shaffer,
Derrick Stolee
Jeff King <peff@peff.net> writes:
> I think we'd want the patch below. It can be applied independently
> (though if we do take the index-iterating version of Ævar's patch, I
> think it should switch to size_t).
Yeah, I do not see a strong need for _pushvec(), especially the
variant that does not preallocate, when we have _pushv(). But the
type fix below does make sense.
> -- >8 --
> Subject: [PATCH] strvec: use size_t to store nr and alloc
>
> We converted argv_array (which later became strvec) to use size_t in
> 819f0e76b1 (argv-array: use size_t for count and alloc, 2020-07-28) in
> order to avoid the possibility of integer overflow. But later, commit
> d70a9eb611 (strvec: rename struct fields, 2020-07-28) accidentally
> converted these back to ints!
>
> Those two commits were part of the same patch series. I'm pretty sure
> what happened is that they were originally written in the opposite order
> and then cleaned up and re-ordered during an interactive rebase. And
> when resolving the inevitable conflict, I mistakenly took the "rename"
> patch completely, accidentally dropping the type change.
>
> We can correct it now; better late than never.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> strvec.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/strvec.h b/strvec.h
> index fdcad75b45..6b3cbd6758 100644
> --- a/strvec.h
> +++ b/strvec.h
> @@ -29,8 +29,8 @@ extern const char *empty_strvec[];
> */
> struct strvec {
> const char **v;
> - int nr;
> - int alloc;
> + size_t nr;
> + size_t alloc;
> };
>
> #define STRVEC_INIT { empty_strvec, 0, 0 }
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v3 2/5] strvec: add a strvec_pushvec()
2021-08-29 23:54 ` Junio C Hamano
@ 2021-08-30 17:30 ` Derrick Stolee
0 siblings, 0 replies; 42+ messages in thread
From: Derrick Stolee @ 2021-08-30 17:30 UTC (permalink / raw)
To: Junio C Hamano, Jeff King
Cc: Ævar Arnfjörð Bjarmason, git, Emily Shaffer
On 8/29/2021 7:54 PM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
>> I think we'd want the patch below. It can be applied independently
>> (though if we do take the index-iterating version of Ævar's patch, I
>> think it should switch to size_t).
>
> Yeah, I do not see a strong need for _pushvec(), especially the
> variant that does not preallocate, when we have _pushv(). But the
> type fix below does make sense.
Thanks for chiming in. I was not aware of _pushv() when I asked
about the _pushvec() variant. Sorry, Ævar, for sending you down
an unnecessary direction.
>> -- >8 --
>> Subject: [PATCH] strvec: use size_t to store nr and alloc
>>
>> We converted argv_array (which later became strvec) to use size_t in
>> 819f0e76b1 (argv-array: use size_t for count and alloc, 2020-07-28) in
>> order to avoid the possibility of integer overflow. But later, commit
>> d70a9eb611 (strvec: rename struct fields, 2020-07-28) accidentally
>> converted these back to ints!
>>
>> Those two commits were part of the same patch series. I'm pretty sure
>> what happened is that they were originally written in the opposite order
>> and then cleaned up and re-ordered during an interactive rebase. And
>> when resolving the inevitable conflict, I mistakenly took the "rename"
>> patch completely, accidentally dropping the type change.
>>
>> We can correct it now; better late than never.
...
>> struct strvec {
>> const char **v;
>> - int nr;
>> - int alloc;
>> + size_t nr;
>> + size_t alloc;
>> };
This is also a good change to take.
Thanks,
-Stolee
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v3 2/5] strvec: add a strvec_pushvec()
2021-08-28 4:12 ` Jeff King
2021-08-29 23:54 ` Junio C Hamano
@ 2021-09-02 23:19 ` Ævar Arnfjörð Bjarmason
2021-09-03 5:05 ` Junio C Hamano
2021-09-03 11:06 ` Jeff King
1 sibling, 2 replies; 42+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-02 23:19 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Emily Shaffer, Derrick Stolee
On Sat, Aug 28 2021, Jeff King wrote:
> On Fri, Aug 27, 2021 at 06:29:30PM -0700, Junio C Hamano wrote:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> >> +void strvec_pushvec(struct strvec *array, const struct strvec *items)
>> >> +{
>> >> + int i;
>> >> +
>> >> + for (i = 0; i < items->nr; i++)
>> >> + strvec_push(array, items->v[i]);
>> >> +}
>> >
>> > This implementation is not wrong per-se, but is somewhat
>> > disappointing. When items->nr is large, especially relative to the
>> > original array->alloc, it would incur unnecessary reallocations that
>> > we can easily avoid by pre-sizing the array before pushing the
>> > elements of items from it.
>> >
>> > In the original code that became the first user of this helper, it
>> > may not have made much difference, but now it is becoming a more
>> > generally reusable API function, we should care.
>>
>> And if we do not care, you can rewrite the code that became the
>> first user of this helper to instead call strvec_pushv() on the
>> items->v array that is guaranteed to be NULL terminated, without
>> inventing this new helper.
>
> I came here to say that. ;)
>
> I do not mind using pushv() directly, or a pushvec() that is a
> convenience wrapper for pushv(). Even better if that wrapper is smart
> enough to pre-allocate based on items->nr, as you mentioned, but that
> can also come later.
>
> One thing that did surprise me: the use of "int" here for iterating,
> rather than size_t. But it seems that strvec is already storing ints,
> which is an accident!
Is it really? If you temporarily try to say convert that to "size_t *nr"
to have the compiler catch all the cases where we use "nr", and then
s/size_t/int/g those all, you'll find that e.g. setup_revisions() and
the like expect to take either "int argc" or the strvec equivalent.
We can sensibly convert some of those to size_t, but not all, and the
int v.s. size_t inconsistency as a result feels weird.
Since the main point of this API is to be a wrapper for what a C main()
would take, shouldn't its prototype mirror its prototype? I.e. we should
stick to "int" here?
> I think we'd want the patch below. It can be applied independently
> (though if we do take the index-iterating version of Ævar's patch, I
> think it should switch to size_t).
>
> -- >8 --
> Subject: [PATCH] strvec: use size_t to store nr and alloc
>
> We converted argv_array (which later became strvec) to use size_t in
> 819f0e76b1 (argv-array: use size_t for count and alloc, 2020-07-28) in
> order to avoid the possibility of integer overflow. But later, commit
> d70a9eb611 (strvec: rename struct fields, 2020-07-28) accidentally
> converted these back to ints!
>
> Those two commits were part of the same patch series. I'm pretty sure
> what happened is that they were originally written in the opposite order
> and then cleaned up and re-ordered during an interactive rebase. And
> when resolving the inevitable conflict, I mistakenly took the "rename"
> patch completely, accidentally dropping the type change.
>
> We can correct it now; better late than never.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> strvec.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/strvec.h b/strvec.h
> index fdcad75b45..6b3cbd6758 100644
> --- a/strvec.h
> +++ b/strvec.h
> @@ -29,8 +29,8 @@ extern const char *empty_strvec[];
> */
> struct strvec {
> const char **v;
> - int nr;
> - int alloc;
> + size_t nr;
> + size_t alloc;
> };
>
> #define STRVEC_INIT { empty_strvec, 0, 0 }
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v3 2/5] strvec: add a strvec_pushvec()
2021-09-02 23:19 ` Ævar Arnfjörð Bjarmason
@ 2021-09-03 5:05 ` Junio C Hamano
2021-09-03 11:06 ` Jeff King
1 sibling, 0 replies; 42+ messages in thread
From: Junio C Hamano @ 2021-09-03 5:05 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Jeff King, git, Emily Shaffer, Derrick Stolee
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Since the main point of this API is to be a wrapper for what a C main()
> would take, shouldn't its prototype mirror its prototype? I.e. we should
> stick to "int" here?
That's a fair-enough argument.
In other words, we do not mind the upper limit of 2 billion strings,
even though we do care that we can have more than 4 billion bytes in
any of these strings ;-)
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v3 2/5] strvec: add a strvec_pushvec()
2021-09-02 23:19 ` Ævar Arnfjörð Bjarmason
2021-09-03 5:05 ` Junio C Hamano
@ 2021-09-03 11:06 ` Jeff King
1 sibling, 0 replies; 42+ messages in thread
From: Jeff King @ 2021-09-03 11:06 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Junio C Hamano, git, Emily Shaffer, Derrick Stolee
On Fri, Sep 03, 2021 at 01:19:33AM +0200, Ævar Arnfjörð Bjarmason wrote:
> > One thing that did surprise me: the use of "int" here for iterating,
> > rather than size_t. But it seems that strvec is already storing ints,
> > which is an accident!
>
> Is it really? If you temporarily try to say convert that to "size_t *nr"
> to have the compiler catch all the cases where we use "nr", and then
> s/size_t/int/g those all, you'll find that e.g. setup_revisions() and
> the like expect to take either "int argc" or the strvec equivalent.
It most definitely is an accident, in the sense that the commit which
changed it did not mean to. :)
> We can sensibly convert some of those to size_t, but not all, and the
> int v.s. size_t inconsistency as a result feels weird.
>
> Since the main point of this API is to be a wrapper for what a C main()
> would take, shouldn't its prototype mirror its prototype? I.e. we should
> stick to "int" here?
That isn't the main point of this API. The reason the name changed away
from argv_array was so that it would be more obviously a generally
useful array of strings.
But even if that were the use, the point isn't that we expect to see 2B
or even 4B strings in any reasonable use case. The point is that we
don't want to accidentally overflow the integer counter, leading to
reads and writes of random bits of memory. And all it takes to do that
is some code like:
while (strbuf_readline(stdin, &buf))
strvec_push(&foo, buf.buf);
On a system with 32-bit size_t, you are not likely to actually allocate
2B strings before you'd fail. But on most 64-bit systems, you have
plenty of address space (and these days, often RAM) to do so, but you
still only need 2B strings, because "int" is 32 bits.
So code like setup_revisions() which uses an int is fine. It is reading
from a source with that much smaller "int" limit in the first place.
Whether strvec can handle bigger arrays or not does not matter either
way. And when it later uses ints to operate on such a strvec, it's OK in
practice; even if the size_t gets truncated on the fly to an int, we
know we did not put more than an int's worth of items into the array in
the first place.
But code which has potentially larger inputs (either because it reads
iteratively into the strvec as above, or because it is itself using a
larger data type) is now also OK, because it's using size_t.
What's not OK is code which operates on a potentially-unbounded strvec
using an int. E.g., following the code I showed above with something
like:
int nr = foo.nr;
foo.v[nr] = xstrdup("replacement string");
which may write to memory outside of foo.v.
Obviously that's nonsense nobody would ever write directly. It is
possible for some code to do this inadvertently (say, passing a ptr/int
pair through a function interface), but I think it's fairly unlikely in
practice. So while I do think more code ought to be using size_t in
general, I don't think it's necessary to audit this carefully. The
important thing to me is protecting strvec itself.
-Peff
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v3 3/5] bundle API: change "flags" to be "extra_index_pack_args"
2021-08-26 14:05 ` [PATCH v3 0/5] " Ævar Arnfjörð Bjarmason
2021-08-26 14:05 ` [PATCH v3 1/5] bundle API: start writing API documentation Ævar Arnfjörð Bjarmason
2021-08-26 14:05 ` [PATCH v3 2/5] strvec: add a strvec_pushvec() Ævar Arnfjörð Bjarmason
@ 2021-08-26 14:05 ` Ævar Arnfjörð Bjarmason
2021-08-28 1:44 ` Junio C Hamano
2021-08-26 14:05 ` [PATCH v3 4/5] index-pack: add --progress-title option Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
5 siblings, 1 reply; 42+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-08-26 14:05 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Emily Shaffer, Derrick Stolee,
Ævar Arnfjörð Bjarmason
Since the "flags" parameter was added in be042aff24c (Teach progress
eye-candy to fetch_refs_from_bundle(), 2011-09-18) there's never been
more than the one flag: BUNDLE_VERBOSE.
Let's have the only caller who cares about that pass "-v" itself
instead through new "extra_index_pack_args" parameter. The flexibility
of being able to pass arbitrary arguments to "unbundle" will be used
in a subsequent commit.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/bundle.c | 2 +-
bundle.c | 14 ++++++++------
bundle.h | 8 ++++++--
transport.c | 6 +++++-
4 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/builtin/bundle.c b/builtin/bundle.c
index 053a51bea1b..f9360c32c6c 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -177,7 +177,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
}
if (!startup_info->have_repository)
die(_("Need a repository to unbundle."));
- ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
+ ret = !!unbundle(the_repository, &header, bundle_fd, NULL) ||
list_bundle_refs(&header, argc, argv);
bundle_header_release(&header);
cleanup:
diff --git a/bundle.c b/bundle.c
index ab63f402261..10d20bb0c48 100644
--- a/bundle.c
+++ b/bundle.c
@@ -569,18 +569,20 @@ int create_bundle(struct repository *r, const char *path,
}
int unbundle(struct repository *r, struct bundle_header *header,
- int bundle_fd, int flags)
+ int bundle_fd, struct strvec *extra_index_pack_args)
{
- const char *argv_index_pack[] = {"index-pack",
- "--fix-thin", "--stdin", NULL, NULL};
struct child_process ip = CHILD_PROCESS_INIT;
- if (flags & BUNDLE_VERBOSE)
- argv_index_pack[3] = "-v";
+ strvec_push(&ip.args, "index-pack");
+ strvec_push(&ip.args, "--fix-thin");
+ strvec_push(&ip.args, "--stdin");
+ if (extra_index_pack_args) {
+ strvec_pushvec(&ip.args, extra_index_pack_args);
+ strvec_clear(extra_index_pack_args);
+ }
if (verify_bundle(r, header, 0))
return -1;
- ip.argv = argv_index_pack;
ip.in = bundle_fd;
ip.no_stdout = 1;
ip.git_cmd = 1;
diff --git a/bundle.h b/bundle.h
index 84a6df1b65d..d47a7a3c69c 100644
--- a/bundle.h
+++ b/bundle.h
@@ -26,16 +26,20 @@ int create_bundle(struct repository *r, const char *path,
int argc, const char **argv, struct strvec *pack_options,
int version);
int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);
-#define BUNDLE_VERBOSE 1
/**
* Unbundle after reading the header with read_bundle_header().
*
* We'll invoke "git index-pack --stdin --fix-thin" for you on the
* provided `bundle_fd` from read_bundle_header().
+ *
+ * Provide extra_index_pack_args to pass any extra arguments
+ * (e.g. "-v" for verbose/progress), NULL otherwise. The provided
+ * extra_index_pack_args (if any) will be strvec_clear()'d for you
+ * (like the run-command.h API itself does).
*/
int unbundle(struct repository *r, struct bundle_header *header,
- int bundle_fd, int flags);
+ int bundle_fd, struct strvec *extra_index_pack_args);
int list_bundle_refs(struct bundle_header *header,
int argc, const char **argv);
diff --git a/transport.c b/transport.c
index 17e9629710a..99b2498e5dd 100644
--- a/transport.c
+++ b/transport.c
@@ -162,12 +162,16 @@ static int fetch_refs_from_bundle(struct transport *transport,
int nr_heads, struct ref **to_fetch)
{
struct bundle_transport_data *data = transport->data;
+ struct strvec extra_index_pack_args = STRVEC_INIT;
int ret;
+ if (transport->progress)
+ strvec_push(&extra_index_pack_args, "-v");
+
if (!data->get_refs_from_bundle_called)
get_refs_from_bundle(transport, 0, NULL);
ret = unbundle(the_repository, &data->header, data->fd,
- transport->progress ? BUNDLE_VERBOSE : 0);
+ transport->progress ? &extra_index_pack_args : NULL);
transport->hash_algo = data->header.hash_algo;
return ret;
}
--
2.33.0.733.ga72a4f1c2e1
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v3 3/5] bundle API: change "flags" to be "extra_index_pack_args"
2021-08-26 14:05 ` [PATCH v3 3/5] bundle API: change "flags" to be "extra_index_pack_args" Ævar Arnfjörð Bjarmason
@ 2021-08-28 1:44 ` Junio C Hamano
0 siblings, 0 replies; 42+ messages in thread
From: Junio C Hamano @ 2021-08-28 1:44 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Emily Shaffer, Derrick Stolee
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Since the "flags" parameter was added in be042aff24c (Teach progress
> eye-candy to fetch_refs_from_bundle(), 2011-09-18) there's never been
> more than the one flag: BUNDLE_VERBOSE.
>
> Let's have the only caller who cares about that pass "-v" itself
> instead through new "extra_index_pack_args" parameter. The flexibility
> of being able to pass arbitrary arguments to "unbundle" will be used
> in a subsequent commit.
As bundle API has been stable over a long period of time, I do not
think the loss of flexibility is too bad (i.e. we no longer can cary
any extra information that cannot be expressed as a textual argument
given to the index-pack process), compared to gained flexibility of
the index-pack command line we can use.
> int unbundle(struct repository *r, struct bundle_header *header,
> - int bundle_fd, int flags)
> + int bundle_fd, struct strvec *extra_index_pack_args)
> {
> - const char *argv_index_pack[] = {"index-pack",
> - "--fix-thin", "--stdin", NULL, NULL};
> struct child_process ip = CHILD_PROCESS_INIT;
>
> - if (flags & BUNDLE_VERBOSE)
> - argv_index_pack[3] = "-v";
> + strvec_push(&ip.args, "index-pack");
> + strvec_push(&ip.args, "--fix-thin");
> + strvec_push(&ip.args, "--stdin");
I would have expected you'd use pushl() here.
> + if (extra_index_pack_args) {
> + strvec_pushvec(&ip.args, extra_index_pack_args);
> + strvec_clear(extra_index_pack_args);
> + }
>
> if (verify_bundle(r, header, 0))
> return -1;
> - ip.argv = argv_index_pack;
> ip.in = bundle_fd;
> ip.no_stdout = 1;
> ip.git_cmd = 1;
> diff --git a/bundle.h b/bundle.h
> index 84a6df1b65d..d47a7a3c69c 100644
> --- a/bundle.h
> +++ b/bundle.h
> @@ -26,16 +26,20 @@ int create_bundle(struct repository *r, const char *path,
> int argc, const char **argv, struct strvec *pack_options,
> int version);
> int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);
> -#define BUNDLE_VERBOSE 1
>
> /**
> * Unbundle after reading the header with read_bundle_header().
> *
> * We'll invoke "git index-pack --stdin --fix-thin" for you on the
> * provided `bundle_fd` from read_bundle_header().
> + *
> + * Provide extra_index_pack_args to pass any extra arguments
> + * (e.g. "-v" for verbose/progress), NULL otherwise. The provided
> + * extra_index_pack_args (if any) will be strvec_clear()'d for you
Good to comment on this. I found it a bit surprising that the in
argument "extra_index_pack_args" is eaten by the consumer, instead
of leaving it up to the caller when to clear (or reuse before clear)
it.
> + * (like the run-command.h API itself does).
This is not even run-command.h, so the mention of "run-command.h API
itself" sounded strange to me. Without "itself", perhaps I would
have slightly less puzzled. IIRC, run_command() only clears upon
success and does not clear if start_command() fails, so it is not a
very good comparison. I think the comment reads better without this
last parenthesized note.
> */
> int unbundle(struct repository *r, struct bundle_header *header,
> - int bundle_fd, int flags);
> + int bundle_fd, struct strvec *extra_index_pack_args);
> int list_bundle_refs(struct bundle_header *header,
> int argc, const char **argv);
>
> diff --git a/transport.c b/transport.c
> index 17e9629710a..99b2498e5dd 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -162,12 +162,16 @@ static int fetch_refs_from_bundle(struct transport *transport,
> int nr_heads, struct ref **to_fetch)
> {
> struct bundle_transport_data *data = transport->data;
> + struct strvec extra_index_pack_args = STRVEC_INIT;
> int ret;
>
> + if (transport->progress)
> + strvec_push(&extra_index_pack_args, "-v");
> +
> if (!data->get_refs_from_bundle_called)
> get_refs_from_bundle(transport, 0, NULL);
> ret = unbundle(the_repository, &data->header, data->fd,
> - transport->progress ? BUNDLE_VERBOSE : 0);
> + transport->progress ? &extra_index_pack_args : NULL);
Why conditional?
If transport->progress is false, extra_index_pack_args is a strvec
that has 0 elements in it, and the strvec_pushvec() code knows how
to handle it correctly and efficiently anyway.
I do not see a reason why we want to make the caller harder to read
with the same conditional twice (i.e. if we pass NULL in non-verbose
case, we can unconditionally put "-v" in the array, and clear the
array on the side of this caller, and that would allow us lose the
clear from the unbundle() function).
Other than that, looks very simple. Nicely done.
Thanks.
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v3 4/5] index-pack: add --progress-title option
2021-08-26 14:05 ` [PATCH v3 0/5] " Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-08-26 14:05 ` [PATCH v3 3/5] bundle API: change "flags" to be "extra_index_pack_args" Ævar Arnfjörð Bjarmason
@ 2021-08-26 14:05 ` Ævar Arnfjörð Bjarmason
2021-08-28 1:50 ` Junio C Hamano
2021-08-26 14:05 ` [PATCH v3 5/5] bundle: show progress on "unbundle" Ævar Arnfjörð Bjarmason
2021-09-05 7:34 ` [PATCH v4 0/4] " Ævar Arnfjörð Bjarmason
5 siblings, 1 reply; 42+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-08-26 14:05 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Emily Shaffer, Derrick Stolee,
Ævar Arnfjörð Bjarmason
Add a --progress-title option to index-pack, when data is piped into
index-pack its progress is a proxy for whatever's feeding it data.
This option will allow us to set a more relevant progress bar title in
"git bundle unbundle", and is also used in my "bundle-uri" RFC
patches[1] by a new caller in fetch-pack.c.
The code change in cmd_index_pack() won't handle
"--progress-title=xyz", only "--progress-title xyz", and the "(i+1)"
style (as opposed to "i + 1") is a bit odd.
Not using the "--long-option=value" style is inconsistent with
existing long options handled by cmd_index_pack(), but makes the code
that needs to call it better (two strvec_push(), instead of needing a
strvec_pushf()).
Since the option is internal-only the inconsistency shouldn't
matter. I'm copying the pattern to handle it as-is from the handling
of the existing "-o" option in the same function, see 9cf6d3357aa (Add
git-index-pack utility, 2005-10-12) for its addition.
Eventually we'd like to migrate all of this this to parse_options(),
which would make these differences in behavior go away.
1. https://lore.kernel.org/git/RFC-cover-00.13-0000000000-20210805T150534Z-avarab@gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Documentation/git-index-pack.txt | 6 ++++++
builtin/index-pack.c | 6 ++++++
2 files changed, 12 insertions(+)
diff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt
index 7fa74b9e798..1f1e3592251 100644
--- a/Documentation/git-index-pack.txt
+++ b/Documentation/git-index-pack.txt
@@ -82,6 +82,12 @@ OPTIONS
--strict::
Die, if the pack contains broken objects or links.
+--progress-title::
+ For internal use only.
++
+Set the title of the progress bar. The title is "Receiving objects" by
+default and "Indexing objects" when `--stdin` is specified.
+
--check-self-contained-and-connected::
Die if the pack contains broken links. For internal use only.
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 8336466865c..0841c039ae2 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -122,6 +122,7 @@ static int strict;
static int do_fsck_object;
static struct fsck_options fsck_options = FSCK_OPTIONS_MISSING_GITMODULES;
static int verbose;
+static const char *progress_title;
static int show_resolving_progress;
static int show_stat;
static int check_self_contained_and_connected;
@@ -1157,6 +1158,7 @@ static void parse_pack_objects(unsigned char *hash)
if (verbose)
progress = start_progress(
+ progress_title ? progress_title :
from_stdin ? _("Receiving objects") : _("Indexing objects"),
nr_objects);
for (i = 0; i < nr_objects; i++) {
@@ -1806,6 +1808,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
input_len = sizeof(*hdr);
} else if (!strcmp(arg, "-v")) {
verbose = 1;
+ } else if (!strcmp(arg, "--progress-title")) {
+ if (progress_title || (i+1) >= argc)
+ usage(index_pack_usage);
+ progress_title = argv[++i];
} else if (!strcmp(arg, "--show-resolving-progress")) {
show_resolving_progress = 1;
} else if (!strcmp(arg, "--report-end-of-input")) {
--
2.33.0.733.ga72a4f1c2e1
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v3 4/5] index-pack: add --progress-title option
2021-08-26 14:05 ` [PATCH v3 4/5] index-pack: add --progress-title option Ævar Arnfjörð Bjarmason
@ 2021-08-28 1:50 ` Junio C Hamano
0 siblings, 0 replies; 42+ messages in thread
From: Junio C Hamano @ 2021-08-28 1:50 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Emily Shaffer, Derrick Stolee
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Since the option is internal-only the inconsistency shouldn't
> matter.
OK.
> I'm copying the pattern to handle it as-is from the handling
> of the existing "-o" option in the same function, see 9cf6d3357aa (Add
> git-index-pack utility, 2005-10-12) for its addition.
That is apples-to-oranges comparison, as the original is not even a
long option so it won't be -o=filename anyway. So the "i+1" (as
opposed to "i + 1") loses justification. Since the option is
implementation detail, having to spell its value separately is OK,
though.
> Eventually we'd like to migrate all of this this to parse_options(),
> which would make these differences in behavior go away.
Perhaps, but if it does not matter now, it shouldn't be worth
code-churn later, either.
Thanks.
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v3 5/5] bundle: show progress on "unbundle"
2021-08-26 14:05 ` [PATCH v3 0/5] " Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2021-08-26 14:05 ` [PATCH v3 4/5] index-pack: add --progress-title option Ævar Arnfjörð Bjarmason
@ 2021-08-26 14:05 ` Ævar Arnfjörð Bjarmason
2021-08-28 1:54 ` Junio C Hamano
2021-09-05 7:34 ` [PATCH v4 0/4] " Ævar Arnfjörð Bjarmason
5 siblings, 1 reply; 42+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-08-26 14:05 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Emily Shaffer, Derrick Stolee,
Ævar Arnfjörð Bjarmason
The "unbundle" command added in 2e0afafebd8 (Add git-bundle: move
objects and references by archive, 2007-02-22) did not show progress
output, even though the underlying API learned how to show progress in
be042aff24c (Teach progress eye-candy to fetch_refs_from_bundle(),
2011-09-18).
Now we'll show "Unbundling objects" using the new --progress-title
option to "git index-pack", to go with its existing "Receiving
objects" and "Indexing objects" (which it shows when invoked with
"--stdin", and with a pack file, respectively).
Unlike "git bundle create" we don't handle "--quiet" here, nor
"--all-progress" and "--all-progress-implied". Those are all specific
to "create" (and "verify", in the case of "--quiet").
The structure of the existing documentation is a bit unclear, e.g. the
documentation for the "--quiet" option added in
79862b6b77c (bundle-create: progress output control, 2019-11-10) only
describes how it works for "create", and not for "verify". That and
other issues in it should be fixed, but I'd like to avoid untangling
that mess right now. Let's just support the standard "--no-progress"
implicitly here, and leave cleaning up the general behavior of "git
bundle" for a later change.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Documentation/git-bundle.txt | 2 +-
builtin/bundle.c | 15 ++++++++++++++-
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt
index ac0d0038350..71b5ecabd1f 100644
--- a/Documentation/git-bundle.txt
+++ b/Documentation/git-bundle.txt
@@ -13,7 +13,7 @@ SYNOPSIS
[--version=<version>] <file> <git-rev-list-args>
'git bundle' verify [-q | --quiet] <file>
'git bundle' list-heads <file> [<refname>...]
-'git bundle' unbundle <file> [<refname>...]
+'git bundle' unbundle [--progress] <file> [<refname>...]
DESCRIPTION
-----------
diff --git a/builtin/bundle.c b/builtin/bundle.c
index f9360c32c6c..1fbfe280c57 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -162,10 +162,15 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
struct bundle_header header = BUNDLE_HEADER_INIT;
int bundle_fd = -1;
int ret;
+ int progress = isatty(2);
+
struct option options[] = {
+ OPT_BOOL(0, "progress", &progress,
+ N_("show progress meter")),
OPT_END()
};
char *bundle_file;
+ struct strvec extra_args = STRVEC_INIT;
argc = parse_options_cmd_bundle(argc, argv, prefix,
builtin_bundle_unbundle_usage, options, &bundle_file);
@@ -177,7 +182,15 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
}
if (!startup_info->have_repository)
die(_("Need a repository to unbundle."));
- ret = !!unbundle(the_repository, &header, bundle_fd, NULL) ||
+
+ if (progress) {
+ strvec_push(&extra_args, "-v");
+ strvec_push(&extra_args, "--progress-title");
+ strvec_push(&extra_args, _("Unbundling objects"));
+ }
+
+ ret = !!unbundle(the_repository, &header, bundle_fd, progress ?
+ &extra_args : NULL) ||
list_bundle_refs(&header, argc, argv);
bundle_header_release(&header);
cleanup:
--
2.33.0.733.ga72a4f1c2e1
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v3 5/5] bundle: show progress on "unbundle"
2021-08-26 14:05 ` [PATCH v3 5/5] bundle: show progress on "unbundle" Ævar Arnfjörð Bjarmason
@ 2021-08-28 1:54 ` Junio C Hamano
2021-09-02 22:47 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 42+ messages in thread
From: Junio C Hamano @ 2021-08-28 1:54 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Emily Shaffer, Derrick Stolee
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> + if (progress) {
> + strvec_push(&extra_args, "-v");
> + strvec_push(&extra_args, "--progress-title");
> + strvec_push(&extra_args, _("Unbundling objects"));
Nice. I would have expected to see pushl() though.
> + }
> +
> + ret = !!unbundle(the_repository, &header, bundle_fd, progress ?
> + &extra_args : NULL) ||
Again, I wouldn't make the &extra_args conditional to progress
here. Future code change may decide to pass more args to underlying
index-pack and the criteria for doing so may be different from
progress.
If this code cares about readability, it should uncondtionally pass
&extra_args.
If this code cares about readability *and* micro-optimization, then
the condition should be on !!extra_args.nr, not on whatever set of
conditions happen to be used in today's code to throw items into
extra_args array.
Other than that, this was a pleasant read.
Thanks.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v3 5/5] bundle: show progress on "unbundle"
2021-08-28 1:54 ` Junio C Hamano
@ 2021-09-02 22:47 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 42+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-02 22:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Emily Shaffer, Derrick Stolee
On Fri, Aug 27 2021, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> + if (progress) {
>> + strvec_push(&extra_args, "-v");
>> + strvec_push(&extra_args, "--progress-title");
>> + strvec_push(&extra_args, _("Unbundling objects"));
>
> Nice. I would have expected to see pushl() though.
Will fix. Thanks!
>> + }
>> +
>> + ret = !!unbundle(the_repository, &header, bundle_fd, progress ?
>> + &extra_args : NULL) ||
>
> Again, I wouldn't make the &extra_args conditional to progress
> here. Future code change may decide to pass more args to underlying
> index-pack and the criteria for doing so may be different from
> progress.
>
> If this code cares about readability, it should uncondtionally pass
> &extra_args.
I agree, but I landed myself in a game of reviewer ping-pong. I had it
that way originally, then Derrick Stolee suggested changing it in this
way in <30620e13-4509-1905-7644-9962b6adf9c5@gmail.com>, I'll just
change it back :)
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v4 0/4] bundle: show progress on "unbundle"
2021-08-26 14:05 ` [PATCH v3 0/5] " Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2021-08-26 14:05 ` [PATCH v3 5/5] bundle: show progress on "unbundle" Ævar Arnfjörð Bjarmason
@ 2021-09-05 7:34 ` Ævar Arnfjörð Bjarmason
2021-09-05 7:34 ` [PATCH v4 1/4] bundle API: start writing API documentation Ævar Arnfjörð Bjarmason
` (3 more replies)
5 siblings, 4 replies; 42+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-05 7:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Emily Shaffer, Derrick Stolee, Jeff King,
Ævar Arnfjörð Bjarmason
This straightforward series addr progress output on "git bundle
unbundle", we already had progress output if bundles were fetched from
via the transport.c (i.e. "git clone/fetch" etc.), but not from "git
bundle unbundle" directly.
In the v3 I added conditionals to pass the &extra_index_pack_args only
when we had something meanignful to pass along, based on Derrick
Stolee's feedback. Based on the feedback to v4 always passing it is
now back, which makes the progression and end-state more readable.
I removed the new strvec_pushvec() function, now I just use
strvec_pushl() or strvec_pushv(), depending. Perhaps a
strvec_pushvec() makes sense, but let's consider that separate from
this series.
There's also comment & commit message changes here in response to
feedback.
Ævar Arnfjörð Bjarmason (4):
bundle API: start writing API documentation
bundle API: change "flags" to be "extra_index_pack_args"
index-pack: add --progress-title option
bundle: show progress on "unbundle"
Documentation/git-bundle.txt | 2 +-
Documentation/git-index-pack.txt | 6 ++++++
builtin/bundle.c | 11 ++++++++++-
builtin/index-pack.c | 6 ++++++
bundle.c | 12 ++++++------
bundle.h | 14 ++++++++++++--
transport.c | 6 +++++-
7 files changed, 46 insertions(+), 11 deletions(-)
Range-diff against v3:
1: 9fb2f7a3a80 = 1: 05be8cb0fc3 bundle API: start writing API documentation
2: 321b8ba3f0e < -: ----------- strvec: add a strvec_pushvec()
3: 637039634e7 ! 2: 9255c766484 bundle API: change "flags" to be "extra_index_pack_args"
@@ Commit message
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## builtin/bundle.c ##
+@@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
+ OPT_END()
+ };
+ char *bundle_file;
++ struct strvec extra_index_pack_args = STRVEC_INIT;
+
+ argc = parse_options_cmd_bundle(argc, argv, prefix,
+ builtin_bundle_unbundle_usage, options, &bundle_file);
@@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
}
if (!startup_info->have_repository)
die(_("Need a repository to unbundle."));
- ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
-+ ret = !!unbundle(the_repository, &header, bundle_fd, NULL) ||
++ ret = !!unbundle(the_repository, &header, bundle_fd,
++ &extra_index_pack_args) ||
list_bundle_refs(&header, argc, argv);
bundle_header_release(&header);
cleanup:
@@ bundle.c: int create_bundle(struct repository *r, const char *path,
- const char *argv_index_pack[] = {"index-pack",
- "--fix-thin", "--stdin", NULL, NULL};
struct child_process ip = CHILD_PROCESS_INIT;
++ strvec_pushl(&ip.args, "index-pack", "--fix-thin", "--stdin", NULL);
- if (flags & BUNDLE_VERBOSE)
- argv_index_pack[3] = "-v";
-+ strvec_push(&ip.args, "index-pack");
-+ strvec_push(&ip.args, "--fix-thin");
-+ strvec_push(&ip.args, "--stdin");
+ if (extra_index_pack_args) {
-+ strvec_pushvec(&ip.args, extra_index_pack_args);
++ strvec_pushv(&ip.args, extra_index_pack_args->v);
+ strvec_clear(extra_index_pack_args);
+ }
@@ bundle.h: int create_bundle(struct repository *r, const char *path,
* We'll invoke "git index-pack --stdin --fix-thin" for you on the
* provided `bundle_fd` from read_bundle_header().
+ *
-+ * Provide extra_index_pack_args to pass any extra arguments
++ * Provide "extra_index_pack_args" to pass any extra arguments
+ * (e.g. "-v" for verbose/progress), NULL otherwise. The provided
-+ * extra_index_pack_args (if any) will be strvec_clear()'d for you
-+ * (like the run-command.h API itself does).
++ * "extra_index_pack_args" (if any) will be strvec_clear()'d for you.
*/
int unbundle(struct repository *r, struct bundle_header *header,
- int bundle_fd, int flags);
@@ transport.c: static int fetch_refs_from_bundle(struct transport *transport,
get_refs_from_bundle(transport, 0, NULL);
ret = unbundle(the_repository, &data->header, data->fd,
- transport->progress ? BUNDLE_VERBOSE : 0);
-+ transport->progress ? &extra_index_pack_args : NULL);
++ &extra_index_pack_args);
transport->hash_algo = data->header.hash_algo;
return ret;
}
4: e44d825e5df ! 3: 338c0e1e518 index-pack: add --progress-title option
@@ Commit message
Not using the "--long-option=value" style is inconsistent with
existing long options handled by cmd_index_pack(), but makes the code
that needs to call it better (two strvec_push(), instead of needing a
- strvec_pushf()).
+ strvec_pushf()). Since the option is internal-only the inconsistency
+ shouldn't matter.
- Since the option is internal-only the inconsistency shouldn't
- matter. I'm copying the pattern to handle it as-is from the handling
- of the existing "-o" option in the same function, see 9cf6d3357aa (Add
- git-index-pack utility, 2005-10-12) for its addition.
-
- Eventually we'd like to migrate all of this this to parse_options(),
- which would make these differences in behavior go away.
+ I'm copying the pattern to handle it as-is from the handling of the
+ existing "-o" option in the same function, see 9cf6d3357aa (Add
+ git-index-pack utility, 2005-10-12) for its addition. That's a short
+ option, but the code to implement the two is the same in functionality
+ and style. Eventually we'd like to migrate all of this this to
+ parse_options(), which would make these differences in behavior go
+ away.
1. https://lore.kernel.org/git/RFC-cover-00.13-0000000000-20210805T150534Z-avarab@gmail.com/
5: cd38b0f0fed ! 4: 8f4c7f99799 bundle: show progress on "unbundle"
@@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, co
OPT_END()
};
char *bundle_file;
-+ struct strvec extra_args = STRVEC_INIT;
-
- argc = parse_options_cmd_bundle(argc, argv, prefix,
- builtin_bundle_unbundle_usage, options, &bundle_file);
@@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
}
if (!startup_info->have_repository)
die(_("Need a repository to unbundle."));
-- ret = !!unbundle(the_repository, &header, bundle_fd, NULL) ||
-+
-+ if (progress) {
-+ strvec_push(&extra_args, "-v");
-+ strvec_push(&extra_args, "--progress-title");
-+ strvec_push(&extra_args, _("Unbundling objects"));
-+ }
-+
-+ ret = !!unbundle(the_repository, &header, bundle_fd, progress ?
-+ &extra_args : NULL) ||
++ if (progress)
++ strvec_pushl(&extra_index_pack_args, "-v", "--progress-title",
++ _("Unbundling objects"), NULL);
+ ret = !!unbundle(the_repository, &header, bundle_fd,
+ &extra_index_pack_args) ||
list_bundle_refs(&header, argc, argv);
- bundle_header_release(&header);
- cleanup:
--
2.33.0.813.g41c39388776
^ permalink raw reply [flat|nested] 42+ messages in thread* [PATCH v4 1/4] bundle API: start writing API documentation
2021-09-05 7:34 ` [PATCH v4 0/4] " Ævar Arnfjörð Bjarmason
@ 2021-09-05 7:34 ` Ævar Arnfjörð Bjarmason
2021-09-05 7:34 ` [PATCH v4 2/4] bundle API: change "flags" to be "extra_index_pack_args" Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
3 siblings, 0 replies; 42+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-05 7:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Emily Shaffer, Derrick Stolee, Jeff King,
Ævar Arnfjörð Bjarmason
There are no other API docs in bundle.h, but this is at least a
start. We'll add a parameter to this function in a subsequent commit,
but let's start by documenting it.
The "/**" comment (as opposed to "/*") signifies the start of API
documentation. See [1] and bdfdaa4978d (strbuf.h: integrate
api-strbuf.txt documentation, 2015-01-16) and 6afbbdda333 (strbuf.h:
unify documentation comments beginnings, 2015-01-16) for a discussion
of that convention.
1. https://lore.kernel.org/git/874kbeecfu.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
bundle.h | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/bundle.h b/bundle.h
index 1927d8cd6a4..84a6df1b65d 100644
--- a/bundle.h
+++ b/bundle.h
@@ -27,6 +27,13 @@ int create_bundle(struct repository *r, const char *path,
int version);
int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);
#define BUNDLE_VERBOSE 1
+
+/**
+ * Unbundle after reading the header with read_bundle_header().
+ *
+ * We'll invoke "git index-pack --stdin --fix-thin" for you on the
+ * provided `bundle_fd` from read_bundle_header().
+ */
int unbundle(struct repository *r, struct bundle_header *header,
int bundle_fd, int flags);
int list_bundle_refs(struct bundle_header *header,
--
2.33.0.813.g41c39388776
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v4 2/4] bundle API: change "flags" to be "extra_index_pack_args"
2021-09-05 7:34 ` [PATCH v4 0/4] " Ævar Arnfjörð Bjarmason
2021-09-05 7:34 ` [PATCH v4 1/4] bundle API: start writing API documentation Ævar Arnfjörð Bjarmason
@ 2021-09-05 7:34 ` Ævar Arnfjörð Bjarmason
2021-09-05 7:34 ` [PATCH v4 3/4] index-pack: add --progress-title option Ævar Arnfjörð Bjarmason
2021-09-05 7:34 ` [PATCH v4 4/4] bundle: show progress on "unbundle" Ævar Arnfjörð Bjarmason
3 siblings, 0 replies; 42+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-05 7:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Emily Shaffer, Derrick Stolee, Jeff King,
Ævar Arnfjörð Bjarmason
Since the "flags" parameter was added in be042aff24c (Teach progress
eye-candy to fetch_refs_from_bundle(), 2011-09-18) there's never been
more than the one flag: BUNDLE_VERBOSE.
Let's have the only caller who cares about that pass "-v" itself
instead through new "extra_index_pack_args" parameter. The flexibility
of being able to pass arbitrary arguments to "unbundle" will be used
in a subsequent commit.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/bundle.c | 4 +++-
bundle.c | 12 ++++++------
bundle.h | 7 +++++--
transport.c | 6 +++++-
4 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/builtin/bundle.c b/builtin/bundle.c
index 053a51bea1b..9b86c8529c7 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -166,6 +166,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
OPT_END()
};
char *bundle_file;
+ struct strvec extra_index_pack_args = STRVEC_INIT;
argc = parse_options_cmd_bundle(argc, argv, prefix,
builtin_bundle_unbundle_usage, options, &bundle_file);
@@ -177,7 +178,8 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
}
if (!startup_info->have_repository)
die(_("Need a repository to unbundle."));
- ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
+ ret = !!unbundle(the_repository, &header, bundle_fd,
+ &extra_index_pack_args) ||
list_bundle_refs(&header, argc, argv);
bundle_header_release(&header);
cleanup:
diff --git a/bundle.c b/bundle.c
index ab63f402261..a0bb687b0f4 100644
--- a/bundle.c
+++ b/bundle.c
@@ -569,18 +569,18 @@ int create_bundle(struct repository *r, const char *path,
}
int unbundle(struct repository *r, struct bundle_header *header,
- int bundle_fd, int flags)
+ int bundle_fd, struct strvec *extra_index_pack_args)
{
- const char *argv_index_pack[] = {"index-pack",
- "--fix-thin", "--stdin", NULL, NULL};
struct child_process ip = CHILD_PROCESS_INIT;
+ strvec_pushl(&ip.args, "index-pack", "--fix-thin", "--stdin", NULL);
- if (flags & BUNDLE_VERBOSE)
- argv_index_pack[3] = "-v";
+ if (extra_index_pack_args) {
+ strvec_pushv(&ip.args, extra_index_pack_args->v);
+ strvec_clear(extra_index_pack_args);
+ }
if (verify_bundle(r, header, 0))
return -1;
- ip.argv = argv_index_pack;
ip.in = bundle_fd;
ip.no_stdout = 1;
ip.git_cmd = 1;
diff --git a/bundle.h b/bundle.h
index 84a6df1b65d..06009fe6b1f 100644
--- a/bundle.h
+++ b/bundle.h
@@ -26,16 +26,19 @@ int create_bundle(struct repository *r, const char *path,
int argc, const char **argv, struct strvec *pack_options,
int version);
int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);
-#define BUNDLE_VERBOSE 1
/**
* Unbundle after reading the header with read_bundle_header().
*
* We'll invoke "git index-pack --stdin --fix-thin" for you on the
* provided `bundle_fd` from read_bundle_header().
+ *
+ * Provide "extra_index_pack_args" to pass any extra arguments
+ * (e.g. "-v" for verbose/progress), NULL otherwise. The provided
+ * "extra_index_pack_args" (if any) will be strvec_clear()'d for you.
*/
int unbundle(struct repository *r, struct bundle_header *header,
- int bundle_fd, int flags);
+ int bundle_fd, struct strvec *extra_index_pack_args);
int list_bundle_refs(struct bundle_header *header,
int argc, const char **argv);
diff --git a/transport.c b/transport.c
index 17e9629710a..ab9b03ae9ff 100644
--- a/transport.c
+++ b/transport.c
@@ -162,12 +162,16 @@ static int fetch_refs_from_bundle(struct transport *transport,
int nr_heads, struct ref **to_fetch)
{
struct bundle_transport_data *data = transport->data;
+ struct strvec extra_index_pack_args = STRVEC_INIT;
int ret;
+ if (transport->progress)
+ strvec_push(&extra_index_pack_args, "-v");
+
if (!data->get_refs_from_bundle_called)
get_refs_from_bundle(transport, 0, NULL);
ret = unbundle(the_repository, &data->header, data->fd,
- transport->progress ? BUNDLE_VERBOSE : 0);
+ &extra_index_pack_args);
transport->hash_algo = data->header.hash_algo;
return ret;
}
--
2.33.0.813.g41c39388776
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v4 3/4] index-pack: add --progress-title option
2021-09-05 7:34 ` [PATCH v4 0/4] " Ævar Arnfjörð Bjarmason
2021-09-05 7:34 ` [PATCH v4 1/4] bundle API: start writing API documentation Ævar Arnfjörð Bjarmason
2021-09-05 7:34 ` [PATCH v4 2/4] bundle API: change "flags" to be "extra_index_pack_args" Ævar Arnfjörð Bjarmason
@ 2021-09-05 7:34 ` Ævar Arnfjörð Bjarmason
2021-09-05 7:34 ` [PATCH v4 4/4] bundle: show progress on "unbundle" Ævar Arnfjörð Bjarmason
3 siblings, 0 replies; 42+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-05 7:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Emily Shaffer, Derrick Stolee, Jeff King,
Ævar Arnfjörð Bjarmason
Add a --progress-title option to index-pack, when data is piped into
index-pack its progress is a proxy for whatever's feeding it data.
This option will allow us to set a more relevant progress bar title in
"git bundle unbundle", and is also used in my "bundle-uri" RFC
patches[1] by a new caller in fetch-pack.c.
The code change in cmd_index_pack() won't handle
"--progress-title=xyz", only "--progress-title xyz", and the "(i+1)"
style (as opposed to "i + 1") is a bit odd.
Not using the "--long-option=value" style is inconsistent with
existing long options handled by cmd_index_pack(), but makes the code
that needs to call it better (two strvec_push(), instead of needing a
strvec_pushf()). Since the option is internal-only the inconsistency
shouldn't matter.
I'm copying the pattern to handle it as-is from the handling of the
existing "-o" option in the same function, see 9cf6d3357aa (Add
git-index-pack utility, 2005-10-12) for its addition. That's a short
option, but the code to implement the two is the same in functionality
and style. Eventually we'd like to migrate all of this this to
parse_options(), which would make these differences in behavior go
away.
1. https://lore.kernel.org/git/RFC-cover-00.13-0000000000-20210805T150534Z-avarab@gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Documentation/git-index-pack.txt | 6 ++++++
builtin/index-pack.c | 6 ++++++
2 files changed, 12 insertions(+)
diff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt
index 7fa74b9e798..1f1e3592251 100644
--- a/Documentation/git-index-pack.txt
+++ b/Documentation/git-index-pack.txt
@@ -82,6 +82,12 @@ OPTIONS
--strict::
Die, if the pack contains broken objects or links.
+--progress-title::
+ For internal use only.
++
+Set the title of the progress bar. The title is "Receiving objects" by
+default and "Indexing objects" when `--stdin` is specified.
+
--check-self-contained-and-connected::
Die if the pack contains broken links. For internal use only.
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 8336466865c..0841c039ae2 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -122,6 +122,7 @@ static int strict;
static int do_fsck_object;
static struct fsck_options fsck_options = FSCK_OPTIONS_MISSING_GITMODULES;
static int verbose;
+static const char *progress_title;
static int show_resolving_progress;
static int show_stat;
static int check_self_contained_and_connected;
@@ -1157,6 +1158,7 @@ static void parse_pack_objects(unsigned char *hash)
if (verbose)
progress = start_progress(
+ progress_title ? progress_title :
from_stdin ? _("Receiving objects") : _("Indexing objects"),
nr_objects);
for (i = 0; i < nr_objects; i++) {
@@ -1806,6 +1808,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
input_len = sizeof(*hdr);
} else if (!strcmp(arg, "-v")) {
verbose = 1;
+ } else if (!strcmp(arg, "--progress-title")) {
+ if (progress_title || (i+1) >= argc)
+ usage(index_pack_usage);
+ progress_title = argv[++i];
} else if (!strcmp(arg, "--show-resolving-progress")) {
show_resolving_progress = 1;
} else if (!strcmp(arg, "--report-end-of-input")) {
--
2.33.0.813.g41c39388776
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v4 4/4] bundle: show progress on "unbundle"
2021-09-05 7:34 ` [PATCH v4 0/4] " Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-09-05 7:34 ` [PATCH v4 3/4] index-pack: add --progress-title option Ævar Arnfjörð Bjarmason
@ 2021-09-05 7:34 ` Ævar Arnfjörð Bjarmason
3 siblings, 0 replies; 42+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-05 7:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Emily Shaffer, Derrick Stolee, Jeff King,
Ævar Arnfjörð Bjarmason
The "unbundle" command added in 2e0afafebd8 (Add git-bundle: move
objects and references by archive, 2007-02-22) did not show progress
output, even though the underlying API learned how to show progress in
be042aff24c (Teach progress eye-candy to fetch_refs_from_bundle(),
2011-09-18).
Now we'll show "Unbundling objects" using the new --progress-title
option to "git index-pack", to go with its existing "Receiving
objects" and "Indexing objects" (which it shows when invoked with
"--stdin", and with a pack file, respectively).
Unlike "git bundle create" we don't handle "--quiet" here, nor
"--all-progress" and "--all-progress-implied". Those are all specific
to "create" (and "verify", in the case of "--quiet").
The structure of the existing documentation is a bit unclear, e.g. the
documentation for the "--quiet" option added in
79862b6b77c (bundle-create: progress output control, 2019-11-10) only
describes how it works for "create", and not for "verify". That and
other issues in it should be fixed, but I'd like to avoid untangling
that mess right now. Let's just support the standard "--no-progress"
implicitly here, and leave cleaning up the general behavior of "git
bundle" for a later change.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Documentation/git-bundle.txt | 2 +-
builtin/bundle.c | 7 +++++++
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt
index ac0d0038350..71b5ecabd1f 100644
--- a/Documentation/git-bundle.txt
+++ b/Documentation/git-bundle.txt
@@ -13,7 +13,7 @@ SYNOPSIS
[--version=<version>] <file> <git-rev-list-args>
'git bundle' verify [-q | --quiet] <file>
'git bundle' list-heads <file> [<refname>...]
-'git bundle' unbundle <file> [<refname>...]
+'git bundle' unbundle [--progress] <file> [<refname>...]
DESCRIPTION
-----------
diff --git a/builtin/bundle.c b/builtin/bundle.c
index 9b86c8529c7..91975def2da 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -162,7 +162,11 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
struct bundle_header header = BUNDLE_HEADER_INIT;
int bundle_fd = -1;
int ret;
+ int progress = isatty(2);
+
struct option options[] = {
+ OPT_BOOL(0, "progress", &progress,
+ N_("show progress meter")),
OPT_END()
};
char *bundle_file;
@@ -178,6 +182,9 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
}
if (!startup_info->have_repository)
die(_("Need a repository to unbundle."));
+ if (progress)
+ strvec_pushl(&extra_index_pack_args, "-v", "--progress-title",
+ _("Unbundling objects"), NULL);
ret = !!unbundle(the_repository, &header, bundle_fd,
&extra_index_pack_args) ||
list_bundle_refs(&header, argc, argv);
--
2.33.0.813.g41c39388776
^ permalink raw reply related [flat|nested] 42+ messages in thread