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

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

Jakub Narebski [off-list ref] writes:

quoted

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.

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