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

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.

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.

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

          |  cache_enable     |    caching_enabled
----------+-------------------+---------------------
enabled:  |        1          |            1
disabled: |        0          |            0

doing a double negative like that doesn't really buy you much except
turning 0 into NULL or '' which is equivalent to 0...

- John 'Warthog9' Hawley