Re: [PATCH] Refactoring url to original url (storeurl step 1)

On 11/28/2012 6:36 PM, Alex Rousskov wrote:
> On 11/27/2012 11:37 PM, Eliezer Croitoru wrote:
>> The patch is refactoring of two variables:
>> mem_object->url into mem_obj->original_url
>> Store_entry->url into Store_entry->originalUrl
>>
>> Both are named inline with other variables in their scope.
>> This is the first step towards Store_url completion.
>
> Please consider documenting the renamed method and data member by adding
> a brief doxygen comment for them. After spending so much time with this
> code, you probably know what these members are! :-)

I kind of hope I know it :D
Now I remember only little about it and I know more about specific other
points in the code that make use of it.
But I will try to put it in my list.
>
> The new comment in StoreMetaURL::checkConsistency() does not belong to
> this patch, IMO. If you insist on keeping it, then please spellcheck
> "rewritted".
OK, I will add it later on.

>
> Overall, I think this change is a very important step, but it should not
> be committed until we have other steps (or at all):
>
> * The primary point of this change is to track cases where the rewritten
> "store URL" should be used instead of the "original URL". If we commit
> this patch now, we will not longer be able to track those cases when
> reviewing your future patches.

It seems to me This is not the real idea.
A while back We discussed about it and Henrik if i'm not wrong suggested
what you did.

The basic idea of this major change is to take the "url" statement and
change the meaning of it to something usable.
I do not expect anyone that works with squid code ever to understand the
exact reasons I did the change since they almost always dont know other
code.
But These point of change do reflect in major points of the code the
distinction between the "store url" which is the original url and to the
"store rewritten url".

>
> * There is no need to create hundreds of conflicts with pending and
> future changes for the renaming sake alone. At the very end of the
> project, the renaming-only changes can be removed OR, if we decide that
> they should stay, there will be a compelling reason to keep them -- we
> would get the store_url feature as a payoff.
Indeed if it was avoidable I would agree with you.
At the beginning I thought the same but since I researched squid code
for a long time to make it work it seems unlikely to understand the code
without changing it(unless you know the code like the palm of your hand).

Consider For now that the only distance between the feature to be from
beta to stable is that the code wouldn't change every couple days or
Basic changes would be done to allow slowly migrating the code changes.

Just notice that this is the most painful patch in the whole feature.

>
>
> Thank you,
>
> Alex.
>
Eliezer

-- 
Eliezer Croitoru
https://www1.ngtech.co.il
sip:ngtech_at_sip2sip.info
IT consulting for Nonprofit organizations
eliezer <at> ngtech.co.il