Re: [PATCH] gitweb: fixes to gitweb feature check code

From: Jakub Narebski <jnareb@gmail.com>
Date: Dec 3, 2008, 01:21 UTC
Message-ID: <200812030221.31180.jnareb@gmail.com>
In-Reply-To: <7v4p1mp7hx.fsf@gitster.siamese.dyndns.org>

Dnia wtorek 2. grudnia 2008 22:55, Junio C Hamano napisał:

Show 24 quoted lines

> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > What I like about having all this, i.e. fix, futureproof and style
> > correction in one single patch is the fact that fix doesn't introduce
> > strange looking (gitweb_check_feature('bool_feat'))[0]... well, except
> > encapsulated in a subroutine.
> >
> > From all possible splits of this feature into series of up to three
> > patches I think I like the one with pure subroutine rename from *check*
> > to *get* least...
> 
> Well, I have to say that you have a strange taste, sense of priorities,
> and perhaps aversion to logical progression.  Let's explain one more
> time.
> 
> The case we had at hand was that a callee has a less-than-ideal calling
> convention that has caused a few bugs by callers because they did not
> understand the calling convention.  You can argue it is not entirely
> caller's fault that they failed to follow the calling convention, but the
> fact remains that there are bugs taken as a whole.
> 
> First we fix the callers, because existing bugs get highest priority.
> This is a pure bugfix patch that could even go to maintenance "bugfix
> only" branch.

I agree here.

Show 5 quoted lines

> Then we fix the calling convention because we all know that the calling
> convention was less-than-ideal.  A large part of the reason the calling
> convention was confusing was because the wording "check" implied it was a
> boolean function.  Logically, s/check/get/ would be a major part of fixing
> that.

And here I slightly disagree. The lone change s/check/get/ doesn't
actually do anything. Splitting wrongly named 'check' into checking
if feature is enabled and actual getting config does. And it is IMVHO
quote obvious which calls are to be to new semantic of 'check':
  my ($var) = gitweb_check_feature('feature');
and
  (gitweb_check_feature('feature'))[0]

and which are to be renamed to 'get':
  my ($a, $b) = gitweb_check_feature('feature');
and
  my @a  = gitweb_check_feature('feature');

Perhaps it is more clear from the point of view of reviewing individual pieces...

-- 
Jakub Narebski
Poland