Git.pm

Git.pm

[Subho Banerjee]

Hello,
I had made a proposal for the Git.pm project in GSoC. The proposal did
not get accepted, however, I see that no one in the GSoC accepted list
is actually working on the Git.pm project. If some of you could give
me some of your time in terms of advice on what exactly is needed for
the module, I am willing to work over the summer to get this module
production-ready. I can probably put in 15-20 hours a week on this
project from May to August. I believe that will be enough time to
roughly complete all that I had enumerated in my GSoC proposal. This
of course will be strictly outside the GSoC framework.

I plan to start coding on the project by 7th May and use the time from
then to now to investigate what code is there/ what is to be done etc.
I had made an approximate timeline for the GSoC proposal and I would
like to follow it -
---> [By 15th May] Get the current perl code, to use another mechanism
of throwing errors(Try:Tiny)
---> [By August] Get in place a more robust perl wrapper ie. expand
the code have a couple of more objects, Git::Repo, Git::Config etc.
---> If all goes well, then by the beginning of August, get the perl
module ready for CPANfication

I also had a couple of questions -
---> Do I base my code revisions on the master branch of the Git
codebase[https://github.com/git/git]? Or is there some other
repository which might be more recent.
---> I saw gitweb-caching code from a previous GSoC project, the perl
module there seems to have been developed beyond what is there in the
master brach? However, these changes are atleast a couple of years old
and havent been incorporated in the main codebase... Is there any
particular reason for that?
---> I see in the code that it says that the API is experimental. Is
there any absolute need for backward compatibility, or can I try to
redesign the API somewhat extensively?

Also, any suggestions and tips you can give me about the project will
be very helpful.

Cheers,
Subho.

Re: Git.pm

[Randal L. Schwartz]

>>>>> "Subho" == Subho Banerjee <subs.zero@gmail.com> writes:

Subho> Also, any suggestions and tips you can give me about the project will
Subho> be very helpful.

I offer my services for code-review and consultation.

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Smalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.
See http://methodsandmessages.posterous.com/ for Smalltalk discussion

Re: Git.pm

[Tim Henigan]

On Thu, Apr 26, 2012 at 12:15 AM, Subho Banerjee <subs.zero@gmail.com> wrote:
>
> ---> I see in the code that it says that the API is experimental. Is
> there any absolute need for backward compatibility, or can I try to
> redesign the API somewhat extensively?

A quick grep of the code in 'master' shows Git.pm used in the following:

    - contrib/examples/git-remote.perl
    - git-add--interactive.perl
    - git-cvsexportcommit.perl
    - git-send-email.perl
    - git-svn.perl
    - t/perf/aggregate.perl

There is also work in progress on 'pu' that relies on Git.pm.

Breaking any of these scripts would be bad.  You may be able to
refactor them at the same time Git.pm is modified, but it would be
wise to contact the authors before making any major changes.

Re: Git.pm

[Junio C Hamano]

Subho Banerjee <subs.zero@gmail.com> writes:

> ... If some of you could give
> me some of your time in terms of advice on what exactly is needed for
> the module, I am willing to work over the summer to get this module
> production-ready.

Well, that sounds as if the module is currently not production ready,
but it has been used in the wild for quite a long time.

> I see in the code that it says that the API is experimental. Is
> there any absolute need for backward compatibility, or can I try to
> redesign the API somewhat extensively?

Being experimental merely means that we do not support out of tree
users; it does not mean you are allowed to break it in any way for the
in-tree users.

Re: Git.pm

[Sam Vilain]

On 4/25/12 9:15 PM, Subho Banerjee wrote:
> --->  I see in the code that it says that the API is experimental. Is
> there any absolute need for backward compatibility, or can I try to
> redesign the API somewhat extensively?

If you stick to putting new APIs under different namespaces, or new 
functions, then you should be able to preserve API compatibility.  I 
think Git.pm is now too widely used for breaking compatibility to be an 
option.

I think I submitted a Git::Config to this list some time ago; did you 
find that?

Sam

Re: Git.pm

[Subho Banerjee]

Hello,
I will take care that I dont break those. Should the tests in the t/
folder of the codebase be enough to make sure everything is working as
it should be even in the Git perl module? Also is there anything like
a public build server which actually catalogs which tests are
currently failing so that I know what has gone wrong after my changes,
or are all commits supposed to pass every test?

Cheers,
Subho.

On Fri, Apr 27, 2012 at 12:28 AM, Tim Henigan <tim.henigan@gmail.com> wrote:
> On Thu, Apr 26, 2012 at 12:15 AM, Subho Banerjee <subs.zero@gmail.com> wrote:
>>
>> ---> I see in the code that it says that the API is experimental. Is
>> there any absolute need for backward compatibility, or can I try to
>> redesign the API somewhat extensively?
>
> A quick grep of the code in 'master' shows Git.pm used in the following:
>
>    - contrib/examples/git-remote.perl
>    - git-add--interactive.perl
>    - git-cvsexportcommit.perl
>    - git-send-email.perl
>    - git-svn.perl
>    - t/perf/aggregate.perl
>
> There is also work in progress on 'pu' that relies on Git.pm.
>
> Breaking any of these scripts would be bad.  You may be able to
> refactor them at the same time Git.pm is modified, but it would be
> wise to contact the authors before making any major changes.

Re: Git.pm

[Jonathan Nieder]

Hi,

Subho Banerjee wrote:

> I will take care that I dont break those. 

Thanks, sounds good.

>                                           Should the tests in the t/
> folder of the codebase be enough to make sure everything is working as
> it should be even in the Git perl module?

No. :)

>                                           Also is there anything like
> a public build server which actually catalogs which tests are
> currently failing so that I know what has gone wrong after my changes,
> or are all commits supposed to pass every test?

When tests are known to fail, they are marked with test_expect_failure
so they don't affect the test result.

Hope that helps,
Jonathan

Re: Git.pm

[Subho Banerjee]

Hello,
I have started looking into how the error catching mechanism
implemented right now. I have looked into the more modern error
catching/throwing mechanisms in use in perl, and I am of the opinion
that Try::Simple would probably be the best candidate for being the
new error catching mechanism. I also wanted to discuss some aspects of
the changes to be made -
------- Replacing the Error::Simple stuff should be relatively
straightforward. It can be achieved with simple changes to the syntax
of the perl module itself.

------- What I feel will be more complicated, and will require some
discussion before it is implemented is the Git::Error module. This has
modified some of the code in the original Error module and is used
only when there are calls made to the git system command. Using the
Try::Tiny will mean that this can be simplfied to a very large extent.
As a mater of fact I am in favor of getting rid of this completely and
implementing whatever is required in the Git.pm as required. Because
the Try::Tiny module no longer requires exception objects to be
thrown. Its just simply passing strings around.

This I believe is a big decision, and I would like to hear what you
guys have to say before I actually get along changing and playing
around with stuff inside the code.

Cheers,
Subho.

On Fri, Apr 27, 2012 at 2:01 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,
>
> Subho Banerjee wrote:
>
>> I will take care that I dont break those.
>
> Thanks, sounds good.
>
>>                                           Should the tests in the t/
>> folder of the codebase be enough to make sure everything is working as
>> it should be even in the Git perl module?
>
> No. :)
>
>>                                           Also is there anything like
>> a public build server which actually catalogs which tests are
>> currently failing so that I know what has gone wrong after my changes,
>> or are all commits supposed to pass every test?
>
> When tests are known to fail, they are marked with test_expect_failure
> so they don't affect the test result.
>
> Hope that helps,
> Jonathan

Re: Git.pm

[Jonathan Nieder]

Hi Subho,

Subho Banerjee wrote:

> I have started looking into how the error catching mechanism
> implemented right now. I have looked into the more modern error
> catching/throwing mechanisms in use in perl, and
[...]
> This I believe is a big decision, and I would like to hear what you
> guys have to say before I actually get along changing and playing
> around with stuff inside the code.

I'm cc-ing Jakub in case he has thoughts on this.  Otherwise, my
suggestion would be to make a small trial change and send out a
patch to get feedback.  It's not a big decision until we actually
apply the patch. :)

Hope that helps,
Jonathan

Re: Git.pm

[demerphq]

On 10 May 2012 15:19, Subho Banerjee <subs.zero@gmail.com> wrote:
> Hello,
> I have started looking into how the error catching mechanism
> implemented right now. I have looked into the more modern error
> catching/throwing mechanisms in use in perl, and I am of the opinion
> that Try::Simple would probably be the best candidate for being the
> new error catching mechanism. I also wanted to discuss some aspects of
> the changes to be made -
> ------- Replacing the Error::Simple stuff should be relatively
> straightforward. It can be achieved with simple changes to the syntax
> of the perl module itself.
>
> ------- What I feel will be more complicated, and will require some
> discussion before it is implemented is the Git::Error module. This has
> modified some of the code in the original Error module and is used
> only when there are calls made to the git system command. Using the
> Try::Tiny will mean that this can be simplfied to a very large extent.
> As a mater of fact I am in favor of getting rid of this completely and
> implementing whatever is required in the Git.pm as required. Because
> the Try::Tiny module no longer requires exception objects to be
> thrown. Its just simply passing strings around.
>
> This I believe is a big decision, and I would like to hear what you
> guys have to say before I actually get along changing and playing
> around with stuff inside the code.

Personally I would prefer it just does error handling like any other
standard Perl code does: either return false, or dies with a useful
error message. One of the things I find annoying about Git.pm is it
forces its authors non-standard preferences for exception handling
onto its users.

Any other approach forces people to use the exception framework you
have chosen. Which is just a pain in the ass.

Similar logic for Try::Tiny. Why bother with it? It is pretty close to
a fancy way to write eval { ...; 1 } or do { .... };  It is just one
more module for people to misunderstand, and then make bugs with.

Why require people coding on your module to learn a new way to eval code?

Yes I know in some circles these are probably controversial points,
but in all the core, heavily used Perl code I know of none of it uses
either exception objects nor Try::Tiny. I think there is a reason why.

So think carefully. Look at DBI.pm for guidance. That module is
probably the single most stable, well maintained and widely used
module in Perl. And it does none of the tricks you discuss here.

Yves




-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

Re: Git.pm

[Subho Banerjee]

Hello Yves,
I am aware of that. But you see the problem is that using eval/warn/do
is that the $@ has to be localized every time eval is called. From my
understanding of how the Try::Tiny package works, this is exactly what
happens. So we are just calling a simple eval statement but in a block
where the $@ is handled properly, which is eventually what we would
have to do if we wrote it ourselves(Though I am not sure about how DBI
does it, I will have a look into that). And that is why I arrived in
favor of the Try::Tiny module in the first place. Well, that and the
ability to throw exception objects if required.

Cheers,
Subho.

On Thu, May 10, 2012 at 9:24 PM, demerphq <demerphq@gmail.com> wrote:
> On 10 May 2012 15:19, Subho Banerjee <subs.zero@gmail.com> wrote:
>> Hello,
>> I have started looking into how the error catching mechanism
>> implemented right now. I have looked into the more modern error
>> catching/throwing mechanisms in use in perl, and I am of the opinion
>> that Try::Simple would probably be the best candidate for being the
>> new error catching mechanism. I also wanted to discuss some aspects of
>> the changes to be made -
>> ------- Replacing the Error::Simple stuff should be relatively
>> straightforward. It can be achieved with simple changes to the syntax
>> of the perl module itself.
>>
>> ------- What I feel will be more complicated, and will require some
>> discussion before it is implemented is the Git::Error module. This has
>> modified some of the code in the original Error module and is used
>> only when there are calls made to the git system command. Using the
>> Try::Tiny will mean that this can be simplfied to a very large extent.
>> As a mater of fact I am in favor of getting rid of this completely and
>> implementing whatever is required in the Git.pm as required. Because
>> the Try::Tiny module no longer requires exception objects to be
>> thrown. Its just simply passing strings around.
>>
>> This I believe is a big decision, and I would like to hear what you
>> guys have to say before I actually get along changing and playing
>> around with stuff inside the code.
>
> Personally I would prefer it just does error handling like any other
> standard Perl code does: either return false, or dies with a useful
> error message. One of the things I find annoying about Git.pm is it
> forces its authors non-standard preferences for exception handling
> onto its users.
>
> Any other approach forces people to use the exception framework you
> have chosen. Which is just a pain in the ass.
>
> Similar logic for Try::Tiny. Why bother with it? It is pretty close to
> a fancy way to write eval { ...; 1 } or do { .... };  It is just one
> more module for people to misunderstand, and then make bugs with.
>
> Why require people coding on your module to learn a new way to eval code?
>
> Yes I know in some circles these are probably controversial points,
> but in all the core, heavily used Perl code I know of none of it uses
> either exception objects nor Try::Tiny. I think there is a reason why.
>
> So think carefully. Look at DBI.pm for guidance. That module is
> probably the single most stable, well maintained and widely used
> module in Perl. And it does none of the tricks you discuss here.
>
> Yves
>
>
>
>
> --
> perl -Mre=debug -e "/just|another|perl|hacker/"

Re: Git.pm

[Junio C Hamano]

demerphq <demerphq@gmail.com> writes:

> Similar logic for Try::Tiny. Why bother with it? It is pretty close to
> a fancy way to write eval { ...; 1 } or do { .... };  It is just one
> more module for people to misunderstand, and then make bugs with.

I personally like the approach to stick to bare "eval {}; if ($@) { ... }"
sequence, as it is much more explicit and easier to understand what is
happening underneath.  IOW, I like what I read in demerphq's message.

But it could be that these many people who wrote these different
catch/throw things did so for a reason that I am missing, and if that is
the case, I am interested to hear what benefit we will get from using
them.

"It looks more familiar to people with (your favorite language)" could be
it, but then I would not regret missing such a reason ;-)

Thanks.

Re: Git.pm

[demerphq]

On 10 May 2012 18:18, Subho Banerjee <subs.zero@gmail.com> wrote:
> Hello Yves,
> I am aware of that. But you see the problem is that using eval/warn/do
> is that the $@ has to be localized every time eval is called.

It doesn't *have* to be localized every time. Although it does help a
bit. On the other hand it also hinders.

> From my
> understanding of how the Try::Tiny package works, this is exactly what
> happens. So we are just calling a simple eval statement but in a block
> where the $@ is handled properly, which is eventually what we would
> have to do if we wrote it ourselves(Though I am not sure about how DBI
> does it, I will have a look into that). And that is why I arrived in
> favor of the Try::Tiny module in the first place. Well, that and the
> ability to throw exception objects if required.

As far as I know Try::Tiny exists because of a bug in Perl.

The bug is that since $@ is a global, if an object is created in an
eval, and then its DESTROY itself does an eval it can "hide" the
error, if one uses eval incorrectly. However if one does things
correctly one can detect this case regardless, although perhaps in the
process losing the error message. Ny simply arranging that every eval
statements returns a true value then you can ALWAYS detect when an
eval failed, even if $@ is clobbered.

$ perl -le'sub Foo::DESTROY { print "in DESTROY"; eval 1 } my $ok=
eval q(my $o= bless {}, "Foo"; my $zero=0; print 1/$zero; 1); if ($@)
{ print $@ } if (!$ok) { print "error in eval: ", $@||"Zombie Error"}'
in DESTROY
error in eval: Zombie Error

remove the eval 1 in the DESTROY method:

$ perl -le'sub Foo::DESTROY { print "in DESTROY"; } my $ok= eval q(my
$o= bless {}, "Foo"; my $zero=0; print 1/$zero; 1); if ($@) { print $@
} if (!$ok) { print "error in eval: ", $@||"Zombie Error"}'
in DESTROY
Illegal division by zero at (eval 1) line 1.

error in eval: Illegal division by zero at (eval 1) line 1.

Now it is true that using local does somewhat save things, however
ONLY if EVERYONE uses local when they eval. Which they dont. Nor do
they use Try::Tiny. So in regards to localization the only thing
Try::Tiny saves you from is other people using Try::Tiny or similar
functionality.

Now, it is true that Try::Tiny arranges to check the eval, and it
localizes $@, so I suppose it does save some people from shooting
themselves in the foot. But Perl is most definitely not about not
shooting yourself in the foot, indeed, if you ask it nicely Perl will
hand you a loaded shotgun to make it easier.

Basically all Try::Tiny does is make:

try {
     do_something();
} catch {
     do_something_with_error($_);
};

behave the same as this:

local $@;
eval {
    do_something();
    1;
} or do {
    my $error= $@ || "Zombie Error";
    do_something_with_error($error);
};

Which for me is silly. Id rather see the code than have a module wrap
it up in sub calls, replace $@ with $_ and related junk. Yes this
requires Perl programmers to know their stuff. But then so does any
non-trivial programming task.

Now notice some issues with Try::Tiny, it doesnt support string eval
anyway, so you can just swap it into the code I wrote, nor the general
case of eval. And even if you use it, it doesnt save you if the code
you are executing does not ALSO use it:

$ perl -MTry::Tiny -le'sub Foo::DESTROY { print "in DESTROY"; eval 1 }
my $ok= eval {my $o= bless {}, "Foo"; my $zero=0; print 1/$zero; 1};
if ($@) { print $@ } if (!$ok) { print "error in eval: ", $@||"Zombie
Error"}'
in DESTROY
error in eval: Zombie Error
$ perl -MTry::Tiny -le'sub Foo::DESTROY { print "in DESTROY"; eval 1 }
my $ok= try {my $o= bless {}, "Foo"; my $zero=0; print 1/$zero; 1}; if
($@) { print $@ } if (!$ok) { print "error in eval: ", $@||"Zombie
Error"}'
in DESTROY
error in eval: Zombie Error
$ perl -MTry::Tiny -le'sub Foo::DESTROY { print "in DESTROY"; eval 1 }
my $ok= try {my $o= bless {}, "Foo"; my $zero=0; print 1/$zero; 1}
catch { print "error in eval: ", $_||"Zombie Error"}'
in DESTROY
error in eval: Zombie Error

Notice how it provides no more benefit than writing eval like this:

eval {
     stuff();
     1; # this is important
} or do {
     my $error= $@ || "zombie error"; # must do this as early as possible
     do_something_with_error($error);
};

And for me, if I see code written like this I KNOW it works, i know
what side effects it has (localizing $@ has its own issues), and I
know I can understand it.

So for me Try::Tiny is a waste of time. Using it saves you from almost
nothing, and just forces consumers of your code to know how to do eval
properly AND how Try::Tiny works. Not using Try::Tiny at all means
people JUST have to know how to use eval properly, which if they want
to do any kind of real Perl work they need to know anyway.

Anyway, on the side of annoying you about Perl trivia, you can find a
lot of reusable library code in git-deploy on github. Feel free to
steal whatever you like.

  https://github.com/git-deploy

I would have loved to have a better git library module than Git.pm
when I wrote the original version of git-deploy. So I am all in favour
of your project.

cheers,
Yves








-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

Re: Git.pm

[demerphq]

On 10 May 2012 18:20, Junio C Hamano <gitster@pobox.com> wrote:
> demerphq <demerphq@gmail.com> writes:
>
>> Similar logic for Try::Tiny. Why bother with it? It is pretty close to
>> a fancy way to write eval { ...; 1 } or do { .... };  It is just one
>> more module for people to misunderstand, and then make bugs with.
>
> I personally like the approach to stick to bare "eval {}; if ($@) { ... }"
> sequence, as it is much more explicit and easier to understand what is
> happening underneath.  IOW, I like what I read in demerphq's message.

Basically that is the idiom that Try::Tiny encourages people not to
use. Unfortunately the idiom documented in the Perl docs has been
subtly wrong for pretty much ever due to a subtle bug in perl (which
took years to come to light). :-(

Anyway, anything written like this:

eval {
       whatever();
       1;
} or do {
    my $error= $@ || "Zombie Error";
    do_something_with_error($error);
};

is fine. See my other post to the list for details.

> But it could be that these many people who wrote these different
> catch/throw things did so for a reason that I am missing, and if that is
> the case, I am interested to hear what benefit we will get from using
> them.
>
> "It looks more familiar to people with (your favorite language)" could be
> it, but then I would not regret missing such a reason ;-)

A few might justify things based on the bug in Perl, but I suspect the
real motivation people have to use it is to emulate other languages
constructs.

The author of Try::Tiny might be an exception, I am reasonably
convinced he was trying to provide a service to the community, but IMO
on the balance of things the module muddies the water more than it
improves things.

cheers,
Yves


-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

Re: Git.pm

[Andrew Sayers]

Try::Tiny is an increasingly standard part of Perl - for example, it's
used extensively in Moose.  There's a good list of arguments about why
you should use it instead of eval in the Try::Tiny documentation:
http://search.cpan.org/~nuffin/Try-Tiny-0.01/lib/Try/Tiny.pm

Now I've got that talking point done, here's what I really think :)

Try::Tiny is designed on the assumption that throwing and catching
objects is something people should do all the time, and it can cause
subtle errors that are only worth the hassle if you get a lot of benefit
from doing so.  It's easy enough to come up with ideas for where they
might be useful, but in the real world advanced uses for exceptions are
usually a sign you're doing it wrong.  Three of the most common reasons
for frequent/complex exceptions are handling errors further up the call
stack, recovering from operations that fail, and clever error-handling.


If you want exceptions to be caught by code further up the call stack
than the immediate caller, you're likely to be disappointed.  This is
one of the places where "separation of concerns" applies - if I use a
module that uses a module that uses your module, then catching
exceptions from your code will just cause my program to break when some
module in the middle obscures your error by adding its own layer of
error handling.


If you have an operation that really might fail, and you want to
encourage most people to handle it most of the time, it's better to have
a function with a meaningful name and good documentation.  This puts the
burden on the calling function to handle the error instead of letting
them think "oh well, if it dies someone else will handle it".  It also
forces you to split functions along boundaries that make your code
readable, instead of falling for the temptation to make something that
"just works"... until it doesn't, and the maintainer has to go
spelunking through code they don't know.  So instead of:

   try {
       Foo::frobnicate( widgets => 3 );
   } catch {
      if (ref($_) eq 'Error::Widget') {
          die "Could not add 3 widgets";
      }
   }

It's better to ask the people using your module to write:

   my $foo = Foo->new;
   $foo->add_widgets(3) or die "could not add 3 widgets"
   $foo->frobnicate;

This is easier to document, easier to write and easier to read.


If you have an operation where calling code is supposed to do something
more complicated than give up, it's better to use a callback.  This
gives you an opportunity to document what's needed, and to check that
the calling code is doing the right thing before it's too late.  So
instead of:

    my $widgets = 3;
    while ( $widgets ) {

        try {
            Foo::frobnicate($widgets);
            $widgets = 0;
        } catch {
            if ( $_->{remaining_widgets} < 2 ) {
                die $_->{error};
            } elsif ( $_->{remaining_widgets} == 2 ) {
                $widgets = 0;
            }
        }

    }

It's better to ask people using your module to write:

    Foo::Frobnicate(
        widgets => 3,
        error_handler => sub {
             my ( $remaining_widgets, $error ) = @_;
             die $error if $broken_widgets < 2;
             return "give up" if $remaining_widgets == 2;
             return "continue";
        },
    );

Again, this is more readable and easier to document.


Aside from the philosophical angle, Try::Tiny is particularly hard to
maintain because it looks like a language extension, but is actually
just an ordinary module.  The try {} and catch {} blocks are anonymous
subroutines, which lead to some wonderfully unintuitive behaviour.  See
what you think these do, then run the code to find out:


    sub foo {
        try {
            return 1;
        }
        return 0;
    }

    sub bar {

        our @args = @_;
        our @ret;

        try {
            @ret =
                wantarray
                    ?   grep( /blah/, @args )
                    : [ grep( /blah/, @args ) ]
        };

        return @ret;
    }

    my $foo = "bar";
    sub baz {
        my $foo = "baz";
        try {
            print $foo;
        }
    }

    sub qux {
        my $ret;
        try {
            $ret = "value";
        }
        return $ret;
    }

    print foo, "\n";
    print bar( "blah", "blip" ), "\n";
    baz;
    print qux, "\n";


In short, Try::Tiny looks like a lot of gain for not much pain, but
actually it's the other way around.

	- Andrew

Re: Git.pm

[demerphq]

On 10 May 2012 22:55, Andrew Sayers <andrew-git@pileofstuff.org> wrote:
> Try::Tiny is an increasingly standard part of Perl - for example, it's
> used extensively in Moose.  There's a good list of arguments about why
> you should use it instead of eval in the Try::Tiny documentation:
> http://search.cpan.org/~nuffin/Try-Tiny-0.01/lib/Try/Tiny.pm
>
> Now I've got that talking point done, here's what I really think :)
>
> Try::Tiny is designed on the assumption that throwing and catching
> objects is something people should do all the time, and it can cause
> subtle errors that are only worth the hassle if you get a lot of benefit
> from doing so.  It's easy enough to come up with ideas for where they
> might be useful, but in the real world advanced uses for exceptions are
> usually a sign you're doing it wrong.  Three of the most common reasons
> for frequent/complex exceptions are handling errors further up the call
> stack, recovering from operations that fail, and clever error-handling.
>
>
> If you want exceptions to be caught by code further up the call stack
> than the immediate caller, you're likely to be disappointed.  This is
> one of the places where "separation of concerns" applies - if I use a
> module that uses a module that uses your module, then catching
> exceptions from your code will just cause my program to break when some
> module in the middle obscures your error by adding its own layer of
> error handling.
>
>
> If you have an operation that really might fail, and you want to
> encourage most people to handle it most of the time, it's better to have
> a function with a meaningful name and good documentation.  This puts the
> burden on the calling function to handle the error instead of letting
> them think "oh well, if it dies someone else will handle it".  It also
> forces you to split functions along boundaries that make your code
> readable, instead of falling for the temptation to make something that
> "just works"... until it doesn't, and the maintainer has to go
> spelunking through code they don't know.  So instead of:
>
>   try {
>       Foo::frobnicate( widgets => 3 );
>   } catch {
>      if (ref($_) eq 'Error::Widget') {
>          die "Could not add 3 widgets";
>      }
>   }
>
> It's better to ask the people using your module to write:
>
>   my $foo = Foo->new;
>   $foo->add_widgets(3) or die "could not add 3 widgets"
>   $foo->frobnicate;
>
> This is easier to document, easier to write and easier to read.
>
>
> If you have an operation where calling code is supposed to do something
> more complicated than give up, it's better to use a callback.  This
> gives you an opportunity to document what's needed, and to check that
> the calling code is doing the right thing before it's too late.  So
> instead of:
>
>    my $widgets = 3;
>    while ( $widgets ) {
>
>        try {
>            Foo::frobnicate($widgets);
>            $widgets = 0;
>        } catch {
>            if ( $_->{remaining_widgets} < 2 ) {
>                die $_->{error};
>            } elsif ( $_->{remaining_widgets} == 2 ) {
>                $widgets = 0;
>            }
>        }
>
>    }
>
> It's better to ask people using your module to write:
>
>    Foo::Frobnicate(
>        widgets => 3,
>        error_handler => sub {
>             my ( $remaining_widgets, $error ) = @_;
>             die $error if $broken_widgets < 2;
>             return "give up" if $remaining_widgets == 2;
>             return "continue";
>        },
>    );
>
> Again, this is more readable and easier to document.
>
>
> Aside from the philosophical angle, Try::Tiny is particularly hard to
> maintain because it looks like a language extension, but is actually
> just an ordinary module.  The try {} and catch {} blocks are anonymous
> subroutines, which lead to some wonderfully unintuitive behaviour.  See
> what you think these do, then run the code to find out:
>
>
>    sub foo {
>        try {
>            return 1;
>        }
>        return 0;
>    }
>
>    sub bar {
>
>        our @args = @_;
>        our @ret;
>
>        try {
>            @ret =
>                wantarray
>                    ?   grep( /blah/, @args )
>                    : [ grep( /blah/, @args ) ]
>        };
>
>        return @ret;
>    }
>
>    my $foo = "bar";
>    sub baz {
>        my $foo = "baz";
>        try {
>            print $foo;
>        }
>    }
>
>    sub qux {
>        my $ret;
>        try {
>            $ret = "value";
>        }
>        return $ret;
>    }
>
>    print foo, "\n";
>    print bar( "blah", "blip" ), "\n";
>    baz;
>    print qux, "\n";
>
>
> In short, Try::Tiny looks like a lot of gain for not much pain, but
> actually it's the other way around.

Total agreement.

Yves
-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

Re: Git.pm

[Randal L. Schwartz]

>>>>> "Subho" == Subho Banerjee <subs.zero@gmail.com> writes:

Subho> I have started looking into how the error catching mechanism
Subho> implemented right now. I have looked into the more modern error
Subho> catching/throwing mechanisms in use in perl, and I am of the opinion
Subho> that Try::Simple would probably be the best candidate for being the
Subho> new error catching mechanism. I also wanted to discuss some aspects of
Subho> the changes to be made -

Try::Tiny is preferred to Try::Simple.

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Smalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.
See http://methodsandmessages.posterous.com/ for Smalltalk discussion

Re: Git.pm

[Junio C Hamano]

merlyn@stonehenge.com (Randal L. Schwartz) writes:

>>>>>> "Subho" == Subho Banerjee <subs.zero@gmail.com> writes:
>
> Subho> I have started looking into how the error catching mechanism
> Subho> implemented right now. I have looked into the more modern error
> Subho> catching/throwing mechanisms in use in perl, and I am of the opinion
> Subho> that Try::Simple would probably be the best candidate for being the
> Subho> new error catching mechanism. I also wanted to discuss some aspects of
> Subho> the changes to be made -
>
> Try::Tiny is preferred to Try::Simple.

Thanks for an expert input; could you give another comparison between
Try::Tiny vs the use of bare "eval / if ($@)"?

[PATCH][GIT.PM 1/3] Ignore files produced from exuberant-ctags

[Subho Sankar Banerjee]

Signed-off-by: Subho Sankar Banerjee <subs.zero@gmail.com>
---
 .gitignore |    1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index fb7d559..8ecd336 100644
--- a/.gitignore
+++ b/.gitignore
@@ -219,6 +219,7 @@
 /configure
 /tags
 /TAGS
+tags
 /cscope*
 *.obj
 *.lib
-- 
1.7.9.5

[PATCH][GIT.PM 2/3] Getting rid of throwing Error::Simple objects in favour of simple Perl scalars which can be caught in eval{} blocks

[Subho Sankar Banerjee]

Signed-off-by: Subho Sankar Banerjee <subs.zero@gmail.com>
---
 perl/Git.pm |   52 ++++++++++++++++++++++++++--------------------------
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 497f420..52777d4 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -160,7 +160,7 @@ sub repository {
 	if (defined $args[0]) {
 		if ($#args % 2 != 1) {
 			# Not a hash.
-			$#args == 0 or throw Error::Simple("bad usage");
+			$#args == 0 or die "bad usage";
 			%opts = ( Directory => $args[0] );
 		} else {
 			%opts = @args;
@@ -173,7 +173,7 @@ sub repository {
 	}
 
 	if (defined $opts{Directory}) {
-		-d $opts{Directory} or throw Error::Simple("Directory not found: $opts{Directory} $!");
+		-d $opts{Directory} or die "Directory not found: $opts{Directory} $!";
 
 		my $search = Git->repository(WorkingCopy => $opts{Directory});
 		my $dir;
@@ -193,7 +193,7 @@ sub repository {
 			$dir = abs_path($opts{Directory}) . '/';
 			if ($prefix) {
 				if (substr($dir, -length($prefix)) ne $prefix) {
-					throw Error::Simple("rev-parse confused me - $dir does not have trailing $prefix");
+					die "rev-parse confused me - $dir does not have trailing $prefix";
 				}
 				substr($dir, -length($prefix)) = '';
 			}
@@ -206,14 +206,14 @@ sub repository {
 
 			unless (-d "$dir/refs" and -d "$dir/objects" and -e "$dir/HEAD") {
 				# Mimic git-rev-parse --git-dir error message:
-				throw Error::Simple("fatal: Not a git repository: $dir");
+				die "fatal: Not a git repository: $dir";
 			}
 			my $search = Git->repository(Repository => $dir);
 			try {
 				$search->command('symbolic-ref', 'HEAD');
 			} catch Git::Error::Command with {
 				# Mimic git-rev-parse --git-dir error message:
-				throw Error::Simple("fatal: Not a git repository: $dir");
+				die "fatal: Not a git repository: $dir";
 			}
 
 			$opts{Repository} = abs_path($dir);
@@ -469,7 +469,7 @@ sub command_noisy {
 
 	my $pid = fork;
 	if (not defined $pid) {
-		throw Error::Simple("fork failed: $!");
+		die "fork failed: $!";
 	} elsif ($pid == 0) {
 		_cmd_exec($self, $cmd, @args);
 	}
@@ -552,10 +552,10 @@ and the directory must exist.
 sub wc_chdir {
 	my ($self, $subdir) = @_;
 	$self->wc_path()
-		or throw Error::Simple("bare repository");
+		or die "bare repository";
 
 	-d $self->wc_path().'/'.$subdir
-		or throw Error::Simple("subdir not found: $subdir $!");
+		or die "subdir not found: $subdir $!";
 	# Of course we will not "hold" the subdirectory so anyone
 	# can delete it now and we will never know. But at least we tried.
 
@@ -825,13 +825,13 @@ sub hash_and_insert_object {
 
 	unless (print $out $filename, "\n") {
 		$self->_close_hash_and_insert_object();
-		throw Error::Simple("out pipe went bad");
+		die "out pipe went bad";
 	}
 
 	chomp(my $hash = <$in>);
 	unless (defined($hash)) {
 		$self->_close_hash_and_insert_object();
-		throw Error::Simple("in pipe went bad");
+		die "in pipe went bad";
 	}
 
 	return $hash;
@@ -873,7 +873,7 @@ sub cat_blob {
 
 	unless (print $out $sha1, "\n") {
 		$self->_close_cat_blob();
-		throw Error::Simple("out pipe went bad");
+		die "out pipe went bad";
 	}
 
 	my $description = <$in>;
@@ -900,7 +900,7 @@ sub cat_blob {
 		my $read = read($in, $blob, $bytesToRead, $bytesRead);
 		unless (defined($read)) {
 			$self->_close_cat_blob();
-			throw Error::Simple("in pipe went bad");
+			die "in pipe went bad";
 		}
 
 		$bytesRead += $read;
@@ -911,16 +911,16 @@ sub cat_blob {
 	my $read = read($in, $newline, 1);
 	unless (defined($read)) {
 		$self->_close_cat_blob();
-		throw Error::Simple("in pipe went bad");
+		die "in pipe went bad";
 	}
 	unless ($read == 1 && $newline eq "\n") {
 		$self->_close_cat_blob();
-		throw Error::Simple("didn't find newline after blob");
+		die "didn't find newline after blob";
 	}
 
 	unless (print $fh $blob) {
 		$self->_close_cat_blob();
-		throw Error::Simple("couldn't write to passed in filehandle");
+    die "couldn't write to passed in filehandle";
 	}
 
 	return $size;
@@ -1023,8 +1023,8 @@ sub _temp_cache {
 	my $temp_fd = \$TEMP_FILEMAP{$name};
 	if (defined $$temp_fd and $$temp_fd->opened) {
 		if ($TEMP_FILES{$$temp_fd}{locked}) {
-			throw Error::Simple("Temp file with moniker '" .
-				$name . "' already in use");
+			die "Temp file with moniker '" .
+				$name . "' already in use";
 		}
 	} else {
 		if (defined $$temp_fd) {
@@ -1041,7 +1041,7 @@ sub _temp_cache {
 
 		($$temp_fd, $fname) = File::Temp->tempfile(
 			'Git_XXXXXX', UNLINK => 1, DIR => $tmpdir,
-			) or throw Error::Simple("couldn't open new temp file");
+			) or die "couldn't open new temp file";
 
 		$$temp_fd->autoflush;
 		binmode $$temp_fd;
@@ -1052,7 +1052,7 @@ sub _temp_cache {
 
 sub _verify_require {
 	eval { require File::Temp; require File::Spec; };
-	$@ and throw Error::Simple($@);
+	$@ and die "$@";
 }
 
 =item temp_reset ( FILEHANDLE )
@@ -1065,11 +1065,11 @@ sub temp_reset {
 	my ($self, $temp_fd) = _maybe_self(@_);
 
 	truncate $temp_fd, 0
-		or throw Error::Simple("couldn't truncate file");
+		or die "couldn't truncate file";
 	sysseek($temp_fd, 0, SEEK_SET) and seek($temp_fd, 0, SEEK_SET)
-		or throw Error::Simple("couldn't seek to beginning of file");
+		or die "couldn't seek to beginning of file";
 	sysseek($temp_fd, 0, SEEK_CUR) == 0 and tell($temp_fd) == 0
-		or throw Error::Simple("expected file position to be reset");
+		or die "expected file position to be reset";
 }
 
 =item temp_path ( NAME )
@@ -1100,8 +1100,8 @@ sub END {
 =head1 ERROR HANDLING
 
 All functions are supposed to throw Perl exceptions in case of errors.
-See the L<Error> module on how to catch those. Most exceptions are mere
-L<Error::Simple> instances.
+These errors are perl scalars which can be caught in the $@ values in
+eval{} blocks.
 
 However, the C<command()>, C<command_oneline()> and C<command_noisy()>
 functions suite can throw C<Git::Error::Command> exceptions as well: those are
@@ -1227,7 +1227,7 @@ sub _maybe_self {
 # Check if the command id is something reasonable.
 sub _check_valid_cmd {
 	my ($cmd) = @_;
-	$cmd =~ /^[a-z0-9A-Z_-]+$/ or throw Error::Simple("bad command: $cmd");
+	$cmd =~ /^[a-z0-9A-Z_-]+$/ or die "bad command: $cmd";
 }
 
 # Common backend for the pipe creators.
@@ -1261,7 +1261,7 @@ sub _command_common_pipe {
 	} else {
 		my $pid = open($fh, $direction);
 		if (not defined $pid) {
-			throw Error::Simple("open failed: $!");
+			die "open failed: $!";
 		} elsif ($pid == 0) {
 			if (defined $opts{STDERR}) {
 				close STDERR;
-- 
1.7.9.5

[PATCH][GIT.PM 3/3] Perl code uses eval{}/die instead of Error::Simple and Git::Error::Command

[Subho Sankar Banerjee]

Signed-off-by: Subho Sankar Banerjee <subs.zero@gmail.com>
---
 git-send-email.perl |    7 +--
 git-svn.perl        |    2 +-
 perl/Git.pm         |  170 ++++++++++++---------------------------------------
 3 files changed, 41 insertions(+), 138 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index ef30c55..e56b379 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -26,7 +26,6 @@ use Data::Dumper;
 use Term::ANSIColor;
 use File::Temp qw/ tempdir tempfile /;
 use File::Spec::Functions qw(catfile);
-use Error qw(:try);
 use Git;
 
 Getopt::Long::Configure qw/ pass_through /;
@@ -512,7 +511,7 @@ if (@alias_files and $aliasfiletype and defined $parse_alias{$aliasfiletype}) {
 sub check_file_rev_conflict($) {
 	return unless $repo;
 	my $f = shift;
-	try {
+	eval {
 		$repo->command('rev-parse', '--verify', '--quiet', $f);
 		if (defined($format_patch)) {
 			return $format_patch;
@@ -524,9 +523,7 @@ to produce patches for.  Please disambiguate by...
     * Saying "./$f" if you mean a file; or
     * Giving --format-patch option if you mean a range.
 EOF
-	} catch Git::Error::Command with {
-		return 0;
-	}
+	} or return 0;
 }
 
 # Now that all the defaults are set, process the rest of the command line
diff --git a/git-svn.perl b/git-svn.perl
index 31d02b5..c299137 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -3714,7 +3714,7 @@ sub find_extra_svn_parents {
 		};
 		if ($@) {
 			die "An error occurred during merge-base"
-				unless $@->isa("Git::Error::Command");
+				unless $@ =~ /command returned error/;
 
 			warn "W: Cannot find common ancestor between ".
 			     "@$parents and $merge_tip. Ignoring merge info.\n";
diff --git a/perl/Git.pm b/perl/Git.pm
index 52777d4..a025f5d 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -97,8 +97,7 @@ increase notwithstanding).
 =cut
 
 
-use Carp qw(carp croak); # but croak is bad - throw instead
-use Error qw(:try);
+use Carp qw(carp croak);
 use Cwd qw(abs_path cwd);
 use IPC::Open2 qw(open2);
 use Fcntl qw(SEEK_SET SEEK_CUR);
@@ -123,7 +122,6 @@ B<Repository> - Path to the Git repository.
 
 B<WorkingCopy> - Path to the associated working copy; not strictly required
 as many commands will happily crunch on a bare repository.
-
 B<WorkingSubdir> - Subdirectory in the working copy to work inside.
 Just left undefined if you do not want to limit the scope of operations.
 
@@ -160,7 +158,7 @@ sub repository {
 	if (defined $args[0]) {
 		if ($#args % 2 != 1) {
 			# Not a hash.
-			$#args == 0 or die "bad usage";
+			$#args == 0 or croak "bad usage";
 			%opts = ( Directory => $args[0] );
 		} else {
 			%opts = @args;
@@ -177,12 +175,10 @@ sub repository {
 
 		my $search = Git->repository(WorkingCopy => $opts{Directory});
 		my $dir;
-		try {
+		eval {
 			$dir = $search->command_oneline(['rev-parse', '--git-dir'],
-			                                STDERR => 0);
-		} catch Git::Error::Command with {
-			$dir = undef;
-		};
+			                                STDERR => 0);1;
+		} or $dir = undef and print $@;
 
 		if ($dir) {
 			$dir =~ m#^/# or $dir = $opts{Directory} . '/' . $dir;
@@ -206,15 +202,12 @@ sub repository {
 
 			unless (-d "$dir/refs" and -d "$dir/objects" and -e "$dir/HEAD") {
 				# Mimic git-rev-parse --git-dir error message:
-				die "fatal: Not a git repository: $dir";
+				croak "fatal: Not a git repository: $dir";
 			}
 			my $search = Git->repository(Repository => $dir);
-			try {
-				$search->command('symbolic-ref', 'HEAD');
-			} catch Git::Error::Command with {
-				# Mimic git-rev-parse --git-dir error message:
-				die "fatal: Not a git repository: $dir";
-			}
+			eval {
+				$search->command('symbolic-ref', 'HEAD');1;
+			} or croak "fatal: Not a git repository: $dir"; # Mimic git-rev-parse --git-dir error message
 
 			$opts{Repository} = abs_path($dir);
 		}
@@ -265,33 +258,20 @@ In both cases, the command's stdin and stderr are the same as the caller's.
 sub command {
 	my ($fh, $ctx) = command_output_pipe(@_);
 
+	local $@;
 	if (not defined wantarray) {
-		# Nothing to pepper the possible exception with.
 		_cmd_close($fh, $ctx);
 
 	} elsif (not wantarray) {
 		local $/;
 		my $text = <$fh>;
-		try {
-			_cmd_close($fh, $ctx);
-		} catch Git::Error::Command with {
-			# Pepper with the output:
-			my $E = shift;
-			$E->{'-outputref'} = \$text;
-			throw $E;
-		};
+	eval { _cmd_close($fh, $ctx); 1; } or die $@;
 		return $text;
 
 	} else {
 		my @lines = <$fh>;
 		defined and chomp for @lines;
-		try {
-			_cmd_close($fh, $ctx);
-		} catch Git::Error::Command with {
-			my $E = shift;
-			$E->{'-outputref'} = \@lines;
-			throw $E;
-		};
+		eval { _cmd_close($fh, $ctx); 1; } or die $@;
 		return @lines;
 	}
 }
@@ -312,14 +292,7 @@ sub command_oneline {
 
 	my $line = <$fh>;
 	defined $line and chomp $line;
-	try {
-		_cmd_close($fh, $ctx);
-	} catch Git::Error::Command with {
-		# Pepper with the output:
-		my $E = shift;
-		$E->{'-outputref'} = \$line;
-		throw $E;
-	};
+	eval { _cmd_close($fh, $ctx); }	or die $@;
 	return $line;
 }
 
@@ -436,7 +409,7 @@ sub command_close_bidi_pipe {
 			if ($!) {
 				carp "error closing pipe: $!";
 			} elsif ($? >> 8) {
-				throw Git::Error::Command($ctx, $? >>8);
+				die $ctx." : command returned error : ".($? >> 8)."\n";
 			}
 		}
 	}
@@ -444,7 +417,7 @@ sub command_close_bidi_pipe {
 	waitpid $pid, 0;
 
 	if ($? >> 8) {
-		throw Git::Error::Command($ctx, $? >>8);
+		die $ctx." : command returned error : ".($? >> 8)."\n";
 	}
 }
 
@@ -474,7 +447,7 @@ sub command_noisy {
 		_cmd_exec($self, $cmd, @args);
 	}
 	if (waitpid($pid, 0) > 0 and $?>>8 != 0) {
-		throw Git::Error::Command(join(' ', $cmd, @args), $? >> 8);
+		die join(' ', $cmd, @args)." : command returned error : ".($? >> 8)."\n";
 	}
 }
 
@@ -552,10 +525,10 @@ and the directory must exist.
 sub wc_chdir {
 	my ($self, $subdir) = @_;
 	$self->wc_path()
-		or die "bare repository";
+		or croak "bare repository";
 
 	-d $self->wc_path().'/'.$subdir
-		or die "subdir not found: $subdir $!";
+		or croak "subdir not found: $subdir $!";
 	# Of course we will not "hold" the subdirectory so anyone
 	# can delete it now and we will never know. But at least we tried.
 
@@ -629,8 +602,9 @@ sub config_int {
 sub _config_common {
 	my ($opts) = shift @_;
 	my ($self, $var) = _maybe_self(@_);
-
-	try {
+	
+	local $@;
+	eval {
 		my @cmd = ('config', $opts->{'kind'} ? $opts->{'kind'} : ());
 		unshift @cmd, $self if $self;
 		if (wantarray) {
@@ -638,15 +612,9 @@ sub _config_common {
 		} else {
 			return command_oneline(@cmd, '--get', $var);
 		}
-	} catch Git::Error::Command with {
-		my $E = shift;
-		if ($E->value() == 1) {
-			# Key not found.
-			return;
-		} else {
-			throw $E;
-		}
-	};
+		1;
+	} or $@ =~ /([\d]+)$/ and (0+$1 == 1) or die $@;
+	return;
 }
 
 =item get_colorbool ( NAME )
@@ -920,7 +888,7 @@ sub cat_blob {
 
 	unless (print $fh $blob) {
 		$self->_close_cat_blob();
-    die "couldn't write to passed in filehandle";
+		die "couldn't write to passed in filehandle";
 	}
 
 	return $size;
@@ -1023,7 +991,7 @@ sub _temp_cache {
 	my $temp_fd = \$TEMP_FILEMAP{$name};
 	if (defined $$temp_fd and $$temp_fd->opened) {
 		if ($TEMP_FILES{$$temp_fd}{locked}) {
-			die "Temp file with moniker '" .
+			croak "Temp file with moniker '" .
 				$name . "' already in use";
 		}
 	} else {
@@ -1103,72 +1071,13 @@ All functions are supposed to throw Perl exceptions in case of errors.
 These errors are perl scalars which can be caught in the $@ values in
 eval{} blocks.
 
-However, the C<command()>, C<command_oneline()> and C<command_noisy()>
-functions suite can throw C<Git::Error::Command> exceptions as well: those are
-thrown when the external command returns an error code and contain the error
-code as well as access to the captured command's output. The exception class
-provides the usual C<stringify> and C<value> (command's exit code) methods and
-in addition also a C<cmd_output> method that returns either an array or a
-string with the captured command output (depending on the original function
-call context; C<command_noisy()> returns C<undef>) and $<cmdline> which
-returns the command and its arguments (but without proper quoting).
-
-Note that the C<command_*_pipe()> functions cannot throw this exception since
-it has no idea whether the command failed or not. You will only find out
-at the time you C<close> the pipe; if you want to have that automated,
-use C<command_close_pipe()>, which can throw the exception.
-
 =cut
 
-{
-	package Git::Error::Command;
-
-	@Git::Error::Command::ISA = qw(Error);
-
-	sub new {
-		my $self = shift;
-		my $cmdline = '' . shift;
-		my $value = 0 + shift;
-		my $outputref = shift;
-		my(@args) = ();
-
-		local $Error::Depth = $Error::Depth + 1;
-
-		push(@args, '-cmdline', $cmdline);
-		push(@args, '-value', $value);
-		push(@args, '-outputref', $outputref);
-
-		$self->SUPER::new(-text => 'command returned error', @args);
-	}
-
-	sub stringify {
-		my $self = shift;
-		my $text = $self->SUPER::stringify;
-		$self->cmdline() . ': ' . $text . ': ' . $self->value() . "\n";
-	}
-
-	sub cmdline {
-		my $self = shift;
-		$self->{'-cmdline'};
-	}
-
-	sub cmd_output {
-		my $self = shift;
-		my $ref = $self->{'-outputref'};
-		defined $ref or undef;
-		if (ref $ref eq 'ARRAY') {
-			return @$ref;
-		} else { # SCALAR
-			return $$ref;
-		}
-	}
-}
-
 =over 4
 
 =item git_cmd_try { CODE } ERRMSG
 
-This magical statement will automatically catch any C<Git::Error::Command>
+This magical statement will automatically catch any 
 exceptions thrown by C<CODE> and make your program die with C<ERRMSG>
 on its lips; the message will have %s substituted for the command line
 and %d for the exit status. This statement is useful mostly for producing
@@ -1184,22 +1093,20 @@ sub git_cmd_try(&$) {
 	my ($code, $errmsg) = @_;
 	my @result;
 	my $err;
+	my $err_string;
+	my $err_value;
 	my $array = wantarray;
-	try {
+	eval {
 		if ($array) {
 			@result = &$code;
 		} else {
 			$result[0] = &$code;
-		}
-	} catch Git::Error::Command with {
-		my $E = shift;
-		$err = $errmsg;
-		$err =~ s/\%s/$E->cmdline()/ge;
-		$err =~ s/\%d/$E->value()/ge;
-		# We can't croak here since Error.pm would mangle
-		# that to Error::Simple.
-	};
-	$err and croak $err;
+		}1;
+	} or $@ =~ /^([a-zA-Z ]+) : .* : ([\d]+)$/
+	and ($err, $err_string, $err_value) = ($errmsg, $1, $2)
+	and $err =~ s/\%s/$1/ge and $err =~ s/\%d/$2/ge; 
+  
+ 	$err and croak $err;
 	return $array ? @result : $result[0];
 }
 
@@ -1227,7 +1134,7 @@ sub _maybe_self {
 # Check if the command id is something reasonable.
 sub _check_valid_cmd {
 	my ($cmd) = @_;
-	$cmd =~ /^[a-z0-9A-Z_-]+$/ or die "bad command: $cmd";
+	$cmd =~ /^[a-z0-9A-Z_-]+$/ or croak "bad command: $cmd";
 }
 
 # Common backend for the pipe creators.
@@ -1309,8 +1216,7 @@ sub _cmd_close {
 			# It's just close, no point in fatalities
 			carp "error closing pipe: $!";
 		} elsif ($? >> 8) {
-			# The caller should pepper this.
-			throw Git::Error::Command($ctx, $? >> 8);
+			die $ctx." : command returned error : ".($? >> 8)."\n";
 		}
 		# else we might e.g. closed a live stream; the command
 		# dying of SIGPIPE would drive us here.
-- 
1.7.9.5

Re: [PATCH][GIT.PM 2/3] Getting rid of throwing Error::Simple objects in favour of simple Perl scalars which can be caught in eval{} blocks

[Andrew Sayers]

I'll limit myself to a style review here - other people can say better
than me about the deeper issues.

On 19/05/12 08:08, Subho Sankar Banerjee wrote:
<snip>
> @@ -160,7 +160,7 @@ sub repository {
>  	if (defined $args[0]) {
>  		if ($#args % 2 != 1) {
>  			# Not a hash.
> -			$#args == 0 or throw Error::Simple("bad usage");
> +			$#args == 0 or die "bad usage";

This is valid and no worse than before, but I find this use of the "or"
operator slightly confusing.  I find it easier to read either:

	<verb> or <fail>
	OR:
	<fail> unless <noun>

For example:

	do_something($foo) or die "couldn't do_something with '$foo'";
	OR:
	die "'$foo' is not a something" unless is_something($foo);

<snip>
> @@ -1041,7 +1041,7 @@ sub _temp_cache {
>  
>  		($$temp_fd, $fname) = File::Temp->tempfile(
>  			'Git_XXXXXX', UNLINK => 1, DIR => $tmpdir,
> -			) or throw Error::Simple("couldn't open new temp file");
> +			) or die "couldn't open new temp file";

This is a good example of where I think "or" is appropriate.

Think of it in terms of an English sentence.  Which of these would you
find easier to read:

	It is raining or go out and play
	OR:
	Go out and play unless it is raining

	Find your umbrella or cancel the trip
	OR:
	Cancel the trip unless find your umbrella


A bit of background for people who aren't (primarily) Perl programmers:

As an expressive language that promotes "more than one way to do it",
Perl has a long tradition of supporting many redundant ways of spelling
"if (...) { ... }".  Common examples include:

	if ( $x ) { do_something() }
	do_something() if $x;

	unless ( $x ) { do_something() }
	do_something() unless $x;

	$x && do_something();
	$x || do_something();

	$x and do_something();
	$x or do_something();

Sometimes people find very practical reasons why these aren't good
programming practice, but the rest of the time everyone just argues
about whether they're good grammar.

The "&&" and "||" operators are an example of bad programming practice -
these operators have relatively high precedence, so tend to behave
unintuitively when used in (often regrettably) complex ways.  The "and"
and "or" operators behave just like "&&" and "||", but with a precedence
low enough to avoid weirdness.  See [1] for an example.

Some people consider anything but a traditional prefix-if() statement to
be bad grammar (I believe "Perl Best Practices" makes the argument,
which is definitive for many people).  Other people say anything in the
language is by definition fair game.  The rest of us spend a lot of time
making arguments like the above, and frankly I think we gain more from
the debate than the conclusion.

	- Andrew

[1] http://perldoc.perl.org/perlop.html#C-style-Logical-Defined-Or

Re: [PATCH][GIT.PM 2/3] Getting rid of throwing Error::Simple objects in favour of simple Perl scalars which can be caught in eval{} blocks

[Subho Banerjee]

Hi,
The semantic
>        <fail> unless <noun>
works well when the <fail> part of the code is a singular statement.
But it is of ungainly when there are a couple of statements to be
executed as a block. In this case, I believe that a the conjunctive
,,or''/,,and'' statement makes more sense. In the sense -
                 <verb1> or <die_gracefully1> and <die_gracefully2>
I believe this is easier to read compared to -
                 <die_gracefully1> and <die_gracefully2> unless <noun>
especially if you have a larger block of commands to execute in case
of the failure. I believe the easiest to read would be a classical C
styled if() block, but that would make the code more "verbose" :-)

But I am open to the change of the ,,or''s to ,,unless"s. They are
just cosmetic changes. I can submit patches to that effect if that's
what you guys want.

Cheers,
Subho.

On Sat, May 19, 2012 at 3:08 PM, Andrew Sayers
<andrew-git@pileofstuff.org> wrote:
> I'll limit myself to a style review here - other people can say better
> than me about the deeper issues.
>
> On 19/05/12 08:08, Subho Sankar Banerjee wrote:
> <snip>
>> @@ -160,7 +160,7 @@ sub repository {
>>       if (defined $args[0]) {
>>               if ($#args % 2 != 1) {
>>                       # Not a hash.
>> -                     $#args == 0 or throw Error::Simple("bad usage");
>> +                     $#args == 0 or die "bad usage";
>
> This is valid and no worse than before, but I find this use of the "or"
> operator slightly confusing.  I find it easier to read either:
>
>        <verb> or <fail>
>        OR:
>        <fail> unless <noun>
>
> For example:
>
>        do_something($foo) or die "couldn't do_something with '$foo'";
>        OR:
>        die "'$foo' is not a something" unless is_something($foo);
>
> <snip>
>> @@ -1041,7 +1041,7 @@ sub _temp_cache {
>>
>>               ($$temp_fd, $fname) = File::Temp->tempfile(
>>                       'Git_XXXXXX', UNLINK => 1, DIR => $tmpdir,
>> -                     ) or throw Error::Simple("couldn't open new temp file");
>> +                     ) or die "couldn't open new temp file";
>
> This is a good example of where I think "or" is appropriate.
>
> Think of it in terms of an English sentence.  Which of these would you
> find easier to read:
>
>        It is raining or go out and play
>        OR:
>        Go out and play unless it is raining
>
>        Find your umbrella or cancel the trip
>        OR:
>        Cancel the trip unless find your umbrella
>
>
> A bit of background for people who aren't (primarily) Perl programmers:
>
> As an expressive language that promotes "more than one way to do it",
> Perl has a long tradition of supporting many redundant ways of spelling
> "if (...) { ... }".  Common examples include:
>
>        if ( $x ) { do_something() }
>        do_something() if $x;
>
>        unless ( $x ) { do_something() }
>        do_something() unless $x;
>
>        $x && do_something();
>        $x || do_something();
>
>        $x and do_something();
>        $x or do_something();
>
> Sometimes people find very practical reasons why these aren't good
> programming practice, but the rest of the time everyone just argues
> about whether they're good grammar.
>
> The "&&" and "||" operators are an example of bad programming practice -
> these operators have relatively high precedence, so tend to behave
> unintuitively when used in (often regrettably) complex ways.  The "and"
> and "or" operators behave just like "&&" and "||", but with a precedence
> low enough to avoid weirdness.  See [1] for an example.
>
> Some people consider anything but a traditional prefix-if() statement to
> be bad grammar (I believe "Perl Best Practices" makes the argument,
> which is definitive for many people).  Other people say anything in the
> language is by definition fair game.  The rest of us spend a lot of time
> making arguments like the above, and frankly I think we gain more from
> the debate than the conclusion.
>
>        - Andrew
>
> [1] http://perldoc.perl.org/perlop.html#C-style-Logical-Defined-Or

Re: [PATCH][GIT.PM 2/3] Getting rid of throwing Error::Simple objects in favour of simple Perl scalars which can be caught in eval{} blocks

[Andrew Sayers]

On 23/05/12 12:02, Subho Banerjee wrote:
> Hi,
> The semantic
>>        <fail> unless <noun>
> works well when the <fail> part of the code is a singular statement.
> But it is of ungainly when there are a couple of statements to be
> executed as a block. In this case, I believe that a the conjunctive
> ,,or''/,,and'' statement makes more sense. In the sense -
>                  <verb1> or <die_gracefully1> and <die_gracefully2>
> I believe this is easier to read compared to -
>                  <die_gracefully1> and <die_gracefully2> unless <noun>
> especially if you have a larger block of commands to execute in case
> of the failure. I believe the easiest to read would be a classical C
> styled if() block, but that would make the code more "verbose" :-)

To be honest, I drop straight back to C-style if() statements as soon as
I have to start thinking consciously about precedence rules (where by
"thinking" I mean "creating bugs then failing to see them under my
nose").  I also don't think anyone would object if you wanted to stick
with classic if() statements everywhere - colloquialisms are supported,
not required :)

So long as you're aware this is an exception to the rule about matching
the style of surrounding code, I'm personally quite relaxed about fixing
these specific instances.  If nobody else has any opinions, maybe hold
off and see how much change you're planning to make elsewhere?  No sense
getting too attached to code you might have to throw away.

	- Andrew