Re: [PATCH 17/18] Place the open() call inside the do{} struct and prevent failing close

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH 17/18] Place the open() call inside the do{} struct and prevent failing close

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:57:34

Célestin Matte [off-list ref] writes:

quoted hunk

Placing the open() call inside the do{} struct will automatically close the
filehandle if possible.
Placing the close() call outside the do{} struct is useless and will make it
fail systematically
Change the error message to state that what fails is a fork(), not a file
opening.
Use autodie to properly exit when a print or open call fails.

Signed-off-by: Célestin Matte <redacted>
Signed-off-by: Matthieu Moy <redacted>
---
 contrib/mw-to-git/git-remote-mediawiki.perl |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
index 952ddcc..20ddccb 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl

@@ -23,6 +23,7 @@ binmode STDOUT, ':encoding(UTF-8)';
 
 use URI::Escape;
 use Readonly;
+use autodie;
 
 # Mediawiki filenames can contain forward slashes. This variable decides by which pattern they should be replaced
 Readonly my $SLASH_REPLACEMENT => '%2F';

@@ -363,8 +364,6 @@ sub run_git {
 		local $/ = undef;
 		<$git>
 	};
-	close($git);
-
 	return $res;
 }

Confused.  Which part of this patch moves open inside a do{} block?
This was last touched by [9/18] but it doesn't do any such thing,
either.

Upon leaving this subroutine, $git filehandle will go out of scope,
so in that sense, close may not be necessary, but that does not
match what the proposed log message claims what the patch does.

Also, this patch does not remove "or die" 9/18 added, even though
the proposed log message claims that with autodie it is no longer
necessary.

I am not convinced that using autodie globally, without vetting the
calls the original code make, is a good idea in the first place.
How does this change interact with existing calls to open, close,
etc. that check the return value from them, now these calls throw
exception and will not give a chance for the existing error handling
codepath to intervene?

Re: [PATCH 17/18] Place the open() call inside the do{} struct and prevent failing close

From: Célestin Matte <hidden>
Date: 2016-06-15 22:57:34

Le 06/06/2013 23:13, Junio C Hamano a écrit :

Confused.  Which part of this patch moves open inside a do{} block?
This was last touched by [9/18] but it doesn't do any such thing,
either.

I must have failed the rebase, as the first part of the commit moved to
[14/18] because it modifies a part of it:

quoted hunk

@@ -344,10 +344,10 @@ sub get_mw_pages {

#        $out = run_git("command args", "raw"); # don't interpret
output as UTF-8.
sub run_git {
	my $args = shift;
-	my $encoding = (shift || "encoding(UTF-8)");
-	open(my $git, "-|:$encoding", "git " . $args)
-	    or die "Unable to open: $!\n";
-	my $res = do {
+	my $encoding = (shift || 'encoding(UTF-8)');
+	my $res = do {
+		open(my $git, "-|:$encoding", "git ${args}")
+		    or die "Unable to fork: $!\n";
		local $/ = undef;
		<$git>
	};

I'm not sure how I should correct this. I'll have a look if this commit
actually is useful.

Upon leaving this subroutine, $git filehandle will go out of scope,
so in that sense, close may not be necessary, but that does not
match what the proposed log message claims what the patch does.

Also, this patch does not remove "or die" 9/18 added, even though
the proposed log message claims that with autodie it is no longer
necessary.

I am not convinced that using autodie globally, without vetting the
calls the original code make, is a good idea in the first place.
How does this change interact with existing calls to open, close,
etc. that check the return value from them, now these calls throw
exception and will not give a chance for the existing error handling
codepath to intervene?

So using autodie may not be a good idea.
But the problem is that in the current state, open() return values are
checked, but print ones are not, although it should be. So, either:
- we use autodie and remove checking of existing return values, or
- we don't use autodie and add checking of return value of print calls
- or I'm missing some point :)


-- 
Célestin Matte

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help