{"thread":{"id":"33659","subject":"[PATCH] send-email: improve recipients_cmd()","startedAt":"2013-04-27T22:26:01Z","lastAt":"2013-04-29T08:15:56Z","messageCount":6,"participants":["Felipe Contreras","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"215767","messageId":"1367101561-14671-1-git-send-email-felipe.contreras@gmail.com","threadId":"33659","inReplyTo":null,"subject":"[PATCH] send-email: improve recipients_cmd()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-27T22:26:01Z","receivedAt":"2013-04-27T22:26:01Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"We don't need to quote the filename to pass to the command, we can use\nan array of all the arguments to pass to the command, which is safer,\nand more extensible.\n\nCommit a47eab0 (send-email: use the three-arg form of open in\nrecipients_cmd) stated we couldn't pass $file directly, but in fact, we\ncan, the multi-word string is passed as is, and we can pass an array\ntoo.\n\nSigned-off-by: Felipe Contreras \n---\n git-send-email.perl | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex bd13cc8..7880d12 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1452,11 +1452,11 @@ foreach my $t (@files) {\n # Execute a command (e.g. $to_cmd) to get a list of email addresses\n # and return a results array\n sub recipients_cmd {\n-\tmy ($prefix, $what, $cmd, $file) = @_;\n+\tmy ($prefix, $what, $cmd, @args) = @_;\n \n \tmy $sanitized_sender = sanitize_address($sender);\n \tmy @addresses = ();\n-\topen my $fh, \"-|\", \"$cmd \\Q$file\\E\"\n+\topen my $fh, \"-|\", $cmd, @args\n \t or die \"($prefix) Could not execute '$cmd'\";\n \twhile (my $address = <$fh>) {\n \t\t$address =~ s/^\\s*//g;\n-- \n1.8.2.1.1031.g2ee5873\n"},{"id":"215801","messageId":"7vbo8yqxas.fsf@alter.siamese.dyndns.org","threadId":"33659","inReplyTo":"1367101561-14671-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH] send-email: improve recipients_cmd()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-28T19:18:03Z","receivedAt":"2013-04-28T19:18:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> We don't need to quote the filename to pass to the command, we can use\n> an array of all the arguments to pass to the command, which is safer,\n> and more extensible.\n>\n> Commit a47eab0 (send-email: use the three-arg form of open in\n> recipients_cmd) stated we couldn't pass $file directly, but in fact, we\n> can, the multi-word string is passed as is, and we can pass an array\n> too.\n\nI think the comment is not about passing $file directly, but is\nabout passing $cmd that could be multi-word string directly. The\ncaller expects it be split into command and its earlier part of\ncommand line parameters, so that you can say\n\n $cmd = \"cccmd --frotz --nitfol\"\n\nbut the non-string form of open would not give you that, unless you\nrewrite it to\n\n\topen $fh, \"-|\", qw(sh -c), $cmd, @args\n\nor something, no?\n\n> Signed-off-by: Felipe Contreras \n> ---\n> git-send-email.perl | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index bd13cc8..7880d12 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -1452,11 +1452,11 @@ foreach my $t (@files) {\n> # Execute a command (e.g. $to_cmd) to get a list of email addresses\n> # and return a results array\n> sub recipients_cmd {\n> -\tmy ($prefix, $what, $cmd, $file) = @_;\n> +\tmy ($prefix, $what, $cmd, @args) = @_;\n> \n> \tmy $sanitized_sender = sanitize_address($sender);\n> \tmy @addresses = ();\n> -\topen my $fh, \"-|\", \"$cmd \\Q$file\\E\"\n> +\topen my $fh, \"-|\", $cmd, @args\n> \t or die \"($prefix) Could not execute '$cmd'\";\n> \twhile (my $address = <$fh>) {\n> \t\t$address =~ s/^\\s*//g;\n"},{"id":"215824","messageId":"CAMP44s0x8fK-6vbY20eKA9QjWqhiOCvx+KFOgKOZ8q58bwk8Tw@mail.gmail.com","threadId":"33659","inReplyTo":"7vbo8yqxas.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] send-email: improve recipients_cmd()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-29T05:14:52Z","receivedAt":"2013-04-29T05:14:52Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Apr 28, 2013 at 2:18 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> We don't need to quote the filename to pass to the command, we can use\n>> an array of all the arguments to pass to the command, which is safer,\n>> and more extensible.\n>>\n>> Commit a47eab0 (send-email: use the three-arg form of open in\n>> recipients_cmd) stated we couldn't pass $file directly, but in fact, we\n>> can, the multi-word string is passed as is, and we can pass an array\n>> too.\n>\n> I think the comment is not about passing $file directly, but is\n> about passing $cmd that could be multi-word string directly. The\n> caller expects it be split into command and its earlier part of\n> command line parameters, so that you can say\n>\n> $cmd = \"cccmd --frotz --nitfol\"\n\nI see.\n\n> but the non-string form of open would not give you that, unless you\n> rewrite it to\n>\n> open $fh, \"-|\", qw(sh -c), $cmd, @args\n\nThat doesn't seem to work for me.\n\nIt would have to be:\n\n open $fh, \"-|\", qw(sh -c), \"$cmd \\Q$args\\E\"\n\nSo we end up in the same place.\n\n-- \nFelipe Contreras\n"},{"id":"215826","messageId":"7vd2tdoqna.fsf@alter.siamese.dyndns.org","threadId":"33659","inReplyTo":"CAMP44s0x8fK-6vbY20eKA9QjWqhiOCvx+KFOgKOZ8q58bwk8Tw@mail.gmail.com","subject":"Re: [PATCH] send-email: improve recipients_cmd()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-29T05:24:41Z","receivedAt":"2013-04-29T05:24:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n>> open $fh, \"-|\", qw(sh -c), $cmd, @args\n>\n> That doesn't seem to work for me.\n\nMy fault. It needs to form a command line like this:\n\n\tsh -c 'cccmd --frotz --nitfol \"$@\"' - a r g s\n\n[jc: goes and tries\n\t$ sh -c 'echo X Y \"$@\"' - a r g s\n\tX Y a r g s\n]\n\nso if we want to get rid of \\Q\\E, it would be:\n\n\topen $fh, '-|', qw(sh -c), \"$cmd \" . '\"$@\"', '-', @args\n"},{"id":"215828","messageId":"CAMP44s3QkiQhYr7pKHgKFoXH5m9_ZHLHmwC4c9WC2OGjv7LHdg@mail.gmail.com","threadId":"33659","inReplyTo":"7vd2tdoqna.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] send-email: improve recipients_cmd()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-29T05:32:09Z","receivedAt":"2013-04-29T05:32:09Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Apr 29, 2013 at 12:24 AM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>>> open $fh, \"-|\", qw(sh -c), $cmd, @args\n>>\n>> That doesn't seem to work for me.\n>\n> My fault. It needs to form a command line like this:\n>\n> sh -c 'cccmd --frotz --nitfol \"$@\"' - a r g s\n>\n> [jc: goes and tries\n> $ sh -c 'echo X Y \"$@\"' - a r g s\n> X Y a r g s\n> ]\n>\n> so if we want to get rid of \\Q\\E, it would be:\n>\n> open $fh, '-|', qw(sh -c), \"$cmd \" . '\"$@\"', '-', @args\n>\n\nI don't know if that would be better, or converting @args to a list of\nquoted strings, essentially keeping the current behavior.\n\n\n-- \nFelipe Contreras\n"},{"id":"215835","messageId":"7vhaipn45f.fsf@alter.siamese.dyndns.org","threadId":"33659","inReplyTo":"CAMP44s3QkiQhYr7pKHgKFoXH5m9_ZHLHmwC4c9WC2OGjv7LHdg@mail.gmail.com","subject":"Re: [PATCH] send-email: improve recipients_cmd()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-29T08:15:56Z","receivedAt":"2013-04-29T08:15:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n>> so if we want to get rid of \\Q\\E, it would be:\n>>\n>> open $fh, '-|', qw(sh -c), \"$cmd \" . '\"$@\"', '-', @args\n>>\n>\n> I don't know if that would be better, or converting @args to a list of\n> quoted strings, essentially keeping the current behavior.\n\nI tend to agree, so let's drop it for now.\n"}]}