Volume XXII, number 279Tuesday, October 6, 2026Latest message 42 minutes ago

The Git List

News and archive of git@vger.kernel.org, since April 2005

patchremote-curl: simplify passing of push specs

5 messages between Jul 15, 2026 and Jul 16, 2026, from René Scharfe, Patrick Steinhardt, Junio C Hamano.

Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.

René ScharfeJul 15, 2026, 04:41 UTC on lore

The push specs are kept in a strvec, whose array is NULL-terminated. Pass only that to the protocol handlers, which avoids dealing with item counts and their conversions from size_t to int, slightly simplifying the code.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 remote-curl.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)
Show changes to remote-curl.c +9 −11
diff --git a/remote-curl.c b/remote-curl.c
index 9e614c5567..2c35dd5240 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1340,10 +1340,9 @@ static void parse_get(const char *arg)
 	fflush(stdout);
 }
 
-static int push_dav(int nr_spec, const char **specs)
+static int push_dav(const char **specs)
 {
 	struct child_process child = CHILD_PROCESS_INIT;
-	size_t i;
 
 	child.git_cmd = 1;
 	strvec_push(&child.args, "http-push");
@@ -1353,15 +1352,14 @@ static int push_dav(int nr_spec, const char **specs)
 	if (options.verbosity > 1)
 		strvec_push(&child.args, "--verbose");
 	strvec_push(&child.args, url.buf);
-	for (i = 0; i < nr_spec; i++)
-		strvec_push(&child.args, specs[i]);
+	strvec_pushv(&child.args, specs);
 
 	if (run_command(&child))
 		die(_("git-http-push failed"));
 	return 0;
 }
 
-static int push_git(struct discovery *heads, int nr_spec, const char **specs)
+static int push_git(struct discovery *heads, const char **specs)
 {
 	struct rpc_state rpc = RPC_STATE_INIT;
 	int i, err;
@@ -1400,8 +1398,8 @@ static int push_git(struct discovery *heads, int nr_spec, const char **specs)
 		strvec_push(&args, "--force-if-includes");
 
 	strvec_push(&args, "--stdin");
-	for (i = 0; i < nr_spec; i++)
-		packet_buf_write(&preamble, "%s\n", specs[i]);
+	for (; *specs; specs++)
+		packet_buf_write(&preamble, "%s\n", *specs);
 	packet_buf_flush(&preamble);
 
 	memset(&rpc, 0, sizeof(rpc));
@@ -1416,15 +1414,15 @@ static int push_git(struct discovery *heads, int nr_spec, const char **specs)
 	return err;
 }
 
-static int push(int nr_spec, const char **specs)
+static int push(const char **specs)
 {
 	struct discovery *heads = discover_refs("git-receive-pack", 1);
 	int ret;
 
 	if (heads->proto_git)
-		ret = push_git(heads, nr_spec, specs);
+		ret = push_git(heads, specs);
 	else
-		ret = push_dav(nr_spec, specs);
+		ret = push_dav(specs);
 	free_discovery(heads);
 	return ret;
 }
@@ -1448,7 +1446,7 @@ static void parse_push(struct strbuf *buf)
 			break;
 	} while (1);
 
-	ret = push(specs.nr, specs.v);
+	ret = push(specs.v);
 	printf("\n");
 	fflush(stdout);
 

-- 
2.55.0
Patrick SteinhardtJul 15, 2026, 06:41 UTC in reply to René Scharfe on lore

Re: [PATCH] remote-curl: simplify passing of push specs

On Wed, Jul 15, 2026 at 06:41:17AM +0200, René Scharfe wrote:
Show 16 quoted lines
> diff --git a/remote-curl.c b/remote-curl.c
> index 9e614c5567..2c35dd5240 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -1340,10 +1340,9 @@ static void parse_get(const char *arg)
>  	fflush(stdout);
>  }
>  
> -static int push_dav(int nr_spec, const char **specs)
> +static int push_dav(const char **specs)
>  {
>  	struct child_process child = CHILD_PROCESS_INIT;
> -	size_t i;
>  
>  	child.git_cmd = 1;
>  	strvec_push(&child.args, "http-push");

I wonder whether the interface would be even better if we simply passed around a `const struct strvec *` directly. That makes it explicit what kind of guarantees we have, and all transitive callers already have one available anyway.

Show 7 quoted lines
> @@ -1353,15 +1352,14 @@ static int push_dav(int nr_spec, const char **specs)
>  	if (options.verbosity > 1)
>  		strvec_push(&child.args, "--verbose");
>  	strvec_push(&child.args, url.buf);
> -	for (i = 0; i < nr_spec; i++)
> -		strvec_push(&child.args, specs[i]);
> +	strvec_pushv(&child.args, specs);

I thought that we had something like `strvec_pushvec()` that knew to also optimize for this case so that we don't have to reallocate the vector multiple times. And if we had that function it would even be more efficient to pass it down the stack. But we seemingly don't have it, so that argument is kind of moot.

Other than those nits the patch looks good to me, thanks!
Patrick
René ScharfeJul 15, 2026, 15:39 UTC in reply to Patrick Steinhardt on lore

Re: [PATCH] remote-curl: simplify passing of push specs

On 7/15/26 8:41 AM, Patrick Steinhardt wrote:
Show 22 quoted lines
> On Wed, Jul 15, 2026 at 06:41:17AM +0200, René Scharfe wrote:
>> diff --git a/remote-curl.c b/remote-curl.c
>> index 9e614c5567..2c35dd5240 100644
>> --- a/remote-curl.c
>> +++ b/remote-curl.c
>> @@ -1340,10 +1340,9 @@ static void parse_get(const char *arg)
>>  	fflush(stdout);
>>  }
>>  
>> -static int push_dav(int nr_spec, const char **specs)
>> +static int push_dav(const char **specs)
>>  {
>>  	struct child_process child = CHILD_PROCESS_INIT;
>> -	size_t i;
>>  
>>  	child.git_cmd = 1;
>>  	strvec_push(&child.args, "http-push");
> 
> I wonder whether the interface would be even better if we simply passed
> around a `const struct strvec *` directly. That makes it explicit what
> kind of guarantees we have, and all transitive callers already have one
> available anyway.

You mean that passing a managed array instead of a plain NULL-terminated one would make more places visibly safer at almost no cost?

Show 13 quoted lines
>> @@ -1353,15 +1352,14 @@ static int push_dav(int nr_spec, const char **specs)
>>  	if (options.verbosity > 1)
>>  		strvec_push(&child.args, "--verbose");
>>  	strvec_push(&child.args, url.buf);
>> -	for (i = 0; i < nr_spec; i++)
>> -		strvec_push(&child.args, specs[i]);
>> +	strvec_pushv(&child.args, specs);
> 
> I thought that we had something like `strvec_pushvec()` that knew to
> also optimize for this case so that we don't have to reallocate the
> vector multiple times. And if we had that function it would even be more
> efficient to pass it down the stack. But we seemingly don't have it, so
> that argument is kind of moot.

We could add one. Not sure it would make a measurable difference; if the number of specs is huge there are probably other costs that dwarf pushing them to a strvec.

I have to admit that the simplicity of strvec_pushv() nudged me towards using a NULL-terminated array here, though. So just having a strvec_pushvec() available could guide towards using the length-limited strvec instead of a simpler NULL-terminated array (which explodes if left unterminated).

René
Patrick SteinhardtJul 16, 2026, 05:27 UTC in reply to René Scharfe on lore

Re: [PATCH] remote-curl: simplify passing of push specs

On Wed, Jul 15, 2026 at 05:39:51PM +0200, René Scharfe wrote:
Show 43 quoted lines
> On 7/15/26 8:41 AM, Patrick Steinhardt wrote:
> > On Wed, Jul 15, 2026 at 06:41:17AM +0200, René Scharfe wrote:
> >> diff --git a/remote-curl.c b/remote-curl.c
> >> index 9e614c5567..2c35dd5240 100644
> >> --- a/remote-curl.c
> >> +++ b/remote-curl.c
> >> @@ -1340,10 +1340,9 @@ static void parse_get(const char *arg)
> >>  	fflush(stdout);
> >>  }
> >>  
> >> -static int push_dav(int nr_spec, const char **specs)
> >> +static int push_dav(const char **specs)
> >>  {
> >>  	struct child_process child = CHILD_PROCESS_INIT;
> >> -	size_t i;
> >>  
> >>  	child.git_cmd = 1;
> >>  	strvec_push(&child.args, "http-push");
> > 
> > I wonder whether the interface would be even better if we simply passed
> > around a `const struct strvec *` directly. That makes it explicit what
> > kind of guarantees we have, and all transitive callers already have one
> > available anyway.
> 
> You mean that passing a managed array instead of a plain NULL-terminated
> one would make more places visibly safer at almost no cost?
> 
> >> @@ -1353,15 +1352,14 @@ static int push_dav(int nr_spec, const char **specs)
> >>  	if (options.verbosity > 1)
> >>  		strvec_push(&child.args, "--verbose");
> >>  	strvec_push(&child.args, url.buf);
> >> -	for (i = 0; i < nr_spec; i++)
> >> -		strvec_push(&child.args, specs[i]);
> >> +	strvec_pushv(&child.args, specs);
> > 
> > I thought that we had something like `strvec_pushvec()` that knew to
> > also optimize for this case so that we don't have to reallocate the
> > vector multiple times. And if we had that function it would even be more
> > efficient to pass it down the stack. But we seemingly don't have it, so
> > that argument is kind of moot.
> We could add one.  Not sure it would make a measurable difference; if
> the number of specs is huge there are probably other costs that dwarf
> pushing them to a strvec.

Yeah, I don't expect it to make a difference here, either. But by having it we could use it in more places going forward, and that might lead to tiny savings here and there that ultimately add up. So it'd be nudging folks to "do the right thing".

Show 5 quoted lines
> I have to admit that the simplicity of strvec_pushv() nudged me towards
> using a NULL-terminated array here, though.  So just having a
> strvec_pushvec() available could guide towards using the length-limited
> strvec instead of a simpler NULL-terminated array (which explodes if
> left unterminated).

And that's not a huge issue by itself. I think the version you have here is totally fine, and I won't insist on a reroll. But I think it gives us a good opportunity to improve the status quo, if we want to take it.

Thanks!
Patrick
Junio C HamanoJul 16, 2026, 14:28 UTC in reply to Patrick Steinhardt on lore

Re: [PATCH] remote-curl: simplify passing of push specs

Patrick Steinhardt <ps@pks.im> writes:
Show 10 quoted lines
> On Wed, Jul 15, 2026 at 05:39:51PM +0200, René Scharfe wrote:
>> > 
>> We could add one.  Not sure it would make a measurable difference; if
>> the number of specs is huge there are probably other costs that dwarf
>> pushing them to a strvec.
>
> Yeah, I don't expect it to make a difference here, either. But by having
> it we could use it in more places going forward, and that might lead to
> tiny savings here and there that ultimately add up. So it'd be nudging
> folks to "do the right thing".
That's a sensible thought.
Show 9 quoted lines
>> I have to admit that the simplicity of strvec_pushv() nudged me towards
>> using a NULL-terminated array here, though.  So just having a
>> strvec_pushvec() available could guide towards using the length-limited
>> strvec instead of a simpler NULL-terminated array (which explodes if
>> left unterminated).
>
> And that's not a huge issue by itself. I think the version you have here
> is totally fine, and I won't insist on a reroll. But I think it gives us
> a good opportunity to improve the status quo, if we want to take it.

Yeah, strvec_pushvec() might be a worthwhile thing to do, but that can come independent of this topic. The output from:

    $ git grep -n -e strvec_pushv\(

is easy enough to look through to find which callers pass a strvec as the second parameter. It should be quite straightforward to find conversion candidates once the helper is actually implemented. It might even be possible to use Coccinelle for such a conversion.

Back to recent threads