{"thread":{"id":"66001","subject":"[PATCH] remote-curl: simplify passing of push specs","startedAt":"2026-07-15T04:41:19Z","lastAt":"2026-07-16T14:29:00Z","messageCount":5,"participants":["René Scharfe","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"548203","messageId":"935883f3-3be4-4c51-9711-5208b9ef9ca1@web.de","threadId":"66001","inReplyTo":null,"subject":"[PATCH] remote-curl: simplify passing of push specs","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2026-07-15T04:41:17Z","receivedAt":"2026-07-15T04:41:19Z","isPatch":true,"body":"The push specs are kept in a strvec, whose array is NULL-terminated.\nPass only that to the protocol handlers, which avoids dealing with item\ncounts and their conversions from size_t to int, slightly simplifying\nthe code.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n remote-curl.c | 20 +++++++++-----------\n 1 file changed, 9 insertions(+), 11 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 9e614c5567..2c35dd5240 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1340,10 +1340,9 @@ static void parse_get(const char *arg)\n \tfflush(stdout);\n }\n \n-static int push_dav(int nr_spec, const char **specs)\n+static int push_dav(const char **specs)\n {\n \tstruct child_process child = CHILD_PROCESS_INIT;\n-\tsize_t i;\n \n \tchild.git_cmd = 1;\n \tstrvec_push(&child.args, \"http-push\");\n@@ -1353,15 +1352,14 @@ static int push_dav(int nr_spec, const char **specs)\n \tif (options.verbosity > 1)\n \t\tstrvec_push(&child.args, \"--verbose\");\n \tstrvec_push(&child.args, url.buf);\n-\tfor (i = 0; i < nr_spec; i++)\n-\t\tstrvec_push(&child.args, specs[i]);\n+\tstrvec_pushv(&child.args, specs);\n \n \tif (run_command(&child))\n \t\tdie(_(\"git-http-push failed\"));\n \treturn 0;\n }\n \n-static int push_git(struct discovery *heads, int nr_spec, const char **specs)\n+static int push_git(struct discovery *heads, const char **specs)\n {\n \tstruct rpc_state rpc = RPC_STATE_INIT;\n \tint i, err;\n@@ -1400,8 +1398,8 @@ static int push_git(struct discovery *heads, int nr_spec, const char **specs)\n \t\tstrvec_push(&args, \"--force-if-includes\");\n \n \tstrvec_push(&args, \"--stdin\");\n-\tfor (i = 0; i < nr_spec; i++)\n-\t\tpacket_buf_write(&preamble, \"%s\\n\", specs[i]);\n+\tfor (; *specs; specs++)\n+\t\tpacket_buf_write(&preamble, \"%s\\n\", *specs);\n \tpacket_buf_flush(&preamble);\n \n \tmemset(&rpc, 0, sizeof(rpc));\n@@ -1416,15 +1414,15 @@ static int push_git(struct discovery *heads, int nr_spec, const char **specs)\n \treturn err;\n }\n \n-static int push(int nr_spec, const char **specs)\n+static int push(const char **specs)\n {\n \tstruct discovery *heads = discover_refs(\"git-receive-pack\", 1);\n \tint ret;\n \n \tif (heads->proto_git)\n-\t\tret = push_git(heads, nr_spec, specs);\n+\t\tret = push_git(heads, specs);\n \telse\n-\t\tret = push_dav(nr_spec, specs);\n+\t\tret = push_dav(specs);\n \tfree_discovery(heads);\n \treturn ret;\n }\n@@ -1448,7 +1446,7 @@ static void parse_push(struct strbuf *buf)\n \t\t\tbreak;\n \t} while (1);\n \n-\tret = push(specs.nr, specs.v);\n+\tret = push(specs.v);\n \tprintf(\"\\n\");\n \tfflush(stdout);\n \n-- \n2.55.0\n"},{"id":"548225","messageId":"alcrhGUCVMCnm2-i@pks.im","threadId":"66001","inReplyTo":"935883f3-3be4-4c51-9711-5208b9ef9ca1@web.de","subject":"Re: [PATCH] remote-curl: simplify passing of push specs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-07-15T06:41:08Z","receivedAt":"2026-07-15T06:41:14Z","isPatch":true,"body":"On Wed, Jul 15, 2026 at 06:41:17AM +0200, René Scharfe wrote:\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 9e614c5567..2c35dd5240 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -1340,10 +1340,9 @@ static void parse_get(const char *arg)\n>  \tfflush(stdout);\n>  }\n>  \n> -static int push_dav(int nr_spec, const char **specs)\n> +static int push_dav(const char **specs)\n>  {\n>  \tstruct child_process child = CHILD_PROCESS_INIT;\n> -\tsize_t i;\n>  \n>  \tchild.git_cmd = 1;\n>  \tstrvec_push(&child.args, \"http-push\");\n\nI wonder whether the interface would be even better if we simply passed\naround a `const struct strvec *` directly. That makes it explicit what\nkind of guarantees we have, and all transitive callers already have one\navailable anyway.\n\n> @@ -1353,15 +1352,14 @@ static int push_dav(int nr_spec, const char **specs)\n>  \tif (options.verbosity > 1)\n>  \t\tstrvec_push(&child.args, \"--verbose\");\n>  \tstrvec_push(&child.args, url.buf);\n> -\tfor (i = 0; i < nr_spec; i++)\n> -\t\tstrvec_push(&child.args, specs[i]);\n> +\tstrvec_pushv(&child.args, specs);\n\nI thought that we had something like `strvec_pushvec()` that knew to\nalso optimize for this case so that we don't have to reallocate the\nvector multiple times. And if we had that function it would even be more\nefficient to pass it down the stack. But we seemingly don't have it, so\nthat argument is kind of moot.\n\nOther than those nits the patch looks good to me, thanks!\n\nPatrick\n"},{"id":"548302","messageId":"3b29757e-abcd-4235-a829-ea67c19e71d0@web.de","threadId":"66001","inReplyTo":"alcrhGUCVMCnm2-i@pks.im","subject":"Re: [PATCH] remote-curl: simplify passing of push specs","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2026-07-15T15:39:51Z","receivedAt":"2026-07-15T15:39:56Z","isPatch":true,"body":"On 7/15/26 8:41 AM, Patrick Steinhardt wrote:\n> On Wed, Jul 15, 2026 at 06:41:17AM +0200, René Scharfe wrote:\n>> diff --git a/remote-curl.c b/remote-curl.c\n>> index 9e614c5567..2c35dd5240 100644\n>> --- a/remote-curl.c\n>> +++ b/remote-curl.c\n>> @@ -1340,10 +1340,9 @@ static void parse_get(const char *arg)\n>>  \tfflush(stdout);\n>>  }\n>>  \n>> -static int push_dav(int nr_spec, const char **specs)\n>> +static int push_dav(const char **specs)\n>>  {\n>>  \tstruct child_process child = CHILD_PROCESS_INIT;\n>> -\tsize_t i;\n>>  \n>>  \tchild.git_cmd = 1;\n>>  \tstrvec_push(&child.args, \"http-push\");\n> \n> I wonder whether the interface would be even better if we simply passed\n> around a `const struct strvec *` directly. That makes it explicit what\n> kind of guarantees we have, and all transitive callers already have one\n> available anyway.\n\nYou mean that passing a managed array instead of a plain NULL-terminated\none would make more places visibly safer at almost no cost?\n\n>> @@ -1353,15 +1352,14 @@ static int push_dav(int nr_spec, const char **specs)\n>>  \tif (options.verbosity > 1)\n>>  \t\tstrvec_push(&child.args, \"--verbose\");\n>>  \tstrvec_push(&child.args, url.buf);\n>> -\tfor (i = 0; i < nr_spec; i++)\n>> -\t\tstrvec_push(&child.args, specs[i]);\n>> +\tstrvec_pushv(&child.args, specs);\n> \n> I thought that we had something like `strvec_pushvec()` that knew to\n> also optimize for this case so that we don't have to reallocate the\n> vector multiple times. And if we had that function it would even be more\n> efficient to pass it down the stack. But we seemingly don't have it, so\n> that argument is kind of moot.\nWe could add one.  Not sure it would make a measurable difference; if\nthe number of specs is huge there are probably other costs that dwarf\npushing them to a strvec.\n\nI have to admit that the simplicity of strvec_pushv() nudged me towards\nusing a NULL-terminated array here, though.  So just having a\nstrvec_pushvec() available could guide towards using the length-limited\nstrvec instead of a simpler NULL-terminated array (which explodes if\nleft unterminated).\n\nRené\n\n"},{"id":"548354","messageId":"alhr2bb0lUTHtvjO@pks.im","threadId":"66001","inReplyTo":"3b29757e-abcd-4235-a829-ea67c19e71d0@web.de","subject":"Re: [PATCH] remote-curl: simplify passing of push specs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-07-16T05:27:53Z","receivedAt":"2026-07-16T05:28:00Z","isPatch":true,"body":"On Wed, Jul 15, 2026 at 05:39:51PM +0200, René Scharfe wrote:\n> On 7/15/26 8:41 AM, Patrick Steinhardt wrote:\n> > On Wed, Jul 15, 2026 at 06:41:17AM +0200, René Scharfe wrote:\n> >> diff --git a/remote-curl.c b/remote-curl.c\n> >> index 9e614c5567..2c35dd5240 100644\n> >> --- a/remote-curl.c\n> >> +++ b/remote-curl.c\n> >> @@ -1340,10 +1340,9 @@ static void parse_get(const char *arg)\n> >>  \tfflush(stdout);\n> >>  }\n> >>  \n> >> -static int push_dav(int nr_spec, const char **specs)\n> >> +static int push_dav(const char **specs)\n> >>  {\n> >>  \tstruct child_process child = CHILD_PROCESS_INIT;\n> >> -\tsize_t i;\n> >>  \n> >>  \tchild.git_cmd = 1;\n> >>  \tstrvec_push(&child.args, \"http-push\");\n> > \n> > I wonder whether the interface would be even better if we simply passed\n> > around a `const struct strvec *` directly. That makes it explicit what\n> > kind of guarantees we have, and all transitive callers already have one\n> > available anyway.\n> \n> You mean that passing a managed array instead of a plain NULL-terminated\n> one would make more places visibly safer at almost no cost?\n> \n> >> @@ -1353,15 +1352,14 @@ static int push_dav(int nr_spec, const char **specs)\n> >>  \tif (options.verbosity > 1)\n> >>  \t\tstrvec_push(&child.args, \"--verbose\");\n> >>  \tstrvec_push(&child.args, url.buf);\n> >> -\tfor (i = 0; i < nr_spec; i++)\n> >> -\t\tstrvec_push(&child.args, specs[i]);\n> >> +\tstrvec_pushv(&child.args, specs);\n> > \n> > I thought that we had something like `strvec_pushvec()` that knew to\n> > also optimize for this case so that we don't have to reallocate the\n> > vector multiple times. And if we had that function it would even be more\n> > efficient to pass it down the stack. But we seemingly don't have it, so\n> > that argument is kind of moot.\n> We could add one.  Not sure it would make a measurable difference; if\n> the number of specs is huge there are probably other costs that dwarf\n> pushing them to a strvec.\n\nYeah, I don't expect it to make a difference here, either. But by having\nit we could use it in more places going forward, and that might lead to\ntiny savings here and there that ultimately add up. So it'd be nudging\nfolks to \"do the right thing\".\n\n> I have to admit that the simplicity of strvec_pushv() nudged me towards\n> using a NULL-terminated array here, though.  So just having a\n> strvec_pushvec() available could guide towards using the length-limited\n> strvec instead of a simpler NULL-terminated array (which explodes if\n> left unterminated).\n\nAnd that's not a huge issue by itself. I think the version you have here\nis totally fine, and I won't insist on a reroll. But I think it gives us\na good opportunity to improve the status quo, if we want to take it.\n\nThanks!\n\nPatrick\n"},{"id":"548419","messageId":"xmqqpl0nhutx.fsf@gitster.g","threadId":"66001","inReplyTo":"alhr2bb0lUTHtvjO@pks.im","subject":"Re: [PATCH] remote-curl: simplify passing of push specs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-16T14:28:58Z","receivedAt":"2026-07-16T14:29:00Z","isPatch":true,"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Wed, Jul 15, 2026 at 05:39:51PM +0200, René Scharfe wrote:\n>> > \n>> We could add one.  Not sure it would make a measurable difference; if\n>> the number of specs is huge there are probably other costs that dwarf\n>> pushing them to a strvec.\n>\n> Yeah, I don't expect it to make a difference here, either. But by having\n> it we could use it in more places going forward, and that might lead to\n> tiny savings here and there that ultimately add up. So it'd be nudging\n> folks to \"do the right thing\".\n\nThat's a sensible thought.\n\n>> I have to admit that the simplicity of strvec_pushv() nudged me towards\n>> using a NULL-terminated array here, though.  So just having a\n>> strvec_pushvec() available could guide towards using the length-limited\n>> strvec instead of a simpler NULL-terminated array (which explodes if\n>> left unterminated).\n>\n> And that's not a huge issue by itself. I think the version you have here\n> is totally fine, and I won't insist on a reroll. But I think it gives us\n> a good opportunity to improve the status quo, if we want to take it.\n\nYeah, strvec_pushvec() might be a worthwhile thing to do, but that\ncan come independent of this topic.  The output from:\n\n    $ git grep -n -e strvec_pushv\\(\n\nis easy enough to look through to find which callers pass a strvec\nas the second parameter.  It should be quite straightforward to find\nconversion candidates once the helper is actually implemented.  It\nmight even be possible to use Coccinelle for such a conversion.\n\n"}]}