remote-testsvn: use internal argv_array of struct child_process in cmd_import()

PATCH In review René Scharfe 2 participants 6 messages Jun 15, 2016

Share & tools

René Scharfe

author

2016-06-15 23:01 UTC #0 lore raw

PATCH 1/1 remote-testsvn: use internal argv_array of struct child_process in cmd_import()

Use the existing argv_array member instead of providing our own. This way we don't have to initialize or clean it up explicitly.

Signed-off-by Rene Scharfe <[email protected]>

---
 remote-testsvn.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

remote-testsvn.c +4 -7

index 6be55cb..31415bd 100644
@@ -175,7 +175,6 @@ 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;
 
 	if (read_ref(private_ref, head_sha1))

@@ -202,11 +201,10 @@ static int cmd_import(const char *line)
 	} else {
 		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, "svnrdump");
+		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)

@@ -227,7 +225,6 @@ static int cmd_import(const char *line)
 		code = finish_command(&svndump_proc);
 		if (code)
 			warning("%s, returned %d", svndump_proc.argv[0], code);
-		argv_array_clear(&svndump_argv);
 	}
 
 	return 0;

Signature · 1 line

2.0.0

Junio C Hamano

2016-06-15 23:01 UTC #1 lore raw

Quoted 5 lines · René Scharfe

Use the existing argv_array member instead of providing our own. This way we don't have to initialize or clean it up explicitly.
Signed-off-by Rene Scharfe <[email protected]>
---

The change below looks so trivial and I cannot offhand see why it would break t9020 in a reproducible way.

Puzzled...

Quoted 39 lines

 remote-testsvn.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

remote-testsvn.c +4 -7

index 6be55cb..31415bd 100644
@@ -175,7 +175,6 @@ 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;
 
 	if (read_ref(private_ref, head_sha1))

@@ -202,11 +201,10 @@ static int cmd_import(const char *line)
 	} else {
 		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, "svnrdump");
+		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)

@@ -227,7 +225,6 @@ static int cmd_import(const char *line)
 		code = finish_command(&svndump_proc);
 		if (code)
 			warning("%s, returned %d", svndump_proc.argv[0], code);
-		argv_array_clear(&svndump_argv);
 	}
 
 	return 0;

René Scharfe

author

2016-06-15 23:01 UTC #2 lore raw

Am 18.07.2014 21:10, schrieb Junio C Hamano:

Quoted 42 lines

Quoted 5 lines · René Scharfe

Use the existing argv_array member instead of providing our own. This way we don't have to initialize or clean it up explicitly.
Signed-off-by Rene Scharfe <[email protected]>
---

The change below looks so trivial and I cannot offhand see why it would break t9020 in a reproducible way.

Puzzled...

Quoted 29 lines

  remote-testsvn.c | 11 ++++-------
  1 file changed, 4 insertions(+), 7 deletions(-)

remote-testsvn.c +4 -6

index 6be55cb..31415bd 100644
@@ -175,7 +175,6 @@ 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;
 
  	if (read_ref(private_ref, head_sha1))

@@ -202,11 +201,10 @@ static int cmd_import(const char *line)
  	} else {
  		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, "svnrdump");
+		argv_array_push(&svndump_proc.args, "dump");
+		argv_array_push(&svndump_proc.args, url);
+		argv_array_pushf(&svndump_proc.args, "-r%u:HEAD", startrev);

Quoted 7 lines

remote-testsvn.c static int cmd_import(const char *line) · line 230

Quoted 7 lines

remote-testsvn.c +0 -1

  		code = start_command(&svndump_proc);
  		if (code)

@@ -227,7 +225,6 @@ static int cmd_import(const char *line)
  		code = finish_command(&svndump_proc);
  		if (code)
  			warning("%s, returned %d", svndump_proc.argv[0], code);
-		argv_array_clear(&svndump_argv);

Unfortunately I don't get a test failure, but I think I see what's
wrong: The warning line above references the argv array after it was freed by finish_command. Ouch. Fixup below:

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

remote-testsvn.c +5 -3

index 31415bd..e3ad11b 100644
@@ -176,6 +176,7 @@ static int cmd_import(const char *line)
 	unsigned char head_sha1[20];
 	unsigned int startrev;
 	struct child_process svndump_proc;
+	const char *command;
 
 	if (read_ref(private_ref, head_sha1))
 		startrev = 0;

@@ -199,16 +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_proc.args, "svnrdump");
+		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 */

@@ -224,7 +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);
+			warning("%s, returned %d", command, code);
 	}
 
 	return 0;

Signature · 1 line

2.0.2

René Scharfe

author

2016-06-15 23:01 UTC #3 lore raw

PATCH v2 1/1 remote-testsvn: use internal argv_array of struct child_process in cmd_import()

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 <[email protected]>

---
The added command pointer makes the patch more complicated, but I think it still counts as a cleanup.

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

remote-testsvn.c +8 -9

index 6be55cb..e3ad11b 100644
@@ -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;

Signature · 1 line

2.0.2

Junio C Hamano

2016-06-15 23:01 UTC #4 lore raw

Quoted 9 lines · René Scharfe

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 <[email protected]>
---
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.

Quoted 52 lines

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

remote-testsvn.c +8 -9

index 6be55cb..e3ad11b 100644
@@ -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;

René Scharfe

author

2016-06-15 23:01 UTC #5 lore raw

Am 18.07.2014 23:18, schrieb Junio C Hamano:

Quoted 7 lines

Quoted 5 lines · René Scharfe
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.

Quoted 8 lines

Quoted 4 lines
Signed-off-by Rene Scharfe <[email protected]>
---
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.

Quoted 53 lines


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

remote-testsvn.c +8 -9

index 6be55cb..e3ad11b 100644
@@ -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;