Created attachment 251398 [details] v0 (completely untested, apply via `git am`) Really sorry that I'm posting this 4 days (!) after release, however it seems I'm anyway fastest.
Created attachment 251418 [details] v1 (apply via `git am`) Sort plist.
Created attachment 251419 [details] poudriere build log
Friendly reminder.
Maintainer timeout.
(In reply to Anton Saietskii from comment #1) Hi! Thanks a lot for that work. First of all, I'm really sorry, I've been away for family reasons and after that, my infrastructure was unusable for ports work for many weeks because of upgrade issues (which I think I solved now). Fixing the plist sorting is certainly a good thing to do, I never got around since adopting the port. But please note it should be sorted according to the *expanded* variables (so e.g. moving the .mo files was not correct). Do you want to fix that yourself? (If so, ideally make it two commits, to separate stylistic from functional stuff). Otherwise I'd take your commit without sorting and do the sorting again myself...
Created attachment 253114 [details] v2 (apply via `git am`) Apologies for the delay. Re-done plist sorting.
(In reply to Anton Saietskii from comment #6) Note here: looking at the ports git log, I don't see plist sorting as separate commits when something else being done _at the same time,_ it's (almost?) always 'while here, sort plist'. Which makes sense IMO--we shouldn't create more commits just to create commits, and the plist sorting (along with, for example 'pet portfmt' things) isn't significant enough to deserve it's own commit, it's no-op basically.
(In reply to Anton Saietskii from comment #7) > looking at the ports git log, I don't see plist sorting as separate commits > when something else being done _at the same time,_ it's (almost?) always > 'while here, sort plist'. Old habits are hard to change. The "while here, do home improvement" pattern made perfect sense before moving to git. In CVS and SVN, you didn't have an easy way to draft a series of commits; if you wanted to separate stuff, it was a lot of manual work while actually committing ... a single larger commit was the safer choice. > Which makes sense IMO--we shouldn't create more commits just to create commits, > and the plist sorting (along with, for example 'pet portfmt' things) isn't > significant enough to deserve it's own commit, it's no-op basically. Being a no-op is exactly the reason why it *should* be a separate commit. Mixed into functional changes, it just distracts from these when later reading the history. More commits on the other hand don't do any harm, you just skip over the uninteresting ones, while the diffs for the interesting ones are quickly understood.
(In reply to Felix Palmen from comment #8) Could you please point me to something in Porter's Handbook or Commiter's Guide to support the statement? Thanks in advance!
(In reply to Anton Saietskii from comment #9) If you expect every single detail to be documented in one of the "books", you'll be disappointed quite often. The porters' handbook arguably doesn't need it, unless we start *expecting* contributors to shape commits (for git-am). Maybe that could be a nice idea. The committers guide has the generic advice to split up changes. I guess it "goes without saying" not to mix up completely unrelated aspects in a single commit, but yes, it isn't explicitly stated. Neither is "while here, do xxx" of course, this has always been a hack. You could have a look at the commit history of the 'src' repository, there you'll find a LOT more examples of separate non-functional commits ... because the src team seems to have adopted a git workflow quickly and mixing in unrelated stuff is actually criticized in reviews. Anyways, I want to do the right thing here, so I'll split it up.
(In reply to Felix Palmen from comment #10) This may not work well for ports. For example, one creates 2 PRs -- a 'no-op plist sorting' one and 'update port' one. One of them got forgotten for some reason, while other being reviewed -- problem here. Ok, to avoid that, we put 2 patches to single PR -- which order they should be applied in if both touching same files, like plist? That's important because reversed order may lead to merge conflict (separate PRs struggle from this one as well). So, correct order should be indicated in file names or a comment, which doesn't seem to be a fool-proof way for me. I don't expect answer to above from you, this is just a couple of most obvious difficulties and rationale for this to be specifically documented, especially while most committers don't ask for separate commits as of now. > Anyways, I want to do the right thing here, so I'll split it up. If this would help to land update finally, nice.
Comment on attachment 253114 [details] v2 (apply via `git am`) (In reply to Anton Saietskii from comment #11) So far, we don't expect contributors to shape commits at all (although I personally like seeing a "git patch" to be applied with git am). Officially, it's the committer's job to split the stuff if necessary (among other things like simply writing the commit message, setting the correct author, etc). But then, if you WANT to do it, just concatenating git patches works. It confuses bugzilla's diff-view a bit ;) but git-am will happily apply the series of commits. Of course, if you like to contribute "complex" changes (typically more than a single commit), there's also phabricator, anyone can ask for an account there and if you show a reason (contribution work!), you'll get it. There you can both have multiple commits in a single review or have multiple reviews organized in a "stack". There's also "git-arc" helping create and manage the latter, see devel/freebsd-git-devtools and related docs: https://freebsdfoundation.org/wp-content/uploads/2021/11/FreeBSD-Code-Review-with-git-arc.pdf That all said, I already split your patch (easy enough with a git cherry-pick), I'll push it after a few more test builds, thanks again.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=4eaac6b3f67120ccd465272e22fccf200b96f70c commit 4eaac6b3f67120ccd465272e22fccf200b96f70c Author: Anton Saietskii <vsasjason@gmail.com> AuthorDate: 2024-06-11 14:36:48 +0000 Commit: Felix Palmen <zirias@FreeBSD.org> CommitDate: 2024-08-27 11:48:19 +0000 x11/xscreensaver: Update to 6.09 New upstream release. Changes: * New hacks, kallisti and highvoltage. * Formal-wear for headroom. PR: 279682 x11/xscreensaver/Makefile | 3 +-- x11/xscreensaver/distinfo | 6 +++--- x11/xscreensaver/pkg-plist | 8 ++++++++ 3 files changed, 12 insertions(+), 5 deletions(-)
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=0d3415eeba849378d1111176d67b394309adf012 commit 0d3415eeba849378d1111176d67b394309adf012 Author: Anton Saietskii <vsasjason@gmail.com> AuthorDate: 2024-06-11 14:36:21 +0000 Commit: Felix Palmen <zirias@FreeBSD.org> CommitDate: 2024-08-27 11:48:19 +0000 x11/xscreensaver: Fix pkg-plist sorting PR: 279682 x11/xscreensaver/pkg-plist | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-)
Committed, thanks! Had to add two new manpages as well, just as a hint, poudriere-testport spots these things.