Cleanup HERE step 1

[yadij]

Step 1:
Add script to replace HERE symbol in a way that does
not break branches during the remainder of the transition.

The script is added first as a separate commit for large
branches to merge and run before attempting to merge onto a
cleaned master branch.

The Enforcement commit is provided for review purposes to show
the changes made by the script. It should be removed before
merge.

Step 2:
Master branch has a regular automated Enforcement commit.

Step 3:
Then full removal of the HERE symbol as a third PR followup to
drop it completely.

[rousskov]

I see no reason to commit this temporary (and somewhat dangerous) HERE-removal script. The script should be posted somewhere like a GitHub gist or even a squid-dev email, for long-term use by developers (which is obviously not limited to a few official branches!). FWIW, I said pretty much the same in #653 (review) with no opposition (that I could detect).

And without committing that script, we can:

• do everything in just one step/PR for v4 and v5 branches: those branches should keep HERE definition to avoid breaking patches; and
• two steps for master/v6: automated usage removal step followed by the manual HERE definition removal and minor cleanup like src/tests/stub_cache_manager.cc.

[yadij]

The difference between this and earlier attempts is that HERE is not going away as part of this PR. This is just automating a lot of the easy to remove removals, in the main branch and in dev branches that update and run source maintenance.

[rousskov]

Please note that your response does not seem to be related to my change request and is certainly not addressing my concern and not providing feedback regarding my suggestions.

---

The difference between this and earlier attempts is that HERE is not going away as part of this PR.

FWIW, some of the earlier attempts also keep HERE definition (e.g. #653 keeps it), and the difference you are describing disappears completely once one considers the whole sequence of HERE-removal steps/actions/PRs. However, we do not need to agree on this -- my change request is not about the existence of this difference at all!

[yadij]

Please note that your response does not seem to be related to my change request and is certainly not addressing my concern and not providing feedback regarding my suggestions.

Since you require statement-by-statement response to every point you bring up...

I see no reason to commit this temporary (and somewhat dangerous) HERE-removal script. The script should be posted somewhere like a GitHub gist or even a squid-dev email, for long-term use by developers (which is obviously not limited to a few official branches!).

Reasons:

A) Merging of this script is to enforce code conformance of developer branches PR when merging into master without adding new scripts to hunt down and run. We already request they run the source maintenance script as a pre-submission check. This merges makes that existing process check that their branch is suitably up to date past any automated HERE removal and that new code fits well with that removal. Such that they resolve any identifiable code conflicts before wasting reviewers time.

B) Your second sentence answers the point you made in the first. The best place to make a script available long-term is to add it to the repository. A github webpage, email, or anything detached from the code and maintenance process will get lost over time and forgotten.

C) Each time one of us has privately run a script and supplied the results for submission there has been long quibbling discussions about what needs to be done that has not already done by the private script. Merging like this allows the scripted changes to be done by squidadm in a commit that can be replicated and verified as impartial by anyone.

• any quibbling we have about what the script produces can take the form of concrete changes to the script rules.

FWIW, I said pretty much the same in #653 (review) with no opposition (that I could detect).

As with this PR review comment you stated the lack of understanding many times in many places. It was responded to elsewhere in other threads about that topic.

And without committing that script, we can:

* do everything in just one step/PR for v4 and v5 branches: those branches should keep HERE _definition_ to avoid breaking patches; and

Under the time-based release cycles you insisted on us doing changes to HERE do not qualify for backport.

* two steps for master/v6: automated usage removal step followed by the manual HERE definition removal and minor cleanup like src/tests/stub_cache_manager.cc.

As you can see from this PR description there are actually 3 steps. What you describe are steps following this PR which makes the script available for the automation to use. Without the script being added to the automation, it is no different from a manual submission which does the same changes a a single point of time.

The "reason" you mention not seeing is that developer branches do not universally and globally run this script. Our source-enforcement automation will be needing to run it at least a few times before it is generally no longer needed.

[rousskov]

Warning: @yadij has edited my comment instead of responding to it. Be careful when interpreting who said what here.

---

Alex: I see no reason to commit this temporary (and somewhat dangerous) HERE-removal script. The script should be posted somewhere like a GitHub gist or even a squid-dev email, for long-term use by developers (which is obviously not limited to a few official branches!).

That fact you have forgotten we already have such a script demonstrates how useful that approach is.

The first and second version of this script was published in squid-dev August 2015. An updated copy was published a few years later.

I gave up on others using it voluntarily. Thus PR #653 and now this PR with updated AWK version of the sed rules.

Amos: Reasons:

A) Merging of this script is to enforce code conformance of developer branches PR when merging into master without adding new scripts to hunt down and run.

Alex:
I disagree that this is a valid reason because the compiler will do that enforcement much better than a script. Since > master will have no HERE definition, the compiler will immediately detect any violations.

Your disagreement assumes that the removal of the HERE definition occurs at exactly the same time as the automation runs. Past reviews and discussion have already vetoed the plan of mixing manual changes and automated updates, as well as the plan of performing a flag-day change of the entire codebase and branches.

When there are two separate commits with many days of time between them we face the situation of developer branch still containing the old code and will compile fine. In the PR submission it still builds fine as well, we reviewers only see a "this branch is out of date" button and the code still containing the macro we want gone.

B) Your second sentence answers the point you made in the first. The best place to make a script available long-term is to add it to the repository. A github webpage, email, or anything detached from the code and maintenance process will get lost over time and forgotten.

Alex:
I disagree that this is a valid reason because the link to the script can be added to the HERE-removing commit message. With only a handful of people needing that script for long-term use (and even that use will ultimately end), committing a link is a much better solution than polluting the official repository and source-maintenance.sh process with a somewhat dangerous (and normally completely unnecessary!) script.

Been there tried that. Got vetoed - linking makes it impossible to update the script when we find ways to improve its activity. Because it would then no longer produce the same commit changes. The workaround for that is committing the scripts as source enforcement automation and having followup improvements as followup commits.

C) Each time one of us has privately run a script and supplied the results for submission there has been long quibbling discussions about what needs to be done that has not already done by the private script. Merging like this allows the scripted changes to be done by squidadm in a commit that can be replicated and verified as impartial by anyone.

Alex:
This is a valid reason not to commit the script IMO. Once HERE is gone, it stops being a factor. If my PR is
poorly written, I should not be able to blame some script for my mistakes. Keeping that script unofficial will help
avoid those (invalid) arguments.

^^ Strawman.

a) this script is about easing branch updates prior to, or during, review.
b) the point of source-maintenance scripting is to ease detection and/or fixes of poorly written code. Not to excuse it.

• any quibbling we have about what the script produces can take the form of concrete changes to the script rules.

Since master code will have no HERE, the script will not change anything in the official code. PRs should follow
the usual rules. There is no need to make any exceptions or special cases for HERE!

The prerequisite of HERE being gone is false.

Alex: two steps for master/v6: automated usage removal step followed by the manual HERE definition removal and minor cleanup like src/tests/stub_cache_manager.cc.

Amos: As you can see from this PR description there are actually 3 steps.

Alex:
I think there should be just two. The PR description should be adjusted to reflect a better 2-step plan, of course.

That suggested plan has been vetoed by yourself repeatedly ... citing the one-off extra work ("pain") for branch developers to merge changes in a step 1 flag-day update. The solution is this PR's 3-step process where the script is added / made available first and automation performs the flag-day changes as the second step.

FTR;
.. a) BZR patches submitting just automated changes were vetoed - because some desired outcome cannot be automated.
.. b) BZR patches submitting just manual changes were vetoed - because some desired outcome can be automated.
i.e. (a) got rejected because it did not include (b) changes, (b) rejected because it did not contain (a). This happened twice, once when done separately, and once when submitted as a pair to be applied immediately together.

.. c) BZR patches submitting both sets combined (without script) were vetoed - because reviewer cannot tell which are which by only reading the code.
.. d) submitting both sets with a copy of the script as code were vetoed - because the script is apparently unnecessary.

.. e) submitting both sets with a copy of the script as description were vetoed - because describing the script is apparently unnecessary.

.. f) script-only submissions vetoed - because both

1. they did not do everything (2015 script mk1 for dev use, then PR Cleanup HERE usage #653 merging script mk4), or
2. they did do everything (PR Cleanup HERE step 1 #881 with script mk5).

Amos:
Without the script being added to the automation, it is no different from a manual submission which does the same changes a a single point of time.

Alex:
Yes, the two steps we should do consist of two simple manual PRs. I see no problems with that, and your response does not seem to highlight any.

Patches performing a non-scripted update were vetoed long ago. It is no longer being considered as an option.

The "reason" you mention not seeing is that developer branches do not universally and globally run this script. Our source-enforcement automation will be needing to run it at least a few times before it is generally no longer needed.

I do not understand how the "developer branches do not run this script" fact is a reason for officially committing this script. Active developers will know what to do. Other developers working on their old branches will not care about this script until they are ready to post a PR. At that point, they will successfully merge HERE-free master into their branches (no script needed at all) or run the script (regardless of whether the script is officially merged or not).

The process for such a merge on large branches is a multi-step process. Rebase on the commit prior to automatic flag-day (where the script was added to master), run the resulting maintenance/enforcement scripts, rebase the cleaned code on latest master.

And we can even run it ourselves for their PRs, just like we sometimes run formater.pl for them now -- we do not have enough 3rd party submissions for that (temporary inconvenience) to become a serious problem worth discussing.

Do you? If/when needed I ask the submitter to run source-maintenance.sh, not run anything myself.

In summary, I still see no valid reasons for committing this script (and doing three steps). We will be better off without such commit and with just two steps, as detailed earlier.

---

This part of the discussion seems to divert from the primary subject, so I moved it to the bottom.

Alex: And without committing that script, we can:

• do everything in just one step/PR for v4 and v5 branches: those branches should keep HERE definition to avoid breaking patches; and

Amos: Under the time-based release cycles you insisted on us doing changes to HERE do not qualify for backport.

Alex:
Time-based releases do not prevent such backporting - the new rules are about scheduling, not development.

I mentioned before we switched that the backport criteria would be more strict. It now prohibits backport of non-bug changes such as these HERE changes.

I am sure that HERE usage should be removed from v5 (at least!) after HERE is removed from master. I cannot imagine any justification for creating so much backporting overhead, forcing developers to insert a macro (that expands to nothing) in numerous places just to backport some HERE-unrelated fix from master to v5!

This overhead pain already exists with all backports regardless of HERE being part of it. It is just a thing backporters deal with as the price of not upgrading.

[yadij]

Alex: I see no reason to commit this temporary (and somewhat dangerous) HERE-removal script. The script should be posted somewhere like a GitHub gist or even a squid-dev email, for long-term use by developers (which is obviously not limited to a few official branches!).

That fact you have forgotten we already have such a script demonstrates how useful that approach is.

The first and second version of this script was published in squid-dev August 2015. An updated copy was published a few years later.

I gave up on others using it voluntarily. Thus PR #653 and now this PR with updated AWK version of the sed rules.

Amos: Reasons:

A) Merging of this script is to enforce code conformance of developer branches PR when merging into master without adding new scripts to hunt down and run.

Alex:
I disagree that this is a valid reason because the compiler will do that enforcement much better than a script. Since > master will have no HERE definition, the compiler will immediately detect any violations.

Your disagreement assumes that the removal of the HERE definition occurs at exactly the same time as the automation runs. Past reviews and discussion have already vetoed the plan of mixing manual changes and automated updates, as well as the plan of performing a flag-day change of the entire codebase and branches.

When there are two separate commits with many days of time between them we face the situation of developer branch still containing the old code and will compile fine. In the PR submission it still builds fine as well, we reviewers only see a "this branch is out of date" button and the code still containing the macro we want gone.

B) Your second sentence answers the point you made in the first. The best place to make a script available long-term is to add it to the repository. A github webpage, email, or anything detached from the code and maintenance process will get lost over time and forgotten.

Alex:
I disagree that this is a valid reason because the link to the script can be added to the HERE-removing commit message. With only a handful of people needing that script for long-term use (and even that use will ultimately end), committing a link is a much better solution than polluting the official repository and source-maintenance.sh process with a somewhat dangerous (and normally completely unnecessary!) script.

Been there tried that. Got vetoed - linking makes it impossible to update the script when we find ways to improve its activity. Because it would then no longer produce the same commit changes. The workaround for that is committing the scripts as source enforcement automation and having followup improvements as followup commits.

C) Each time one of us has privately run a script and supplied the results for submission there has been long quibbling discussions about what needs to be done that has not already done by the private script. Merging like this allows the scripted changes to be done by squidadm in a commit that can be replicated and verified as impartial by anyone.

Alex:
This is a valid reason not to commit the script IMO. Once HERE is gone, it stops being a factor. If my PR is
poorly written, I should not be able to blame some script for my mistakes. Keeping that script unofficial will help
avoid those (invalid) arguments.

^^ Strawman.

a) this script is about easing branch updates prior to, or during, review.
b) the point of source-maintenance scripting is to ease detection and/or fixes of poorly written code. Not to excuse it.

• any quibbling we have about what the script produces can take the form of concrete changes to the script rules.

Since master code will have no HERE, the script will not change anything in the official code. PRs should follow
the usual rules. There is no need to make any exceptions or special cases for HERE!

The prerequisite of HERE being gone is false.

Alex: two steps for master/v6: automated usage removal step followed by the manual HERE definition removal and minor cleanup like src/tests/stub_cache_manager.cc.

Amos: As you can see from this PR description there are actually 3 steps.

Alex:
I think there should be just two. The PR description should be adjusted to reflect a better 2-step plan, of course.

That suggested plan has been vetoed by yourself repeatedly ... citing the one-off extra work ("pain") for branch developers to merge changes in a step 1 flag-day update. The solution is this PR's 3-step process where the script is added / made available first and automation performs the flag-day changes as the second step.

FTR;
.. a) BZR patches submitting just automated changes were vetoed - because some desired outcome cannot be automated.
.. b) BZR patches submitting just manual changes were vetoed - because some desired outcome can be automated.
i.e. (a) got rejected because it did not include (b) changes, (b) rejected because it did not contain (a). This happened twice, once when done separately, and once when submitted as a pair to be applied immediately together.

.. c) BZR patches submitting both sets combined (without script) were vetoed - because reviewer cannot tell which are which by only reading the code.
.. d) submitting both sets with a copy of the script as code were vetoed - because the script is apparently unnecessary.

.. e) submitting both sets with a copy of the script as description were vetoed - because describing the script is apparently unnecessary.

.. f) script-only submissions vetoed - because both

1. they did not do everything (2015 script mk1 for dev use, then PR Cleanup HERE usage #653 merging script mk4), or
2. they did do everything (PR Cleanup HERE step 1 #881 with script mk5).

Amos:
Without the script being added to the automation, it is no different from a manual submission which does the same changes a a single point of time.

Alex:
Yes, the two steps we should do consist of two simple manual PRs. I see no problems with that, and your response does not seem to highlight any.

Patches performing a non-scripted update were vetoed long ago. It is no longer being considered as an option.

The "reason" you mention not seeing is that developer branches do not universally and globally run this script. Our source-enforcement automation will be needing to run it at least a few times before it is generally no longer needed.

I do not understand how the "developer branches do not run this script" fact is a reason for officially committing this script. Active developers will know what to do. Other developers working on their old branches will not care about this script until they are ready to post a PR. At that point, they will successfully merge HERE-free master into their branches (no script needed at all) or run the script (regardless of whether the script is officially merged or not).

The process for such a merge on large branches is a multi-step process. Rebase on the commit prior to automatic flag-day (where the script was added to master), run the resulting maintenance/enforcement scripts, rebase the cleaned code on latest master.

And we can even run it ourselves for their PRs, just like we sometimes run formater.pl for them now -- we do not have enough 3rd party submissions for that (temporary inconvenience) to become a serious problem worth discussing.

Do you? If/when needed I ask the submitter to run source-maintenance.sh, not run anything myself.

In summary, I still see no valid reasons for committing this script (and doing three steps). We will be better off without such commit and with just two steps, as detailed earlier.

---

This part of the discussion seems to divert from the primary subject, so I moved it to the bottom.

Alex: And without committing that script, we can:

• do everything in just one step/PR for v4 and v5 branches: those branches should keep HERE definition to avoid breaking patches; and

Amos: Under the time-based release cycles you insisted on us doing changes to HERE do not qualify for backport.

Alex:
Time-based releases do not prevent such backporting - the new rules are about scheduling, not development.

I mentioned before we switched that the backport criteria would be more strict. It now prohibits backport of non-bug changes such as these HERE changes.

I am sure that HERE usage should be removed from v5 (at least!) after HERE is removed from master. I cannot imagine any justification for creating so much backporting overhead, forcing developers to insert a macro (that expands to nothing) in numerous places just to backport some HERE-unrelated fix from master to v5!

This overhead pain already exists with all backports regardless of HERE being part of it. It is just a thing backporters deal with as the price of not upgrading.

[rousskov]

Alex: I see no reason to commit this temporary (and somewhat dangerous) HERE-removal script. The script should be posted somewhere like a GitHub gist or even a squid-dev email, for long-term use by developers (which is obviously not limited to a few official branches!).

That fact you have forgotten we already have such a script demonstrates how useful that approach is.

That is not a "fact"; that is a misrepresentation of reality. And even if I had forgotten about existing HERE-removal scripts, it would not demonstrate the script usefulness in master -- the script is harmful before HERE is removed from master and useless (at best) for master work after HERE is removed from master. The script is only useful for bringing unofficial branches in sync with the official post-HERE code (and only where basic merging is not a good option).

The first and second version of this script was published in squid-dev August 2015. An updated copy was published a few years later.

I gave up on others using it voluntarily.

IIRC, those scripts are pretty much useless until HERE is officially removed. I am surprised you expected somebody to use them before that removal.

Amos: Reasons:
A) Merging of this script is to enforce code conformance of developer branches PR when merging into master without adding new scripts to hunt down and run.

Alex:
I disagree that this is a valid reason because the compiler will do that enforcement much better than a script. Since master will have no HERE definition, the compiler will immediately detect any violations.

Your disagreement assumes that the removal of the HERE definition occurs at exactly the same time as the automation runs.

No, there is no such assumption.

Past reviews and discussion have already vetoed the plan of mixing manual changes and automated updates, as well as the plan of performing a flag-day change of the entire codebase and branches.

AFAIK, the plans I have proposed have not been vetoed.

When there are two separate commits with many days of time between them we face the situation of developer branch still containing the old code and will compile fine. In the PR submission it still builds fine as well, we reviewers only see a "this branch is out of date" button and the code still containing the macro we want gone.

Yes, and that "situation" is not a problem (worth solving) because there is no harm to master here. If needed, we can delay merging of PRs that re-introduce master for a few hours (or days) that it takes to merge a HERE-definition removing PR (and that PR can be reviewed/approved in advance!). Or we can apply the same HERE-removal script to those few PRs. You might have a point if we had dozens of urgent PRs getting merged every day, but that is not the reality we are actually dealing with.

Moreover, committing the HERE-removal script does not automatically avoid this "situation" either. This "situation" may exist even if the script is officially committed. We can ask PR authors to apply (or ourselves apply) the script whether it is committed or not. Thus, this whole part of the discussion is a red herring as far as script commit is concerned.

B) Your second sentence answers the point you made in the first. The best place to make a script available long-term is to add it to the repository. A github webpage, email, or anything detached from the code and maintenance process will get lost over time and forgotten.

Alex:
I disagree that this is a valid reason because the link to the script can be added to the HERE-removing commit message. With only a handful of people needing that script for long-term use (and even that use will ultimately end), committing a link is a much better solution than polluting the official repository and source-maintenance.sh process with a somewhat dangerous (and normally completely unnecessary!) script.

Been there tried that. Got vetoed

I do not think including a link to the HERE-removal script in the official commit has been vetoed.

• linking makes it impossible to update the script

Not true. GitHub gists can be versioned/updated (without changing the published URL) if needed.

Moreover, the script primary value is in correctly handling the code it was officially applied to. We do not have to update it once it was applied. For any official git branch we decide to modify, the script is applied exactly once, and the link to that exact applied script can be easily included in the corresponding commit message.

when we find ways to improve its activity. Because it would then no longer produce the same commit changes. The workaround for that is committing the scripts as source enforcement automation and having followup improvements as followup commits.

If we follow my plan, there will be no followup official commits because the first official PR will remove all HERE uses that are worth automatically removing. End of story.

C) Each time one of us has privately run a script and supplied the results for submission there has been long quibbling discussions about what needs to be done that has not already done by the private script. Merging like this allows the scripted changes to be done by squidadm in a commit that can be replicated and verified as impartial by anyone.

Alex:
This is a valid reason not to commit the script IMO. Once HERE is gone, it stops being a factor. If my PR is
poorly written, I should not be able to blame some script for my mistakes. Keeping that script unofficial will help
avoid those (invalid) arguments.

^^ Strawman.

a) this script is about easing branch updates prior to, or during, review.

The script does not need to be officially committed to do that.

b) the point of source-maintenance scripting is to ease detection and/or fixes of poorly written code. Not to excuse it.

"detection" is a red herring -- the compiler will do all the detection here.

A few PRs that will need formatting without merging from master can be formatted without committing the script. It is a temporary situation. Officially committing an always-running code-modifying script to address a temporary problem is not a good idea. The "point" of source-maintenance scripting is code generation and formatting, not code fixing.

• any quibbling we have about what the script produces can take the form of concrete changes to the script rules.

Since master code will have no HERE, the script will not change anything in the official code. PRs should follow
the usual rules. There is no need to make any exceptions or special cases for HERE!

The prerequisite of HERE being gone is false.

That prerequisite is true: There will be no HERE definitions. No need to deny that reality just because that reality will happen a few hours (or even a few days) after HERE uses are removed. We fully control whether that will happen or not. I hope we are not going to create problems just to solve it later.

Alex: two steps for master/v6: automated usage removal step followed by the manual HERE definition removal and minor cleanup like src/tests/stub_cache_manager.cc.

Amos: As you can see from this PR description there are actually 3 steps.

Alex:
I think there should be just two. The PR description should be adjusted to reflect a better 2-step plan, of course.

That suggested plan has been vetoed by yourself repeatedly

It was not, but even if it was, it should not matter today since I am (obviously) not vetoing my own plan today.

FTR; ...

That is not what actually happened.

And we can even run it ourselves for their PRs, just like we sometimes run formater.pl for them now -- we do not have enough 3rd party submissions for that (temporary inconvenience) to become a serious problem worth discussing.

Do you?

Yes, I do.

---

Amos: Under the time-based release cycles ... changes to HERE do not qualify for backport.

Alex: Time-based releases do not prevent such backporting - the new rules are about scheduling, not development.

I mentioned before we switched that the backport criteria would be more strict. It now prohibits backport of non-bug changes such as these HERE changes.

You can invent whatever backporting criteria you want, but not all the criteria can be (reasonably) justified by time-based releases. Not applying HERE removal to at least v5 will significantly and needlessly increase backporting overheads. I see no justification for that, not in time-based release schedule, not elsewhere. To me, keeping HERE in v5 after removing it from master sounds like an arbitrary decision that creates serious problems (and solves virtually none).

I am sure that HERE usage should be removed from v5 (at least!) after HERE is removed from master. I cannot imagine any justification for creating so much backporting overhead, forcing developers to insert a macro (that expands to nothing) in numerous places just to backport some HERE-unrelated fix from master to v5!

This overhead pain already exists with all backports regardless of HERE being part of it. It is just a thing backporters deal with as the price of not upgrading.

The backporting overhead exists, of course, but there is no justification for increasing it so much!