[1.8.0] perl/Git.pm: moving away from using Error.pm module

Proposal:

Replace use of Error.pm module in Git.pm with either Exception::Class based error class, or using 'carp'/'croak' from Carp, or both by adding an option to set error handler in 'Git' class (like e.g. in 'CHI' module on CPAN).

While at it, if we are to require some extra non-core module, instead of using 'private-<module>.pm', use more standard 'inc/' directory (i.e. 'inc/<module>.pm').

Also get rid of git_cmd_try - encourage to use TryCatch or Try::Tiny instead, or even 'eval { ... }' as a way to catch thrown exceptions.

Rationale:

An extract from the Error.pm documentation: 

  Using the "Error" module is *no longer recommended* due to the
  black-magical nature of its syntactic sugar, which often tends to
  break. Its maintainers have stopped actively writing code that uses
  it, and discourage people from doing so.

"SEE ALSO" section therein mentions the following possible replacements:

  See Exception::Class for a different module providing Object-Oriented
  exception handling, along with a convenient syntax for declaring
  hierarchies for them. It doesn't provide Error's syntactic sugar of
  `try { ... }, catch { ... }`, etc. which may be a good thing or a bad
  thing based on what you want. (Because Error's syntactic sugar tends
  to break.)

  Error::Exception aims to combine Error and Exception::Class "with
  correct stringification".

  TryCatch and Try::Tiny are similar in concept to Error.pm only
  providing a syntax that hopefully breaks less.

Unfortunately, neither of those modules is in core (well, neither is Error.pm).

Risks:

Out of git commands and helpers implemented in Perl and using Git.pm module, only git-svn.perl uses 'try_git_cmd' directly. git-send-email uses 'eval { ... }' to catch exceptions thrown by ->repository() constructor; perhaps other scripts do the same. There is some risk of breaking git with this change...

Third party modules and scripts might have also depend on Git.pm using Error.pm... though I wonder how many of Perl scripts use Git instead of for example Git::Wrapper or other git-related Perl module from CPAN.

Migration plan:

I don't really have migration plan yet, because I amnot sure what solution should be implemented.

1. One possible solution would be to just replace Error with 
Exception::Class (or Git::Exception based in this class), and leave 
everything else as close to current state as possible.  Removing 
try_git_cmd would be second step...

2. Another solution would be to use 'on_error' to set error handler,
with support for 'die'/'croak', Error and Exception::Class based 
exceptions, with default to 'croak'.  In this case we wouldn't need any 
extra module, but testing structural exceptions would be harder.
We would have to replace try_git_cmd with eval, or Try::Tiny.

3. Yet another would be to leave Git module as it is now, and create
new modules: Git::Cmd, Git::Repo, Git::Config etc.

Jakub Narebski <jnareb@gmail.com> writes:

Personally, I was never a big fan of the syntax magic with Error.pm, but I refrained from commenting on it as I am not heavily involved in that part of the system. If we are going to change things so that everybody uses a more traditional "eval {}; if ($@) { ... }", it would be a welcome change from my point of view.

Do we even need one?

As far as an external caller is concerned, it would have been expecting us to throw an exception by dying, and it wouldn't have mattered if it used Error.pm or "eval { $call_to_Git_pm }; if ($@) {...}", I think.

On Mon, 21 Feb 2011, Junio C Hamano wrote:

Structured exceptions are usually better than 'die <string>' if
you need to examine error in more detail and act on this detail:
  http://www.modernperlbooks.com/mt/2010/10/structured-data-and-knowing-versus-guessing.html
  http://www.modernperlbooks.com/mt/2010/08/the-stringceptional-difficulty-of-changing-error-messages.html
  http://www.modernperlbooks.com/mt/2010/07/dont-parse-that-string.html

I think that is why Git.pm uses Error module (which seemed like a good choice in 2006), and it is why I propose using Exception::Class, perhaps as an option.

Well, it depends if external scripts use try_git_cmd sugar... and whether they try to act on details of caught exception.

Also if Git->repository would return 'undef' on failure instead of throwing an exception, this would require changes to external scripts.

On 21/02/11 07:20, Junio C Hamano wrote:

A small aside - note the "Dangers of using $@" described here:

  http://www.socialtext.net/perl5/exception_handling

To paraphrase, this:

  eval { stuff ; 1} or do { handle_exception };

is marginally safer than:

  eval { stuff }; if (defined $@) { handle_exception }

because it is possible that $@ can be modified (say, by a DESTROY method) before the if clause sees it. The former idiom does not stop that, it just means your exception handler is executed reliably.

Normally it is not a problem, but this is still something worth knowing.

N

On Mon, 21 Feb 2011, Nick wrote:

Important note: it is "if ($@)", not "if (defined $@)":

  If there was no error, $@ is guaranteed to be a null string.
                                                  ^^^^^^^^^^^

It is empty string, not undef.

Or better use Try::Tiny, which takes care of this and more

  use Try::Tiny;
  try { stuff  } catch { handle_exception };

On Mon, Feb 21, 2011 at 12:02, Nick <oinksocket@letterboxes.org> wrote:

Note that that DESTROY clobbering has been fixed in later versions of Perl.

Using the packages Exception::Class and Carp together, compete to set the eval_error variable ($@) For example, throwing an object (of type Exception::Class) can result in the eval_error variable ($@) getting a scalar type after the eval statement if the Carp is fast enough to set a string (of type scalar) that contains the error stack. The result is that following the eval statement the catch sees a different type than what it expects and does not react as planned.

Avi

-- View this message in context: http://git.661346.n2.nabble.com/1-8-0-perl-Git-pm-moving-away-from-using-Error-pm-module-tp6046799p6277964.html Sent from the git mailing list archive at Nabble.com.