Page MenuHomeFreeBSD

mfh: check out full port directory even if merging only files/ changes
ClosedPublic

Authored by mandree on May 9 2020, 6:13 PM.

Details

Summary

This is

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

adamw added inline comments.
Tools/scripts/mfh
145 ↗(On Diff #71598)

Why this change?

If someone wasn't going to test before committing, dropping them into a shell isn't going to change that.

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

  1. that prior to the to-be-MFH'd commit, head/ and quarterly were in synch
  2. 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 ↗(On Diff #71598)

The quick answer however is I'd be good if we left the old ask "PROMPTTEXT" from old line 140 in.

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.

same code, rediffed with "full" context

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
121–126 ↗(On Diff #71701)

No way. The loop fully runs inside the shell and fast and I will not add external utility calls that are not needed.
However, I can add a comment such as # strip down f to first two path components - or suggest something better.

141–145 ↗(On Diff #71701)

will do.

mfh
141–145 ↗(On Diff #71701)

it is not possible to test anything with a partial checkout,

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.

  • 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).
mandree marked an inline comment as done.
Tools/scripts/mfh
138 ↗(On Diff #71760)

While you are at it, you can remove the whole $(printf ...) here and use $filelist directly.

146 ↗(On Diff #71760)

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
138 ↗(On Diff #71760)

$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.
$filelist It is actually a misnomer, because it contains only a highly-redundant list of directories.

146 ↗(On Diff #71760)

can do that.

(Note that printf(1) is also a built-in utility to sh(1).)

Tools/scripts/mfh
138 ↗(On Diff #71760)

Mmmm, at this point filelist is alreayd deduplicated, the dedup is done on the previous line.
You do not need to use echo or anything.
At this point, all the entries in that list are of the form xxxxQy/some/thing, and you do not need to use printf to pass them to svn, it can be done directly.

Tools/scripts/mfh
138 ↗(On Diff #71760)

Pardon. I was referring to l. 137 not l. 138. The latter can indeed be simplified.

  • 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.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 4 2020, 9:20 AM
This revision was automatically updated to reflect the committed changes.