{"thread":{"id":"28517","subject":"[PATCH] git-send-email.perl: expand filename of aliasesfile","startedAt":"2011-09-28T13:13:07Z","lastAt":"2011-10-19T00:19:37Z","messageCount":24,"participants":["Cord Seele","Matthieu Moy","Junio C Hamano","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"176400","messageId":"20110928131307.GB12586@laptop","threadId":"28517","inReplyTo":null,"subject":"[PATCH] git-send-email.perl: expand filename of aliasesfile","fromName":"Cord Seele","fromEmail":"cowose@googlemail.com","sentAt":"2011-09-28T13:13:07Z","receivedAt":"2011-09-28T13:13:07Z","isPatch":true,"sender":{"key":"cowose@gmail.com","avatar":null},"body":"Allow config variable sendemail.aliasesfile to contain a tilde, i.e.\nmake ~/.mutt/aliases work.\n\nSigned-off-by: Cord Seele <cowose@gmail.com>\n---\n git-send-email.perl |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 98ab33a..b50b35f 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -481,7 +481,7 @@ my %parse_alias = (\n \n if (@alias_files and $aliasfiletype and defined $parse_alias{$aliasfiletype}) {\n \tforeach my $file (@alias_files) {\n-\t\topen my $fh, '<', $file or die \"opening $file: $!\\n\";\n+\t\topen my $fh, '<', glob($file) or die \"opening $file: $!\\n\";\n \t\t$parse_alias{$aliasfiletype}->($fh);\n \t\tclose $fh;\n \t}\n-- \n1.7.6.4.dirty\n\n\n-- Cord\n"},{"id":"176402","messageId":"vpqwrcspyvq.fsf@bauges.imag.fr","threadId":"28517","inReplyTo":"20110928131307.GB12586@laptop","subject":"Re: [PATCH] git-send-email.perl: expand filename of aliasesfile","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2011-09-28T13:42:01Z","receivedAt":"2011-09-28T13:42:01Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Cord Seele <cowose@googlemail.com> writes:\n\n> -\t\topen my $fh, '<', $file or die \"opening $file: $!\\n\";\n> +\t\topen my $fh, '<', glob($file) or die \"opening $file: $!\\n\";\n\nThat'd be cleaner to use\n\ngit config --path sendemail.aliasesfile\n\nto let Git do the right expansion, in a way consistant with other places\nof Git.\n\nIn practice, that would imply adding %config_path_settings like the\nexisting %config_bool_settings, and add config_path to Git.pm (again,\nsimilarly to config_bool).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"176416","messageId":"20110928144057.GC12586@laptop","threadId":"28517","inReplyTo":"vpqwrcspyvq.fsf@bauges.imag.fr","subject":"Re: [PATCH] git-send-email.perl: expand filename of aliasesfile","fromName":"Cord Seele","fromEmail":"cowose@googlemail.com","sentAt":"2011-09-28T14:40:57Z","receivedAt":"2011-09-28T14:40:57Z","isPatch":true,"sender":{"key":"cowose@gmail.com","avatar":null},"body":"On Wed 28 Sep 2011 15:42:01 +0200, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:\n\n> Cord Seele <cowose@googlemail.com> writes:\n> \n> > -\t\topen my $fh, '<', $file or die \"opening $file: $!\\n\";\n> > +\t\topen my $fh, '<', glob($file) or die \"opening $file: $!\\n\";\n> \n> That'd be cleaner to use\n> \n> git config --path sendemail.aliasesfile\n> \n> to let Git do the right expansion, in a way consistant with other places\n> of Git.\n\nThis means to expand it at 'git config' time? Wouldn't it be nicer to\nhave it expanded when you run 'git send-email'? Then you could move your\n~/.gitconfig (that's where I have my aliasesfile configured) between different\naccounts and it could still work.\n\n-- Cord\n"},{"id":"176419","messageId":"vpqty7wk9km.fsf@bauges.imag.fr","threadId":"28517","inReplyTo":"20110928144057.GC12586@laptop","subject":"Re: [PATCH] git-send-email.perl: expand filename of aliasesfile","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2011-09-28T14:47:37Z","receivedAt":"2011-09-28T14:47:37Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Cord Seele <cowose@googlemail.com> writes:\n\n> On Wed 28 Sep 2011 15:42:01 +0200, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:\n>> That'd be cleaner to use\n>> \n>> git config --path sendemail.aliasesfile\n>> \n>> to let Git do the right expansion, in a way consistant with other places\n>> of Git.\n>\n> This means to expand it at 'git config' time?\n\nYes, but not the \"git config\" ran to set the value. The one ran\ninternally by \"git send-email\" through Git::config(). You may add --get\nto my command line above.\n\n> Wouldn't it be nicer to have it expanded when you run 'git\n> send-email'? Then you could move your ~/.gitconfig (that's where I\n> have my aliasesfile configured) between different accounts and it\n> could still work.\n\nThat works with my proposal.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"176565","messageId":"1317379945-9355-1-git-send-email-cowose@gmail.com","threadId":"28517","inReplyTo":"vpqty7wk9km.fsf@bauges.imag.fr","subject":"[PATCH v2] git-send-email: allow filename expansion","fromName":"Cord Seele","fromEmail":"cowose@googlemail.com","sentAt":"2011-09-30T10:52:23Z","receivedAt":"2011-09-30T10:52:23Z","isPatch":true,"sender":{"key":"cowose@gmail.com","avatar":null},"body":"OK, here's another try to get it more Git-like.\n\nFilename expansion now works as far as I can tell. But I failed actually\nusing my mutt aliases (but also with plain v1.7.6.4). Since I'm not\nfamiliar with perl-debugging could please someone more experienced have a look?\nThanks.\n\n-- Cord\n\n[PATCH 1/2] Add Git::config_path()\n[PATCH 2/2] use new Git::config_path() for aliasesfile\n"},{"id":"176567","messageId":"1317379945-9355-2-git-send-email-cowose@gmail.com","threadId":"28517","inReplyTo":"1317379945-9355-1-git-send-email-cowose@gmail.com","subject":"[PATCH 1/2] Add Git::config_path()","fromName":"Cord Seele","fromEmail":"cowose@googlemail.com","sentAt":"2011-09-30T10:52:24Z","receivedAt":"2011-09-30T10:52:24Z","isPatch":true,"sender":{"key":"cowose@gmail.com","avatar":null},"body":"Use --path option when calling 'git config' thus allow for pathname\nexpansion, e.g. a tilde.\n\nSigned-off-by: Cord Seele <cowose@gmail.com>\n---\n perl/Git.pm |   32 ++++++++++++++++++++++++++++++++\n 1 files changed, 32 insertions(+), 0 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex a86ab70..c279bfb 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -627,6 +627,38 @@ sub config_bool {\n \t};\n }\n \n+\n+=item config_path ( VARIABLE )\n+\n+Retrieve the path configuration C<VARIABLE>. The return value\n+is an expanded path or C<undef> if it's not defined.\n+\n+This currently wraps command('config') so it is not so fast.\n+\n+=cut\n+\n+sub config_path {\n+\tmy ($self, $var) = _maybe_self(@_);\n+\n+\ttry {\n+\t\tmy @cmd = ('config', '--path');\n+\t\tunshift @cmd, $self if $self;\n+\t\tif (wantarray) {\n+\t\t\treturn command(@cmd, '--get-all', $var);\n+\t\t} else {\n+\t\t\treturn command_oneline(@cmd, '--get', $var);\n+\t\t}\n+\t} catch Git::Error::Command with {\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} else {\n+\t\t\tthrow $E;\n+\t\t}\n+\t};\n+}\n+\n =item config_int ( VARIABLE )\n \n Retrieve the integer configuration C<VARIABLE>. The return value\n-- \n1.7.6.4\n"},{"id":"176566","messageId":"1317379945-9355-3-git-send-email-cowose@gmail.com","threadId":"28517","inReplyTo":"1317379945-9355-1-git-send-email-cowose@gmail.com","subject":"[PATCH 2/2] use new Git::config_path() for aliasesfile","fromName":"Cord Seele","fromEmail":"cowose@googlemail.com","sentAt":"2011-09-30T10:52:25Z","receivedAt":"2011-09-30T10:52:25Z","isPatch":true,"sender":{"key":"cowose@gmail.com","avatar":null},"body":"Signed-off-by: Cord Seele <cowose@gmail.com>\n---\n git-send-email.perl |   10 +++++++++-\n 1 files changed, 9 insertions(+), 1 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 98ab33a..f17f7b3 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -225,7 +225,6 @@ my %config_settings = (\n     \"cccmd\" => \\$cc_cmd,\n     \"aliasfiletype\" => \\$aliasfiletype,\n     \"bcc\" => \\@bcclist,\n-    \"aliasesfile\" => \\@alias_files,\n     \"suppresscc\" => \\@suppress_cc,\n     \"envelopesender\" => \\$envelope_sender,\n     \"multiedit\" => \\$multiedit,\n@@ -234,6 +233,10 @@ my %config_settings = (\n     \"assume8bitencoding\" => \\$auto_8bit_encoding,\n );\n \n+my %config_path_settings = (\n+    \"aliasesfile\" => \\@alias_files,\n+);\n+\n # Help users prepare for 1.7.0\n sub chain_reply_to {\n \tif (defined $chain_reply_to &&\n@@ -330,6 +333,11 @@ sub read_config {\n \t\t$$target = Git::config_bool(@repo, \"$prefix.$setting\") unless (defined $$target);\n \t}\n \n+\tforeach my $setting (keys %config_path_settings) {\n+\t\tmy $target = $config_path_settings{$setting}->[0];\n+\t\t$$target = Git::config_path(@repo, \"$prefix.$setting\") unless (defined $$target);\n+\t}\n+\n \tforeach my $setting (keys %config_settings) {\n \t\tmy $target = $config_settings{$setting};\n \t\tnext if $setting eq \"to\" and defined $no_to;\n-- \n1.7.6.4\n"},{"id":"176593","messageId":"7vsjndoldq.fsf@alter.siamese.dyndns.org","threadId":"28517","inReplyTo":"1317379945-9355-3-git-send-email-cowose@gmail.com","subject":"Re: [PATCH 2/2] use new Git::config_path() for aliasesfile","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-30T19:55:45Z","receivedAt":"2011-09-30T19:55:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I think the addition of \"config --path\" support is a good idea, but the\nresulting code suffers from too many cut&paste cruft across the config*\nfamily of methods.\n\nHow about doing a bit of refactoring, perhaps something like this, on top\nas a separate patch?\n\nI tried to be careful to still forcing the \"one value only\" for config_bool\nand config_int, but extra sets of eyeballs would be needed.\n\n perl/Git.pm |   93 +++++++++++++++++-----------------------------------------\n 1 files changed, 27 insertions(+), 66 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex c279bfb..f0a6e92 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -563,22 +563,18 @@ sub wc_chdir {\n }\n \n \n-=item config ( VARIABLE )\n+=item _config_common ( VARIABLE )\n \n-Retrieve the configuration C<VARIABLE> in the same manner as C<config>\n-does. In scalar context requires the variable to be set only one time\n-(exception is thrown otherwise), in array context returns allows the\n-variable to be set multiple times and returns all the values.\n-\n-This currently wraps command('config') so it is not so fast.\n+Common subroutine to implement bulk of what the config* family of methods\n+do. This wraps command('config') so it is not so fast.\n \n =cut\n \n-sub config {\n-\tmy ($self, $var) = _maybe_self(@_);\n-\n+sub _config_common {\n+\tmy ($self, $var, $opts) = _maybe_self(@_);\n+\t\n \ttry {\n-\t\tmy @cmd = ('config');\n+\t\tmy @cmd = ('config', $opts->{'kind'} ? @{$opts->{'kind'}} : ());\n \t\tunshift @cmd, $self if $self;\n \t\tif (wantarray) {\n \t\t\treturn command(@cmd, '--get-all', $var);\n@@ -594,6 +590,21 @@ sub config {\n \t\t\tthrow $E;\n \t\t}\n \t};\n+\n+}\n+\n+=item config ( VARIABLE )\n+\n+Retrieve the configuration C<VARIABLE> in the same manner as C<config>\n+does. In scalar context requires the variable to be set only one time\n+(exception is thrown otherwise), in array context returns allows the\n+variable to be set multiple times and returns all the values.\n+\n+=cut\n+\n+sub config {\n+\tmy ($self, $var) = _maybe_self(@_);\n+\treturn _config_common($self, $var, +{});\n }\n \n \n@@ -603,60 +614,24 @@ Retrieve the bool configuration C<VARIABLE>. The return value\n is usable as a boolean in perl (and C<undef> if it's not defined,\n of course).\n \n-This currently wraps command('config') so it is not so fast.\n-\n =cut\n \n sub config_bool {\n \tmy ($self, $var) = _maybe_self(@_);\n-\n-\ttry {\n-\t\tmy @cmd = ('config', '--bool', '--get', $var);\n-\t\tunshift @cmd, $self if $self;\n-\t\tmy $val = command_oneline(@cmd);\n-\t\treturn undef unless defined $val;\n-\t\treturn $val eq 'true';\n-\t} catch Git::Error::Command with {\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} else {\n-\t\t\tthrow $E;\n-\t\t}\n-\t};\n+\tmy $val = scalar _config_common($self, $var, {'kind' => '--bool'});\n+\treturn (defined $val && $val eq 'true');\n }\n \n-\n =item config_path ( VARIABLE )\n \n Retrieve the path configuration C<VARIABLE>. The return value\n is an expanded path or C<undef> if it's not defined.\n \n-This currently wraps command('config') so it is not so fast.\n-\n =cut\n \n sub config_path {\n \tmy ($self, $var) = _maybe_self(@_);\n-\n-\ttry {\n-\t\tmy @cmd = ('config', '--path');\n-\t\tunshift @cmd, $self if $self;\n-\t\tif (wantarray) {\n-\t\t\treturn command(@cmd, '--get-all', $var);\n-\t\t} else {\n-\t\t\treturn command_oneline(@cmd, '--get', $var);\n-\t\t}\n-\t} catch Git::Error::Command with {\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} else {\n-\t\t\tthrow $E;\n-\t\t}\n-\t};\n+\treturn _config_common($self, $var, +{'kind' => '--path'});\n }\n \n =item config_int ( VARIABLE )\n@@ -667,26 +642,12 @@ or 'g' in the config file will cause the value to be multiplied\n by 1024, 1048576 (1024^2), or 1073741824 (1024^3) prior to output.\n It would return C<undef> if configuration variable is not defined,\n \n-This currently wraps command('config') so it is not so fast.\n-\n =cut\n \n sub config_int {\n \tmy ($self, $var) = _maybe_self(@_);\n-\n-\ttry {\n-\t\tmy @cmd = ('config', '--int', '--get', $var);\n-\t\tunshift @cmd, $self if $self;\n-\t\treturn command_oneline(@cmd);\n-\t} catch Git::Error::Command with {\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} else {\n-\t\t\tthrow $E;\n-\t\t}\n-\t};\n+\tmy $val = scalar _config_common($self, $var, +{'kind' => '--int'});\n+\treturn $val;\n }\n \n =item get_colorbool ( NAME )\n"},{"id":"176596","messageId":"20110930211633.GE13435@laptop","threadId":"28517","inReplyTo":"7vsjndoldq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] use new Git::config_path() for aliasesfile","fromName":"Cord Seele","fromEmail":"cowose@googlemail.com","sentAt":"2011-09-30T21:16:33Z","receivedAt":"2011-09-30T21:16:33Z","isPatch":true,"sender":{"key":"cowose@gmail.com","avatar":null},"body":"On Fri 30 Sep 2011 12:55:45 -0700, Junio C Hamano <gitster@pobox.com> wrote:\n\n> I think the addition of \"config --path\" support is a good idea, but the\n> resulting code suffers from too many cut&paste cruft across the config*\n> family of methods.\n> \n> How about doing a bit of refactoring, perhaps something like this, on top\n> as a separate patch?\n\nSound very reasonable to me - unfortunately it's beyond my perl-scope to be of\nmuch help here.\n\n-- Cord\n"},{"id":"176599","messageId":"201110010000.13328.jnareb@gmail.com","threadId":"28517","inReplyTo":"7vsjndoldq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] use new Git::config_path() for aliasesfile","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-09-30T22:00:11Z","receivedAt":"2011-09-30T22:00:11Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 30 Sep 2011, Junio C Hamano wrote:\n\n> I think the addition of \"config --path\" support is a good idea, but the\n> resulting code suffers from too many cut&paste cruft across the config*\n> family of methods.\n> \n> How about doing a bit of refactoring, perhaps something like this, on top\n> as a separate patch?\n\nThis is a good idea, in my opinion.\n \n> I tried to be careful to still forcing the \"one value only\" for config_bool\n> and config_int, but extra sets of eyeballs would be needed.\n\nWe do have tests for that, have we?\n \n[...]\n> +Common subroutine to implement bulk of what the config* family of methods\n> +do. This wraps command('config') so it is not so fast.\n\nBTW. I wonder if it wouldn't be a good idea to restart work on Git::Config\nwith lazy/eager loading of the whole config, like git_get_project_config()\nand git_parse_project_config() subroutines from gitweb do it - running \n\"git config -z -l\".  Though then --int/--bool/--path conversion would have\nto be implemented in Perl...\n  \n>  =cut\n>  \n> -sub config {\n> -\tmy ($self, $var) = _maybe_self(@_);\n> -\n> +sub _config_common {\n> +\tmy ($self, $var, $opts) = _maybe_self(@_);\n> +\t\n>  \ttry {\n> -\t\tmy @cmd = ('config');\n> +\t\tmy @cmd = ('config', $opts->{'kind'} ? @{$opts->{'kind'}} : ());\n\nHow it is supposed to work?\n\nFirst, you probably want to check for \"exists $opts->{'kind'}\", or even\n\"defined $opts->{'kind'}\".\n\nSecond, \"@{$opts->{'kind'}}\" assumes that $opts->{'kind'} is an array\nreference (something we didn't check)... and it isn't.\n\n\nBTW. why do you use hashref?  Do you plan for the future to pass more\noptions that 'kind'?\n\n>  \t\tunshift @cmd, $self if $self;\n>  \t\tif (wantarray) {\n>  \t\t\treturn command(@cmd, '--get-all', $var);\n> @@ -594,6 +590,21 @@ sub config {\n>  \t\t\tthrow $E;\n>  \t\t}\n>  \t};\n> +\n> +}\n> +\n> +=item config ( VARIABLE )\n> +\n> +Retrieve the configuration C<VARIABLE> in the same manner as C<config>\n> +does. In scalar context requires the variable to be set only one time\n> +(exception is thrown otherwise), in array context returns allows the\n> +variable to be set multiple times and returns all the values.\n> +\n> +=cut\n> +\n> +sub config {\n> +\tmy ($self, $var) = _maybe_self(@_);\n> +\treturn _config_common($self, $var, +{});\n\nNo need to pass an empty hash.  Perl has a feature called autovivification:\nwhen an undefined variable is dereferenced, it gets silently upgraded to\nan array or hash reference (depending of the type of the dereferencing).\n\nIt is un-Perlish to do so.\n\n>  sub config_bool {\n>  \tmy ($self, $var) = _maybe_self(@_);\n[...]\n> +\tmy $val = scalar _config_common($self, $var, {'kind' => '--bool'});\n> +\treturn (defined $val && $val eq 'true');\n>  }\n[...]\n\n>  sub config_path {\n>  \tmy ($self, $var) = _maybe_self(@_);\n[...]\n> +\treturn _config_common($self, $var, +{'kind' => '--path'});\n>  }\n\nWhy the difference between {'kind' => '--bool'} and +{'kind' => '--path'}?\n\n> -This currently wraps command('config') so it is not so fast.\n> -\n\nShouldn't this be mentioned somewhat, even indirectly?\n\n-- \nJakub Narebski\nPoland\n"},{"id":"176604","messageId":"7voby1oesm.fsf@alter.siamese.dyndns.org","threadId":"28517","inReplyTo":"201110010000.13328.jnareb@gmail.com","subject":"Re: [PATCH 2/2] use new Git::config_path() for aliasesfile","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-30T22:18:01Z","receivedAt":"2011-09-30T22:18:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> On Fri, 30 Sep 2011, Junio C Hamano wrote:\n>\n>> I think the addition of \"config --path\" support is a good idea, but the\n>> resulting code suffers from too many cut&paste cruft across the config*\n>> family of methods.\n>> \n>> How about doing a bit of refactoring, perhaps something like this, on top\n>> as a separate patch?\n>\n> This is a good idea, in my opinion.\n\nThanks.\n\n>> I tried to be careful to still forcing the \"one value only\" for config_bool\n>> and config_int, but extra sets of eyeballs would be needed.\n>\n> We do have tests for that, have we?\n\nPerhaps, but I consider Perl \"other peoples' problem\" ;-) so...\n\n> BTW. why do you use hashref?  Do you plan for the future to pass more\n> options that 'kind'?\n\nI don't, but other people might, and I didn't want to place undue burden\non them.\n"},{"id":"177184","messageId":"m3vcs01r32.fsf@localhost.localdomain","threadId":"28517","inReplyTo":"1317379945-9355-2-git-send-email-cowose@gmail.com","subject":"Re: [PATCH 1/2] Add Git::config_path()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-10-07T20:32:39Z","receivedAt":"2011-10-07T20:32:39Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Cord Seele <cowose@googlemail.com> writes:\n\n> Use --path option when calling 'git config' thus allow for pathname\n> expansion, e.g. a tilde.\n> \n> Signed-off-by: Cord Seele <cowose@gmail.com>\n> ---\n>  perl/Git.pm |   32 ++++++++++++++++++++++++++++++++\n>  1 files changed, 32 insertions(+), 0 deletions(-)\n\nI think the following minimal test should be squashed in:\n\n---\ndiff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh\nindex 3787186..7558f0c 100755\n--- a/t/t9700-perl-git.sh\n+++ b/t/t9700-perl-git.sh\n@@ -43,7 +43,9 @@ test_expect_success \\\n      git config --add test.booltrue true &&\n      git config --add test.boolfalse no &&\n      git config --add test.boolother other &&\n-     git config --add test.int 2k\n+     git config --add test.int 2k &&\n+     git config --add test.path \"~/foo\" &&\n+     git config --add test.pathexpanded \"$HOME/foo\"\n      '\n \n # The external test will outputs its own plan\ndiff --git a/t/t9700/test.pl b/t/t9700/test.pl\nindex 13ba96e..ce9340c 100755\n--- a/t/t9700/test.pl\n+++ b/t/t9700/test.pl\n@@ -33,6 +33,8 @@ is($r->config_int(\"test.int\"), 2048, \"config_int: integer\");\n is($r->config_int(\"test.nonexistent\"), undef, \"config_int: nonexistent\");\n ok($r->config_bool(\"test.booltrue\"), \"config_bool: true\");\n ok(!$r->config_bool(\"test.boolfalse\"), \"config_bool: false\");\n+is($r->config_path(\"test.path\"), $r->config(\"test.pathexpanded\"),\n+   \"config_path: ~/foo expansion\");\n our $ansi_green = \"\\x1b[32m\";\n is($r->get_color(\"color.test.slot1\", \"red\"), $ansi_green, \"get_color\");\n # Cannot test $r->get_colorbool(\"color.foo\")) because we do not\n"},{"id":"177187","messageId":"201110072317.17436.jnareb@gmail.com","threadId":"28517","inReplyTo":"7voby1oesm.fsf@alter.siamese.dyndns.org","subject":"[PATCH/RFC 3/2] Refactor Git::config_*","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-10-07T21:17:16Z","receivedAt":"2011-10-07T21:17:16Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"From: Junio C Hamano <gitster@pobox.com>\n\nThere is, especially with addition of Git::config_path(), much code\nrepetition in the Git::config_* family of subroutines.  Refactor\ncommon parts of Git::config(), Git::config_bool(), Git::config_int()\nand Git::config_path() into _config_common() subroutine.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nThis is version which has fixed style to be more Perl-ish, and which\nactually works (i.e. t9700 passes).\n\nI have also moved _config_common() after commands that use it, just like\nit is done with other \"private\" methods (methods with names starting with\n'_'), and excluded this private detail of implementation from docs.\n\n perl/Git.pm |   85 ++++++++++++++---------------------------------------------\n 1 files changed, 20 insertions(+), 65 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex c279bfb..d6df2e8 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -562,7 +562,6 @@ sub wc_chdir {\n \t$self->{opts}->{WorkingSubdir} = $subdir;\n }\n \n-\n =item config ( VARIABLE )\n \n Retrieve the configuration C<VARIABLE> in the same manner as C<config>\n@@ -570,30 +569,11 @@ does. In scalar context requires the variable to be set only one time\n (exception is thrown otherwise), in array context returns allows the\n variable to be set multiple times and returns all the values.\n \n-This currently wraps command('config') so it is not so fast.\n-\n =cut\n \n sub config {\n \tmy ($self, $var) = _maybe_self(@_);\n-\n-\ttry {\n-\t\tmy @cmd = ('config');\n-\t\tunshift @cmd, $self if $self;\n-\t\tif (wantarray) {\n-\t\t\treturn command(@cmd, '--get-all', $var);\n-\t\t} else {\n-\t\t\treturn command_oneline(@cmd, '--get', $var);\n-\t\t}\n-\t} catch Git::Error::Command with {\n-\t\tmy $E = shift;\n-\t\tif ($E->value() == 1) {\n-\t\t\t# Key not found.\n-\t\t\treturn;\n-\t\t} else {\n-\t\t\tthrow $E;\n-\t\t}\n-\t};\n+\treturn _config_common($self, $var);\n }\n \n \n@@ -603,60 +583,24 @@ Retrieve the bool configuration C<VARIABLE>. The return value\n is usable as a boolean in perl (and C<undef> if it's not defined,\n of course).\n \n-This currently wraps command('config') so it is not so fast.\n-\n =cut\n \n sub config_bool {\n \tmy ($self, $var) = _maybe_self(@_);\n-\n-\ttry {\n-\t\tmy @cmd = ('config', '--bool', '--get', $var);\n-\t\tunshift @cmd, $self if $self;\n-\t\tmy $val = command_oneline(@cmd);\n-\t\treturn undef unless defined $val;\n-\t\treturn $val eq 'true';\n-\t} catch Git::Error::Command with {\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} else {\n-\t\t\tthrow $E;\n-\t\t}\n-\t};\n+\tmy $val = scalar _config_common($self, $var, {'kind' => '--bool'});\n+\treturn (defined $val && $val eq 'true');\n }\n \n-\n =item config_path ( VARIABLE )\n \n Retrieve the path configuration C<VARIABLE>. The return value\n is an expanded path or C<undef> if it's not defined.\n \n-This currently wraps command('config') so it is not so fast.\n-\n =cut\n \n sub config_path {\n \tmy ($self, $var) = _maybe_self(@_);\n-\n-\ttry {\n-\t\tmy @cmd = ('config', '--path');\n-\t\tunshift @cmd, $self if $self;\n-\t\tif (wantarray) {\n-\t\t\treturn command(@cmd, '--get-all', $var);\n-\t\t} else {\n-\t\t\treturn command_oneline(@cmd, '--get', $var);\n-\t\t}\n-\t} catch Git::Error::Command with {\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} else {\n-\t\t\tthrow $E;\n-\t\t}\n-\t};\n+\treturn _config_common($self, $var, {'kind' => '--path'});\n }\n \n =item config_int ( VARIABLE )\n@@ -667,28 +611,39 @@ or 'g' in the config file will cause the value to be multiplied\n by 1024, 1048576 (1024^2), or 1073741824 (1024^3) prior to output.\n It would return C<undef> if configuration variable is not defined,\n \n-This currently wraps command('config') so it is not so fast.\n-\n =cut\n \n sub config_int {\n \tmy ($self, $var) = _maybe_self(@_);\n+\treturn scalar _config_common($self, $var, {'kind' => '--int'});\n+}\n+\n+# Common subroutine to implement bulk of what the config* family of methods\n+# do. This wraps command('config') so it is not so fast.\n+sub _config_common {\n+\tmy ($self, $var, $opts) = _maybe_self(@_);\n \n \ttry {\n-\t\tmy @cmd = ('config', '--int', '--get', $var);\n+\t\tmy @cmd = ('config', $opts->{'kind'} ? $opts->{'kind'} : ());\n \t\tunshift @cmd, $self if $self;\n-\t\treturn command_oneline(@cmd);\n+\t\tif (wantarray) {\n+\t\t\treturn command(@cmd, '--get-all', $var);\n+\t\t} else {\n+\t\t\treturn command_oneline(@cmd, '--get', $var);\n+\t\t}\n \t} catch Git::Error::Command with {\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;\n \t\t} else {\n \t\t\tthrow $E;\n \t\t}\n \t};\n+\n }\n \n+\n =item get_colorbool ( NAME )\n \n Finds if color should be used for NAMEd operation from the configuration,\n-- \n1.7.6\n"},{"id":"177188","messageId":"7vk48gtrh9.fsf@alter.siamese.dyndns.org","threadId":"28517","inReplyTo":"m3vcs01r32.fsf@localhost.localdomain","subject":"Re: [PATCH 1/2] Add Git::config_path()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-07T21:35:30Z","receivedAt":"2011-10-07T21:35:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh\n> index 3787186..7558f0c 100755\n> --- a/t/t9700-perl-git.sh\n> +++ b/t/t9700-perl-git.sh\n> @@ -43,7 +43,9 @@ test_expect_success \\\n>       git config --add test.booltrue true &&\n>       git config --add test.boolfalse no &&\n>       git config --add test.boolother other &&\n> -     git config --add test.int 2k\n> +     git config --add test.int 2k &&\n> +     git config --add test.path \"~/foo\" &&\n> +     git config --add test.pathexpanded \"$HOME/foo\"\n\nGiven that test-lib.sh sets up the $HOME away from unknown place to ensure\nrepeatability of tests, I am not sure if this test would ever pass.\n"},{"id":"177189","messageId":"201110072344.46556.jnareb@gmail.com","threadId":"28517","inReplyTo":"7vk48gtrh9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] Add Git::config_path()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-10-07T21:44:45Z","receivedAt":"2011-10-07T21:44:45Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 7 Oct 2011, Junio C Hamano wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n> > diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh\n> > index 3787186..7558f0c 100755\n> > --- a/t/t9700-perl-git.sh\n> > +++ b/t/t9700-perl-git.sh\n> > @@ -43,7 +43,9 @@ test_expect_success \\\n> >       git config --add test.booltrue true &&\n> >       git config --add test.boolfalse no &&\n> >       git config --add test.boolother other &&\n> > -     git config --add test.int 2k\n> > +     git config --add test.int 2k &&\n> > +     git config --add test.path \"~/foo\" &&\n> > +     git config --add test.pathexpanded \"$HOME/foo\"\n> \n> Given that test-lib.sh sets up the $HOME away from unknown place to ensure\n> repeatability of tests, I am not sure if this test would ever pass.\n\nWell, it passes.\n\ntest-lib.sh sets $HOME to \"$TRASH_DIRECTORY\", but this value of $HOME\nis then later seen by \"git config --path ...\" run by Git::config_path\nin t9700/test.pl\n\n  char *expand_user_path(const char *path)\n  [...]\n                if (username_len == 0) {\n                        const char *home = getenv(\"HOME\");\n                        if (!home)\n                                goto return_null;\n                        strbuf_add(&user_path, home, strlen(home));\n                } else {\n  [...]\n\n-- \nJakub Narebski\nPoland\n"},{"id":"177194","messageId":"7vbotstp4l.fsf@alter.siamese.dyndns.org","threadId":"28517","inReplyTo":"201110072344.46556.jnareb@gmail.com","subject":"Re: [PATCH 1/2] Add Git::config_path()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-07T22:26:18Z","receivedAt":"2011-10-07T22:26:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n>   char *expand_user_path(const char *path)\n>   [...]\n>                 if (username_len == 0) {\n>                         const char *home = getenv(\"HOME\");\n>                         if (!home)\n>                                 goto return_null;\n>                         strbuf_add(&user_path, home, strlen(home));\n>                 } else {\n>   [...]\n\nAhh, Ok. I was afraid getpwnam() codepath might interfere with it.\n"},{"id":"177887","messageId":"7vsjmrl4ur.fsf@alter.siamese.dyndns.org","threadId":"28517","inReplyTo":"201110072317.17436.jnareb@gmail.com","subject":"Re: [PATCH/RFC 3/2] Refactor Git::config_*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-17T20:50:52Z","receivedAt":"2011-10-17T20:50:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> This is version which has fixed style to be more Perl-ish, and which\n> actually works (i.e. t9700 passes).\n>\n> I have also moved _config_common() after commands that use it, just like\n> it is done with other \"private\" methods (methods with names starting with\n> '_'), and excluded this private detail of implementation from docs.\n\nIt seems that this breaks many tests in t9xxx series for me, especially\nthe 9100 series that cover git-svn.\n"},{"id":"177894","messageId":"201110172347.42568.jnareb@gmail.com","threadId":"28517","inReplyTo":"7vsjmrl4ur.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC 3/2] Refactor Git::config_*","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-10-17T21:47:41Z","receivedAt":"2011-10-17T21:47:41Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 17 Oct 2011, Junio C Hamano wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n> > This is version which has fixed style to be more Perl-ish, and which\n> > actually works (i.e. t9700 passes).\n> >\n> > I have also moved _config_common() after commands that use it, just like\n> > it is done with other \"private\" methods (methods with names starting with\n> > '_'), and excluded this private detail of implementation from docs.\n> \n> It seems that this breaks many tests in t9xxx series for me, especially\n> the 9100 series that cover git-svn.\n\nSorry about that, I have ran only t9700-perl-git.sh, which runs all right.\n\nThe problem was with duplicated _maybe_self(@_), which should be run only\nonce.  t9700-perl-git.sh passed because it uses only (recommended) object\nform, and not procedural form like git-svn.\n\nThe following amend fixes the issue for me:\n\n-- >8 --\ndiff --git i/perl/Git.pm w/perl/Git.pm\nindex 8e52290..32f6533 100644\n--- i/perl/Git.pm\n+++ w/perl/Git.pm\n@@ -653,7 +653,7 @@ sub config_int {\n # Common subroutine to implement bulk of what the config* family of methods\n # do. This wraps command('config') so it is not so fast.\n sub _config_common {\n-\tmy ($self, $var, $opts) = _maybe_self(@_);\n+\tmy ($self, $var, $opts) = @_;\n \n \ttry {\n \t\tmy @cmd = ('config', $opts->{'kind'} ? $opts->{'kind'} : ());\n-- 8< --\n\nI'll resend amended commit.\n-- \nJakub Narebski\nPoland\n"},{"id":"177910","messageId":"7vaa8zjj57.fsf@alter.siamese.dyndns.org","threadId":"28517","inReplyTo":"201110172347.42568.jnareb@gmail.com","subject":"Re: [PATCH/RFC 3/2] Refactor Git::config_*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-17T23:25:08Z","receivedAt":"2011-10-17T23:25:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> The problem was with duplicated _maybe_self(@_), which should be run only\n> once.  t9700-perl-git.sh passed because it uses only (recommended) object\n> form, and not procedural form like git-svn.\n\nThanks.\n\nI have a suspicion that a more standard implementation of _maybe_self()\nwould instead unshift a dummy singleton Git() instance instead of undef to\nlift such a limitation, but perhaps the code relies on undef'ness of self\nwhen called as a vanilla subroutine.\n"},{"id":"177941","messageId":"201110181147.02397.jnareb@gmail.com","threadId":"28517","inReplyTo":"201110172347.42568.jnareb@gmail.com","subject":"[PATCH/RFC 3/2 (fixed)] Refactor Git::config_*","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-10-18T09:47:01Z","receivedAt":"2011-10-18T09:47:01Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"From: Junio C Hamano <gitster@pobox.com>\n\nThere is, especially with addition of Git::config_path(), much code\nrepetition in the Git::config_* family of subroutines.\n\nRefactor common parts of Git::config(), Git::config_bool(),\nGit::config_int() and Git::config_path() into _config_common()\nhelper method, reducing code duplication.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nJakub Narebski wrote:\n>\n> I'll resend amended commit.\n\nHere it is.\n\n perl/Git.pm |   74 ++++++++++++++--------------------------------------------\n 1 files changed, 18 insertions(+), 56 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex c775b4f..8e1e2fd 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -576,24 +576,7 @@ This currently wraps command('config') so it is not so fast.\n \n sub config {\n \tmy ($self, $var) = _maybe_self(@_);\n-\n-\ttry {\n-\t\tmy @cmd = ('config');\n-\t\tunshift @cmd, $self if $self;\n-\t\tif (wantarray) {\n-\t\t\treturn command(@cmd, '--get-all', $var);\n-\t\t} else {\n-\t\t\treturn command_oneline(@cmd, '--get', $var);\n-\t\t}\n-\t} catch Git::Error::Command with {\n-\t\tmy $E = shift;\n-\t\tif ($E->value() == 1) {\n-\t\t\t# Key not found.\n-\t\t\treturn;\n-\t\t} else {\n-\t\t\tthrow $E;\n-\t\t}\n-\t};\n+\treturn _config_common($self, $var);\n }\n \n \n@@ -609,25 +592,10 @@ This currently wraps command('config') so it is not so fast.\n \n sub config_bool {\n \tmy ($self, $var) = _maybe_self(@_);\n-\n-\ttry {\n-\t\tmy @cmd = ('config', '--bool', '--get', $var);\n-\t\tunshift @cmd, $self if $self;\n-\t\tmy $val = command_oneline(@cmd);\n-\t\treturn undef unless defined $val;\n-\t\treturn $val eq 'true';\n-\t} catch Git::Error::Command with {\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} else {\n-\t\t\tthrow $E;\n-\t\t}\n-\t};\n+\tmy $val = scalar _config_common($self, $var, {'kind' => '--bool'});\n+\treturn (defined $val && $val eq 'true');\n }\n \n-\n =item config_path ( VARIABLE )\n \n Retrieve the path configuration C<VARIABLE>. The return value\n@@ -639,24 +607,7 @@ This currently wraps command('config') so it is not so fast.\n \n sub config_path {\n \tmy ($self, $var) = _maybe_self(@_);\n-\n-\ttry {\n-\t\tmy @cmd = ('config', '--path');\n-\t\tunshift @cmd, $self if $self;\n-\t\tif (wantarray) {\n-\t\t\treturn command(@cmd, '--get-all', $var);\n-\t\t} else {\n-\t\t\treturn command_oneline(@cmd, '--get', $var);\n-\t\t}\n-\t} catch Git::Error::Command with {\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} else {\n-\t\t\tthrow $E;\n-\t\t}\n-\t};\n+\treturn _config_common($self, $var, {'kind' => '--path'});\n }\n \n \n@@ -705,16 +656,27 @@ This currently wraps command('config') so it is not so fast.\n \n sub config_int {\n \tmy ($self, $var) = _maybe_self(@_);\n+\treturn scalar _config_common($self, $var, {'kind' => '--int'});\n+}\n+\n+# Common subroutine to implement bulk of what the config* family of methods\n+# do. This curently wraps command('config') so it is not so fast.\n+sub _config_common {\n+\tmy ($self, $var, $opts) = @_;\n \n \ttry {\n-\t\tmy @cmd = ('config', '--int', '--get', $var);\n+\t\tmy @cmd = ('config', $opts->{'kind'} ? $opts->{'kind'} : ());\n \t\tunshift @cmd, $self if $self;\n-\t\treturn command_oneline(@cmd);\n+\t\tif (wantarray) {\n+\t\t\treturn command(@cmd, '--get-all', $var);\n+\t\t} else {\n+\t\t\treturn command_oneline(@cmd, '--get', $var);\n+\t\t}\n \t} catch Git::Error::Command with {\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;\n \t\t} else {\n \t\t\tthrow $E;\n \t\t}\n-- \n1.7.6\n"},{"id":"177964","messageId":"7vty76f57d.fsf@alter.siamese.dyndns.org","threadId":"28517","inReplyTo":"201110181147.02397.jnareb@gmail.com","subject":"Re: [PATCH/RFC 3/2 (fixed)] Refactor Git::config_*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-18T19:52:06Z","receivedAt":"2011-10-18T19:52:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> From: Junio C Hamano <gitster@pobox.com>\n>\n> There is, especially with addition of Git::config_path(), much code\n> repetition in the Git::config_* family of subroutines.\n>\n> Refactor common parts of Git::config(), Git::config_bool(),\n> Git::config_int() and Git::config_path() into _config_common()\n> helper method, reducing code duplication.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> Signed-off-by: Jakub Narebski <jnareb@gmail.com>\n> ---\n> Jakub Narebski wrote:\n>>\n>> I'll resend amended commit.\n>\n> Here it is.\n\nWell, this breaks t9001 and I ended up spending an hour and half figuring\nout why. Admittedly, I was doing something else on the side, so it is not\nlike the whole 90 minutes is the billable hour for reviewing this patch,\nbut as far as the Git project is concerned, I didn't have any other branch\nchecked out in my working tree, so that whole time was what ended up\ncosting.\n\nThe real problem was here.\n\n> @@ -609,25 +592,10 @@ This currently wraps command('config') so it is not so fast.\n>  \n>  sub config_bool {\n>  \tmy ($self, $var) = _maybe_self(@_);\n> -\n> -\ttry {\n> -\t\tmy @cmd = ('config', '--bool', '--get', $var);\n> -\t\tunshift @cmd, $self if $self;\n> -\t\tmy $val = command_oneline(@cmd);\n> -\t\treturn undef unless defined $val;\n> -\t\treturn $val eq 'true';\n> ...\n> -\t};\n> +\tmy $val = scalar _config_common($self, $var, {'kind' => '--bool'});\n> +\treturn (defined $val && $val eq 'true');\n>  }\n\nCan you spot the difference?\n\nThis is the reason why I do not particularly like a rewrite for the sake\nof rewriting.\n"},{"id":"177971","messageId":"201110190009.40470.jnareb@gmail.com","threadId":"28517","inReplyTo":"7vty76f57d.fsf@alter.siamese.dyndns.org","subject":"[PATCH/RFC 3/2 v3] Refactor Git::config_*","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-10-18T22:09:39Z","receivedAt":"2011-10-18T22:09:39Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, 18 Oct 2011, Junio C Hamano wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n> > From: Junio C Hamano <gitster@pobox.com>\n> >\n> > There is, especially with addition of Git::config_path(), much code\n> > repetition in the Git::config_* family of subroutines.\n> >\n> > Refactor common parts of Git::config(), Git::config_bool(),\n> > Git::config_int() and Git::config_path() into _config_common()\n> > helper method, reducing code duplication.\n> >\n> > Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> > Signed-off-by: Jakub Narebski <jnareb@gmail.com>\n> > ---\n> > Jakub Narebski wrote:\n> > >\n> > > I'll resend amended commit.\n> >\n> > Here it is.\n> \n> Well, this breaks t9001 and I ended up spending an hour and half figuring\n> out why. Admittedly, I was doing something else on the side, so it is not\n> like the whole 90 minutes is the billable hour for reviewing this patch,\n> but as far as the Git project is concerned, I didn't have any other branch\n> checked out in my working tree, so that whole time was what ended up\n> costing.\n\nI'm sorry about that.  I have checked t9700-perl-git.sh and \nt9100-git-svn-basic.sh that the version before fixup had problems with,\nbut for some reason I had many spurious test failures, so I have not run\nthe full test suite (well, it would be enough to run those that require\nGit.pm).\n\nNb. I still have:\n\n  Test Summary Report\n  -------------------\n  t1402-check-ref-format.sh                        (Wstat: 256 Tests: 93 Failed: 1)\n    Failed test:  31\n    Non-zero exit status: 1\n  t4034-diff-words.sh                              (Wstat: 256 Tests: 34 Failed: 2)\n    Failed tests:  21, 25\n    Non-zero exit status: 1\n \n> The real problem was here.\n> \n> > @@ -609,25 +592,10 @@ This currently wraps command('config') so it is not so fast.\n> >  \n> >  sub config_bool {\n> >  \tmy ($self, $var) = _maybe_self(@_);\n> > -\n> > -\ttry {\n> > -\t\tmy @cmd = ('config', '--bool', '--get', $var);\n> > -\t\tunshift @cmd, $self if $self;\n> > -\t\tmy $val = command_oneline(@cmd);\n> > -\t\treturn undef unless defined $val;\n> > -\t\treturn $val eq 'true';\n> > ...\n> > -\t};\n> > +\tmy $val = scalar _config_common($self, $var, {'kind' => '--bool'});\n> > +\treturn (defined $val && $val eq 'true');\n> >  }\n> \n> Can you spot the difference?\n\nDamn.\n\nI really should have done refactoring from scratch myself, instead of basing\nit on \"how about that\" throwaway patch.\n\nFixed now in the version below.\n \n> This is the reason why I do not particularly like a rewrite for the sake\n> of rewriting.\n\nThe goal was to reduce code duplication to _avoid_ errors.\n\nWhat I have noticed is that there is slight difference between original\nGit::config_path and the one after refactoring.  In the version before\nthis patch the error catching part of config_path() looks like this:\n\n        } catch Git::Error::Command with {\n                my $E = shift;\n                if ($E->value() == 1) {\n                        # Key not found.\n                        return undef;\n                } else {\n                        throw $E;\n                }\n        };\n\nwhile after this patch (and in config()) it looks like this:\n\n        } catch Git::Error::Command with {\n                my $E = shift;\n                if ($E->value() == 1) {\n                        # Key not found.\n                        return;\n                } else {\n                        throw $E;\n                }\n        };\n\nI am not sure which one is right, but I suspect the latter.\nCord?\n\n-- >8 ------------ >8 -------------- >8 --\nFrom: Junio C Hamano <gitster@pobox.com>\n\nThere is, especially with addition of Git::config_path(), much code\nrepetition in the Git::config_* family of subroutines.\n\nRefactor common parts of Git::config(), Git::config_bool(),\nGit::config_int() and Git::config_path() into _config_common()\nhelper method, reducing code duplication.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\n perl/Git.pm |   73 ++++++++++++++++------------------------------------------\n 1 files changed, 20 insertions(+), 53 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex c279bfb..b7035ad 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -577,23 +577,7 @@ This currently wraps command('config') so it is not so fast.\n sub config {\n \tmy ($self, $var) = _maybe_self(@_);\n \n-\ttry {\n-\t\tmy @cmd = ('config');\n-\t\tunshift @cmd, $self if $self;\n-\t\tif (wantarray) {\n-\t\t\treturn command(@cmd, '--get-all', $var);\n-\t\t} else {\n-\t\t\treturn command_oneline(@cmd, '--get', $var);\n-\t\t}\n-\t} catch Git::Error::Command with {\n-\t\tmy $E = shift;\n-\t\tif ($E->value() == 1) {\n-\t\t\t# Key not found.\n-\t\t\treturn;\n-\t\t} else {\n-\t\t\tthrow $E;\n-\t\t}\n-\t};\n+\treturn _config_common($self, $var);\n }\n \n \n@@ -610,24 +594,11 @@ This currently wraps command('config') so it is not so fast.\n sub config_bool {\n \tmy ($self, $var) = _maybe_self(@_);\n \n-\ttry {\n-\t\tmy @cmd = ('config', '--bool', '--get', $var);\n-\t\tunshift @cmd, $self if $self;\n-\t\tmy $val = command_oneline(@cmd);\n-\t\treturn undef unless defined $val;\n-\t\treturn $val eq 'true';\n-\t} catch Git::Error::Command with {\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} else {\n-\t\t\tthrow $E;\n-\t\t}\n-\t};\n+\tmy $val = scalar _config_common($self, $var, {'kind' => '--bool'});\n+\treturn undef unless defined $val;\n+\treturn $val eq 'true';\n }\n \n-\n =item config_path ( VARIABLE )\n \n Retrieve the path configuration C<VARIABLE>. The return value\n@@ -640,23 +611,7 @@ This currently wraps command('config') so it is not so fast.\n sub config_path {\n \tmy ($self, $var) = _maybe_self(@_);\n \n-\ttry {\n-\t\tmy @cmd = ('config', '--path');\n-\t\tunshift @cmd, $self if $self;\n-\t\tif (wantarray) {\n-\t\t\treturn command(@cmd, '--get-all', $var);\n-\t\t} else {\n-\t\t\treturn command_oneline(@cmd, '--get', $var);\n-\t\t}\n-\t} catch Git::Error::Command with {\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} else {\n-\t\t\tthrow $E;\n-\t\t}\n-\t};\n+\treturn _config_common($self, $var, {'kind' => '--path'});\n }\n \n =item config_int ( VARIABLE )\n@@ -674,15 +629,27 @@ This currently wraps command('config') so it is not so fast.\n sub config_int {\n \tmy ($self, $var) = _maybe_self(@_);\n \n+\treturn scalar _config_common($self, $var, {'kind' => '--int'});\n+}\n+\n+# Common subroutine to implement bulk of what the config* family of methods\n+# do. This curently wraps command('config') so it is not so fast.\n+sub _config_common {\n+\tmy ($self, $var, $opts) = @_;\n+\n \ttry {\n-\t\tmy @cmd = ('config', '--int', '--get', $var);\n+\t\tmy @cmd = ('config', $opts->{'kind'} ? $opts->{'kind'} : ());\n \t\tunshift @cmd, $self if $self;\n-\t\treturn command_oneline(@cmd);\n+\t\tif (wantarray) {\n+\t\t\treturn command(@cmd, '--get-all', $var);\n+\t\t} else {\n+\t\t\treturn command_oneline(@cmd, '--get', $var);\n+\t\t}\n \t} catch Git::Error::Command with {\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;\n \t\t} else {\n \t\t\tthrow $E;\n \t\t}\n-- \n1.7.6\n"},{"id":"177972","messageId":"7vmxcxg9wr.fsf@alter.siamese.dyndns.org","threadId":"28517","inReplyTo":"201110190009.40470.jnareb@gmail.com","subject":"Re: [PATCH/RFC 3/2 v3] Refactor Git::config_*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-18T23:25:08Z","receivedAt":"2011-10-18T23:25:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n>> Well, this breaks t9001 and I ended up spending an hour and half figuring\n>> out why. Admittedly, I was doing something else on the side, so it is not\n>> like the whole 90 minutes is the billable hour for reviewing this patch,\n>> but as far as the Git project is concerned, I didn't have any other branch\n>> checked out in my working tree, so that whole time was what ended up\n>> costing.\n>\n> I'm sorry about that.\n\nNo need to be sorry. After all you just re-sent a throw-away patch with\nminor tweaks, but unfortunately a tricky Perl script sometimes needs line\nby line inspection with fine toothed comb to avoid regression.\n\n>> The real problem was here.\n>> ...\n>> > -\t\tmy $val = command_oneline(@cmd);\n>> > -\t\treturn undef unless defined $val;\n>> > -\t\treturn $val eq 'true';\n>> > ...\n>> > -\t};\n>> > +\tmy $val = scalar _config_common($self, $var, {'kind' => '--bool'});\n>> > +\treturn (defined $val && $val eq 'true');\n>> >  }\n>> \n>> Can you spot the difference?\n>\n> Damn.\n\nFor people following from the sideline, the difference that bit us was\nthat\n\n    return $val eq 'true';\n\nyields \"\" (not undef) for false, but some callers care the distinction\nbetween undef kind of false and other kinds of false. It is not really the\nfault of the language per-se, but still... \n\n>> This is the reason why I do not particularly like a rewrite for the sake\n>> of rewriting.\n>\n> The goal was to reduce code duplication to _avoid_ errors.\n\nThat is exactly why I said I do not like a rewrite for such purpose.\n\nOnce the end result of smaller code is done correctly, it would help\navoiding future errors, but people tend to think unconsciously that the\nchange to reduce code duplication is much safer than adding new code.\nUpon receiving such a patch, without knowing that it was not done with\nenough attention to detail with fine toothed comb, it is me who ends up\nspending nontrivial amount of time fixing the breakage. These \"clean-ups\"\nare not cheap.\n\n> What I have noticed is that there is slight difference between original\n> Git::config_path and the one after refactoring.  In the version before\n> this patch the error catching part of config_path() looks like this:\n> ...\n>                         return undef;\n> ...\n> while after this patch (and in config()) it looks like this:\n> ...\n>                         return;\n> ...\n\n> I am not sure which one is right, but I suspect the latter.\n\nThis is Perl---\"return;\" returns undef, so there is no right or wrong.\n\nHaving said that, I tend to prefer being explicit so that people not so\nfamiliar with the language do not have to waste time wondering about such\ndifferences. Especially where it matters, like this case where some\ncallers may care about different kinds of falsehood.\n\nThat is another reason I tend to hate the kind of \"this makes it more\nPerl-ish\" changes, as they tend to force readers to spend extra brain\ncells to see what is going on. I'd rather spell things more explicit,\nespecially when the distinction matters.\n\nI've already pushed out the fixed one as 6942a3d (libperl-git: refactor\nGit::config_*, 2011-10-18), with this bit:\n\n    - ...\n    -               my $val = command_oneline(@cmd);\n    -               return undef unless defined $val;\n    +       # Do not rewrite this as return (defined $val && $val eq 'true')\n    +       # as some callers do care what kind of falsehood they receive.\n    +       if (!defined $val) {\n    +               return undef;\n    +       } else {\n                    return $val eq 'true';\n      ...\n"},{"id":"177973","messageId":"201110190219.38331.jnareb@gmail.com","threadId":"28517","inReplyTo":"7vmxcxg9wr.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC 3/2 v3] Refactor Git::config_*","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-10-19T00:19:37Z","receivedAt":"2011-10-19T00:19:37Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n>>> The real problem was here.\n>>> ...\n>>>> -\t\tmy $val = command_oneline(@cmd);\n>>>> -\t\treturn undef unless defined $val;\n>>>> -\t\treturn $val eq 'true';\n>>>> ...\n>>>> -\t};\n>>>> +\tmy $val = scalar _config_common($self, $var, {'kind' => '--bool'});\n>>>> +\treturn (defined $val && $val eq 'true');\n>>>>  }\n>>> \n>>> Can you spot the difference?\n>>\n>> Damn.\n> \n> For people following from the sideline, the difference that bit us was\n> that\n> \n>     return $val eq 'true';\n> \n> yields \"\" (not undef) for false, but some callers care the distinction\n> between undef kind of false and other kinds of false. It is not really the\n> fault of the language per-se, but still... \n\nAnd Git::config* family of commands return 'undef' if key was not found.\nSo Git::config_bool() can return true, false or undef, and the last part\nis important. \n \n>> What I have noticed is that there is slight difference between original\n>> Git::config_path and the one after refactoring.  In the version before\n>> this patch the error catching part of config_path() looks like this:\n>> ...\n>>                         return undef;\n>> ...\n>> while after this patch (and in config()) it looks like this:\n>> ...\n>>                         return;\n>> ...\n>> \n>> I am not sure which one is right, but I suspect the latter.\n> \n> This is Perl---\"return;\" returns undef, so there is no right or wrong.\n\nActually this is not true: \"return;\" returns undef in scalar context,\nand empty list in list context.  This means that result always evaluates\nto false...\n \n> Having said that, I tend to prefer being explicit so that people not so\n> familiar with the language do not have to waste time wondering about such\n> differences. Especially where it matters, like this case where some\n> callers may care about different kinds of falsehood.\n> \n> That is another reason I tend to hate the kind of \"this makes it more\n> Perl-ish\" changes, as they tend to force readers to spend extra brain\n> cells to see what is going on. I'd rather spell things more explicit,\n> especially when the distinction matters.\n\n...and that is why \"return;\" is more Perl-ish, and usually safer.\n\nThere might be exceptions where we want to return one-element list\ncontaining single undef element in list context, but I guess they\nare exceedingly rare.\n \n> I've already pushed out the fixed one as 6942a3d (libperl-git: refactor\n> Git::config_*, 2011-10-18), with this bit:\n> \n>     - ...\n>     -               my $val = command_oneline(@cmd);\n>     -               return undef unless defined $val;\n>     +       # Do not rewrite this as return (defined $val && $val eq 'true')\n>     +       # as some callers do care what kind of falsehood they receive.\n>     +       if (!defined $val) {\n>     +               return undef;\n>     +       } else {\n>                     return $val eq 'true';\n>       ...\n\nI like mine better... ;-))))\n\n-- \nJakub Narebski\nPoland\n"}]}