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

From: J.H. <warthog9@eaglescrag.net>
Date: Dec 10, 2010, 02:38 UTC
Message-ID: <4D019288.9060503@eaglescrag.net>
In-Reply-To: <m3y67y1psd.fsf@localhost.localdomain>

> 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.

Show 17 quoted lines

> 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;
> 
>>
>> 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