{"thread":{"id":"26565","subject":"[1.8.0] perl/Git.pm: moving away from using Error.pm module","startedAt":"2011-02-20T22:46:33Z","lastAt":"2011-04-15T23:35:45Z","messageCount":7,"participants":["Jakub Narebski","Junio C Hamano","Nick","Ævar Arnfjörð Bjarmason","Avner"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"161769","messageId":"201102202346.36410.jnareb@gmail.com","threadId":"26565","inReplyTo":null,"subject":"[1.8.0] perl/Git.pm: moving away from using Error.pm module","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-02-20T22:46:33Z","receivedAt":"2011-02-20T22:46:33Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Proposal:\n\nReplace use of Error.pm module in Git.pm with either Exception::Class\nbased error class, or using 'carp'/'croak' from Carp, or both by adding \nan option to set error handler in 'Git' class (like e.g. in 'CHI' \nmodule on CPAN).\n\nWhile at it, if we are to require some extra non-core module, instead\nof using 'private-<module>.pm', use more standard 'inc/' directory\n(i.e. 'inc/<module>.pm').\n\nAlso get rid of git_cmd_try - encourage to use TryCatch or Try::Tiny\ninstead, or even 'eval { ... }' as a way to catch thrown exceptions.\n\n\nRationale:\n\nAn extract from the Error.pm documentation: \n\n  Using the \"Error\" module is *no longer recommended* due to the\n  black-magical nature of its syntactic sugar, which often tends to\n  break. Its maintainers have stopped actively writing code that uses\n  it, and discourage people from doing so.\n\n\"SEE ALSO\" section therein mentions the following possible replacements:\n\n  See Exception::Class for a different module providing Object-Oriented\n  exception handling, along with a convenient syntax for declaring\n  hierarchies for them. It doesn't provide Error's syntactic sugar of\n  `try { ... }, catch { ... }`, etc. which may be a good thing or a bad\n  thing based on what you want. (Because Error's syntactic sugar tends\n  to break.)\n\n  Error::Exception aims to combine Error and Exception::Class \"with\n  correct stringification\".\n\n  TryCatch and Try::Tiny are similar in concept to Error.pm only\n  providing a syntax that hopefully breaks less.\n\nUnfortunately, neither of those modules is in core (well, neither is \nError.pm).\n\n\nRisks:\n\nOut of git commands and helpers implemented in Perl and using Git.pm\nmodule, only git-svn.perl uses 'try_git_cmd' directly.  git-send-email\nuses 'eval { ... }' to catch exceptions thrown by ->repository() \nconstructor; perhaps other scripts do the same.  There is some risk of \nbreaking git with this change...\n\nThird party modules and scripts might have also depend on Git.pm using \nError.pm... though I wonder how many of Perl scripts use Git instead of \nfor example Git::Wrapper or other git-related Perl module from CPAN.\n\n\nMigration plan:\n\nI don't really have migration plan yet, because  I amnot sure what \nsolution  should be implemented.\n\n1. One possible solution would be to just replace Error with \nException::Class (or Git::Exception based in this class), and leave \neverything else as close to current state as possible.  Removing \ntry_git_cmd would be second step...\n\n2. Another solution would be to use 'on_error' to set error handler,\nwith support for 'die'/'croak', Error and Exception::Class based \nexceptions, with default to 'croak'.  In this case we wouldn't need any \nextra module, but testing structural exceptions would be harder.\nWe would have to replace try_git_cmd with eval, or Try::Tiny.\n\n3. Yet another would be to leave Git module as it is now, and create\nnew modules: Git::Cmd, Git::Repo, Git::Config etc.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"161778","messageId":"7v4o7xluph.fsf@alter.siamese.dyndns.org","threadId":"26565","inReplyTo":"201102202346.36410.jnareb@gmail.com","subject":"Re: [1.8.0] perl/Git.pm: moving away from using Error.pm module","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-21T07:20:42Z","receivedAt":"2011-02-21T07:20:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> Proposal:\n>\n> Replace use of Error.pm module in Git.pm with either Exception::Class\n> based error class, or using 'carp'/'croak' from Carp, or both by adding \n> an option to set error handler in 'Git' class (like e.g. in 'CHI' \n> module on CPAN).\n\nPersonally, I was never a big fan of the syntax magic with Error.pm, but I\nrefrained from commenting on it as I am not heavily involved in that part\nof the system.  If we are going to change things so that everybody uses a\nmore traditional \"eval {}; if ($@) { ... }\", it would be a welcome change\nfrom my point of view.\n\n> Migration plan:\n\nDo we even need one?\n\nAs far as an external caller is concerned, it would have been expecting us\nto throw an exception by dying, and it wouldn't have mattered if it used\nError.pm or \"eval { $call_to_Git_pm }; if ($@) {...}\", I think.\n"},{"id":"161783","messageId":"201102211031.11308.jnareb@gmail.com","threadId":"26565","inReplyTo":"7v4o7xluph.fsf@alter.siamese.dyndns.org","subject":"Re: [1.8.0] perl/Git.pm: moving away from using Error.pm module","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-02-21T09:31:09Z","receivedAt":"2011-02-21T09:31:09Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 21 Feb 2011, Junio C Hamano wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n> > Proposal:\n> >\n> > Replace use of Error.pm module in Git.pm with either Exception::Class\n> > based error class, or using 'carp'/'croak' from Carp, or both by adding \n> > an option to set error handler in 'Git' class (like e.g. in 'CHI' \n> > module on CPAN).\n> \n> Personally, I was never a big fan of the syntax magic with Error.pm, but I\n> refrained from commenting on it as I am not heavily involved in that part\n> of the system.  If we are going to change things so that everybody uses a\n> more traditional \"eval {}; if ($@) { ... }\", it would be a welcome change\n> from my point of view.\n\nStructured exceptions are usually better than 'die <string>' if\nyou need to examine error in more detail and act on this detail:\n  http://www.modernperlbooks.com/mt/2010/10/structured-data-and-knowing-versus-guessing.html\n  http://www.modernperlbooks.com/mt/2010/08/the-stringceptional-difficulty-of-changing-error-messages.html\n  http://www.modernperlbooks.com/mt/2010/07/dont-parse-that-string.html\n\nI think that is why Git.pm uses Error module (which seemed like a good\nchoice in 2006), and it is why I propose using Exception::Class, perhaps\nas an option. \n\n> > Migration plan:\n> \n> Do we even need one?\n> \n> As far as an external caller is concerned, it would have been expecting us\n> to throw an exception by dying, and it wouldn't have mattered if it used\n> Error.pm or \"eval { $call_to_Git_pm }; if ($@) {...}\", I think.\n\nWell, it depends if external scripts use try_git_cmd sugar... and whether\nthey try to act on details of caught exception.\n\nAlso if Git->repository would return 'undef' on failure instead of throwing\nan exception, this would require changes to external scripts.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"161805","messageId":"4D624632.80904@letterboxes.org","threadId":"26565","inReplyTo":"7v4o7xluph.fsf@alter.siamese.dyndns.org","subject":"Re: [1.8.0] perl/Git.pm: moving away from using Error.pm module","fromName":"Nick","fromEmail":"oinksocket@letterboxes.org","sentAt":"2011-02-21T11:02:10Z","receivedAt":"2011-02-21T11:02:10Z","isPatch":false,"sender":{"key":"oinksocket@letterboxes.org","avatar":null},"body":"On 21/02/11 07:20, Junio C Hamano wrote:\n> If we are going to change things so that everybody uses a\n> more traditional \"eval {}; if ($@) { ... }\", it would be a welcome change\n> from my point of view.\n\nA small aside - note the \"Dangers of using $@\" described here:\n\n  http://www.socialtext.net/perl5/exception_handling\n\nTo paraphrase, this:\n\n  eval { stuff ; 1} or do { handle_exception };\n\nis marginally safer than:\n\n  eval { stuff }; if (defined $@) { handle_exception }\n\nbecause it is possible that $@ can be modified (say, by a DESTROY method) before\nthe if clause sees it.  The former idiom does not stop that, it just means your\nexception handler is executed reliably.\n\nNormally it is not a problem, but this is still something worth knowing.\n\nN\n"},{"id":"161806","messageId":"201102211312.48333.jnareb@gmail.com","threadId":"26565","inReplyTo":"4D624632.80904@letterboxes.org","subject":"Re: [1.8.0] perl/Git.pm: moving away from using Error.pm module","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-02-21T12:12:44Z","receivedAt":"2011-02-21T12:12:44Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 21 Feb 2011, Nick wrote:\n> On 21/02/11 07:20, Junio C Hamano wrote:\n\n> > If we are going to change things so that everybody uses a\n> > more traditional \"eval {}; if ($@) { ... }\", it would be a welcome change\n> > from my point of view.\n> \n> A small aside - note the \"Dangers of using $@\" described here:\n> \n>   http://www.socialtext.net/perl5/exception_handling\n> \n> To paraphrase, this:\n> \n>   eval { stuff ; 1} or do { handle_exception };\n> \n> is marginally safer than:\n> \n>   eval { stuff }; if (defined $@) { handle_exception }\n\nImportant note: it is \"if ($@)\", not \"if (defined $@)\":\n\n  If there was no error, $@ is guaranteed to be a null string.\n                                                  ^^^^^^^^^^^\n\nIt is empty string, not undef.\n\n> because it is possible that $@ can be modified (say, by a DESTROY method) before\n> the if clause sees it.  The former idiom does not stop that, it just means your\n> exception handler is executed reliably.\n> \n> Normally it is not a problem, but this is still something worth knowing.\n\nOr better use Try::Tiny, which takes care of this and more\n\n  use Try::Tiny;\n  try { stuff  } catch { handle_exception };\n\n-- \nJakub Narebski\nPoland\n"},{"id":"161809","messageId":"AANLkTimwhYwQz9W3tAa2=Q0nJY8AoZYq=7KeX5O2Ca_G@mail.gmail.com","threadId":"26565","inReplyTo":"4D624632.80904@letterboxes.org","subject":"Re: [1.8.0] perl/Git.pm: moving away from using Error.pm module","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2011-02-21T12:31:19Z","receivedAt":"2011-02-21T12:31:19Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, Feb 21, 2011 at 12:02, Nick <oinksocket@letterboxes.org> wrote:\n> because it is possible that $@ can be modified (say, by a DESTROY method) before\n> the if clause sees it.  The former idiom does not stop that, it just means your\n> exception handler is executed reliably.\n\nNote that that DESTROY clobbering has been fixed in later versions of Perl.\n"},{"id":"165938","messageId":"1302910545319-6277964.post@n2.nabble.com","threadId":"26565","inReplyTo":"AANLkTimwhYwQz9W3tAa2=Q0nJY8AoZYq=7KeX5O2Ca_G@mail.gmail.com","subject":"Re: [1.8.0] perl/Git.pm: moving away from using Error.pm module","fromName":"Avner","fromEmail":"avnermoshkovitz@lighthauslogic.com","sentAt":"2011-04-15T23:35:45Z","receivedAt":"2011-04-15T23:35:45Z","isPatch":false,"sender":{"key":"avnermoshkovitz@lighthauslogic.com","avatar":null},"body":"Using the packages Exception::Class and Carp together, compete to set the\neval_error variable ($@)\nFor example, throwing an object (of type Exception::Class) can result in the\neval_error variable ($@) getting a scalar type after the eval statement if\nthe Carp is fast enough to set a string (of type scalar) that contains the\nerror stack.\nThe result is that following the eval statement the catch sees a different\ntype than what it expects and does not react as planned.\n\nAvi\n\n\n--\nView 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\nSent from the git mailing list archive at Nabble.com.\n"}]}