Re: [PATCH 07/18] gitweb: Revert back to $cache_enable vs. $caching_enabled

On Fri, 10 Dec 2010, J.H. wrote:

quoted

Formally, there is no backward compatibility with any released code.
Using out-of-tree patches is on one's own risk.

I will have to beg to differ with you on this, the entirety of the
existing caching engine has been released code for a number of years,
there are rpm packages available for it, at the very least, in Fedora
and in EPEL.

The caching engine *IS* released code, and this patchset is as much a
new feature as an attempt to merge a fork.  Kernel.org isn't the only
one running this code, and that has been the case for several years now
already.

Claiming that this isn't released code is doing me a disservice to me,
and those who have submitted patches to it independent of git and the
mainline gitweb.

Thinking about the patch series outside of that context will lead to me
putting my foot down and arguing on those other users behalf.  I'm not
keen on breaking them for no good reason, and I'm not seeing your change
here as one that's particularly worthwhile, while causing external
breakage for no reason.

I am so very sorry.  Please excuse me.  I didn't intent this to be arguing
against backwards compatibility with what amounts to gitweb fork, but rather
grumbling about maintaining our mistakes due to backwards compatibility
requirement.  I see now that it reads as arguing for breaking backwards
compatibility: the "Formally" qualifier is too weak.

That said I would rather there was no need for forking, or at least for
the caching patches to be peer-reviewed on git mailing list, even if they
wouldn't be accepted / merged in, or merged in soon enough to avoid need
for fork.

quoted

But even discarding that, I'd rather use the same solution as in

  [PATCHv6/RFC 22/24] gitweb: Support legacy options used by kernel.org caching engine
  http://thread.gmane.org/gmane.comp.version-control.git/163052/focus=163058
  http://repo.or.cz/w/git/jnareb-git.git/commitdiff/27ec67ad90ecd56ac3d05f6a9ea49b6faabf7d0a

i.e.

  our $cache_enable;

  [...]

  # somewhere just before call to cache_fetch()
  $caching_enabled = !!$cache_enable if defined $cache_enable;

quoted

This reverts back to the previous variable to enable / disable caching

Is there really any point in changing the name at all?  The intention of
cache_enable, at one point, was to allow for other caching engines and
while there aren't any other caching engines that use it, it's already
treated identically to cache_enable.

If it really adds enough to the readability to the code, then I'm fine
with adding:

	$caching_enabled = $cache_enable if defined $cache_enable;

But now you are setting up two variables that control the same thing,
adding the possibility for conflicts and confusion to end users.

I just want that stated.

I guess I can live (I'd have to live) with $cache_enable instead of
$caching_enabled as name of *boolean* variable controlling whether
caching is turned on or off.  Though I'd argue that $caching_enabled
is better name:

  if ($caching_enabled) {

reads naturally as "if caching [is] enabled"; not so with $cache_enable.
$cache_enable as enum is just a bad, bad idea, as is conflating enabling
caching with selecting caching engine (c.f. http://lwn.net/Articles/412131/
though only very peripherally - it is about other "conflated designs").


BTW. when leaving $cache_enable from "[PATCHv6/RFC 22/24] gitweb: Support
legacy options used by kernel.org caching engine" I forgot that it is
actually $cache_enable (which is 0 by default) that needs to be set up
if one wants caching.  All the rest of cache config variables can be left
at their default values... though, J.H., are they?

Also, why the double negative in your original snippet - that doesn't
entirely make sense....

I don't know why I felt that I needed to convert it to bool...

-- 
Jakub Narebski
Poland