Cleanup: storeAppendPrintf removal script

[yadij]

Automatically convert deprecated uses of storeAppendPrintf()
to use the Packable API method.

The store API definitions and some difficult to auto-remove
uses remain for future manual removal once all developer
branches have been updated by this script.

[rousskov]

1. Unlike HERE, storeAppendPrintf() is not a major annoyance. Few cycles are wasted on fixing/unfixing storeAppendPrintf() calls in pull requests.
2. The vast majority of the changed calls will need to be changed again to properly support YAML output format.

Each fact in isolation may not be sufficient to block this PR, but the combination is enough IMO.

P.S. Ironically, the author should reject his own PR for the same (invalid) reasons he rejected #45 -- the proposed script does not remove some of the inconsistencies in storeAppendPrintf() calls.

[yadij]

This removal is to prevent unnecessary dependencies on store.cc just because some objects have a dump()'er by every other library and binary in the Squid codebase.

Please point out the inconsistencies mentioned in your 'P.S.' they should have been fixed in the
afda12b manual followup commit for pieces that cannot be easily scripted (whitespace one by the scripted format enforcement). No irony, this PR uses the change process I advocated for the (unrelated) HERE removal PR to follow:
a) bulk change with script command(s) that branches/patches can use independently, followed by
b) additional manual polish and removal of the macro/symbol itself and trickier uses in a smaller patch.
ie. not trying to get the script to be perfect, just skip the parts it cannot do easily in (a).

[rousskov]

This removal is to prevent unnecessary dependencies on store.cc

Please note that the PR description is missing this important information.

This removal is to prevent unnecessary dependencies on store.cc

I cannot find a way to interpret the above correctly because the removed storeAppendPrintf() and its replacement StoreEntry::appendf() are both defined in store.cc. Replacing one with the other will not, AFAICT, change dependencies. Furthermore, if anything, replacing a stand-alone function call with a method call makes future reduction a lot more difficult and, with future YAML support in mind, is a step in the wrong direction.

Please point out the inconsistencies mentioned in your 'P.S.'

Again, IMO, these inconsistencies are not relevant to this PR, just like the inconsistencies that you blocked PR #45 for were not relevant to that PR. I do not see a point in discussing non-existent and not-requested out-of-scope changes here. I am glad you did not waste your time on them here. I just wish you ignored them in PR #45 as well, instead of blocking that PR because it does not address them.

[rousskov]

My 2018 review concerns remain valid AFAICT. However, even if we decide that those concerns are insufficient to block this PR, the current PR approach to solving the problem would still need to be changed as detailed below: After a C++ construct X is removed, we should use the compiler (not the maintenance script) to reject any new code containing X.

PR description: some difficult to auto-remove uses remain for future manual removal once all developer branches have been updated by this script.

The "all developer branches are updated" is not a precondition for removing widespread deprecated calls. One of those preconditions is, roughly speaking, "there is a script to remove the vast majority of those deprecated calls from developer branches". Individual developers will decide whether/when to apply that script to their branches.

AFAICT, until we support merging unsquashed changes, the overall procedure for the removal of widespread deprecated calls should follow this outline:

0. Develop consensus that a change dedicated to the removal is welcomed.

1. Develop a stand-alone script that removes the vast majority of cases. Posting the script as a GitHub gist may be a good idea.

2. Post a PR that manually removes the remaining cases (if any). In the PR description, provide a link to the script from item 1. In a PR comment, provide a link to a commit that shows the effects of applying the script to the PR branch. At this time, that commit should have the tip of the PR branch as a parent but not belong to the PR branch (so that it does not pollute the primary diff under review).

3. Get PR approved using the regular procedure for approving Squid changes. If the PR branch changes during this PR review, the commit with automated changes should be updated as needed, of course.

4. Fast-forward the PR branch to include the commit with automated changes. No changes to the already reviewed PR branch or that commit are allowed at this point -- the commit hash must remain the same as it was at the end of the PR review, guaranteeing that there are no last-minute "surprises" in automated changes.

5. Clear the PR for merging.

The above procedure does not pollute Squid code with irrelevant scripts, reduces automation noise during review of manual changes, and still allows the folks to (re)view those anticipated automated changes.

Right now we are at step 0. If I remove my objections to proceeding beyond step 0 (e.g., as a result of discovering new relevant facts and/or flaws in the original review), then we will be close to the end of step 1.