{"thread":{"id":"15952","subject":"[PATCH] Git.pm: do not break inheritance","startedAt":"2008-10-18T18:25:12Z","lastAt":"2008-10-18T22:21:06Z","messageCount":3,"participants":["Christian Jaeger","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"93386","messageId":"2980b5cead38d5ae3510e4ed9adc847c80be1075.1224360106.git.christian@jaeger.mine.nu","threadId":"15952","inReplyTo":null,"subject":"[PATCH] Git.pm: do not break inheritance","fromName":"Christian Jaeger","fromEmail":"christian@jaeger.mine.nu","sentAt":"2008-10-18T18:25:12Z","receivedAt":"2008-10-18T18:25:12Z","isPatch":true,"sender":{"key":"christian@jaeger.mine.nu","avatar":null},"body":"Make it possible to write subclasses of Git.pm\n\nSigned-off-by: Christian Jaeger <christian@jaeger.mine.nu>\n---\n\n I don't really know what the reason for the _maybe_self behaviour\n was; I'm hoping this fix doesn't break anything, I haven't run any\n tests with it except with my own code; the fix works on the\n assumptions that if an object does indeed have Git.pm in it's\n ancestry, _maybe_self should work just as if the object is a 'Git'\n object without inheritance.\n\n I'm currently using the following hack to make my scripts be able to\n inherit from a non-patched Git.pm: I inherit instead from a wrapper\n around Git.pm which inherits from and patches the latter at runtime\n using this code:\n\n if (do {\n     my @res= Git::_maybe_self ( (bless {}, __PACKAGE__) );\n     not $res[0]\n }) {\n     #warn \"patching Git.pm\";#\n     no warnings;\n     *Git::_maybe_self= sub {\n\t UNIVERSAL::isa($_[0], 'Git') ? @_ : (undef, @_);\n     }\n }\n\n While this currently works, a proper fix would of course be\n preferable (like: when in the future will the above hack break?..).\n\n\n perl/Git.pm |    3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 6aab712..ba94453 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -1203,8 +1203,7 @@ either version 2, or (at your option) any later version.\n # the method was called upon an instance and (undef, @args) if\n # it was called directly.\n sub _maybe_self {\n-\t# This breaks inheritance. Oh well.\n-\tref $_[0] eq 'Git' ? @_ : (undef, @_);\n+\tUNIVERSAL::isa($_[0], 'Git') ? @_ : (undef, @_);\n }\n \n # Check if the command id is something reasonable.\n-- \n1.6.0.2\n"},{"id":"93389","messageId":"7vabd1aaqx.fsf@gitster.siamese.dyndns.org","threadId":"15952","inReplyTo":"2980b5cead38d5ae3510e4ed9adc847c80be1075.1224360106.git.christian@jaeger.mine.nu","subject":"Re: [PATCH] Git.pm: do not break inheritance","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-18T20:50:30Z","receivedAt":"2008-10-18T20:50:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Jaeger <christian@jaeger.mine.nu> writes:\n\n> Make it possible to write subclasses of Git.pm\n>\n> Signed-off-by: Christian Jaeger <christian@jaeger.mine.nu>\n> ---\n>\n>  I don't really know what the reason for the _maybe_self behaviour\n>  was; I'm hoping this fix doesn't break anything,\n\nThat's how you would write class methods, isn't it?  IOW, your callers\ncan say:\n\n\tmy $self = new Git();\n        $self->method(qw(a b c));\n        Git::method(qw(a b c))\n\nand you can start your method like this:\n\n\tsub method {\n        \tmy ($self, @args) = _maybe_self(@_)\n                ...\n\t}\n\nand use @args the same way for either form of the call in the\nimplementation.  Two obvious pitfalls are:\n\n - You cannot use $self if you set up your parameters with _maybe_self;\n\n - The second form of the call would call directly into Git::method, never\n   your subclasses implementation, even if you write:\n\n\tuse Git;\n        package CJGit;\n        our @ISA = qw(Git);\n\n        sub method {\n        \t...\n\t}\n\n>  sub _maybe_self {\n> -\t# This breaks inheritance. Oh well.\n> -\tref $_[0] eq 'Git' ? @_ : (undef, @_);\n> +\tUNIVERSAL::isa($_[0], 'Git') ? @_ : (undef, @_);\n>  }\n>  \n>  # Check if the command id is something reasonable.\n\nThe patch looks Ok, as long as you have a working UNIVERSAL::isa() in your\nversion of Perl.  My reading of perl561delta,pod says that Perl 5.6.1 and\nlater should have a working implementation.\n"},{"id":"93395","messageId":"48FA6152.6020006@jaeger.mine.nu","threadId":"15952","inReplyTo":"7vabd1aaqx.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Git.pm: do not break inheritance","fromName":"Christian Jaeger","fromEmail":"christian@jaeger.mine.nu","sentAt":"2008-10-18T22:21:06Z","receivedAt":"2008-10-18T22:21:06Z","isPatch":true,"sender":{"key":"christian@jaeger.mine.nu","avatar":null},"body":"Junio C Hamano wrote:\n> That's how you would write class methods, isn't it?  IOW, your callers\n> can say:\n>\n> \tmy $self = new Git();\n>         $self->method(qw(a b c));\n>         Git::method(qw(a b c))\n>\n> and you can start your method like this:\n>\n> \tsub method {\n>         \tmy ($self, @args) = _maybe_self(@_)\n>                 ...\n> \t}\n>\n> and use @args the same way for either form of the call in the\n> implementation.  \n\nI see, a magic way to offer both an OO and procedural api. Well, partial \nprocedural api, since not all of the methods can work without the $self.\n\n> Two obvious pitfalls are:\n>\n>  - You cannot use $self if you set up your parameters with _maybe_self;\n>\n>  - The second form of the call would call directly into Git::method, never\n>    your subclasses implementation, even if you write:\n>\n> \tuse Git;\n>         package CJGit;\n>         our @ISA = qw(Git);\n>\n>         sub method {\n>         \t...\n> \t}\n>\n>   \n\n(This is no problem iff people are calling procedures in object notation \nif they actually have got an object at hands. I.e. if you've got some \ncode which does:\n\nmy $repo= Git->repository(...);\n$repo->foo;\n\nthen all is fine, and I would think it would be weird if people called \nGit::foo($repo, ... ) in such a case. Well for my purposes it's not a \nproblem anyway, since if a user wants to use the extensions, he needs to \nactually do this:\n\nmy $repo= CJGit->repository(...);\n\nand then it should be clear that one shouldn't call Git:: directly. The \nproblem will only be in cases where an extended object is being fed to \nexisting code which calls Git:: with objects in the hope that virtual \nmethod calls are going to the extended class; well, let's fix those bad \ncall sites when they are being discovered... Maybe this warrants a \nwarning somewhere.)\n\n>>  sub _maybe_self {\n>> -\t# This breaks inheritance. Oh well.\n>> -\tref $_[0] eq 'Git' ? @_ : (undef, @_);\n>> +\tUNIVERSAL::isa($_[0], 'Git') ? @_ : (undef, @_);\n>>  }\n>>  \n>>  # Check if the command id is something reasonable.\n>>     \n>\n> The patch looks Ok, as long as you have a working UNIVERSAL::isa() in your\n> version of Perl.  My reading of perl561delta,pod says that Perl 5.6.1 and\n> later should have a working implementation.\n>   \n\n From http://perldoc.perl.org/perl561delta.html:\n\n> UNIVERSAL::isa()\n>\n> A bug in the caching mechanism used by UNIVERSAL::isa() that affected \n> base.pm has been fixed. The bug has existed since the 5.005 releases, \n> but wasn't tickled by base.pm in those releases.\n\nThis looks like only a bug in some (corner?) cases; I've verified that \nI've been using the isa() method *or* function since perl 5.005_03, and \nhaven't had issues with it. I can't easily verify though when I switched \nfrom\n\n#if (Scalar::Util::blessed($value) and $value->isa(\"Eile::Html\")) {\nto\nif (UNIVERSAL::isa($value,\"Eile::Html\")) {\n\n(because it was simpler to write, once I discovered that I could just \ncall the function directly instead of relying on method dispatch) but \nthat might not have made a difference.\n\nChristian.\n"}]}