{"thread":{"id":"36418","subject":"[PATCH v3] send-email: recognize absolute path on Windows","startedAt":"2014-04-16T08:08:18Z","lastAt":"2014-04-23T09:30:36Z","messageCount":7,"participants":["Erik Faye-Lund","Junio C Hamano","brian m. carlson"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"238917","messageId":"1397635698-6252-1-git-send-email-kusmabite@gmail.com","threadId":"36418","inReplyTo":null,"subject":"[PATCH v3] send-email: recognize absolute path on Windows","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2014-04-16T08:08:18Z","receivedAt":"2014-04-16T08:08:18Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"From: Erik Faye-Lund <kusmabite@googlemail.com>\n\nOn Windows, absolute paths might start with a DOS drive prefix,\nwhich these two checks failed to recognize.\n\nUnfortunately, we cannot simply use the file_name_is_absolute\nhelper in File::Spec::Functions, because Git for Windows has an\nMSYS-based Perl, where this helper doesn't grok DOS\ndrive-prefixes.\n\nSo let's manually check for these in that case, and fall back to\nthe File::Spec-helper on other platforms (e.g Win32 with native\nPerl)\n\nSigned-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n---\n\nSo here's a version that does the old and long-time tested\napproach without requiring breaking changes to msysGit's perl.\n\n git-send-email.perl | 16 ++++++++++++++--\n 1 file changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex fdb0029..8f5f986 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1113,6 +1113,18 @@ sub ssl_verify_params {\n \t}\n }\n \n+sub file_name_is_absolute {\n+\tmy ($path) = @_;\n+\n+\t# msys does not grok DOS drive-prefixes\n+\tif ($^O eq 'msys') {\n+\t\treturn ($path =~ m#^/# || $path =~ m#[a-zA-Z]\\:#)\n+\t}\n+\n+\trequire File::Spec::Functions;\n+\treturn File::Spec::Functions::file_name_is_absolute($path);\n+}\n+\n # Returns 1 if the message was sent, and 0 otherwise.\n # In actuality, the whole program dies when there\n # is an error sending a message.\n@@ -1197,7 +1209,7 @@ X-Mailer: git-send-email $gitversion\n \n \tif ($dry_run) {\n \t\t# We don't want to send the email.\n-\t} elsif ($smtp_server =~ m#^/#) {\n+\t} elsif (file_name_is_absolute($smtp_server)) {\n \t\tmy $pid = open my $sm, '|-';\n \t\tdefined $pid or die $!;\n \t\tif (!$pid) {\n@@ -1271,7 +1283,7 @@ X-Mailer: git-send-email $gitversion\n \t\tprintf (($dry_run ? \"Dry-\" : \"\").\"Sent %s\\n\", $subject);\n \t} else {\n \t\tprint (($dry_run ? \"Dry-\" : \"\").\"OK. Log says:\\n\");\n-\t\tif ($smtp_server !~ m#^/#) {\n+\t\tif (!file_name_is_absolute($smtp_server)) {\n \t\t\tprint \"Server: $smtp_server\\n\";\n \t\t\tprint \"MAIL FROM:<$raw_from>\\n\";\n \t\t\tforeach my $entry (@recipients) {\n-- \n1.9.0.msysgit.0\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/d/optout.\n"},{"id":"238947","messageId":"xmqqfvldi4ue.fsf@gitster.dls.corp.google.com","threadId":"36418","inReplyTo":"1397635698-6252-1-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH v3] send-email: recognize absolute path on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-16T17:03:53Z","receivedAt":"2014-04-16T17:03:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund <kusmabite@gmail.com> writes:\n\n> So let's manually check for these in that case, and fall back to\n> the File::Spec-helper on other platforms (e.g Win32 with native\n> Perl)\n> ...\n> +sub file_name_is_absolute {\n> +\tmy ($path) = @_;\n> +\n> +\t# msys does not grok DOS drive-prefixes\n> +\tif ($^O eq 'msys') {\n> +\t\treturn ($path =~ m#^/# || $path =~ m#[a-zA-Z]\\:#)\n\nShouldn't the latter also be anchored at the beginning of the string\nwith a leading \"^\"?\n\n> +\t}\n> +\n> +\trequire File::Spec::Functions;\n> +\treturn File::Spec::Functions::file_name_is_absolute($path);\n\nWe already \"use File::Spec qw(something else)\" at the beginning, no?\nWhy not throw file_name_is_absolute into that qw() instead?\n\n> +}\n> +\n>  # Returns 1 if the message was sent, and 0 otherwise.\n>  # In actuality, the whole program dies when there\n>  # is an error sending a message.\n> @@ -1197,7 +1209,7 @@ X-Mailer: git-send-email $gitversion\n>  \n>  \tif ($dry_run) {\n>  \t\t# We don't want to send the email.\n> -\t} elsif ($smtp_server =~ m#^/#) {\n> +\t} elsif (file_name_is_absolute($smtp_server)) {\n>  \t\tmy $pid = open my $sm, '|-';\n>  \t\tdefined $pid or die $!;\n>  \t\tif (!$pid) {\n> @@ -1271,7 +1283,7 @@ X-Mailer: git-send-email $gitversion\n>  \t\tprintf (($dry_run ? \"Dry-\" : \"\").\"Sent %s\\n\", $subject);\n>  \t} else {\n>  \t\tprint (($dry_run ? \"Dry-\" : \"\").\"OK. Log says:\\n\");\n> -\t\tif ($smtp_server !~ m#^/#) {\n> +\t\tif (!file_name_is_absolute($smtp_server)) {\n>  \t\t\tprint \"Server: $smtp_server\\n\";\n>  \t\t\tprint \"MAIL FROM:<$raw_from>\\n\";\n>  \t\t\tforeach my $entry (@recipients) {\n> -- \n> 1.9.0.msysgit.0\n>\n> -- \n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/d/optout.\n"},{"id":"238949","messageId":"xmqqbnw1i43p.fsf@gitster.dls.corp.google.com","threadId":"36418","inReplyTo":"xmqqfvldi4ue.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] send-email: recognize absolute path on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-16T17:19:54Z","receivedAt":"2014-04-16T17:19:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Erik Faye-Lund <kusmabite@gmail.com> writes:\n>\n>> So let's manually check for these in that case, and fall back to\n>> the File::Spec-helper on other platforms (e.g Win32 with native\n>> Perl)\n>> ...\n>> +sub file_name_is_absolute {\n>> +\tmy ($path) = @_;\n>> +\n>> +\t# msys does not grok DOS drive-prefixes\n>> +\tif ($^O eq 'msys') {\n>> +\t\treturn ($path =~ m#^/# || $path =~ m#[a-zA-Z]\\:#)\n>\n> Shouldn't the latter also be anchored at the beginning of the string\n> with a leading \"^\"?\n>\n>> +\t}\n>> +\n>> +\trequire File::Spec::Functions;\n>> +\treturn File::Spec::Functions::file_name_is_absolute($path);\n>\n> We already \"use File::Spec qw(something else)\" at the beginning, no?\n> Why not throw file_name_is_absolute into that qw() instead?\n\nAhh, OK, if you did so, you won't have any place to hook the \"only\non msys do this\" trick into.\n\nIt somehow feels somewhat confusing that we define a sub with the\nsame name as the system one, while not overriding it entirely but\ndelegate back to the system one.  I am debating myself if it is more\nobvious if it is done this way:\n\n        use File::Spec::Functions qw(file_name_is_absolute);\n        if ($^O eq 'msys') {\n                sub file_name_is_absolute {\n                \treturn $_[0] =~ /^\\// || $_[0] =~ /^[A-Z]:/i;\n                }\n        }\n\n>>  # Returns 1 if the message was sent, and 0 otherwise.\n>>  # In actuality, the whole program dies when there\n>>  # is an error sending a message.\n>> @@ -1197,7 +1209,7 @@ X-Mailer: git-send-email $gitversion\n>>  \n>>  \tif ($dry_run) {\n>>  \t\t# We don't want to send the email.\n>> -\t} elsif ($smtp_server =~ m#^/#) {\n>> +\t} elsif (file_name_is_absolute($smtp_server)) {\n>>  \t\tmy $pid = open my $sm, '|-';\n>>  \t\tdefined $pid or die $!;\n>>  \t\tif (!$pid) {\n>> @@ -1271,7 +1283,7 @@ X-Mailer: git-send-email $gitversion\n>>  \t\tprintf (($dry_run ? \"Dry-\" : \"\").\"Sent %s\\n\", $subject);\n>>  \t} else {\n>>  \t\tprint (($dry_run ? \"Dry-\" : \"\").\"OK. Log says:\\n\");\n>> -\t\tif ($smtp_server !~ m#^/#) {\n>> +\t\tif (!file_name_is_absolute($smtp_server)) {\n>>  \t\t\tprint \"Server: $smtp_server\\n\";\n>>  \t\t\tprint \"MAIL FROM:<$raw_from>\\n\";\n>>  \t\t\tforeach my $entry (@recipients) {\n>> -- \n>> 1.9.0.msysgit.0\n>>\n>> -- \n>\n> -- \n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/d/optout.\n"},{"id":"238998","messageId":"20140417014532.GA579226@vauxhall.crustytoothpaste.net","threadId":"36418","inReplyTo":"xmqqbnw1i43p.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] send-email: recognize absolute path on Windows","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-04-17T01:45:32Z","receivedAt":"2014-04-17T01:45:32Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Apr 16, 2014 at 10:19:54AM -0700, Junio C Hamano wrote:\n> Ahh, OK, if you did so, you won't have any place to hook the \"only\n> on msys do this\" trick into.\n> \n> It somehow feels somewhat confusing that we define a sub with the\n> same name as the system one, while not overriding it entirely but\n> delegate back to the system one.  I am debating myself if it is more\n> obvious if it is done this way:\n> \n>         use File::Spec::Functions qw(file_name_is_absolute);\n>         if ($^O eq 'msys') {\n\nYou would probably want a \"no warnings 'redefine'\" here as well.\n\n>                 sub file_name_is_absolute {\n>                 \treturn $_[0] =~ /^\\// || $_[0] =~ /^[A-Z]:/i;\n>                 }\n>         }\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"239330","messageId":"CABPQNSbcWjg3nLPD9U114zSk5rBNupOGLr901u4ptCkdiiKvCA@mail.gmail.com","threadId":"36418","inReplyTo":"xmqqbnw1i43p.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] send-email: recognize absolute path on Windows","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2014-04-22T12:15:59Z","receivedAt":"2014-04-22T12:15:59Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Apr 16, 2014 at 7:19 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Erik Faye-Lund <kusmabite@gmail.com> writes:\n>>\n>>> So let's manually check for these in that case, and fall back to\n>>> the File::Spec-helper on other platforms (e.g Win32 with native\n>>> Perl)\n>>> ...\n>>> +sub file_name_is_absolute {\n>>> +    my ($path) = @_;\n>>> +\n>>> +    # msys does not grok DOS drive-prefixes\n>>> +    if ($^O eq 'msys') {\n>>> +            return ($path =~ m#^/# || $path =~ m#[a-zA-Z]\\:#)\n>>\n>> Shouldn't the latter also be anchored at the beginning of the string\n>> with a leading \"^\"?\n>>\n>>> +    }\n>>> +\n>>> +    require File::Spec::Functions;\n>>> +    return File::Spec::Functions::file_name_is_absolute($path);\n>>\n>> We already \"use File::Spec qw(something else)\" at the beginning, no?\n>> Why not throw file_name_is_absolute into that qw() instead?\n>\n> Ahh, OK, if you did so, you won't have any place to hook the \"only\n> on msys do this\" trick into.\n>\n> It somehow feels somewhat confusing that we define a sub with the\n> same name as the system one, while not overriding it entirely but\n> delegate back to the system one.  I am debating myself if it is more\n> obvious if it is done this way:\n>\n>         use File::Spec::Functions qw(file_name_is_absolute);\n>         if ($^O eq 'msys') {\n>                 sub file_name_is_absolute {\n>                         return $_[0] =~ /^\\// || $_[0] =~ /^[A-Z]:/i;\n>                 }\n>         }\n>\n\nIn this case, we end up requiring that module even when we end up\nusing it, no? Not that I have very strong objections for doing just\nthat, after all, it appears to be built-in. (As you might understand\nfrom this message, my perl-fu is really lacking :-P)\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/d/optout.\n"},{"id":"239349","messageId":"xmqqk3ah71g8.fsf@gitster.dls.corp.google.com","threadId":"36418","inReplyTo":"CABPQNSbcWjg3nLPD9U114zSk5rBNupOGLr901u4ptCkdiiKvCA@mail.gmail.com","subject":"Re: [PATCH v3] send-email: recognize absolute path on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-22T16:50:47Z","receivedAt":"2014-04-22T16:50:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund <kusmabite@gmail.com> writes:\n\n>>> Shouldn't the latter also be anchored at the beginning of the string\n>>> with a leading \"^\"?\n>>>\n>>>> +    }\n>>>> +\n>>>> +    require File::Spec::Functions;\n>>>> +    return File::Spec::Functions::file_name_is_absolute($path);\n>>>\n>>> We already \"use File::Spec qw(something else)\" at the beginning, no?\n>>> Why not throw file_name_is_absolute into that qw() instead?\n>>\n>> Ahh, OK, if you did so, you won't have any place to hook the \"only\n>> on msys do this\" trick into.\n>>\n>> It somehow feels somewhat confusing that we define a sub with the\n>> same name as the system one, while not overriding it entirely but\n>> delegate back to the system one.  I am debating myself if it is more\n>> obvious if it is done this way:\n>>\n>>         use File::Spec::Functions qw(file_name_is_absolute);\n>>         if ($^O eq 'msys') {\n>>                 sub file_name_is_absolute {\n>>                         return $_[0] =~ /^\\// || $_[0] =~ /^[A-Z]:/i;\n>>                 }\n>>         }\n>>\n>\n> In this case, we end up requiring that module even when we end up\n> using it, no?\n\nAlso somebody earlier mentioned that we would be redefining, which\nhas a different kind of ugliness, so I'd agree with the code structure\nof what you sent out (which has been queued on 'pu').\n\nMy earlier question \"don't we want to make sure 'C:' is at the\nbetginning of the string?\" still stands, though.  I do not think I\nfutzed with your regexp in the version I queued on 'pu'.\n\nThanks.\n\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/d/optout.\n"},{"id":"239415","messageId":"CABPQNSZ_d0KwEo-P5_Hx5QdV-tzYKMfZQA9SzEj3PD-wed6_bA@mail.gmail.com","threadId":"36418","inReplyTo":"xmqqk3ah71g8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] send-email: recognize absolute path on Windows","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2014-04-23T09:30:36Z","receivedAt":"2014-04-23T09:30:36Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Apr 22, 2014 at 6:50 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Erik Faye-Lund <kusmabite@gmail.com> writes:\n>\n>>>> Shouldn't the latter also be anchored at the beginning of the string\n>>>> with a leading \"^\"?\n>>>>\n>>>>> +    }\n>>>>> +\n>>>>> +    require File::Spec::Functions;\n>>>>> +    return File::Spec::Functions::file_name_is_absolute($path);\n>>>>\n>>>> We already \"use File::Spec qw(something else)\" at the beginning, no?\n>>>> Why not throw file_name_is_absolute into that qw() instead?\n>>>\n>>> Ahh, OK, if you did so, you won't have any place to hook the \"only\n>>> on msys do this\" trick into.\n>>>\n>>> It somehow feels somewhat confusing that we define a sub with the\n>>> same name as the system one, while not overriding it entirely but\n>>> delegate back to the system one.  I am debating myself if it is more\n>>> obvious if it is done this way:\n>>>\n>>>         use File::Spec::Functions qw(file_name_is_absolute);\n>>>         if ($^O eq 'msys') {\n>>>                 sub file_name_is_absolute {\n>>>                         return $_[0] =~ /^\\// || $_[0] =~ /^[A-Z]:/i;\n>>>                 }\n>>>         }\n>>>\n>>\n>> In this case, we end up requiring that module even when we end up\n>> using it, no?\n>\n> Also somebody earlier mentioned that we would be redefining, which\n> has a different kind of ugliness, so I'd agree with the code structure\n> of what you sent out (which has been queued on 'pu').\n>\n> My earlier question \"don't we want to make sure 'C:' is at the\n> betginning of the string?\" still stands, though.  I do not think I\n> futzed with your regexp in the version I queued on 'pu'.\n\nAh, yes of course. Thanks for spotting that. I also like the other\nclean-ups you did to the regex (above).\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/d/optout.\n"}]}