Re: [PATCH v2] remote-testsvn: use internal argv_array of struct child_process in cmd_import()

2014-07-18 Thread Junio C Hamano 
René Scharfe l@web.de writes:

 Use the existing argv_array member instead of providing our own.  This
 way we don't have to initialize or clean it up explicitly.  Because of
 that automatic cleanup, we need to keep our own reference to the
 command name instead of using .argv[0] to print the warning at the end.

 Signed-off-by: Rene Scharfe l@web.de
 ---
 The added command pointer makes the patch more complicated, but I think
 it still counts as a cleanup.

Surely.  I'd move the svnrdump assignment to where the variable is
defined, though; we do not switch what command to run depending on
some computed conditions anyway.


  remote-testsvn.c | 17 -
  1 file changed, 8 insertions(+), 9 deletions(-)

 diff --git a/remote-testsvn.c b/remote-testsvn.c
 index 6be55cb..e3ad11b 100644
 --- a/remote-testsvn.c
 +++ b/remote-testsvn.c
 @@ -175,8 +175,8 @@ static int cmd_import(const char *line)
   char *note_msg;
   unsigned char head_sha1[20];
   unsigned int startrev;
 - struct argv_array svndump_argv = ARGV_ARRAY_INIT;
   struct child_process svndump_proc;
 + const char *command;
  
   if (read_ref(private_ref, head_sha1))
   startrev = 0;
 @@ -200,17 +200,17 @@ static int cmd_import(const char *line)
   if(dumpin_fd  0)
   die_errno(Couldn't open svn dump file %s., url);
   } else {
 + command = svnrdump;
   memset(svndump_proc, 0, sizeof(struct child_process));
   svndump_proc.out = -1;
 - argv_array_push(svndump_argv, svnrdump);
 - argv_array_push(svndump_argv, dump);
 - argv_array_push(svndump_argv, url);
 - argv_array_pushf(svndump_argv, -r%u:HEAD, startrev);
 - svndump_proc.argv = svndump_argv.argv;
 + argv_array_push(svndump_proc.args, command);
 + argv_array_push(svndump_proc.args, dump);
 + argv_array_push(svndump_proc.args, url);
 + argv_array_pushf(svndump_proc.args, -r%u:HEAD, startrev);
  
   code = start_command(svndump_proc);
   if (code)
 - die(Unable to start %s, code %d, 
 svndump_proc.argv[0], code);
 + die(Unable to start %s, code %d, command, code);
   dumpin_fd = svndump_proc.out;
   }
   /* setup marks file import/export */
 @@ -226,8 +226,7 @@ static int cmd_import(const char *line)
   if (!dump_from_file) {
   code = finish_command(svndump_proc);
   if (code)
 - warning(%s, returned %d, svndump_proc.argv[0], code);
 - argv_array_clear(svndump_argv);
 + warning(%s, returned %d, command, code);
   }
  
   return 0;
--
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 v2] remote-testsvn: use internal argv_array of struct child_process in cmd_import()

2014-07-18 Thread René Scharfe 

Am 18.07.2014 23:18, schrieb Junio C Hamano:

René Scharfe l@web.de writes:


Use the existing argv_array member instead of providing our own.  This
way we don't have to initialize or clean it up explicitly.  Because of
that automatic cleanup, we need to keep our own reference to the
command name instead of using .argv[0] to print the warning at the end.

Signed-off-by: Rene Scharfe l@web.de
---
The added command pointer makes the patch more complicated, but I think
it still counts as a cleanup.


Surely.  I'd move the svnrdump assignment to where the variable is
defined, though; we do not switch what command to run depending on
some computed conditions anyway.


OK; I see you already did that in pu, thanks.  My point was to allow the 
compiler to warn if the variable command was used in the case 
start_command was not actually called.  That's probably too subtle to be 
useful.






  remote-testsvn.c | 17 -
  1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/remote-testsvn.c b/remote-testsvn.c
index 6be55cb..e3ad11b 100644
--- a/remote-testsvn.c
+++ b/remote-testsvn.c
@@ -175,8 +175,8 @@ static int cmd_import(const char *line)
char *note_msg;
unsigned char head_sha1[20];
unsigned int startrev;
-   struct argv_array svndump_argv = ARGV_ARRAY_INIT;
struct child_process svndump_proc;
+   const char *command;

if (read_ref(private_ref, head_sha1))
startrev = 0;
@@ -200,17 +200,17 @@ static int cmd_import(const char *line)
if(dumpin_fd  0)
die_errno(Couldn't open svn dump file %s., url);
} else {
+   command = svnrdump;
memset(svndump_proc, 0, sizeof(struct child_process));
svndump_proc.out = -1;
-   argv_array_push(svndump_argv, svnrdump);
-   argv_array_push(svndump_argv, dump);
-   argv_array_push(svndump_argv, url);
-   argv_array_pushf(svndump_argv, -r%u:HEAD, startrev);
-   svndump_proc.argv = svndump_argv.argv;
+   argv_array_push(svndump_proc.args, command);
+   argv_array_push(svndump_proc.args, dump);
+   argv_array_push(svndump_proc.args, url);
+   argv_array_pushf(svndump_proc.args, -r%u:HEAD, startrev);

code = start_command(svndump_proc);
if (code)
-   die(Unable to start %s, code %d, 
svndump_proc.argv[0], code);
+   die(Unable to start %s, code %d, command, code);
dumpin_fd = svndump_proc.out;
}
/* setup marks file import/export */
@@ -226,8 +226,7 @@ static int cmd_import(const char *line)
if (!dump_from_file) {
code = finish_command(svndump_proc);
if (code)
-   warning(%s, returned %d, svndump_proc.argv[0], code);
-   argv_array_clear(svndump_argv);
+   warning(%s, returned %d, command, code);
}

return 0;

--
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