{"thread":{"id":"37163","subject":"[PATCH] remote-testsvn: use internal argv_array of struct child_process in cmd_import()","startedAt":"2014-07-18T15:20:19Z","lastAt":"2014-07-18T21:45:50Z","messageCount":6,"participants":["René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"246314","messageId":"53C93B33.5070006@web.de","threadId":"37163","inReplyTo":null,"subject":"[PATCH] remote-testsvn: use internal argv_array of struct child_process in cmd_import()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-07-18T15:20:19Z","receivedAt":"2014-07-18T15:20:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use the existing argv_array member instead of providing our own.  This\nway we don't have to initialize or clean it up explicitly.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n remote-testsvn.c | 11 ++++-------\n 1 file changed, 4 insertions(+), 7 deletions(-)\n\ndiff --git a/remote-testsvn.c b/remote-testsvn.c\nindex 6be55cb..31415bd 100644\n--- a/remote-testsvn.c\n+++ b/remote-testsvn.c\n@@ -175,7 +175,6 @@ static int cmd_import(const char *line)\n \tchar *note_msg;\n \tunsigned char head_sha1[20];\n \tunsigned int startrev;\n-\tstruct argv_array svndump_argv = ARGV_ARRAY_INIT;\n \tstruct child_process svndump_proc;\n \n \tif (read_ref(private_ref, head_sha1))\n@@ -202,11 +201,10 @@ static int cmd_import(const char *line)\n \t} else {\n \t\tmemset(&svndump_proc, 0, sizeof(struct child_process));\n \t\tsvndump_proc.out = -1;\n-\t\targv_array_push(&svndump_argv, \"svnrdump\");\n-\t\targv_array_push(&svndump_argv, \"dump\");\n-\t\targv_array_push(&svndump_argv, url);\n-\t\targv_array_pushf(&svndump_argv, \"-r%u:HEAD\", startrev);\n-\t\tsvndump_proc.argv = svndump_argv.argv;\n+\t\targv_array_push(&svndump_proc.args, \"svnrdump\");\n+\t\targv_array_push(&svndump_proc.args, \"dump\");\n+\t\targv_array_push(&svndump_proc.args, url);\n+\t\targv_array_pushf(&svndump_proc.args, \"-r%u:HEAD\", startrev);\n \n \t\tcode = start_command(&svndump_proc);\n \t\tif (code)\n@@ -227,7 +225,6 @@ static int cmd_import(const char *line)\n \t\tcode = finish_command(&svndump_proc);\n \t\tif (code)\n \t\t\twarning(\"%s, returned %d\", svndump_proc.argv[0], code);\n-\t\targv_array_clear(&svndump_argv);\n \t}\n \n \treturn 0;\n-- \n2.0.0\n"},{"id":"246330","messageId":"xmqq8unqmqk7.fsf@gitster.dls.corp.google.com","threadId":"37163","inReplyTo":"53C93B33.5070006@web.de","subject":"Re: [PATCH] remote-testsvn: use internal argv_array of struct child_process in cmd_import()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-18T19:10:00Z","receivedAt":"2014-07-18T19:10:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Use the existing argv_array member instead of providing our own.  This\n> way we don't have to initialize or clean it up explicitly.\n>\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n\nThe change below looks so trivial and I cannot offhand see why it\nwould break t9020 in a reproducible way.\n\nPuzzled...\n\n>  remote-testsvn.c | 11 ++++-------\n>  1 file changed, 4 insertions(+), 7 deletions(-)\n>\n> diff --git a/remote-testsvn.c b/remote-testsvn.c\n> index 6be55cb..31415bd 100644\n> --- a/remote-testsvn.c\n> +++ b/remote-testsvn.c\n> @@ -175,7 +175,6 @@ static int cmd_import(const char *line)\n>  \tchar *note_msg;\n>  \tunsigned char head_sha1[20];\n>  \tunsigned int startrev;\n> -\tstruct argv_array svndump_argv = ARGV_ARRAY_INIT;\n>  \tstruct child_process svndump_proc;\n>  \n>  \tif (read_ref(private_ref, head_sha1))\n> @@ -202,11 +201,10 @@ static int cmd_import(const char *line)\n>  \t} else {\n>  \t\tmemset(&svndump_proc, 0, sizeof(struct child_process));\n>  \t\tsvndump_proc.out = -1;\n> -\t\targv_array_push(&svndump_argv, \"svnrdump\");\n> -\t\targv_array_push(&svndump_argv, \"dump\");\n> -\t\targv_array_push(&svndump_argv, url);\n> -\t\targv_array_pushf(&svndump_argv, \"-r%u:HEAD\", startrev);\n> -\t\tsvndump_proc.argv = svndump_argv.argv;\n> +\t\targv_array_push(&svndump_proc.args, \"svnrdump\");\n> +\t\targv_array_push(&svndump_proc.args, \"dump\");\n> +\t\targv_array_push(&svndump_proc.args, url);\n> +\t\targv_array_pushf(&svndump_proc.args, \"-r%u:HEAD\", startrev);\n>  \n>  \t\tcode = start_command(&svndump_proc);\n>  \t\tif (code)\n> @@ -227,7 +225,6 @@ static int cmd_import(const char *line)\n>  \t\tcode = finish_command(&svndump_proc);\n>  \t\tif (code)\n>  \t\t\twarning(\"%s, returned %d\", svndump_proc.argv[0], code);\n> -\t\targv_array_clear(&svndump_argv);\n>  \t}\n>  \n>  \treturn 0;\n"},{"id":"246333","messageId":"53C975C5.8020709@web.de","threadId":"37163","inReplyTo":"xmqq8unqmqk7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] remote-testsvn: use internal argv_array of struct child_process in cmd_import()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-07-18T19:30:13Z","receivedAt":"2014-07-18T19:30:13Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"\n\nAm 18.07.2014 21:10, schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n> \n>> Use the existing argv_array member instead of providing our own.  This\n>> way we don't have to initialize or clean it up explicitly.\n>>\n>> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n>> ---\n> \n> The change below looks so trivial and I cannot offhand see why it\n> would break t9020 in a reproducible way.\n> \n> Puzzled...\n> \n>>   remote-testsvn.c | 11 ++++-------\n>>   1 file changed, 4 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/remote-testsvn.c b/remote-testsvn.c\n>> index 6be55cb..31415bd 100644\n>> --- a/remote-testsvn.c\n>> +++ b/remote-testsvn.c\n>> @@ -175,7 +175,6 @@ static int cmd_import(const char *line)\n>>   \tchar *note_msg;\n>>   \tunsigned char head_sha1[20];\n>>   \tunsigned int startrev;\n>> -\tstruct argv_array svndump_argv = ARGV_ARRAY_INIT;\n>>   \tstruct child_process svndump_proc;\n>>   \n>>   \tif (read_ref(private_ref, head_sha1))\n>> @@ -202,11 +201,10 @@ static int cmd_import(const char *line)\n>>   \t} else {\n>>   \t\tmemset(&svndump_proc, 0, sizeof(struct child_process));\n>>   \t\tsvndump_proc.out = -1;\n>> -\t\targv_array_push(&svndump_argv, \"svnrdump\");\n>> -\t\targv_array_push(&svndump_argv, \"dump\");\n>> -\t\targv_array_push(&svndump_argv, url);\n>> -\t\targv_array_pushf(&svndump_argv, \"-r%u:HEAD\", startrev);\n>> -\t\tsvndump_proc.argv = svndump_argv.argv;\n>> +\t\targv_array_push(&svndump_proc.args, \"svnrdump\");\n>> +\t\targv_array_push(&svndump_proc.args, \"dump\");\n>> +\t\targv_array_push(&svndump_proc.args, url);\n>> +\t\targv_array_pushf(&svndump_proc.args, \"-r%u:HEAD\", startrev);\n>>   \n>>   \t\tcode = start_command(&svndump_proc);\n>>   \t\tif (code)\n>> @@ -227,7 +225,6 @@ static int cmd_import(const char *line)\n>>   \t\tcode = finish_command(&svndump_proc);\n>>   \t\tif (code)\n>>   \t\t\twarning(\"%s, returned %d\", svndump_proc.argv[0], code);\n>> -\t\targv_array_clear(&svndump_argv);\n\nUnfortunately I don't get a test failure, but I think I see what's\nwrong: The warning line above references the argv array after it was\nfreed by finish_command.  Ouch.  Fixup below:\n\n---\n remote-testsvn.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/remote-testsvn.c b/remote-testsvn.c\nindex 31415bd..e3ad11b 100644\n--- a/remote-testsvn.c\n+++ b/remote-testsvn.c\n@@ -176,6 +176,7 @@ static int cmd_import(const char *line)\n \tunsigned char head_sha1[20];\n \tunsigned int startrev;\n \tstruct child_process svndump_proc;\n+\tconst char *command;\n \n \tif (read_ref(private_ref, head_sha1))\n \t\tstartrev = 0;\n@@ -199,16 +200,17 @@ static int cmd_import(const char *line)\n \t\tif(dumpin_fd < 0)\n \t\t\tdie_errno(\"Couldn't open svn dump file %s.\", url);\n \t} else {\n+\t\tcommand = \"svnrdump\";\n \t\tmemset(&svndump_proc, 0, sizeof(struct child_process));\n \t\tsvndump_proc.out = -1;\n-\t\targv_array_push(&svndump_proc.args, \"svnrdump\");\n+\t\targv_array_push(&svndump_proc.args, command);\n \t\targv_array_push(&svndump_proc.args, \"dump\");\n \t\targv_array_push(&svndump_proc.args, url);\n \t\targv_array_pushf(&svndump_proc.args, \"-r%u:HEAD\", startrev);\n \n \t\tcode = start_command(&svndump_proc);\n \t\tif (code)\n-\t\t\tdie(\"Unable to start %s, code %d\", svndump_proc.argv[0], code);\n+\t\t\tdie(\"Unable to start %s, code %d\", command, code);\n \t\tdumpin_fd = svndump_proc.out;\n \t}\n \t/* setup marks file import/export */\n@@ -224,7 +226,7 @@ static int cmd_import(const char *line)\n \tif (!dump_from_file) {\n \t\tcode = finish_command(&svndump_proc);\n \t\tif (code)\n-\t\t\twarning(\"%s, returned %d\", svndump_proc.argv[0], code);\n+\t\t\twarning(\"%s, returned %d\", command, code);\n \t}\n \n \treturn 0;\n-- \n2.0.2\n"},{"id":"246335","messageId":"53C97BA4.7020503@web.de","threadId":"37163","inReplyTo":"53C975C5.8020709@web.de","subject":"[PATCH v2] remote-testsvn: use internal argv_array of struct child_process in cmd_import()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-07-18T19:55:16Z","receivedAt":"2014-07-18T19:55:16Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use the existing argv_array member instead of providing our own.  This\nway we don't have to initialize or clean it up explicitly.  Because of\nthat automatic cleanup, we need to keep our own reference to the\ncommand name instead of using .argv[0] to print the warning at the end.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\nThe added command pointer makes the patch more complicated, but I think\nit still counts as a cleanup.\n\n remote-testsvn.c | 17 ++++++++---------\n 1 file changed, 8 insertions(+), 9 deletions(-)\n\ndiff --git a/remote-testsvn.c b/remote-testsvn.c\nindex 6be55cb..e3ad11b 100644\n--- a/remote-testsvn.c\n+++ b/remote-testsvn.c\n@@ -175,8 +175,8 @@ static int cmd_import(const char *line)\n \tchar *note_msg;\n \tunsigned char head_sha1[20];\n \tunsigned int startrev;\n-\tstruct argv_array svndump_argv = ARGV_ARRAY_INIT;\n \tstruct child_process svndump_proc;\n+\tconst char *command;\n \n \tif (read_ref(private_ref, head_sha1))\n \t\tstartrev = 0;\n@@ -200,17 +200,17 @@ static int cmd_import(const char *line)\n \t\tif(dumpin_fd < 0)\n \t\t\tdie_errno(\"Couldn't open svn dump file %s.\", url);\n \t} else {\n+\t\tcommand = \"svnrdump\";\n \t\tmemset(&svndump_proc, 0, sizeof(struct child_process));\n \t\tsvndump_proc.out = -1;\n-\t\targv_array_push(&svndump_argv, \"svnrdump\");\n-\t\targv_array_push(&svndump_argv, \"dump\");\n-\t\targv_array_push(&svndump_argv, url);\n-\t\targv_array_pushf(&svndump_argv, \"-r%u:HEAD\", startrev);\n-\t\tsvndump_proc.argv = svndump_argv.argv;\n+\t\targv_array_push(&svndump_proc.args, command);\n+\t\targv_array_push(&svndump_proc.args, \"dump\");\n+\t\targv_array_push(&svndump_proc.args, url);\n+\t\targv_array_pushf(&svndump_proc.args, \"-r%u:HEAD\", startrev);\n \n \t\tcode = start_command(&svndump_proc);\n \t\tif (code)\n-\t\t\tdie(\"Unable to start %s, code %d\", svndump_proc.argv[0], code);\n+\t\t\tdie(\"Unable to start %s, code %d\", command, code);\n \t\tdumpin_fd = svndump_proc.out;\n \t}\n \t/* setup marks file import/export */\n@@ -226,8 +226,7 @@ static int cmd_import(const char *line)\n \tif (!dump_from_file) {\n \t\tcode = finish_command(&svndump_proc);\n \t\tif (code)\n-\t\t\twarning(\"%s, returned %d\", svndump_proc.argv[0], code);\n-\t\targv_array_clear(&svndump_argv);\n+\t\t\twarning(\"%s, returned %d\", command, code);\n \t}\n \n \treturn 0;\n-- \n2.0.2\n"},{"id":"246339","messageId":"xmqqtx6el619.fsf@gitster.dls.corp.google.com","threadId":"37163","inReplyTo":"53C97BA4.7020503@web.de","subject":"Re: [PATCH v2] remote-testsvn: use internal argv_array of struct child_process in cmd_import()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-18T21:18:42Z","receivedAt":"2014-07-18T21:18:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Use the existing argv_array member instead of providing our own.  This\n> way we don't have to initialize or clean it up explicitly.  Because of\n> that automatic cleanup, we need to keep our own reference to the\n> command name instead of using .argv[0] to print the warning at the end.\n>\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n> The added command pointer makes the patch more complicated, but I think\n> it still counts as a cleanup.\n\nSurely.  I'd move the \"svnrdump\" assignment to where the variable is\ndefined, though; we do not switch what \"command\" to run depending on\nsome computed conditions anyway.\n\n>\n>  remote-testsvn.c | 17 ++++++++---------\n>  1 file changed, 8 insertions(+), 9 deletions(-)\n>\n> diff --git a/remote-testsvn.c b/remote-testsvn.c\n> index 6be55cb..e3ad11b 100644\n> --- a/remote-testsvn.c\n> +++ b/remote-testsvn.c\n> @@ -175,8 +175,8 @@ static int cmd_import(const char *line)\n>  \tchar *note_msg;\n>  \tunsigned char head_sha1[20];\n>  \tunsigned int startrev;\n> -\tstruct argv_array svndump_argv = ARGV_ARRAY_INIT;\n>  \tstruct child_process svndump_proc;\n> +\tconst char *command;\n>  \n>  \tif (read_ref(private_ref, head_sha1))\n>  \t\tstartrev = 0;\n> @@ -200,17 +200,17 @@ static int cmd_import(const char *line)\n>  \t\tif(dumpin_fd < 0)\n>  \t\t\tdie_errno(\"Couldn't open svn dump file %s.\", url);\n>  \t} else {\n> +\t\tcommand = \"svnrdump\";\n>  \t\tmemset(&svndump_proc, 0, sizeof(struct child_process));\n>  \t\tsvndump_proc.out = -1;\n> -\t\targv_array_push(&svndump_argv, \"svnrdump\");\n> -\t\targv_array_push(&svndump_argv, \"dump\");\n> -\t\targv_array_push(&svndump_argv, url);\n> -\t\targv_array_pushf(&svndump_argv, \"-r%u:HEAD\", startrev);\n> -\t\tsvndump_proc.argv = svndump_argv.argv;\n> +\t\targv_array_push(&svndump_proc.args, command);\n> +\t\targv_array_push(&svndump_proc.args, \"dump\");\n> +\t\targv_array_push(&svndump_proc.args, url);\n> +\t\targv_array_pushf(&svndump_proc.args, \"-r%u:HEAD\", startrev);\n>  \n>  \t\tcode = start_command(&svndump_proc);\n>  \t\tif (code)\n> -\t\t\tdie(\"Unable to start %s, code %d\", svndump_proc.argv[0], code);\n> +\t\t\tdie(\"Unable to start %s, code %d\", command, code);\n>  \t\tdumpin_fd = svndump_proc.out;\n>  \t}\n>  \t/* setup marks file import/export */\n> @@ -226,8 +226,7 @@ static int cmd_import(const char *line)\n>  \tif (!dump_from_file) {\n>  \t\tcode = finish_command(&svndump_proc);\n>  \t\tif (code)\n> -\t\t\twarning(\"%s, returned %d\", svndump_proc.argv[0], code);\n> -\t\targv_array_clear(&svndump_argv);\n> +\t\t\twarning(\"%s, returned %d\", command, code);\n>  \t}\n>  \n>  \treturn 0;\n"},{"id":"246340","messageId":"53C9958E.8080105@web.de","threadId":"37163","inReplyTo":"xmqqtx6el619.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] remote-testsvn: use internal argv_array of struct child_process in cmd_import()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-07-18T21:45:50Z","receivedAt":"2014-07-18T21:45:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 18.07.2014 23:18, schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>\n>> Use the existing argv_array member instead of providing our own.  This\n>> way we don't have to initialize or clean it up explicitly.  Because of\n>> that automatic cleanup, we need to keep our own reference to the\n>> command name instead of using .argv[0] to print the warning at the end.\n>>\n>> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n>> ---\n>> The added command pointer makes the patch more complicated, but I think\n>> it still counts as a cleanup.\n>\n> Surely.  I'd move the \"svnrdump\" assignment to where the variable is\n> defined, though; we do not switch what \"command\" to run depending on\n> some computed conditions anyway.\n\nOK; I see you already did that in pu, thanks.  My point was to allow the \ncompiler to warn if the variable command was used in the case \nstart_command was not actually called.  That's probably too subtle to be \nuseful.\n\n>\n>>\n>>   remote-testsvn.c | 17 ++++++++---------\n>>   1 file changed, 8 insertions(+), 9 deletions(-)\n>>\n>> diff --git a/remote-testsvn.c b/remote-testsvn.c\n>> index 6be55cb..e3ad11b 100644\n>> --- a/remote-testsvn.c\n>> +++ b/remote-testsvn.c\n>> @@ -175,8 +175,8 @@ static int cmd_import(const char *line)\n>>   \tchar *note_msg;\n>>   \tunsigned char head_sha1[20];\n>>   \tunsigned int startrev;\n>> -\tstruct argv_array svndump_argv = ARGV_ARRAY_INIT;\n>>   \tstruct child_process svndump_proc;\n>> +\tconst char *command;\n>>\n>>   \tif (read_ref(private_ref, head_sha1))\n>>   \t\tstartrev = 0;\n>> @@ -200,17 +200,17 @@ static int cmd_import(const char *line)\n>>   \t\tif(dumpin_fd < 0)\n>>   \t\t\tdie_errno(\"Couldn't open svn dump file %s.\", url);\n>>   \t} else {\n>> +\t\tcommand = \"svnrdump\";\n>>   \t\tmemset(&svndump_proc, 0, sizeof(struct child_process));\n>>   \t\tsvndump_proc.out = -1;\n>> -\t\targv_array_push(&svndump_argv, \"svnrdump\");\n>> -\t\targv_array_push(&svndump_argv, \"dump\");\n>> -\t\targv_array_push(&svndump_argv, url);\n>> -\t\targv_array_pushf(&svndump_argv, \"-r%u:HEAD\", startrev);\n>> -\t\tsvndump_proc.argv = svndump_argv.argv;\n>> +\t\targv_array_push(&svndump_proc.args, command);\n>> +\t\targv_array_push(&svndump_proc.args, \"dump\");\n>> +\t\targv_array_push(&svndump_proc.args, url);\n>> +\t\targv_array_pushf(&svndump_proc.args, \"-r%u:HEAD\", startrev);\n>>\n>>   \t\tcode = start_command(&svndump_proc);\n>>   \t\tif (code)\n>> -\t\t\tdie(\"Unable to start %s, code %d\", svndump_proc.argv[0], code);\n>> +\t\t\tdie(\"Unable to start %s, code %d\", command, code);\n>>   \t\tdumpin_fd = svndump_proc.out;\n>>   \t}\n>>   \t/* setup marks file import/export */\n>> @@ -226,8 +226,7 @@ static int cmd_import(const char *line)\n>>   \tif (!dump_from_file) {\n>>   \t\tcode = finish_command(&svndump_proc);\n>>   \t\tif (code)\n>> -\t\t\twarning(\"%s, returned %d\", svndump_proc.argv[0], code);\n>> -\t\targv_array_clear(&svndump_argv);\n>> +\t\t\twarning(\"%s, returned %d\", command, code);\n>>   \t}\n>>\n>>   \treturn 0;\n"}]}