threads / patch / 15952

patchGit.pm: do not break inheritance

Subject: [PATCH] Git.pm: do not break inheritance

## tl;dr

3 messages between Oct 18, 2008 and Oct 18, 2008. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Christian Jaeger· Oct 18, 2008, 18:25 UTC · lore
Make it possible to write subclasses of Git.pm
Signed-off-by: Christian Jaeger <christian@jaeger.mine.nu>
---
 I don't really know what the reason for the _maybe_self behaviour
 was; I'm hoping this fix doesn't break anything, I haven't run any
 tests with it except with my own code; the fix works on the
 assumptions that if an object does indeed have Git.pm in it's
 ancestry, _maybe_self should work just as if the object is a 'Git'
 object without inheritance.
 I'm currently using the following hack to make my scripts be able to
 inherit from a non-patched Git.pm: I inherit instead from a wrapper
 around Git.pm which inherits from and patches the latter at runtime
 using this code:
 if (do {
     my @res= Git::_maybe_self ( (bless {}, __PACKAGE__) );
     not $res[0]
 }) {
     #warn "patching Git.pm";#
     no warnings;
     *Git::_maybe_self= sub {
	 UNIVERSAL::isa($_[0], 'Git') ? @_ : (undef, @_);
     }
 }
 While this currently works, a proper fix would of course be
 preferable (like: when in the future will the above hack break?..).
 perl/Git.pm |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
Show changes to perl/Git.pm +1 −2
diff --git a/perl/Git.pm b/perl/Git.pm
index 6aab712..ba94453 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -1203,8 +1203,7 @@ either version 2, or (at your option) any later version.
 # the method was called upon an instance and (undef, @args) if
 # it was called directly.
 sub _maybe_self {
-	# This breaks inheritance. Oh well.
-	ref $_[0] eq 'Git' ? @_ : (undef, @_);
+	UNIVERSAL::isa($_[0], 'Git') ? @_ : (undef, @_);
 }
 
 # Check if the command id is something reasonable.
-- 
1.6.0.2
Junio C Hamano· Oct 18, 2008, 20:50 UTC · re: Christian Jaeger · lore

Re: [PATCH] Git.pm: do not break inheritance

Christian Jaeger <christian@jaeger.mine.nu> writes:
Show 7 quoted lines
> Make it possible to write subclasses of Git.pm
>
> Signed-off-by: Christian Jaeger <christian@jaeger.mine.nu>
> ---
>
>  I don't really know what the reason for the _maybe_self behaviour
>  was; I'm hoping this fix doesn't break anything,

That's how you would write class methods, isn't it? IOW, your callers can say:

	my $self = new Git();
        $self->method(qw(a b c));
        Git::method(qw(a b c))
and you can start your method like this:
	sub method {
        	my ($self, @args) = _maybe_self(@_)
                ...
	}

and use @args the same way for either form of the call in the implementation. Two obvious pitfalls are:

 - You cannot use $self if you set up your parameters with _maybe_self;
 - The second form of the call would call directly into Git::method, never
   your subclasses implementation, even if you write:
	use Git;
        package CJGit;
        our @ISA = qw(Git);
        sub method {
        	...
	}
Show 7 quoted lines
>  sub _maybe_self {
> -	# This breaks inheritance. Oh well.
> -	ref $_[0] eq 'Git' ? @_ : (undef, @_);
> +	UNIVERSAL::isa($_[0], 'Git') ? @_ : (undef, @_);
>  }
>  
>  # Check if the command id is something reasonable.

The patch looks Ok, as long as you have a working UNIVERSAL::isa() in your version of Perl. My reading of perl561delta,pod says that Perl 5.6.1 and later should have a working implementation.

Christian Jaeger· Oct 18, 2008, 22:21 UTC · re: Junio C Hamano · lore

Re: [PATCH] Git.pm: do not break inheritance

Junio C Hamano wrote:
Show 16 quoted lines
> That's how you would write class methods, isn't it?  IOW, your callers
> can say:
>
> 	my $self = new Git();
>         $self->method(qw(a b c));
>         Git::method(qw(a b c))
>
> and you can start your method like this:
>
> 	sub method {
>         	my ($self, @args) = _maybe_self(@_)
>                 ...
> 	}
>
> and use @args the same way for either form of the call in the
> implementation.  

I see, a magic way to offer both an OO and procedural api. Well, partial procedural api, since not all of the methods can work without the $self.

Show 16 quoted lines
> Two obvious pitfalls are:
>
>  - You cannot use $self if you set up your parameters with _maybe_self;
>
>  - The second form of the call would call directly into Git::method, never
>    your subclasses implementation, even if you write:
>
> 	use Git;
>         package CJGit;
>         our @ISA = qw(Git);
>
>         sub method {
>         	...
> 	}
>
>   

(This is no problem iff people are calling procedures in object notation if they actually have got an object at hands. I.e. if you've got some code which does:

my $repo= Git->repository(...); $repo->foo;

then all is fine, and I would think it would be weird if people called Git::foo($repo, ... ) in such a case. Well for my purposes it's not a problem anyway, since if a user wants to use the extensions, he needs to actually do this:

my $repo= CJGit->repository(...);

and then it should be clear that one shouldn't call Git:: directly. The problem will only be in cases where an extended object is being fed to existing code which calls Git:: with objects in the hope that virtual method calls are going to the extended class; well, let's fix those bad call sites when they are being discovered... Maybe this warrants a warning somewhere.)

Show 13 quoted lines
>>  sub _maybe_self {
>> -	# This breaks inheritance. Oh well.
>> -	ref $_[0] eq 'Git' ? @_ : (undef, @_);
>> +	UNIVERSAL::isa($_[0], 'Git') ? @_ : (undef, @_);
>>  }
>>  
>>  # Check if the command id is something reasonable.
>>     
>
> The patch looks Ok, as long as you have a working UNIVERSAL::isa() in your
> version of Perl.  My reading of perl561delta,pod says that Perl 5.6.1 and
> later should have a working implementation.
>   
 From http://perldoc.perl.org/perl561delta.html:
Show 5 quoted lines
> UNIVERSAL::isa()
>
> A bug in the caching mechanism used by UNIVERSAL::isa() that affected 
> base.pm has been fixed. The bug has existed since the 5.005 releases, 
> but wasn't tickled by base.pm in those releases.

This looks like only a bug in some (corner?) cases; I've verified that I've been using the isa() method *or* function since perl 5.005_03, and haven't had issues with it. I can't easily verify though when I switched from

#if (Scalar::Util::blessed($value) and $value->isa("Eile::Html")) { to if (UNIVERSAL::isa($value,"Eile::Html")) {

(because it was simpler to write, once I discovered that I could just call the function directly instead of relying on method dispatch) but that might not have made a difference.

Christian.

← back to recent threads