[PATCH] run-command: introduce CHILD_PROCESS_INIT
Most struct child_process variables are cleared using memset right after
declaration. Provide a macro, CHILD_PROCESS_INIT, that can be used to
initialize them statically instead. That's shorter, doesn't require a
function call and is slightly more readable (especially given that we
already have similar macros like STRBUF_INIT, ARGV_ARRAY_INIT etc.).
Signed-off-by: Rene Scharfe
---
Documentation/technical/api-run-command.txt | 4 ++--
archive-tar.c | 3 +--
builtin/add.c | 3 +--
builtin/commit.c| 3 +--
builtin/help.c | 3 +--
builtin/merge.c | 3 +--
builtin/notes.c | 3 +--
builtin/remote-ext.c| 3 +--
builtin/repack.c| 3 +--
builtin/replace.c | 4 ++--
builtin/verify-pack.c | 3 +--
bundle.c| 6 ++
connected.c | 3 +--
convert.c | 3 +--
credential-cache.c | 3 +--
credential.c| 3 +--
daemon.c| 8 +++-
diff.c | 3 +--
editor.c| 3 +--
fetch-pack.c| 3 +--
gpg-interface.c | 6 ++
http-backend.c | 3 +--
http.c | 3 +--
imap-send.c | 2 +-
prompt.c| 3 +--
remote-curl.c | 3 +--
remote-testsvn.c| 3 +--
run-command.h | 2 ++
send-pack.c | 3 +--
submodule.c | 21 +++--
test-run-command.c | 4 +---
test-subprocess.c | 3 +--
transport.c | 12
upload-pack.c | 3 +--
wt-status.c | 3 +--
35 files changed, 51 insertions(+), 93 deletions(-)
diff --git a/Documentation/technical/api-run-command.txt
b/Documentation/technical/api-run-command.txt
index 69510ae..ca066bf 100644
--- a/Documentation/technical/api-run-command.txt
+++ b/Documentation/technical/api-run-command.txt
@@ -96,8 +96,8 @@ command to run in a sub-process.
The caller:
-1. allocates and clears (memset(&chld, 0, sizeof(chld));) a
- struct child_process variable;
+1. allocates and clears (memset(&chld, 0, sizeof(chld)); or
+ using CHILD_PROCESS_INIT) a struct child_process variable;
2. initializes the members;
3. calls start_command();
4. processes the data;
diff --git a/archive-tar.c b/archive-tar.c
index 719b629..0d1e6bd 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -395,7 +395,7 @@ static int write_tar_filter_archive(const struct archiver
*ar,
struct archiver_args *args)
{
struct strbuf cmd = STRBUF_INIT;
- struct child_process filter;
+ struct child_process filter = CHILD_PROCESS_INIT;
const char *argv[2];
int r;
@@ -406,7 +406,6 @@ static int write_tar_filter_archive(const struct archiver
*ar,
if (args->compression_level >= 0)
strbuf_addf(&cmd, " -%d", args->compression_level);
- memset(&filter, 0, sizeof(filter));
argv[0] = cmd.buf;
argv[1] = NULL;
filter.argv = argv;
diff --git a/builtin/add.c b/builtin/add.c
index 4baf3a5..352b85e 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -180,7 +180,7 @@ static int edit_patch(int argc, const char **argv, const
char *prefix)
char *file = git_pathdup("ADD_EDIT.patch");
const char *apply_argv[] = { "apply", "--recount", "--cached",
NULL, NULL };
- struct child_process child;
+ struct child_process child = CHILD_PROCESS_INIT;
struct rev_info rev;
int out;
struct stat st;
@@ -214,7 +214,6 @@ static int edit_patch(int argc, const char **argv, const
char *prefix)
if (!st.st_size)
die(_("Empty patch. Aborted."));
- memset(&child, 0, sizeof(child));
child.git_cmd = 1;
child.argv = apply_argv;
if (run_command(&child))
diff --git a/builtin/commit.c b/builtin/commit.c
index 5ed6036..b8b8663 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1508,7 +1508,7 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
{
/* oldsha1 SP newsha1 LF NUL */
static char buf[2*40 + 3];
- struct child_process proc;
+ struct child_process proc = CHILD_PROCESS_INIT;
const char *argv[3];
int code;
size_t n
Re: [PATCH] run-command: introduce CHILD_PROCESS_INIT
On Sun, Aug 17, 2014 at 12:55:23AM +0200, René Scharfe wrote: > Most struct child_process variables are cleared using memset right after > declaration. Provide a macro, CHILD_PROCESS_INIT, that can be used to > initialize them statically instead. That's shorter, doesn't require a > function call and is slightly more readable (especially given that we > already have similar macros like STRBUF_INIT, ARGV_ARRAY_INIT etc.). I think one reason we never had an INIT macro here is that you cannot simply use the struct after zero-ing it anyway. That's just the first step, and then you have to tweak a bunch of fields to get what you want. So the memset is just one setup line out of many. Still, I think this is probably an improvement for the reasons you give above, and we can still memset() in any cases where it makes sense. Patch itself looks obviously correct. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] run-command: introduce CHILD_PROCESS_INIT
Am 17.08.2014 um 09:12 schrieb Jeff King: On Sun, Aug 17, 2014 at 12:55:23AM +0200, René Scharfe wrote: Most struct child_process variables are cleared using memset right after declaration. Provide a macro, CHILD_PROCESS_INIT, that can be used to initialize them statically instead. That's shorter, doesn't require a function call and is slightly more readable (especially given that we already have similar macros like STRBUF_INIT, ARGV_ARRAY_INIT etc.). I think one reason we never had an INIT macro here is that you cannot simply use the struct after zero-ing it anyway. That's just the first step, and then you have to tweak a bunch of fields to get what you want. So the memset is just one setup line out of many. Some (or most?) of these steps could be converted to named initializers -- once all supported platforms provide them.. René -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] run-command: introduce CHILD_PROCESS_INIT
On Sun, Aug 17, 2014 at 09:25:58AM +0200, René Scharfe wrote: > >I think one reason we never had an INIT macro here is that you cannot > >simply use the struct after zero-ing it anyway. That's just the first > >step, and then you have to tweak a bunch of fields to get what you want. > >So the memset is just one setup line out of many. > > Some (or most?) of these steps could be converted to named > initializers -- once all supported platforms provide them.. Yeah, I'd be fine with that, but I am not holding my breath on the "once..." in your statement. :) -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] run-command: introduce CHILD_PROCESS_INIT
Am 17.08.2014 00:55, schrieb René Scharfe:
> Most struct child_process variables are cleared using memset right after
> declaration. Provide a macro, CHILD_PROCESS_INIT, that can be used to
> initialize them statically instead. That's shorter, doesn't require a
> function call and is slightly more readable (especially given that we
> already have similar macros like STRBUF_INIT, ARGV_ARRAY_INIT etc.).
This is a step in the right direction, IMO. This way to initialize the
struct feels mucth better because it does not depend on that the bit
pattern of the NULL pointer is all zeros.
> Signed-off-by: Rene Scharfe
> ---
> Documentation/technical/api-run-command.txt | 4 ++--
> archive-tar.c | 3 +--
> builtin/add.c | 3 +--
> builtin/commit.c| 3 +--
> builtin/help.c | 3 +--
> builtin/merge.c | 3 +--
> builtin/notes.c | 3 +--
> builtin/remote-ext.c| 3 +--
> builtin/repack.c| 3 +--
> builtin/replace.c | 4 ++--
> builtin/verify-pack.c | 3 +--
> bundle.c| 6 ++
> connected.c | 3 +--
> convert.c | 3 +--
> credential-cache.c | 3 +--
> credential.c| 3 +--
> daemon.c| 8 +++-
> diff.c | 3 +--
> editor.c| 3 +--
> fetch-pack.c| 3 +--
> gpg-interface.c | 6 ++
> http-backend.c | 3 +--
> http.c | 3 +--
> imap-send.c | 2 +-
> prompt.c| 3 +--
> remote-curl.c | 3 +--
> remote-testsvn.c| 3 +--
> run-command.h | 2 ++
> send-pack.c | 3 +--
> submodule.c | 21 +++--
> test-run-command.c | 4 +---
> test-subprocess.c | 3 +--
> transport.c | 12
> upload-pack.c | 3 +--
> wt-status.c | 3 +--
> 35 files changed, 51 insertions(+), 93 deletions(-)
You missed a few instances in builtin/receive-pack.c. Is this deliberate?
Another one is in transport-helper.c::fetch_with_import() that is
currently initialized in get_importer(), and another one in
push_refs_with_export() that is initialized in get_exporter().
Should the static struct child_process variables in pager.c and
connect.c use the macro? It would just be for completeness, but does not
change the correctness. I dunno.
> diff --git a/run-command.h b/run-command.h
> index ea73de3..95a8fb8 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -44,6 +44,8 @@ struct child_process {
> unsigned clean_on_exit:1;
> };
>
> +#define CHILD_PROCESS_INIT { NULL }
I would have expected this to read
#define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT }
It does change the bit pattern of the initialized struct child_process
because ARGV_ARRAY_INIT uses a non-NULL address. But IMHO
ARGV_ARRAY_INIT should be used here as a defensive measure.
-- Hannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] run-command: introduce CHILD_PROCESS_INIT
On Sun, Aug 17, 2014 at 09:46:42AM +0200, Johannes Sixt wrote:
> This is a step in the right direction, IMO. This way to initialize the
> struct feels mucth better because it does not depend on that the bit
> pattern of the NULL pointer is all zeros.
I think platforms with NULL as something besides all-bits-zero are a
lost cause with git. There are so many struct memsets that depend on
this (and it's probably not actually worth caring about).
> > +#define CHILD_PROCESS_INIT { NULL }
>
> I would have expected this to read
>
> #define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT }
>
> It does change the bit pattern of the initialized struct child_process
> because ARGV_ARRAY_INIT uses a non-NULL address. But IMHO
> ARGV_ARRAY_INIT should be used here as a defensive measure.
I'd be OK with that. The argv_array code is specifically OK with an
all-bits-zero initialization. The only thing you don't get is that an
empty array is non-NULL, but that should never matter here (true, we'd
segfault if you didn't add anything to the array, but that is clearly a
bug that needs to be fixed either way).
I'm a little worried, though, that use sites without initializers would
be left behind. For example, git_proxy_connect uses xcalloc to allocate
the child_process, which results in all-bits-zero. If we want to start
caring about the initialization, we probably need to provide a
child_process_init() function and use it consistently.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] run-command: introduce CHILD_PROCESS_INIT
Jeff King writes: > I'm a little worried, though, that use sites without initializers would > be left behind. For example, git_proxy_connect uses xcalloc to allocate > the child_process, which results in all-bits-zero. If we want to start > caring about the initialization, we probably need to provide a > child_process_init() function and use it consistently. Thanks. Perhaps I can expect a v2 from René? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
