{"thread":{"id":"10897","subject":"[PATCH] Git.pm: Don't return 'undef' in vector context.","startedAt":"2007-11-16T06:15:47Z","lastAt":"2007-11-17T15:19:17Z","messageCount":8,"participants":["Dan Zwell","Junio C Hamano","Jakub Narebski","Sebastian Harl"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"60059","messageId":"473D3593.9080806@zwell.net","threadId":"10897","inReplyTo":null,"subject":"[PATCH] Git.pm: Don't return 'undef' in vector context.","fromName":"Dan Zwell","fromEmail":"dzwell@gmail.com","sentAt":"2007-11-16T06:15:47Z","receivedAt":"2007-11-16T06:15:47Z","isPatch":true,"sender":{"key":"dzwell@gmail.com","avatar":null},"body":"Previously, the Git->repository()->config('non-existent.key')\nevaluated to as true in a vector context. Return an empty list\ninstead.\n---\nI don't know whether this breaks anything, because I don't use most of \nthe git perl scripts. I can't imagine that there is a script that relies \non the fact that config('non-existent.key') actually returns (''), in an \narray context. Is this a reasonable change?\n\n perl/Git.pm | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex e9dc706..ffcc541 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -508,7 +508,7 @@ sub config {\n \t\tmy $E = shift;\n \t\tif ($E->value() == 1) {\n \t\t\t# Key not found.\n-\t\t\treturn undef;\n+\t\t\treturn wantarray ? () : undef;\n \t\t} else {\n \t\t\tthrow $E;\n \t\t}\n-- \n1.5.3.5.565.gf0b83-dirty\n"},{"id":"60060","messageId":"7vfxz61yox.fsf@gitster.siamese.dyndns.org","threadId":"10897","inReplyTo":"473D3593.9080806@zwell.net","subject":"Re: [PATCH] Git.pm: Don't return 'undef' in vector context.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-16T06:39:26Z","receivedAt":"2007-11-16T06:39:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dan Zwell writes:\n\n> Previously, the Git->repository()->config('non-existent.key')\n> evaluated to as true in a vector context. Return an empty list\n> instead.\n> ---\n> I don't know whether this breaks anything, because I don't use most of\n> the git perl scripts. I can't imagine that there is a script that\n> relies on the fact that config('non-existent.key') actually returns\n> (''), in an array context. Is this a reasonable change?\n\nI did not examine the callers but my gut feeling is that it\nwould be simpler and cleaner to always return () without\nchecking the context. In scalar context:\n\n\tsub null {\n \t...\n return ();\n\t}\n\tmy $scalar = null();\n\nwould assign undef to $scalar anyway.\n\nI generally try to stay away from functions that changes their\nreturn values depending on the context, because they tend to\nmake reading the callers to find bugs more difficult. An\nexception is a function that tries to mimic a built-in operator,\nbecause reading the callers to such a function, as long as it is\nclear which built-in the function is imitating, can apply the\nsame knowledge on how the callee would behave you already have\nby knowing Perl itself.\n\nThe same thing can be said about functions with prototypes to\nforce certain context on the caller's side. Avoid it unless\nthere is a good reason.\n\nMaybe it is just me, but that's my reaction.\n"},{"id":"60065","messageId":"473D4E18.3060006@zwell.net","threadId":"10897","inReplyTo":"7vfxz61yox.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Git.pm: Don't return 'undef' in vector context.","fromName":"Dan Zwell","fromEmail":"dzwell@gmail.com","sentAt":"2007-11-16T08:00:24Z","receivedAt":"2007-11-16T08:00:24Z","isPatch":true,"sender":{"key":"dzwell@gmail.com","avatar":null},"body":"Junio C Hamano wrote:\n> I did not examine the callers but my gut feeling is that it\n> would be simpler and cleaner to always return () without\n> checking the context. In scalar context:\n> \n> \tsub null {\n> \t...\n> return ();\n> \t}\n> \tmy $scalar = null();\n> \n> would assign undef to $scalar anyway.\n> \n> I generally try to stay away from functions that changes their\n> return values depending on the context, because they tend to\n> make reading the callers to find bugs more difficult.\n> \n\nThat's reasonable. I'll resend this as part of the git-add--interactive \ncolor patches. This can be cherry-picked out, but some of the other \nstuff I want to do depends on it (a helper function that I wrote, \nconfig_with_default($repo, $key, $default)).\n\nDan\n"},{"id":"60068","messageId":"fhjl70$db4$1@ger.gmane.org","threadId":"10897","inReplyTo":"473D3593.9080806@zwell.net","subject":"Re: [PATCH] Git.pm: Don't return 'undef' in vector context.","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-11-16T08:43:11Z","receivedAt":"2007-11-16T08:43:11Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"[Cc: Dan Zwell , Petr Baudis ,\n git@vger.kernel.org]\n\nDan Zwell wrote:\n\n> Previously, the Git->repository()->config('non-existent.key')\n> evaluated to as true in a vector context. Return an empty list\n> instead.\n\nNice.\n\nBy the way, what do you think about changing Git.pm config handling\nto the 'eager' one used currently by gitweb, namely reading all the\nconfig to hash, and later getting config values from hash instead of\ncalling git-config? Or at least make it an option?\n\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"60085","messageId":"20071116124746.GW29439@albany.tokkee.org","threadId":"10897","inReplyTo":"7vfxz61yox.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Git.pm: Don't return 'undef' in vector context.","fromName":"Sebastian Harl","fromEmail":"sh@tokkee.org","sentAt":"2007-11-16T12:47:46Z","receivedAt":"2007-11-16T12:47:46Z","isPatch":true,"sender":{"key":"sh@tokkee.org","avatar":"https://gravatar.com/avatar/c19b9c37bd63e5d1dabe8d222904921fb807d53d1d8d41af758a16c066eeb59c?d=mp&s=160"},"body":"Hi,\n\nOn Thu, Nov 15, 2007 at 10:39:26PM -0800, Junio C Hamano wrote:\n> Dan Zwell writes:\n> > Previously, the Git->repository()->config('non-existent.key')\n> > evaluated to as true in a vector context. Return an empty list\n> > instead.\n> > ---\n> > I don't know whether this breaks anything, because I don't use most of\n> > the git perl scripts. I can't imagine that there is a script that\n> > relies on the fact that config('non-existent.key') actually returns\n> > (''), in an array context. Is this a reasonable change?\n> \n> I did not examine the callers but my gut feeling is that it\n> would be simpler and cleaner to always return () without\n> checking the context.\n[...]\n> I generally try to stay away from functions that changes their\n> return values depending on the context, because they tend to\n> make reading the callers to find bugs more difficult.\n\nIn fact, if you simply return without any value, it will evaluate to \"false\"\nno matter which context it has been called in (\"()\" in list context, \"undef\"\nin scalar context, etc.). So, it's generally a good idea to use \"return;\" to\nindicate an error.\n\nCheers,\nSebastian\n\n-- \nSebastian \"tokkee\" Harl +++ GnuPG-ID: 0x8501C7FC +++ http://tokkee.org/\n\nThose who would give up Essential Liberty to purchase a little Temporary\nSafety, deserve neither Liberty nor Safety. -- Benjamin Franklin\n\n"},{"id":"60121","messageId":"7vpry9wvqj.fsf@gitster.siamese.dyndns.org","threadId":"10897","inReplyTo":"473D4E18.3060006@zwell.net","subject":"Re: [PATCH] Git.pm: Don't return 'undef' in vector context.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-17T00:39:48Z","receivedAt":"2007-11-17T00:39:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dan Zwell writes:\n\n> That's reasonable. I'll resend this as part of the\n> git-add--interactive color patches. This can be cherry-picked out, but\n> some of the other stuff I want to do depends on it (a helper function\n> that I wrote, config_with_default($repo, $key, $default)).\n\nSure; as long as the \"Don't return 'undef'\" remains a separate\npatch early in the series, that is easy to manage.\n\nThanks.\n"},{"id":"60123","messageId":"473E5E08.2090609@zwell.net","threadId":"10897","inReplyTo":"fhjl70$db4$1@ger.gmane.org","subject":"Re: [PATCH] Git.pm: Don't return 'undef' in vector context.","fromName":"Dan Zwell","fromEmail":"dzwell@gmail.com","sentAt":"2007-11-17T03:20:40Z","receivedAt":"2007-11-17T03:20:40Z","isPatch":true,"sender":{"key":"dzwell@gmail.com","avatar":null},"body":"Jakub Narebski wrote:\n> \n> By the way, what do you think about changing Git.pm config handling\n> to the 'eager' one used currently by gitweb, namely reading all the\n> config to hash, and later getting config values from hash instead of\n> calling git-config? Or at least make it an option?\n> \n\nThat seems appropriate, though it may be a slight trade-off between \ncomplexity and efficiency. I don't think it's strictly necessary, at \nleast for git-add--interactive. My experience is that the nine calls to \nconfig() (that I am adding) do not slow down the program from a user \nperspective (though I haven't tested on a slower computer).\n\nThe big reason to do it would be if you wanted to convert gitweb to use \nthe standard config() call from Git.pm. Because right now, config() \nisn't efficient, but it probably doesn't need to be.\n\nDan\n"},{"id":"60146","messageId":"200711171619.18141.jnareb@gmail.com","threadId":"10897","inReplyTo":"473E5E08.2090609@zwell.net","subject":"Re: [PATCH] Git.pm: Don't return 'undef' in vector context.","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-11-17T15:19:17Z","receivedAt":"2007-11-17T15:19:17Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Saturday, 17 November 2007, Dan Zwell wrote:\n> Jakub Narebski wrote:\n> > \n> > By the way, what do you think about changing Git.pm config handling\n> > to the 'eager' one used currently by gitweb, namely reading all the\n> > config to hash, and later getting config values from hash instead of\n> > calling git-config? Or at least make it an option?\n> > \n> \n> That seems appropriate, though it may be a slight trade-off between \n> complexity and efficiency. I don't think it's strictly necessary, at \n> least for git-add--interactive. \n\nOn one hand it adds a bit of complexity, as you have to check if\nconfig was parsed (and either parse, as it is done now in gitweb,\nor perhaps fallback on calling git-config per variable), and you\nhave to do type checking / conversion to bool and int (size suffixes!)\nin Perl, and deal with single-value and multi-value variables.\n \nOn the other hand I think that error handling will be simplier.\n\n> My experience is that the nine calls to \n> config() (that I am adding) do not slow down the program from a user \n> perspective (though I haven't tested on a slower computer).\n\nThe problem is not so much with a slower computer, as operating\nsystems with inefficient, slow fork implementation, like MS Windows\nand (if I remember) correctly MacOS X.\n\nBut it is true that the config() performance is needed less for desktop\n(commands like git-add--interactive) than for web (gitweb for example).\n\n> The big reason to do it would be if you wanted to convert gitweb to use \n> the standard config() call from Git.pm. Because right now, config() \n> isn't efficient, but it probably doesn't need to be.\n\nThere are two reasons against convering gitweb to use Git.pm, at least\nfor now.\n\nFirst, it makes installing gitweb harder, as one would have to install\n(and perhaps compile) Git.pm to use gitweb. It would add additional\ndependency. But perhaps this is not as much a problem as I think...\n\nSecond, gitweb code contain in few places pipelines (e.g. tar.gz and\ntar.bz2 snapshot pipelines); even if some of them can be eliminated,\nsome will be added (syntax highlighting in blob view). Git.pm doesn't\nas of yet support this...\n\n\nIt would be nice if Git.pm could take advantage of libification project,\nand git have fast bindings not only for Python but also for Perl. If I\nremember correctly the early attempts to use \"git library\" directly\nwere abandoned becaue they were not sufficiently portable, and the\nfallback-to-Perl idea was thought too complex to implement...\n\n-- \nJakub Narebski\nPoland\n"}]}