On Wed, 11 Jun 2025 at 20:55, Stefan Hajnoczi <stefa...@gmail.com> wrote: > > On Wed, Jun 11, 2025 at 3:25 PM BALATON Zoltan <bala...@eik.bme.hu> wrote: > > > > On Wed, 11 Jun 2025, Stefan Hajnoczi wrote: > > > From: oltolm <oleg.tolmat...@gmail.com> > > > > > > Sorry, I forgot to cc the maintainers. > > > > Do we want comments like this end up in git log? This could have been > > fixed before a pull. Also the other pull request about uninitialised stack > > variables had hw/audio/gus twice which was pointed out by a comment before > > the pull that one of those should be different but the pull request still > > had this error. Did you miss these or aren't these important enough to fix > > before getting in git log forever or there is just no easy way to fix up > > commit messages in pull requests? > > If another reviewer asks for the author to resend then I'll hold off > on merging, but I didn't see the comment about hw/audio/gus. Sorry! > > I did see this "Sorry, I forgot to cc the maintainers" comment. > Although I'm not consistent, nowadays I generally do not fix these > issues when merging, provided it's a small issue that can be ignored > or understood from the context. > > I don't really mind either way, so if there is a consensus that all > maintainers should be strict about this, I'm happy to join.
Personally when I'm accumulating patches as a maintainer I do fix up this kind of commit message. It's generally from a first time contributor, so the commit needs closer attention anyway, and I favour just fixing "this isn't quite the way we usually do things" problems at my end, rather than asking the submitter to resend. I also fix nits from longer term contributors where it's faster to do that than to ask for a respin. In this specific case I'd probably re-cast the commit message entirely, because the first-person past-tense phrasing is not our standard commit log style. I would also want to add something clarifying exactly what we're fixing (clearly "the Windows build" generally was not broken, or we'd have seen it in CI, so this is fixing a more niche build environment setup, and it's worth having the commit message be clear about what that setup is). > One related point I do have a strong opinion on is that the > qemu.git/master maintainer shouldn't be expected to do fixups on a > pull request they receive. Fixups should be done by subsystem > maintainers (and the pull request must be resent) or the original > patch authors. It doesn't scale when the qemu.git/master maintainer > has to make changes to code that they are unfamiliar with. That's not > the case here, but I just wanted to mention it because from time to > time someone requests this. Fixing up a pullreq is also basically not possible -- the pullreq is signed and you can't change the commits in it. I don't recommend throwing random fixes (as opposed to genuine simple merge conflict fixes) into the merge commit. thanks -- PMM
