Re: git hooks for reviews mandatory?
On Friday 20 June 2014 13:13:20 Aleix Pol wrote: On Fri, Jun 20, 2014 at 7:40 AM, Kevin Ottens er...@kde.org wrote: On Friday 20 June 2014 01:46:10 Aleix Pol wrote: On Thu, Jun 19, 2014 at 11:21 PM, Marco Martin notm...@gmail.com wrote: Hi all, I was thinking, since the policy for committing in frameworks is to always asking for a review, what about on repositories under frameworks/* adding an hook that accepts pushes only if the comment has a REVIEW: line? I have been guilty too many times of not respecting that, mostly for not thinking about it at all, maybe I'm not the only one, an artificial enforce of discipline like that *may* make sense. opinions? would be useful, or mostly just an annoyance? I've heard of many complaints about how noisy is kde-frameworks mailing list because of review requests. Well, the policy doesn't necessarily mean going through reviewboard. As pointed by Luigi Reviewed by: is also allowed, and that can be through a pastebin over IRC or pair programming or whatever you want to get something more synchronous and direct as a review. Reviewboard is really for the asynchronous case (no relevant people available) or I'm not sure what I'm doing case. The rest could be done through other means of reviews. Also, I fear there's people not working at full speed in their respective projects because of review requests. Yeah, let's get crap out at full speed. :-) I think I see what you mean. That said, I'd like to remind being careful with the full speed argument. Such change would scare me a bit, TBH. If we limit it to REVIEW: then it would concern me as well as it'd force too much of a workflow. If we support both REVIEW: and Reviewed by: then it is less of a concern IMO. Regards. -- Kévin Ottens, http://ervin.ipsquad.net KDAB - proud supporter of KDE, http://www.kdab.com ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel What about merging a branch? It would be good if only the merge commit required the review statement. Well, as discussed, we try to avoid branches altogether. If that's a local branch, you can always compensate that locally easily... Now if we assume one of those seldom published branches, the question is relevant. I wonder if that's easy to do with the git hook. Doesn't it just mean allowing commits without review tags in the branches other than master? Then the merge commit will be in master and so have the constraint. Looks like something doable to me. Regards. -- Kévin Ottens, http://ervin.ipsquad.net KDAB - proud supporter of KDE, http://www.kdab.com signature.asc Description: This is a digitally signed message part. ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: git hooks for reviews mandatory?
On Friday 20 June 2014 22:11:57 Albert Astals Cid wrote: El Divendres, 20 de juny de 2014, a les 01:46:10, Aleix Pol va escriure: I've heard of many complaints about how noisy is kde-frameworks mailing list because of review requests. Also, I fear there's people not working at full speed in their respective projects because of review requests. Such change would scare me a bit, TBH. +1 Sometimes you just know that a commit is right, making it slower because you need someone to review it is a no go for me (not that i'm doing much/any frameworks developement otoh). There's still the possibility to do like in Qt Reviewed-By: Trust-me. The point of forcing reviews is to make sure people asks themselves the question in the first place. Regards. -- Kévin Ottens, http://ervin.ipsquad.net KDAB - proud supporter of KDE, http://www.kdab.com signature.asc Description: This is a digitally signed message part. ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: git hooks for reviews mandatory?
On Thu, June 19, 2014 23:21:22 Marco Martin wrote: Hi all, I was thinking, since the policy for committing in frameworks is to always asking for a review, what about on repositories under frameworks/* adding an hook that accepts pushes only if the comment has a REVIEW: line? I have been guilty too many times of not respecting that, mostly for not thinking about it at all, maybe I'm not the only one, an artificial enforce of discipline like that *may* make sense. opinions? would be useful, or mostly just an annoyance? I think it would be mostly an annoyance, but if it were possible to override (REVIEW:IRC, REVIEW:TrustMe, etc.) in situations where a Reviewboard request is unneeded it could help with ensuring we don't accidentally commit something needing review. We'd also want to make sure to come up with pre-commit hooks for devs to use client side, or at least a git-commit template reminding to use an appropriate REVIEW keyword so that devs don't have to wait until they try to push to figure out their commit can't land as-is. Regards, - Michael Pyne ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: git hooks for reviews mandatory?
On Saturday 21 June 2014 11:22:28 Michael Pyne wrote: On Thu, June 19, 2014 23:21:22 Marco Martin wrote: Hi all, I was thinking, since the policy for committing in frameworks is to always asking for a review, what about on repositories under frameworks/* adding an hook that accepts pushes only if the comment has a REVIEW: line? I have been guilty too many times of not respecting that, mostly for not thinking about it at all, maybe I'm not the only one, an artificial enforce of discipline like that *may* make sense. opinions? would be useful, or mostly just an annoyance? I think it would be mostly an annoyance, but if it were possible to override (REVIEW:IRC, REVIEW:TrustMe, etc.) in situations where a Reviewboard request is unneeded it could help with ensuring we don't accidentally commit something needing review. Exactly why I think we need to support: * REVIEW: reviewboard id * Reviewed-By: free string The free string then allow to specify trust me or a name. I don't think there's much to do in a hook apart from checking at least one of the two is present. We'd also want to make sure to come up with pre-commit hooks for devs to use client side, or at least a git-commit template reminding to use an appropriate REVIEW keyword so that devs don't have to wait until they try to push to figure out their commit can't land as-is. Sounds harder than it sound, I think quite some people rely on the following workflow for contributing patches: * commit locally; * use a script to pick the commit and push it on reviewboard; * reword the commit log to add the newly allocated reviewboard id. Which means the initial commit can't have a REVIEW tag yet... chicken and egg problem AFAICT. Regards. -- Kévin Ottens, http://ervin.ipsquad.net KDAB - proud supporter of KDE, http://www.kdab.com signature.asc Description: This is a digitally signed message part. ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: git hooks for reviews mandatory?
On Sat, June 21, 2014 19:34:21 Kevin Ottens wrote: On Saturday 21 June 2014 11:22:28 Michael Pyne wrote: On Thu, June 19, 2014 23:21:22 Marco Martin wrote: Hi all, I was thinking, since the policy for committing in frameworks is to always asking for a review, what about on repositories under frameworks/* adding an hook that accepts pushes only if the comment has a REVIEW: line? I have been guilty too many times of not respecting that, mostly for not thinking about it at all, maybe I'm not the only one, an artificial enforce of discipline like that *may* make sense. opinions? would be useful, or mostly just an annoyance? I think it would be mostly an annoyance, but if it were possible to override (REVIEW:IRC, REVIEW:TrustMe, etc.) in situations where a Reviewboard request is unneeded it could help with ensuring we don't accidentally commit something needing review. Exactly why I think we need to support: * REVIEW: reviewboard id * Reviewed-By: free string The free string then allow to specify trust me or a name. I don't think there's much to do in a hook apart from checking at least one of the two is present. Yes. Actually I didn't realize there was an entire rest of thread to this when I sent my reply. KMail sometimes gives me two entries for POP3-filtered mail and I didn't see the rest of the thread until after I'd already read the second entry and sent my reply. We'd also want to make sure to come up with pre-commit hooks for devs to use client side, or at least a git-commit template reminding to use an appropriate REVIEW keyword so that devs don't have to wait until they try to push to figure out their commit can't land as-is. Sounds harder than it sound, I think quite some people rely on the following workflow for contributing patches: * commit locally; * use a script to pick the commit and push it on reviewboard; * reword the commit log to add the newly allocated reviewboard id. Which means the initial commit can't have a REVIEW tag yet... chicken and egg problem AFAICT. Good point, though a git-commit template message would still be useful here, a similar template was made by somebody (forget who) for KDE 4 which I've used for a couple of years now and found helpful. Regards, - Michael Pyne ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: git hooks for reviews mandatory?
On Thursday 19 June 2014 23:38:05 Luigi Toscano wrote: Marco Martin ha scritto: Hi all, I was thinking, since the policy for committing in frameworks is to always asking for a review, what about on repositories under frameworks/* adding an hook that accepts pushes only if the comment has a REVIEW: line? ... or Reviewed by: https://community.kde.org/Frameworks/Policies#Frameworks_commits_are_reviewe ah, yeah, of course -- Marco Martin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: git hooks for reviews mandatory?
On Fri, Jun 20, 2014 at 7:40 AM, Kevin Ottens er...@kde.org wrote: On Friday 20 June 2014 01:46:10 Aleix Pol wrote: On Thu, Jun 19, 2014 at 11:21 PM, Marco Martin notm...@gmail.com wrote: Hi all, I was thinking, since the policy for committing in frameworks is to always asking for a review, what about on repositories under frameworks/* adding an hook that accepts pushes only if the comment has a REVIEW: line? I have been guilty too many times of not respecting that, mostly for not thinking about it at all, maybe I'm not the only one, an artificial enforce of discipline like that *may* make sense. opinions? would be useful, or mostly just an annoyance? I've heard of many complaints about how noisy is kde-frameworks mailing list because of review requests. Well, the policy doesn't necessarily mean going through reviewboard. As pointed by Luigi Reviewed by: is also allowed, and that can be through a pastebin over IRC or pair programming or whatever you want to get something more synchronous and direct as a review. Reviewboard is really for the asynchronous case (no relevant people available) or I'm not sure what I'm doing case. The rest could be done through other means of reviews. Also, I fear there's people not working at full speed in their respective projects because of review requests. Yeah, let's get crap out at full speed. :-) I think I see what you mean. That said, I'd like to remind being careful with the full speed argument. Such change would scare me a bit, TBH. If we limit it to REVIEW: then it would concern me as well as it'd force too much of a workflow. If we support both REVIEW: and Reviewed by: then it is less of a concern IMO. Regards. -- Kévin Ottens, http://ervin.ipsquad.net KDAB - proud supporter of KDE, http://www.kdab.com ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel What about merging a branch? It would be good if only the merge commit required the review statement. Aleix ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: git hooks for reviews mandatory?
On 19/06/14 22:21, Marco Martin wrote: Hi all, I was thinking, since the policy for committing in frameworks is to always asking for a review, what about on repositories under frameworks/* adding an hook that accepts pushes only if the comment has a REVIEW: line? I have been guilty too many times of not respecting that, mostly for not thinking about it at all, maybe I'm not the only one, an artificial enforce of discipline like that *may* make sense. opinions? would be useful, or mostly just an annoyance? My main concern is the scripted changes that some of us occasionally do (especially David with the releasing). I don't think there's any point reviewing more than the first one or two such changes (and none at all for the release version updates). So how do we get those past such hooks? Alex ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: git hooks for reviews mandatory?
On Friday 20 June 2014 19:25:19 Alex Merry wrote: On 19/06/14 22:21, Marco Martin wrote: Hi all, I was thinking, since the policy for committing in frameworks is to always asking for a review, what about on repositories under frameworks/* adding an hook that accepts pushes only if the comment has a REVIEW: line? I have been guilty too many times of not respecting that, mostly for not thinking about it at all, maybe I'm not the only one, an artificial enforce of discipline like that *may* make sense. opinions? would be useful, or mostly just an annoyance? My main concern is the scripted changes that some of us occasionally do (especially David with the releasing). I don't think there's any point reviewing more than the first one or two such changes (and none at all for the release version updates). So how do we get those past such hooks? The old Qt way: Reviewed-by: Trust-me -- David Faure, fa...@kde.org, http://www.davidfaure.fr Working on KDE Frameworks 5 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: git hooks for reviews mandatory?
El Divendres, 20 de juny de 2014, a les 01:46:10, Aleix Pol va escriure: On Thu, Jun 19, 2014 at 11:21 PM, Marco Martin notm...@gmail.com wrote: Hi all, I was thinking, since the policy for committing in frameworks is to always asking for a review, what about on repositories under frameworks/* adding an hook that accepts pushes only if the comment has a REVIEW: line? I have been guilty too many times of not respecting that, mostly for not thinking about it at all, maybe I'm not the only one, an artificial enforce of discipline like that *may* make sense. opinions? would be useful, or mostly just an annoyance? I've heard of many complaints about how noisy is kde-frameworks mailing list because of review requests. Also, I fear there's people not working at full speed in their respective projects because of review requests. Such change would scare me a bit, TBH. +1 Sometimes you just know that a commit is right, making it slower because you need someone to review it is a no go for me (not that i'm doing much/any frameworks developement otoh). Cheers, Albert Aleix ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
git hooks for reviews mandatory?
Hi all, I was thinking, since the policy for committing in frameworks is to always asking for a review, what about on repositories under frameworks/* adding an hook that accepts pushes only if the comment has a REVIEW: line? I have been guilty too many times of not respecting that, mostly for not thinking about it at all, maybe I'm not the only one, an artificial enforce of discipline like that *may* make sense. opinions? would be useful, or mostly just an annoyance? -- Marco Martin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: git hooks for reviews mandatory?
Marco Martin ha scritto: Hi all, I was thinking, since the policy for committing in frameworks is to always asking for a review, what about on repositories under frameworks/* adding an hook that accepts pushes only if the comment has a REVIEW: line? ... or Reviewed by: https://community.kde.org/Frameworks/Policies#Frameworks_commits_are_reviewed Ciao -- Luigi ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: git hooks for reviews mandatory?
2014-06-19 18:21 GMT-03:00 Marco Martin notm...@gmail.com: Hi all, I was thinking, since the policy for committing in frameworks is to always asking for a review, what about on repositories under frameworks/* adding an hook that accepts pushes only if the comment has a REVIEW: line? I have been guilty too many times of not respecting that, mostly for not thinking about it at all, maybe I'm not the only one, an artificial enforce of discipline like that *may* make sense. opinions? would be useful, or mostly just an annoyance? With my sysadmin hat on: this is quite easy to implement in the hook code. -- Nicolás ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: git hooks for reviews mandatory?
On Friday 20 June 2014 01:46:10 Aleix Pol wrote: On Thu, Jun 19, 2014 at 11:21 PM, Marco Martin notm...@gmail.com wrote: Hi all, I was thinking, since the policy for committing in frameworks is to always asking for a review, what about on repositories under frameworks/* adding an hook that accepts pushes only if the comment has a REVIEW: line? I have been guilty too many times of not respecting that, mostly for not thinking about it at all, maybe I'm not the only one, an artificial enforce of discipline like that *may* make sense. opinions? would be useful, or mostly just an annoyance? I've heard of many complaints about how noisy is kde-frameworks mailing list because of review requests. Well, the policy doesn't necessarily mean going through reviewboard. As pointed by Luigi Reviewed by: is also allowed, and that can be through a pastebin over IRC or pair programming or whatever you want to get something more synchronous and direct as a review. Reviewboard is really for the asynchronous case (no relevant people available) or I'm not sure what I'm doing case. The rest could be done through other means of reviews. Also, I fear there's people not working at full speed in their respective projects because of review requests. Yeah, let's get crap out at full speed. :-) I think I see what you mean. That said, I'd like to remind being careful with the full speed argument. Such change would scare me a bit, TBH. If we limit it to REVIEW: then it would concern me as well as it'd force too much of a workflow. If we support both REVIEW: and Reviewed by: then it is less of a concern IMO. Regards. -- Kévin Ottens, http://ervin.ipsquad.net KDAB - proud supporter of KDE, http://www.kdab.com signature.asc Description: This is a digitally signed message part. ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
