Page MenuHomeFreeBSD

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

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



This is

Diff Detail

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

Event Timeline

adamw added inline comments.
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/...

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

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.

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.

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

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

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.

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.