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.
