* [PATCH] send-pack: use internal argv_array of struct child_process
@ 2017-12-22 8:14 René Scharfe
2017-12-22 18:09 ` Stefan Beller
0 siblings, 1 reply; 3+ messages in thread
From: René Scharfe @ 2017-12-22 8:14 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
Avoid a magic number of NULL placeholder values and a magic index by
constructing the command line for pack-objects using the embedded
argv_array of the child_process. The resulting code is shorter and
easier to extend.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
send-pack.c | 28 +++++++++-------------------
1 file changed, 9 insertions(+), 19 deletions(-)
diff --git a/send-pack.c b/send-pack.c
index a8cc6b266e..2112d3b27a 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -58,35 +58,25 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struc
* the revision parameters to it via its stdin and
* let its stdout go back to the other end.
*/
- const char *argv[] = {
- "pack-objects",
- "--all-progress-implied",
- "--revs",
- "--stdout",
- NULL,
- NULL,
- NULL,
- NULL,
- NULL,
- NULL,
- };
struct child_process po = CHILD_PROCESS_INIT;
FILE *po_in;
int i;
int rc;
- i = 4;
+ argv_array_push(&po.args, "pack-objects");
+ argv_array_push(&po.args, "--all-progress-implied");
+ argv_array_push(&po.args, "--revs");
+ argv_array_push(&po.args, "--stdout");
if (args->use_thin_pack)
- argv[i++] = "--thin";
+ argv_array_push(&po.args, "--thin");
if (args->use_ofs_delta)
- argv[i++] = "--delta-base-offset";
+ argv_array_push(&po.args, "--delta-base-offset");
if (args->quiet || !args->progress)
- argv[i++] = "-q";
+ argv_array_push(&po.args, "-q");
if (args->progress)
- argv[i++] = "--progress";
+ argv_array_push(&po.args, "--progress");
if (is_repository_shallow())
- argv[i++] = "--shallow";
- po.argv = argv;
+ argv_array_push(&po.args, "--shallow");
po.in = -1;
po.out = args->stateless_rpc ? -1 : fd;
po.git_cmd = 1;
--
2.15.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] send-pack: use internal argv_array of struct child_process
2017-12-22 8:14 [PATCH] send-pack: use internal argv_array of struct child_process René Scharfe
@ 2017-12-22 18:09 ` Stefan Beller
2017-12-22 20:11 ` Junio C Hamano
0 siblings, 1 reply; 3+ messages in thread
From: Stefan Beller @ 2017-12-22 18:09 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
On Fri, Dec 22, 2017 at 12:14 AM, René Scharfe <l.s.r@web.de> wrote:
> Avoid a magic number of NULL placeholder values and a magic index by
> constructing the command line for pack-objects using the embedded
> argv_array of the child_process. The resulting code is shorter and
> easier to extend.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
Reviewed-by: Stefan Beller <sbeller@google.com>
> ---
> send-pack.c | 28 +++++++++-------------------
> 1 file changed, 9 insertions(+), 19 deletions(-)
>
> diff --git a/send-pack.c b/send-pack.c
> index a8cc6b266e..2112d3b27a 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -58,35 +58,25 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struc
> * the revision parameters to it via its stdin and
> * let its stdout go back to the other end.
> */
> - const char *argv[] = {
> - "pack-objects",
> - "--all-progress-implied",
> - "--revs",
> - "--stdout",
> - NULL,
> - NULL,
> - NULL,
> - NULL,
> - NULL,
> - NULL,
> - };
> struct child_process po = CHILD_PROCESS_INIT;
> FILE *po_in;
> int i;
> int rc;
>
> - i = 4;
> + argv_array_push(&po.args, "pack-objects");
> + argv_array_push(&po.args, "--all-progress-implied");
> + argv_array_push(&po.args, "--revs");
> + argv_array_push(&po.args, "--stdout");
(useless nit of the day, no need to resend:)
These four statements could be done as pushl(a,b,c, NULL);
but it would not differ in readability, so I guess either is fine.
Thanks,
Stefan
> if (args->use_thin_pack)
> - argv[i++] = "--thin";
> + argv_array_push(&po.args, "--thin");
> if (args->use_ofs_delta)
> - argv[i++] = "--delta-base-offset";
> + argv_array_push(&po.args, "--delta-base-offset");
> if (args->quiet || !args->progress)
> - argv[i++] = "-q";
> + argv_array_push(&po.args, "-q");
> if (args->progress)
> - argv[i++] = "--progress";
> + argv_array_push(&po.args, "--progress");
> if (is_repository_shallow())
> - argv[i++] = "--shallow";
> - po.argv = argv;
> + argv_array_push(&po.args, "--shallow");
> po.in = -1;
> po.out = args->stateless_rpc ? -1 : fd;
> po.git_cmd = 1;
> --
> 2.15.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] send-pack: use internal argv_array of struct child_process
2017-12-22 18:09 ` Stefan Beller
@ 2017-12-22 20:11 ` Junio C Hamano
0 siblings, 0 replies; 3+ messages in thread
From: Junio C Hamano @ 2017-12-22 20:11 UTC (permalink / raw)
To: Stefan Beller; +Cc: René Scharfe, Git List
Stefan Beller <sbeller@google.com> writes:
>> + argv_array_push(&po.args, "pack-objects");
>> + argv_array_push(&po.args, "--all-progress-implied");
>> + argv_array_push(&po.args, "--revs");
>> + argv_array_push(&po.args, "--stdout");
>
> (useless nit of the day, no need to resend:)
> These four statements could be done as pushl(a,b,c, NULL);
> but it would not differ in readability, so I guess either is fine.
Yup. Shorter is not necessarily better. If we anticipate that we
will need to update the set of options later (and I suspect we may
be doing so soon, given what JeffH and JTan are working on
recently), one option (or option-argument pair) per line is a more
preferrable form from the maintenance point of view.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-12-22 20:11 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-22 8:14 [PATCH] send-pack: use internal argv_array of struct child_process René Scharfe
2017-12-22 18:09 ` Stefan Beller
2017-12-22 20:11 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).