Re: [PATCH]proposal for a fake_strore_url helper wrapping.

On 09/10/2012 10:16 AM, Eliezer Croitoru wrote:
> On 09/10/2012 05:39 PM, Alex Rousskov wrote:

>> When you think these changes are ready for commit, please include a
>> proposed commit message. It is a good idea to do that even before the
>> last stage, of course -- it tells us what the patch is supposed to do.
>> "what ever seems needed to create a url_store_rewrite helper" is too
>> vague. If the code is committed with such a message, most of the
>> knowledge gained during recent discussions will be lost and somebody
>> would have to start from scratch when they want to understand how the
>> new feature works and, more importantly, why it should work that way.

> it's not that vague if you do ask me.

Please assume that a reader knows a lot about Squid3 but does not know
what a "url_store_rewrite helper" is. Target _that_ reader in your
proposed commit message rather than writing a note to yourself.

> I dont want to maintain a branch but to push it into squid as a feature.

Those two options are not mutually exclusive. And I am not saying you
have to maintain a branch, especially a public one. Using bzr or another
VCS makes it easier to manage and post changes for some, but that is
your personal decision. Do whatever you feel comfortable with.

> I proposed from the beginning to make it in stages:
> 1. embed a fake store url rewrite fake.(naming is not really important)
> as grounds to the next steps.
> this part is pretty important by itself because no one even done that in
> the 3+.
> 2. embed the code that actually handles the data from the external
> program when storing it in squid process.
> 3. use it at every point needed in squid while handling the requests.

Sounds like a good plan for your project. I think we should commit the
results of step #3 once they are ready.

To be clear, I think we should not push dummy code (e.g., code after
step #1 or #2) into Squid trunk. Others may overrule me, of course, but
I do not recommend counting on that or fighting over it.

>> Please do not forget to document all new members (using doxygen
>> comments) -- this is one of the mandatory requirements for patches. And,
>> as Amos pointed out, remove "for storeurl" marks -- everything you
>> change in this patch is for the storeurl feature (or at least it should
>> be) so the diff blobs themselves can be used as such a mark.

> Since I Have never used doxygen and friends in my code before I will try
> to learn them on the way.

We do not need anything fancy here. For new member/function
descriptions, basic "///", "/**", and "//<" comment openers is all it
takes. The important part is the content of that comment, especially for
members with vague names such as "data", "state", "object", etc.

> and a question:
> since the http object has uri,log_uri we used store_uri to match the
> others.
> but at the mem-obj there is a url methods.
> so do we want to stick with anything on the way? nameing the method at
> mem_obj->storeurl ?
> The 2.7 user storeurl for everything and I mean everything till the last
> column.
> So what do you think?

Sorry, I am not sure I understand the question, but I think
MemoryObject::url (and similar ambiguous cases) should be renamed, at
least temporary, to make sure we catch all cases of not-rewritten URL
used for Store purposes.

Cheers,

Alex.
Received on Mon Sep 10 2012 - 16:44:51 MDT