This year's changes to the "Submitting Patches" page
Hello everyone, I've made (and Alexandre has approved) changes to our commit message recommendations <https://wiki.winehq.org/Submitting_Patches> to reflect this year's WineConf discussions. The major differences from the previous information on the wiki are: - Explicitly request use of the imperative mood - Add more instructions on how to write paragraphs of explanation - Recommend inclusion of a Wine-Bug line with a URL to Bugzilla (similar to Gentoo's Gentoo-Bug) - Clarify that Signed-off-by means that you think a patch is good enough to go into Wine (and not that you are obligated fix any regressions it may cause) - Clarify that Signed-off-by also signifies compliance with legal requirements for developing Wine code (although we don't have a "Developer Certificate of Origin" yet) - Tell patch submitters to put extra information (not intended for the commit log) below three dashes Please let me know if you notice any more problems with the page, although it is on a wiki, so you are welcome to improve it yourself ;-) -Alex
On Sun, Jul 01, 2018 at 05:01:13PM +0200, Alex Henrie wrote:
- Clarify that Signed-off-by means that you think a patch is good enough to go into Wine (and not that you are obligated fix any regressions it may cause)
The only part I dislike is this:
You can also use git config format.signOff true to enable this setting by default on every new commit.
Since we are allowed to send sign-offs on others' behalf (a practice I dislike, but it is what it is), I don't think we should make it easy for people to accidentally include sign-offs on patches that aren't intended to go upstream. Otherwise looks great, thanks. Andrew
On 9 July 2018 at 17:08, Andrew Eikum <aeikum(a)codeweavers.com> wrote:
On Sun, Jul 01, 2018 at 05:01:13PM +0200, Alex Henrie wrote: The only part I dislike is this:
You can also use git config format.signOff true to enable this setting by default on every new commit.
Since we are allowed to send sign-offs on others' behalf (a practice I dislike, but it is what it is), I don't think we should make it easy
That doesn't sound right. The way I understand things, we obviously preserve existing sign-offs on a patch unless we make substantial changes to it, but we shouldn't be adding sign-offs that weren't already there. That's not to say I disagree with your point though; I'd prefer signing off on a patch to be an explicit action as well.
On Mon, Jul 9, 2018 at 5:18 PM Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 9 July 2018 at 17:08, Andrew Eikum <aeikum(a)codeweavers.com> wrote:
On Sun, Jul 01, 2018 at 05:01:13PM +0200, Alex Henrie wrote: The only part I dislike is this:
You can also use git config format.signOff true to enable this setting by default on every new commit.
Since we are allowed to send sign-offs on others' behalf (a practice I dislike, but it is what it is), I don't think we should make it easy
That doesn't sound right. The way I understand things, we obviously preserve existing sign-offs on a patch unless we make substantial changes to it, but we shouldn't be adding sign-offs that weren't already there.
That's not to say I disagree with your point though; I'd prefer signing off on a patch to be an explicit action as well.
In addition to the other changes, I changed the page to say not to add Signed-off-by for anyone but yourself, because it didn't make sense to me to say on someone else's behalf that a patch is good enough for Wine. I do agree that we can preserve the Signed-off-by lines of patches in Staging if no substantial changes are made to the patch when it is sent upstream. I think the general consensus among Wine developers is that using `git config format.signOff true` is not recommended. Feel free to delete that sentence from the wiki. If anyone really wants to use the option, they can find it in Git's documentation. -Alex
On 01/07/18 09:01, Alex Henrie wrote:
- Clarify that Signed-off-by means that you think a patch is good enough to go into Wine (and not that you are obligated fix any regressions it may cause)
I know that this is a terrible time to bring this up, since Alexandre is on vacation, but I was just thinking about this and I have a concern I'd like to know how to address. I am quite clearly not the best Wine developer on the block, and, being aware of this, I'm not sure I necessarily feel comfortable saying I am *confident* that many of my patches—as I initially send them—are good enough to go into Wine. I guess the system is sort of designed this way—Alexandre, and the other reviewers, determine whether a patch is good enough, so it ultimately kind of doesn't matter whether anyone else does. But it's been stated explicitly—even at this last Wineconf—that the "standards" of the submitter have direct bearing on their Julliard Rank, and it's obviously in anyone's interest (especially us less proficient contributors) to keep a high rank. And there are patches I send where I not only can't guarantee I haven't made any accidental mistakes but am also generally unsure that I've taken the right approach. This is a concern to me since in my experience sending the patch as a RFC, or even trying to ask what the right approach is, results in a response significantly less often than I'd like. Not that I'm trying to accuse Alexandre or anyone else of being unfairly unresponsive, but my point is that a patch with my sign-off is more likely to get a review than one without, and of course in the case where my approach does seem correct it can't be committed without my sign-off. So, what should I do about this? Am I interpreting the meaning of a sign-off too restrictively? Or is it just a matter of living with the consequences of being a mediocre developer? Which is understandable if that's the case; it's just unfortunate. ἔρρωσθε, Zeb
Funny you should bring it up : I'm currently working on a patch to https://bugs.winehq.org/show_bug.cgi?id=43270 . It's still in a incredibly early stage (but already makes the affected application work, which is nice) at which I wouldn't even consider making it an official submission, but I'd still love to hear comments about it (and have some questions myself anyway), especially seeing how I haven't ever done stuff in Wine before. Is sending "non-official" (i.e. without the [PATCH] prefix in the topic) patches for review to the mailing list acceptable? I don't think I've seen it mentioned in the wiki, since it only talks about sending "official" [PATCH]es. Kind regards, Daniel On 25 July 2018 at 01:33, Zebediah Figura <z.figura12(a)gmail.com> wrote:
On 01/07/18 09:01, Alex Henrie wrote:
- Clarify that Signed-off-by means that you think a patch is good enough to go into Wine (and not that you are obligated fix any regressions it may cause)
I know that this is a terrible time to bring this up, since Alexandre is on vacation, but I was just thinking about this and I have a concern I'd like to know how to address. I am quite clearly not the best Wine developer on the block, and, being aware of this, I'm not sure I necessarily feel comfortable saying I am *confident* that many of my patches—as I initially send them—are good enough to go into Wine. I guess the system is sort of designed this way—Alexandre, and the other reviewers, determine whether a patch is good enough, so it ultimately kind of doesn't matter whether anyone else does. But it's been stated explicitly—even at this last Wineconf—that the "standards" of the submitter have direct bearing on their Julliard Rank, and it's obviously in anyone's interest (especially us less proficient contributors) to keep a high rank. And there are patches I send where I not only can't guarantee I haven't made any accidental mistakes but am also generally unsure that I've taken the right approach. This is a concern to me since in my experience sending the patch as a RFC, or even trying to ask what the right approach is, results in a response significantly less often than I'd like. Not that I'm trying to accuse Alexandre or anyone else of being unfairly unresponsive, but my point is that a patch with my sign-off is more likely to get a review than one without, and of course in the case where my approach does seem correct it can't be committed without my sign-off.
So, what should I do about this? Am I interpreting the meaning of a sign-off too restrictively? Or is it just a matter of living with the consequences of being a mediocre developer? Which is understandable if that's the case; it's just unfortunate.
ἔρρωσθε, Zeb
On Wed, Jul 25, 2018 at 02:13:38AM +0200, Daniel Kamil Kozar wrote:
Funny you should bring it up : I'm currently working on a patch to https://bugs.winehq.org/show_bug.cgi?id=43270 . It's still in a incredibly early stage (but already makes the affected application work, which is nice) at which I wouldn't even consider making it an official submission, but I'd still love to hear comments about it (and have some questions myself anyway), especially seeing how I haven't ever done stuff in Wine before. Is sending "non-official" (i.e. without the [PATCH] prefix in the topic) patches for review to the mailing list acceptable? I don't think I've seen it mentioned in the wiki, since it only talks about sending "official" [PATCH]es.
It's not forbidden, but generally that discussion is kept to bugzilla or personal mails, I think. If you'd like some attention on a bug, you could send a mail to wine-devel referencing it, email a relevant person directly, or CC them on the bug. If you do send a RFC patch to the ML, put "PATCH RFC" in the subject and don't include a sign-off. Andrew
On 25 July 2018 at 01:33, Zebediah Figura <z.figura12(a)gmail.com> wrote:
On 01/07/18 09:01, Alex Henrie wrote:
- Clarify that Signed-off-by means that you think a patch is good enough to go into Wine (and not that you are obligated fix any regressions it may cause)
I know that this is a terrible time to bring this up, since Alexandre is on vacation, but I was just thinking about this and I have a concern I'd like to know how to address. I am quite clearly not the best Wine developer on the block, and, being aware of this, I'm not sure I necessarily feel comfortable saying I am *confident* that many of my patches—as I initially send them—are good enough to go into Wine. I guess the system is sort of designed this way—Alexandre, and the other reviewers, determine whether a patch is good enough, so it ultimately kind of doesn't matter whether anyone else does. But it's been stated explicitly—even at this last Wineconf—that the "standards" of the submitter have direct bearing on their Julliard Rank, and it's obviously in anyone's interest (especially us less proficient contributors) to keep a high rank. And there are patches I send where I not only can't guarantee I haven't made any accidental mistakes but am also generally unsure that I've taken the right approach. This is a concern to me since in my experience sending the patch as a RFC, or even trying to ask what the right approach is, results in a response significantly less often than I'd like. Not that I'm trying to accuse Alexandre or anyone else of being unfairly unresponsive, but my point is that a patch with my sign-off is more likely to get a review than one without, and of course in the case where my approach does seem correct it can't be committed without my sign-off.
So, what should I do about this? Am I interpreting the meaning of a sign-off too restrictively? Or is it just a matter of living with the consequences of being a mediocre developer? Which is understandable if that's the case; it's just unfortunate.
ἔρρωσθε, Zeb
My take is that code can be "good enough" to go in, but also not be what the maintainer wants. You're not responsible for predicting that, nor will you generally have the knowledge and experience they have. However, being able to take feedback and adjust your approach is highly valued here. I know I've made my share of dumb mistakes that ended up in submitted patches, and occasionally in Wine. You're never going to be 100% certain, just do your best. I would like to suggest that maybe we as a community should stop talking about "Julliard rank". While I'm sure he considers your history when reviewing patches, it sounds like the way that's discussed creates undue anxiety. And it's overemphasized IMO, the quality of your work is what ultimately matters. (I ALSO would like to continue to suggest that we put in systems to make sure everyone's patches get reviewed.)
On 24/07/18 20:49, Vincent Povirk wrote:
My take is that code can be "good enough" to go in, but also not be what the maintainer wants. You're not responsible for predicting that, nor will you generally have the knowledge and experience they have. However, being able to take feedback and adjust your approach is highly valued here.
I guess I have a hard time seeing where the difference falls between the two. Besides, it seemed that Alexandre wanted to make it clear that a sign-off means a patch is good enough in every respect—legal, stylistic, correctness.
I know I've made my share of dumb mistakes that ended up in submitted patches, and occasionally in Wine. You're never going to be 100% certain, just do your best.
I would like to suggest that maybe we as a community should stop talking about "Julliard rank". While I'm sure he considers your history when reviewing patches, it sounds like the way that's discussed creates undue anxiety. And it's overemphasized IMO, the quality of your work is what ultimately matters. (I ALSO would like to continue to suggest that we put in systems to make sure everyone's patches get reviewed.)
I guess "Julliard Rank" is just a dumb code word for "Alexandre's level of trust", and it's observable enough that a name had to be invented for it. And how much a maintainer trusts you affects how much patience they're willing to give to reviewing your patches (or answering your questions, I guess). Which is something that I can't help but feel concerned about. But I guess that concern just boils down to "I submit poor-quality patches and I'm concerned about receiving deferential treatement", which is not really something that can or should be helped.
participants (6)
-
Alex Henrie -
Andrew Eikum -
Daniel Kamil Kozar -
Henri Verbeet -
Vincent Povirk -
Zebediah Figura