[PATCH] Git.pm: Don't return 'undef' in vector context.

[PATCH] Git.pm: Don't return 'undef' in vector context.

[Dan Zwell]

Previously, the Git->repository()->config('non-existent.key')
evaluated to as true in a vector context. Return an empty list
instead.
---
I don't know whether this breaks anything, because I don't use most of 
the git perl scripts. I can't imagine that there is a script that relies 
on the fact that config('non-existent.key') actually returns (''), in an 
array context. Is this a reasonable change?

  perl/Git.pm |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index e9dc706..ffcc541 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -508,7 +508,7 @@ sub config {
  		my $E = shift;
  		if ($E->value() == 1) {
  			# Key not found.
-			return undef;
+			return wantarray ? () : undef;
  		} else {
  			throw $E;
  		}
-- 
1.5.3.5.565.gf0b83-dirty

Re: [PATCH] Git.pm: Don't return 'undef' in vector context.

[Junio C Hamano]

Dan Zwell <dzwell@gmail.com> writes:

> Previously, the Git->repository()->config('non-existent.key')
> evaluated to as true in a vector context. Return an empty list
> instead.
> ---
> I don't know whether this breaks anything, because I don't use most of
> the git perl scripts. I can't imagine that there is a script that
> relies on the fact that config('non-existent.key') actually returns
> (''), in an array context. Is this a reasonable change?

I did not examine the callers but my gut feeling is that it
would be simpler and cleaner to always return () without
checking the context.  In scalar context:

	sub null {
        	...
                return ();
	}
	my $scalar = null();

would assign undef to $scalar anyway.

I generally try to stay away from functions that changes their
return values depending on the context, because they tend to
make reading the callers to find bugs more difficult.  An
exception is a function that tries to mimic a built-in operator,
because reading the callers to such a function, as long as it is
clear which built-in the function is imitating, can apply the
same knowledge on how the callee would behave you already have
by knowing Perl itself.

The same thing can be said about functions with prototypes to
force certain context on the caller's side.  Avoid it unless
there is a good reason.

Maybe it is just me, but that's my reaction.

Re: [PATCH] Git.pm: Don't return 'undef' in vector context.

[Dan Zwell]

Junio C Hamano wrote:
> I did not examine the callers but my gut feeling is that it
> would be simpler and cleaner to always return () without
> checking the context.  In scalar context:
> 
> 	sub null {
>         	...
>                 return ();
> 	}
> 	my $scalar = null();
> 
> would assign undef to $scalar anyway.
> 
> I generally try to stay away from functions that changes their
> return values depending on the context, because they tend to
> make reading the callers to find bugs more difficult.
> <snip>

That's reasonable. I'll resend this as part of the git-add--interactive 
color patches. This can be cherry-picked out, but some of the other 
stuff I want to do depends on it (a helper function that I wrote, 
config_with_default($repo, $key, $default)).

Dan

Re: [PATCH] Git.pm: Don't return 'undef' in vector context.

[Jakub Narebski]

[Cc: Dan Zwell <dzwell@gmail.com>, Petr Baudis <pasky@suse.cz>,
     git@vger.kernel.org]

Dan Zwell wrote:

> Previously, the Git->repository()->config('non-existent.key')
> evaluated to as true in a vector context. Return an empty list
> instead.

Nice.

By the way, what do you think about changing Git.pm config handling
to the 'eager' one used currently by gitweb, namely reading all the
config to hash, and later getting config values from hash instead of
calling git-config? Or at least make it an option?

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] Git.pm: Don't return 'undef' in vector context.

[Sebastian Harl]

[-- Attachment #1: Type: text/plain, Size: 1407 bytes --]

Hi,

On Thu, Nov 15, 2007 at 10:39:26PM -0800, Junio C Hamano wrote:
> Dan Zwell <dzwell@gmail.com> writes:
> > Previously, the Git->repository()->config('non-existent.key')
> > evaluated to as true in a vector context. Return an empty list
> > instead.
> > ---
> > I don't know whether this breaks anything, because I don't use most of
> > the git perl scripts. I can't imagine that there is a script that
> > relies on the fact that config('non-existent.key') actually returns
> > (''), in an array context. Is this a reasonable change?
> 
> I did not examine the callers but my gut feeling is that it
> would be simpler and cleaner to always return () without
> checking the context.
[...]
> I generally try to stay away from functions that changes their
> return values depending on the context, because they tend to
> make reading the callers to find bugs more difficult.

In fact, if you simply return without any value, it will evaluate to "false"
no matter which context it has been called in ("()" in list context, "undef"
in scalar context, etc.). So, it's generally a good idea to use "return;" to
indicate an error.

Cheers,
Sebastian

-- 
Sebastian "tokkee" Harl +++ GnuPG-ID: 0x8501C7FC +++ http://tokkee.org/

Those who would give up Essential Liberty to purchase a little Temporary
Safety, deserve neither Liberty nor Safety.         -- Benjamin Franklin


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Git.pm: Don't return 'undef' in vector context.

[Junio C Hamano]

Dan Zwell <dzwell@gmail.com> writes:

> That's reasonable. I'll resend this as part of the
> git-add--interactive color patches. This can be cherry-picked out, but
> some of the other stuff I want to do depends on it (a helper function
> that I wrote, config_with_default($repo, $key, $default)).

Sure; as long as the "Don't return 'undef'" remains a separate
patch early in the series, that is easy to manage.

Thanks.

Re: [PATCH] Git.pm: Don't return 'undef' in vector context.

[Dan Zwell]

Jakub Narebski wrote:
> 
> By the way, what do you think about changing Git.pm config handling
> to the 'eager' one used currently by gitweb, namely reading all the
> config to hash, and later getting config values from hash instead of
> calling git-config? Or at least make it an option?
> 

That seems appropriate, though it may be a slight trade-off between 
complexity and efficiency. I don't think it's strictly necessary, at 
least for git-add--interactive. My experience is that the nine calls to 
config() (that I am adding) do not slow down the program from a user 
perspective (though I haven't tested on a slower computer).

The big reason to do it would be if you wanted to convert gitweb to use 
the standard config() call from Git.pm. Because right now, config() 
isn't efficient, but it probably doesn't need to be.

Dan

Re: [PATCH] Git.pm: Don't return 'undef' in vector context.

[Jakub Narebski]

On Saturday, 17 November 2007, Dan Zwell wrote:
> Jakub Narebski wrote:
> > 
> > By the way, what do you think about changing Git.pm config handling
> > to the 'eager' one used currently by gitweb, namely reading all the
> > config to hash, and later getting config values from hash instead of
> > calling git-config? Or at least make it an option?
> > 
> 
> That seems appropriate, though it may be a slight trade-off between 
> complexity and efficiency. I don't think it's strictly necessary, at 
> least for git-add--interactive. 

On one hand it adds a bit of complexity, as you have to check if
config was parsed (and either parse, as it is done now in gitweb,
or perhaps fallback on calling git-config per variable), and you
have to do type checking / conversion to bool and int (size suffixes!)
in Perl, and deal with single-value and multi-value variables.
 
On the other hand I think that error handling will be simplier.

>                                 My experience is that the nine calls to  
> config() (that I am adding) do not slow down the program from a user 
> perspective (though I haven't tested on a slower computer).

The problem is not so much with a slower computer, as operating
systems with inefficient, slow fork implementation, like MS Windows
and (if I remember) correctly MacOS X.

But it is true that the config() performance is needed less for desktop
(commands like git-add--interactive) than for web (gitweb for example).

> The big reason to do it would be if you wanted to convert gitweb to use 
> the standard config() call from Git.pm. Because right now, config() 
> isn't efficient, but it probably doesn't need to be.

There are two reasons against convering gitweb to use Git.pm, at least
for now.

First, it makes installing gitweb harder, as one would have to install
(and perhaps compile) Git.pm to use gitweb. It would add additional
dependency. But perhaps this is not as much a problem as I think...

Second, gitweb code contain in few places pipelines (e.g. tar.gz and
tar.bz2 snapshot pipelines); even if some of them can be eliminated,
some will be added (syntax highlighting in blob view). Git.pm doesn't
as of yet support this...


It would be nice if Git.pm could take advantage of libification project,
and git have fast bindings not only for Python but also for Perl. If I
remember correctly the early attempts to use "git library" directly
were abandoned becaue they were not sufficiently portable, and the
fallback-to-Perl idea was thought too complex to implement...

-- 
Jakub Narebski
Poland