Re: [PATCH v2] Make git send-email accept $EDITOR with arguments

Re: [PATCH v2] Make git send-email accept $EDITOR with arguments

[Gustaf Hendeby]

Sorry Junio for the double, but I didn't mean to cut the CCs on this one.

On 2007-12-21 18:02, Junio C Hamano wrote:
> Gustaf Hendeby <hendeby@isy.liu.se> writes:
> 
>> Junio, even if this is technically not a bug fix, it would be nice to
>> get this fix into the 1.5.4 so that the usage of $EDITOR becomes more
>> consistent throughout git.
> 
> I can buy that, but at least a single line comment in front of that
> system() explaining why this is safe to do so would be beneficial.  I
> suspect that somebody would propose moving $compse_filename inside
> $GIT_DIR, now people realized $compose_filename is currently "./.msg.$$",
> and $GIT_DIR could be anything.  Quotemeta would probably be better as the
> code you are touching won't be affected by a future change to the value of
> that constant defined far away in the source.

Moving that file makes sense, I'll have a look at that, and do that as
well when I fix this patch.  After a first look at it though, I was
surprised to learn that git send-email actually wont work outside a
repository.  Ok, I can't really see any workflow where git send-email is
called from outside a git repository, but at the same time I don't see
why is should not be possible.  Are there any special reason for this?

Thanks,
	Gustaf

Re: [PATCH] Make git send-email accept $EDITOR with arguments

[Luciano Rocha]

[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]

On Thu, Dec 20, 2007 at 09:14:06PM +0100, Gustaf Hendeby wrote:
<snip>
> I'm not completely satisfied with the problem with embedded spaces,
> but my Perl skills aren't good enough to do anything about it.  If
> anyone have any suggestions on how to do it, it would be greatly
> appreciated.  None-the-less, even with this shortcoming, I think this
> is a step in the right direction.
> 
>  git-send-email.perl |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 248d035..47ae77c 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -400,7 +400,7 @@ EOT
>  	close(C);
>  
>  	my $editor = $ENV{GIT_EDITOR} || $repo->config("core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
> -	system($editor, $compose_filename);
> +	system((split ' ', $editor), $compose_filename);

That should be enough. Use system("$editor $compose_filename") to use
perl's implicit split or, in case of meta-characters in the string,
external sh -c.

Or always use the shell:
$shell = $ENV{SHELL} || "/bin/sh";
system($shell, "-c", "$editor $compose_filename");

BTW, maybe add a check for the return code?
system(...) == 0 or die "editor failed\n";

-- 
Luciano Rocha <luciano@eurotux.com>
Eurotux Informática, S.A. <http://www.eurotux.com/>

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH v2] Make git send-email accept $EDITOR with arguments

[Gustaf Hendeby]

Currently git send-email does not accept $EDITOR with arguments, eg,
emacs -nw, when starting an editor to produce a cover letter.  This
fix uses perl's implicit splitting to perform the task and that should
hopefully cover most interesting cases.

Signed-off-by:  Gustaf Hendeby <hendeby@isy.liu.se>
---

Thanks to Luciano for the tip to use the internal splitting in perl,
that should be a better solution than to split on all spaces.  I don't
think it is necessary, though, to add an extra error message if the
system call fails, system in it self already produces something that
should be clear enough.  If anyone got a strong oppinion for another
error message I'll fix that.

Junio, even if this is technically not a bug fix, it would be nice to
get this fix into the 1.5.4 so that the usage of $EDITOR becomes more
consistent throughout git.

/Gustaf

 git-send-email.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 248d035..5764668 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -400,7 +400,7 @@ EOT
 	close(C);
 
 	my $editor = $ENV{GIT_EDITOR} || $repo->config("core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
-	system($editor, $compose_filename);
+	system("$editor $compose_filename");
 
 	open(C2,">",$compose_filename . ".final")
 		or die "Failed to open $compose_filename.final : " . $!;
-- 
1.5.4.rc1.4.gb8173-dirty

Re: [PATCH v2] Make git send-email accept $EDITOR with arguments

[Jeff King]

On Fri, Dec 21, 2007 at 12:36:42PM +0100, Gustaf Hendeby wrote:

> -	system($editor, $compose_filename);
> +	system("$editor $compose_filename");

If you are going to do it that way, I suspect you want to quotemeta
$compose_filename.

-Peff

Re: [PATCH v2] Make git send-email accept $EDITOR with arguments

[Gustaf Hendeby]

On 12/21/2007 02:34 PM, Jeff King wrote:
> On Fri, Dec 21, 2007 at 12:36:42PM +0100, Gustaf Hendeby wrote:
> 
>> -	system($editor, $compose_filename);
>> +	system("$editor $compose_filename");
> 
> If you are going to do it that way, I suspect you want to quotemeta
> $compose_filename.
> 
> -Peff

Generally that would be true, but is that really necessary when I know
$compose_filename is defined as:

my $compose_filename = ".msg.$$";

Or, should I take it that you prefer the version using split?  I didn't
really feel good about the possibility of splitting paths with spaces
that came with that one though.

/Gustaf

Re: [PATCH v2] Make git send-email accept $EDITOR with arguments

[Jeff King]

On Fri, Dec 21, 2007 at 04:23:11PM +0100, Gustaf Hendeby wrote:

> > If you are going to do it that way, I suspect you want to quotemeta
> > $compose_filename.
> Generally that would be true, but is that really necessary when I know
> $compose_filename is defined as:
> 
> my $compose_filename = ".msg.$$";

I know; it is just easier to see that it is correct with the quotemeta
(and correct in the face of somebody changing the message later).

> Or, should I take it that you prefer the version using split?  I didn't
> really feel good about the possibility of splitting paths with spaces
> that came with that one though.

I am fine with using the shell. Though keep in mind that the two
solutions will behave differently with

  EDITOR='foo; bar'

That is, system("$editor $message") will actually invoke the shell,
whereas system(split(/ /, $editor), $message) will _just_ split on
whitespace. We should do whatever is consistent with the rest of the git
commands (off the top of my head, I don't know).

-Peff

Re: [PATCH v2] Make git send-email accept $EDITOR with arguments

[Gustaf Hendeby]

[-- Attachment #1: Type: text/plain, Size: 1490 bytes --]

On 2007-12-21 20:23, Jeff King wrote:
> On Fri, Dec 21, 2007 at 04:23:11PM +0100, Gustaf Hendeby wrote:
> 
>>> If you are going to do it that way, I suspect you want to quotemeta
>>> $compose_filename.
>> Generally that would be true, but is that really necessary when I know
>> $compose_filename is defined as:
>>
>> my $compose_filename = ".msg.$$";
> 
> I know; it is just easier to see that it is correct with the quotemeta
> (and correct in the face of somebody changing the message later).
Point taken!

> 
>> Or, should I take it that you prefer the version using split?  I didn't
>> really feel good about the possibility of splitting paths with spaces
>> that came with that one though.
> 
> I am fine with using the shell. Though keep in mind that the two
> solutions will behave differently with
> 
>   EDITOR='foo; bar'
> 
> That is, system("$editor $message") will actually invoke the shell,
> whereas system(split(/ /, $editor), $message) will _just_ split on
> whitespace. We should do whatever is consistent with the rest of the git
> commands (off the top of my head, I don't know).

A quick look at the proposed solution to the similar problem with git
commit, using code now in git tag, it seems it uses a split like
solution, though taking " and ' quoting into consideration.  On the top
of my head I can't come up with any other commands using $EDITOR.  I'll
try to find some time the next couple of days to make a reasonable
equivalent solution here.

Thanks,
	Gustaf

[-- Attachment #2: hendeby.vcf --]
[-- Type: text/x-vcard, Size: 378 bytes --]

begin:vcard
fn:Gustaf Hendeby
n:Hendeby;Gustaf
org;quoted-printable:Link=C3=B6ping University;Department of Electrical Engineering
adr;quoted-printable:;;;Link=C3=B6ping;;SE-581 83;Sweden
email;internet:hendeby@isy.liu.se
title:PhD Student
tel;work:+46 (0)13 282226
tel;fax:+46 (0)13 282622
x-mozilla-html:FALSE
url:http://www.control.isy.liu.se/~hendeby
version:2.1
end:vcard

Re: [PATCH v2] Make git send-email accept $EDITOR with arguments

[Junio C Hamano]

Gustaf Hendeby <hendeby@isy.liu.se> writes:

> On 2007-12-21 20:23, Jeff King wrote:
> 
>> I am fine with using the shell. Though keep in mind that the two
>> solutions will behave differently with
>> 
>>   EDITOR='foo; bar'
>> 
>> That is, system("$editor $message") will actually invoke the shell,
>> whereas system(split(/ /, $editor), $message) will _just_ split on
>> whitespace. We should do whatever is consistent with the rest of the git
>> commands (off the top of my head, I don't know).

Personally, I think EDITOR='foo; bar' is a user shooting his
foot off which we do not care about.

> A quick look at the proposed solution to the similar problem with git
> commit, using code now in git tag, it seems it uses a split like
> solution, though taking " and ' quoting into consideration.

I think the easiest to read and compatible way is:

	system('sh', '-c', '$0 $@', $editor, $filename)

Re: [PATCH v2] Make git send-email accept $EDITOR with arguments

[Junio C Hamano]

Gustaf Hendeby <hendeby@isy.liu.se> writes:

> Junio, even if this is technically not a bug fix, it would be nice to
> get this fix into the 1.5.4 so that the usage of $EDITOR becomes more
> consistent throughout git.

I can buy that, but at least a single line comment in front of that
system() explaining why this is safe to do so would be beneficial.  I
suspect that somebody would propose moving $compse_filename inside
$GIT_DIR, now people realized $compose_filename is currently "./.msg.$$",
and $GIT_DIR could be anything.  Quotemeta would probably be better as the
code you are touching won't be affected by a future change to the value of
that constant defined far away in the source.