This is
- to fix https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=246336
- to always drop the user to a shell so he can investigate, test build, everything
- reshuffle svn vs. "All the merge work was done..." message
Details
Details
- Reviewers
- None
- Group Reviewers
portmgr - Commits
- rP537876: Tools/scripts/mfh: check out entire port directory even if touching only files/
Diff Detail
Diff Detail
- Repository
- rP FreeBSD ports repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
| Tools/scripts/mfh | ||
|---|---|---|
| 145 | Why this change? If someone wasn't going to test before committing, dropping them into a shell isn't going to change that. | |
Comment Actions
Adam, thanks for reviewing this.
It is mostly a convenience feature because I personally always read ydiff output (= colored svn diff) and do a make check-plist
We're all to exercise due diligence and check if, say, the merge was incomplete, so I wasn't thinking of "blind" MFHs, i. e. run the script and commit without further inspection.
"Blind" MFHs could be permitted in case
- that prior to the to-be-MFH'd commit, head/ and quarterly were in synch
- the MFH revision list is complete and brings head/ and quarterly back in synch.
(I wasn't going to have the script do a "make check-plist && make package && make test" or something, or poudriere testport, which some might find necessary as future upgrade.)
Here is a scenario that I've encountered on several occasions of a fix, in variations:
- r100 gets copied on day yyyy-{01|04|07|10}-01 to yyyyQn (i. e. quarterly branched)
- r200 217 242 on head/ get fixes not meaning to be ported
- r253 fixes an important bug on head/ that's worth having and would require a backport of r217, too.
If the svn merge per se doesn't throw errors, you may miss the requisite.
A practical example was mail/mailman, where a MFH of 2.1.31 or so might have changed semantics of the pkg-plist WRT @sample lines, so would violate the quarterly "no astonishment" "no breaking changes" policies as I percieve them and in my book isn't MFH worthy.
The cause involved a bit of bad luck on my part with a misunderstood change in 2.1.30 that got MFHd with approval onto 2020Q2 branch, that I reverted less than two weeks afterwards on head after having understood I was overdoing on the "play it safe part" counter to long-standing upstream recommendations and provisions -- and then we had to do the 2.1.31 and 2.1.33 security fixes in quick succession (2.1.32 was fixing up i18n issues overlooked in 2.1.31).
Anyways, I haven't heard back from ports-secteam@ in several days for a discussion or decision on a full 2.1.31 MFH although they normally promise to respond within 24 hours, but I guess through 2020Q2 I'll be patching mailman twice, security backport (if any further needed) onto branches/2020Q2/ and full updates onto head/...
| Tools/scripts/mfh | ||
|---|---|---|
| 145 | The quick answer however is I'd be good if we left the old ask "PROMPTTEXT" from old line 140 in. | |
Comment Actions
Could you use devel/arcanist, or at least generate a diff with full context like it does, with svn diff -x -U9999 or git diff -U9999.
| mfh | ||
|---|---|---|
| 121–126 ↗ | (On Diff #71701) | Could you replace this with a call to sed or something, it took me ages to understand what it does. |
| 141–145 ↗ | (On Diff #71701) | Please revert these changes, it is not possible to test anything with a partial checkout, you would need a complete tree for 1) the framework and 2) dependencies. |
| mfh | ||
|---|---|---|
| 141–145 ↗ | (On Diff #71701) |
Untrue. While not all checks are possible if there are external requisites, it is often sufficient, especially after my changes in the other hunk, to at least to a "make stage stage-qa check-plist DEVELOPER=42" as a quick smoke test. |
Comment Actions
- reverted the change that removed the first prompt "do you want to commit"
- comment the file name mangling code without changing the code (no forking for external utilities needed).
| Tools/scripts/mfh | ||
|---|---|---|
| 147 | While you are at it, you can remove the whole $(printf ...) here and use $filelist directly. | |
| 155 | Can you please put that line back two lines below where it was, as it is now, it gets printed before svn status and svn diff, and is likely to be lost quite a few screens above the question being asked. | |
| Tools/scripts/mfh | ||
|---|---|---|
| 147 | $filelist will usually have many duplicates that should not be handed down to SVN, so we need sort -u, and I wouldn't want any content of that variable get near an options parser if we went back to using echo. | |
| 155 | can do that. | |
| Tools/scripts/mfh | ||
|---|---|---|
| 147 | Mmmm, at this point filelist is alreayd deduplicated, the dedup is done on the previous line. | |
| Tools/scripts/mfh | ||
|---|---|---|
| 147 | Pardon. I was referring to l. 137 not l. 138. The latter can indeed be simplified. | |
Comment Actions
- more printf down two lines as requested, and refer user to the output above that line
- remove clean() function and final rm -rf ...; trap -0, it was duplicating what trap and err(), or trap and exit 0, can already do
- move default assignments up to the top and lump them together (EDITOR, svnserver) - the EDITOR default vi was redundant.
- add code comments
- swap dir=$(mktemp...) and the trap so that if dir was pre-set in the environment and the script fails early, it won't attempt to clean up something that isn't its own but a random bit found in $dir
- rename filelist to dirlist to match its purpose
Note that in new testing, there is one remaining corner case, and that is that the script will not detect early if the MFH was already merged earlier. It will come up with an empty merge but still ask the user if he wants to commit.