git-send-email no longer works outside a repository?

git-send-email no longer works outside a repository?

[Jacob Keller]

I often use git-send-email in order to send patch files. Recently when
I tried to do this outside a repository I got some cryptic failures,
I'm using the master branch, git version 2.13.0.311.g0339965c70d6

I generate the patch files and copy them into a separate folder
outside of the repository, and make sure everything looks good and
write a cover letter, then I try to send them with

$git send-email --to=<address> 00*
Can't call method "repo_path" on an undefined value at
/home/jekeller/libexec/git-core/git-send-email line 1759.

Even weirder, if I move into the repository and try to send files
which are outside, such as:

$git send-email --to=iwl<address> ../patches/00*
fatal: /home/jekeller/git/patches/00*:
'/home/jekeller/git/patches/00*' is outside repository
format-patch -o /tmp/AZatqXB1uD /home/jekeller/git/patches/00*:
command returned error: 128

I would expect that if you're outside a repository the command (as
before) would alllow you to send files. It shouldn't strictly need to
be inside a repository to function.

I found this first on pu, but as above, I checked out master and still
seem to have the problem.

I'm working on a bisect now.

Thanks,
Jake

Re: git-send-email no longer works outside a repository?

[Brandon Williams]

On 06/01, Jacob Keller wrote:
> I often use git-send-email in order to send patch files. Recently when
> I tried to do this outside a repository I got some cryptic failures,
> I'm using the master branch, git version 2.13.0.311.g0339965c70d6
> 
> I generate the patch files and copy them into a separate folder
> outside of the repository, and make sure everything looks good and
> write a cover letter, then I try to send them with
> 
> $git send-email --to=<address> 00*
> Can't call method "repo_path" on an undefined value at
> /home/jekeller/libexec/git-core/git-send-email line 1759.
> 
> Even weirder, if I move into the repository and try to send files
> which are outside, such as:
> 
> $git send-email --to=iwl<address> ../patches/00*
> fatal: /home/jekeller/git/patches/00*:
> '/home/jekeller/git/patches/00*' is outside repository
> format-patch -o /tmp/AZatqXB1uD /home/jekeller/git/patches/00*:
> command returned error: 128
> 
> I would expect that if you're outside a repository the command (as
> before) would alllow you to send files. It shouldn't strictly need to
> be inside a repository to function.
> 
> I found this first on pu, but as above, I checked out master and still
> seem to have the problem.
> 
> I'm working on a bisect now.
> 
> Thanks,
> Jake

This looks like it is due to '6489660b4
(origin/jt/send-email-validate-hook) send-email: support validate hook'.
It's trying to look for a hook in a non-existent repository.

-- 
Brandon Williams

Re: git-send-email no longer works outside a repository?

[Junio C Hamano]

Jacob Keller <jacob.keller@gmail.com> writes:

> I often use git-send-email in order to send patch files. Recently when
> I tried to do this outside a repository I got some cryptic failures,
> I'm using the master branch, git version 2.13.0.311.g0339965c70d6
>
> I generate the patch files and copy them into a separate folder
> outside of the repository, and make sure everything looks good and
> write a cover letter, then I try to send them with
>
> $git send-email --to=<address> 00*
> Can't call method "repo_path" on an undefined value at
> /home/jekeller/libexec/git-core/git-send-email line 1759.
>
> Even weirder, if I move into the repository and try to send files
> which are outside, such as:
>
> $git send-email --to=iwl<address> ../patches/00*
> fatal: /home/jekeller/git/patches/00*:
> '/home/jekeller/git/patches/00*' is outside repository
> format-patch -o /tmp/AZatqXB1uD /home/jekeller/git/patches/00*:
> command returned error: 128
>
> I would expect that if you're outside a repository the command (as
> before) would alllow you to send files. It shouldn't strictly need to
> be inside a repository to function.
>
> I found this first on pu, but as above, I checked out master and still
> seem to have the problem.
>
> I'm working on a bisect now.

This certainly is not an intended change.  That validate-hook thing
must be made optional and conditional to the existence of a
repository.

Thanks for reporting.

[PATCH] send-email: check for repo before invoking hook

[Jonathan Tan]

Unless --no-validate is passed, send-email will invoke
$repo->repo_path() in its search for a validate hook regardless of
whether a Git repo is actually present.  Teach send-email to first check
for repo existence.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Thanks for the notification. Here's a patch to fix that.
---
 git-send-email.perl   | 32 +++++++++++++++++---------------
 t/t9001-send-email.sh |  8 ++++++++
 2 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index f0417f64e..94c54dc5a 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1755,21 +1755,23 @@ sub unique_email_list {
 sub validate_patch {
 	my $fn = shift;
 
-	my $validate_hook = catfile(catdir($repo->repo_path(), 'hooks'),
-				    'sendemail-validate');
-	my $hook_error;
-	if (-x $validate_hook) {
-		my $target = abs_path($fn);
-		# The hook needs a correct cwd and GIT_DIR.
-		my $cwd_save = cwd();
-		chdir($repo->wc_path() or $repo->repo_path())
-			or die("chdir: $!");
-		local $ENV{"GIT_DIR"} = $repo->repo_path();
-		$hook_error = "rejected by sendemail-validate hook"
-			if system($validate_hook, $target);
-		chdir($cwd_save) or die("chdir: $!");
-	}
-	return $hook_error if $hook_error;
+	if ($repo) {
+		my $validate_hook = catfile(catdir($repo->repo_path(), 'hooks'),
+					    'sendemail-validate');
+		my $hook_error;
+		if (-x $validate_hook) {
+			my $target = abs_path($fn);
+			# The hook needs a correct cwd and GIT_DIR.
+			my $cwd_save = cwd();
+			chdir($repo->wc_path() or $repo->repo_path())
+				or die("chdir: $!");
+			local $ENV{"GIT_DIR"} = $repo->repo_path();
+			$hook_error = "rejected by sendemail-validate hook"
+				if system($validate_hook, $target);
+			chdir($cwd_save) or die("chdir: $!");
+		}
+		return $hook_error if $hook_error;
+	}
 
 	open(my $fh, '<', $fn)
 		or die sprintf(__("unable to open %s: %s\n"), $fn, $!);
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 15128c755..d1e4e8ad1 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1953,4 +1953,12 @@ test_expect_success $PREREQ 'invoke hook' '
 	)
 '
 
+test_expect_success $PREREQ 'test that send-email works outside a repo' '
+	nongit git send-email \
+		--from="Example <nobody@example.com>" \
+		--to=nobody@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		"$(pwd)/0001-add-master.patch"
+'
+
 test_done
-- 
2.13.0.506.g27d5fe0cd-goog

Re: [PATCH] send-email: check for repo before invoking hook

[Todd Zullinger]

Hi Jonathan,

Jonathan Tan wrote:
> Thanks for the notification. Here's a patch to fix that. 
> --- 
> git-send-email.perl   | 32 +++++++++++++++++--------------- 
> t/t9001-send-email.sh |  8 ++++++++ 
> 2 files changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl 
> index f0417f64e..94c54dc5a 100755 
> --- a/git-send-email.perl 
> +++ b/git-send-email.perl 
> @@ -1755,21 +1755,23 @@ sub unique_email_list { 
> sub validate_patch { 
> 	my $fn = shift;
>
> -	my $validate_hook = catfile(catdir($repo->repo_path(), 'hooks'), 
> -				    'sendemail-validate'); 
> -	my $hook_error; 
> -	if (-x $validate_hook) { 
> -		my $target = abs_path($fn); 
> -		# The hook needs a correct cwd and GIT_DIR. 
> -		my $cwd_save = cwd(); 
> -		chdir($repo->wc_path() or $repo->repo_path()) 
> -			or die("chdir: $!"); 
> -		local $ENV{"GIT_DIR"} = $repo->repo_path(); 
> -		$hook_error = "rejected by sendemail-validate hook" 
> -			if system($validate_hook, $target); 
> -		chdir($cwd_save) or die("chdir: $!"); 
> -	} 
> -	return $hook_error if $hook_error; 
> +	if ($repo) { 
> +		my $validate_hook = catfile(catdir($repo->repo_path(), 'hooks'), 
> +					    'sendemail-validate'); 
> +		my $hook_error; 
> +		if (-x $validate_hook) { 
> +			my $target = abs_path($fn); 
> +			# The hook needs a correct cwd and GIT_DIR. 
> +			my $cwd_save = cwd(); 
> +			chdir($repo->wc_path() or $repo->repo_path()) 
> +				or die("chdir: $!"); 
> +			local $ENV{"GIT_DIR"} = $repo->repo_path(); 
> +			$hook_error = "rejected by sendemail-validate hook" 
> +				if system($validate_hook, $target); 
> +			chdir($cwd_save) or die("chdir: $!"); 
> +		} 
> +		return $hook_error if $hook_error; 
> +	}

Would it be worth doing the $repo test when defining $validate_hook 
to avoid a layer of indentation?  Something like this (with whatever 
proper wrapping/indentation is used for perl, if I have that wildly 
incorrect, of course)?

-- >8 --
diff --git i/git-send-email.perl w/git-send-email.perl
index f0417f64e7..e78a0a728a 100755
--- i/git-send-email.perl
+++ w/git-send-email.perl
@@ -1755,8 +1755,9 @@ sub unique_email_list {
 sub validate_patch {
 	my $fn = shift;
 
-	my $validate_hook = catfile(catdir($repo->repo_path(), 'hooks'),
-				    'sendemail-validate');
+	my $validate_hook = $repo ?
+		catfile(catdir($repo->repo_path(), 'hooks'),
+			'sendemail-validate') : '';
 	my $hook_error;
 	if (-x $validate_hook) {
 		my $target = abs_path($fn);

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I have to decide between two equally frightening options.  If I wanted
to do that, I'd vote.
    -- Duckman

Re: [PATCH] send-email: check for repo before invoking hook

[Jacob Keller]

On Thu, Jun 1, 2017 at 5:22 PM, Todd Zullinger <tmz@pobox.com> wrote:
> Hi Jonathan,
>
> Jonathan Tan wrote:
>>
>> Thanks for the notification. Here's a patch to fix that. ---
>> git-send-email.perl   | 32 +++++++++++++++++---------------
>> t/t9001-send-email.sh |  8 ++++++++ 2 files changed, 25 insertions(+), 15
>> deletions(-)
>>
>> diff --git a/git-send-email.perl b/git-send-email.perl index
>> f0417f64e..94c54dc5a 100755 --- a/git-send-email.perl +++
>> b/git-send-email.perl @@ -1755,21 +1755,23 @@ sub unique_email_list { sub
>> validate_patch {       my $fn = shift;
>>
>> -       my $validate_hook = catfile(catdir($repo->repo_path(), 'hooks'), -
>> 'sendemail-validate'); -    my $hook_error; -       if (-x $validate_hook) {
>> -              my $target = abs_path($fn); -           # The hook needs a
>> correct cwd and GIT_DIR. -           my $cwd_save = cwd(); -
>> chdir($repo->wc_path() or $repo->repo_path()) -                 or
>> die("chdir: $!"); -          local $ENV{"GIT_DIR"} = $repo->repo_path(); -
>> $hook_error = "rejected by sendemail-validate hook" -                   if
>> system($validate_hook, $target); -           chdir($cwd_save) or die("chdir:
>> $!"); - } -     return $hook_error if $hook_error; +    if ($repo) { +
>> my $validate_hook = catfile(catdir($repo->repo_path(), 'hooks'), +
>> 'sendemail-validate'); +            my $hook_error; +               if (-x
>> $validate_hook) { +                      my $target = abs_path($fn); +
>> # The hook needs a correct cwd and GIT_DIR. +                   my $cwd_save
>> = cwd(); +                 chdir($repo->wc_path() or $repo->repo_path()) +
>> or die("chdir: $!"); +                  local $ENV{"GIT_DIR"} =
>> $repo->repo_path(); +                   $hook_error = "rejected by
>> sendemail-validate hook" +                           if
>> system($validate_hook, $target); +                   chdir($cwd_save) or
>> die("chdir: $!"); +         } +             return $hook_error if
>> $hook_error; +    }
>
>
> Would it be worth doing the $repo test when defining $validate_hook to avoid
> a layer of indentation?  Something like this (with whatever proper
> wrapping/indentation is used for perl, if I have that wildly incorrect, of
> course)?
>

This approach makes more sense to me, but either should fix the bug.
The first approach might be more resilient against future changes
though...?

Thanks,
Jake

> -- >8 --
> diff --git i/git-send-email.perl w/git-send-email.perl
> index f0417f64e7..e78a0a728a 100755
> --- i/git-send-email.perl
> +++ w/git-send-email.perl
> @@ -1755,8 +1755,9 @@ sub unique_email_list {
> sub validate_patch {
>         my $fn = shift;
>
> -       my $validate_hook = catfile(catdir($repo->repo_path(), 'hooks'),
> -                                   'sendemail-validate');
> +       my $validate_hook = $repo ?
> +               catfile(catdir($repo->repo_path(), 'hooks'),
> +                       'sendemail-validate') : '';
>         my $hook_error;
>         if (-x $validate_hook) {
>                 my $target = abs_path($fn);
>
> --
> Todd
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> I have to decide between two equally frightening options.  If I wanted
> to do that, I'd vote.
>    -- Duckman
>

Re: [PATCH] send-email: check for repo before invoking hook

[Junio C Hamano]

Thanks.  

"git show -w" tells readers how this fix is trivially correct ;-)

Will apply.