| Message ID | 20190724170541.19548-1-blp@ovn.org |
|---|---|
| State | Deferred |
| Headers | show |
| Series | [ovs-dev] committer-responsibilities: Add guidelines for fixing up mistakes. | expand |
On 24.07.2019 20:05, Ben Pfaff wrote: > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > Based on an off-lst discussion with Ilya. This is just my first, off-hand > thought on the topic. Feedback welcome! We could also consider suggesting a git hook like that: --- $ cat .git/hooks/pre-push #!/bin/bash remote=$1 protected_remote='upstream' if [ $protected_remote != $remote ]; then exit 0; fi echo "You're about to push to $protected_remote." read -p "Do you want to proceed? [y|n] " reply < /dev/tty if echo $reply | grep -E '^[Yy]$' > /dev/null; then exit 0; fi exit 1 --- > > diff --git a/Documentation/internals/committer-responsibilities.rst b/Documentation/internals/committer-responsibilities.rst > index 4d10c3980875..b34dddaa99fd 100644 > --- a/Documentation/internals/committer-responsibilities.rst > +++ b/Documentation/internals/committer-responsibilities.rst > @@ -94,3 +94,9 @@ Use Reported-by: and Tested-by: tags in commit messages to indicate the > source of a bug report. > > Keep the ``AUTHORS.rst`` file up to date. > + > +If you mistakenly push a commit that should not have been applied, it's OK to > +use a Git "force-push" to remove it from the tree, as long as you do it within > +a few minutes and no one has pushed other commits to the tree in the meantime. > +Otherwise, it's necessary to revert the commit. (If you do this frequently, > +more than once or twice a year, then maybe you should be more careful.) >
On Wed, Jul 24, 2019 at 08:25:54PM +0300, Ilya Maximets wrote: > On 24.07.2019 20:05, Ben Pfaff wrote: > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > --- > > Based on an off-lst discussion with Ilya. This is just my first, off-hand > > thought on the topic. Feedback welcome! > > We could also consider suggesting a git hook like that: > --- > $ cat .git/hooks/pre-push > #!/bin/bash > > remote=$1 > protected_remote='upstream' > > if [ $protected_remote != $remote ]; then exit 0; fi > > echo "You're about to push to $protected_remote." > > read -p "Do you want to proceed? [y|n] " reply < /dev/tty > if echo $reply | grep -E '^[Yy]$' > /dev/null; then exit 0; fi > > exit 1 > --- Have you tried this? How does it work for you in practice?
On 25.07.2019 6:09, Ben Pfaff wrote: > On Wed, Jul 24, 2019 at 08:25:54PM +0300, Ilya Maximets wrote: >> On 24.07.2019 20:05, Ben Pfaff wrote: >>> Signed-off-by: Ben Pfaff <blp@ovn.org> >>> --- >>> Based on an off-lst discussion with Ilya. This is just my first, off-hand >>> thought on the topic. Feedback welcome! >> >> We could also consider suggesting a git hook like that: >> --- >> $ cat .git/hooks/pre-push >> #!/bin/bash >> >> remote=$1 >> protected_remote='upstream' >> >> if [ $protected_remote != $remote ]; then exit 0; fi >> >> echo "You're about to push to $protected_remote." >> >> read -p "Do you want to proceed? [y|n] " reply < /dev/tty >> if echo $reply | grep -E '^[Yy]$' > /dev/null; then exit 0; fi >> >> exit 1 >> --- > > Have you tried this? How does it work for you in practice? Yes. It works for me. 'exit 1' from the script fails the hook and disallows further push processing. I have a remote named "upstream": $ git remote -v <...> upstream https://github.com/openvswitch/ovs.git (fetch) upstream https://github.com/openvswitch/ovs.git (push) <...> Push to that remote: $ git push upstream HEAD:master --dry-run Username for 'https://github.com': username Password for 'https://username@github.com': You're about to push to upstream. Do you want to proceed? [y|n] y To https://github.com/openvswitch/ovs.git d560bc1ba..959dce495 HEAD -> master $ git push upstream HEAD:master --dry-run Username for 'https://github.com': username Password for 'https://username@github.com': You're about to push to upstream. Do you want to proceed? [y|n] n error: failed to push some refs to 'https://github.com/openvswitch/ovs.git' Push to any other remote: $ git push origin HEAD:master --dry-run To ssh://gerrit:29418/ovs c34a87b6c..959dce495 HEAD -> master Few notes why this hook might not work: * Need to replace: s/upstream/<name of the github.com/openvswitch/ovs.git remote>/ * The hook file should be executable. * I guess, there might be issues accessing /dev/tty, but It's unclear what to do in this case. I also added some colours locally to make the warning more visible: echo -e "You're about to push to \e[91m$protected_remote\e[39m." Best regards, Ilya Maximets.
On Thu, Jul 25, 2019 at 11:21:25AM +0300, Ilya Maximets wrote: > On 25.07.2019 6:09, Ben Pfaff wrote: > > On Wed, Jul 24, 2019 at 08:25:54PM +0300, Ilya Maximets wrote: > >> On 24.07.2019 20:05, Ben Pfaff wrote: > >>> Signed-off-by: Ben Pfaff <blp@ovn.org> > >>> --- > >>> Based on an off-lst discussion with Ilya. This is just my first, off-hand > >>> thought on the topic. Feedback welcome! > >> > >> We could also consider suggesting a git hook like that: > >> --- > >> $ cat .git/hooks/pre-push > >> #!/bin/bash > >> > >> remote=$1 > >> protected_remote='upstream' > >> > >> if [ $protected_remote != $remote ]; then exit 0; fi > >> > >> echo "You're about to push to $protected_remote." > >> > >> read -p "Do you want to proceed? [y|n] " reply < /dev/tty > >> if echo $reply | grep -E '^[Yy]$' > /dev/null; then exit 0; fi > >> > >> exit 1 > >> --- > > > > Have you tried this? How does it work for you in practice? > > Yes. It works for me. > 'exit 1' from the script fails the hook and disallows further push processing. OK. Do you want to submit a patch?
On Wed, Jul 24, 2019 at 08:09:25PM -0700, Ben Pfaff wrote: > On Wed, Jul 24, 2019 at 08:25:54PM +0300, Ilya Maximets wrote: > > On 24.07.2019 20:05, Ben Pfaff wrote: > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > > --- > > > Based on an off-lst discussion with Ilya. This is just my first, off-hand > > > thought on the topic. Feedback welcome! > > > > We could also consider suggesting a git hook like that: > > --- > > $ cat .git/hooks/pre-push > > #!/bin/bash > > > > remote=$1 > > protected_remote='upstream' > > > > if [ $protected_remote != $remote ]; then exit 0; fi > > > > echo "You're about to push to $protected_remote." > > > > read -p "Do you want to proceed? [y|n] " reply < /dev/tty > > if echo $reply | grep -E '^[Yy]$' > /dev/null; then exit 0; fi > > > > exit 1 > > --- > > Have you tried this? How does it work for you in practice? Thanks, I tried this and works for me. William > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Thu, Jul 25, 2019 at 08:39:21AM -0700, Ben Pfaff wrote: > On Thu, Jul 25, 2019 at 11:21:25AM +0300, Ilya Maximets wrote: > > On 25.07.2019 6:09, Ben Pfaff wrote: > > > On Wed, Jul 24, 2019 at 08:25:54PM +0300, Ilya Maximets wrote: > > >> On 24.07.2019 20:05, Ben Pfaff wrote: > > >>> Signed-off-by: Ben Pfaff <blp@ovn.org> > > >>> --- > > >>> Based on an off-lst discussion with Ilya. This is just my first, off-hand > > >>> thought on the topic. Feedback welcome! > > >> > > >> We could also consider suggesting a git hook like that: > > >> --- > > >> $ cat .git/hooks/pre-push > > >> #!/bin/bash > > >> > > >> remote=$1 > > >> protected_remote='upstream' > > >> > > >> if [ $protected_remote != $remote ]; then exit 0; fi > > >> > > >> echo "You're about to push to $protected_remote." > > >> > > >> read -p "Do you want to proceed? [y|n] " reply < /dev/tty > > >> if echo $reply | grep -E '^[Yy]$' > /dev/null; then exit 0; fi > > >> > > >> exit 1 > > >> --- > > > > > > Have you tried this? How does it work for you in practice? > > > > Yes. It works for me. > > 'exit 1' from the script fails the hook and disallows further push processing. > > OK. Do you want to submit a patch? I've now installed something similar locally. I wanted to make sure that my script for automatic crossporting didn't prompt me, so I modified it to be selective about the branch name: #!/bin/bash remote=$1 protected_remote='ovs' if [ $protected_remote != $remote ]; then exit 0; fi prompt=no while read local_ref local_sha1 remote_ref remote_sha1; do case $remote_ref in refs/heads/master) prompt=yes ;; esac done if test $prompt = no; then exit 0; fi echo "You're about to push to a protected branch on $protected_remote." read -p "Do you want to proceed? [y|n] " reply < /dev/tty if echo $reply | grep -E '^[Yy]$' > /dev/null; then exit 0; fi exit 1
On 25.07.2019 23:25, Ben Pfaff wrote: > On Thu, Jul 25, 2019 at 08:39:21AM -0700, Ben Pfaff wrote: >> On Thu, Jul 25, 2019 at 11:21:25AM +0300, Ilya Maximets wrote: >>> On 25.07.2019 6:09, Ben Pfaff wrote: >>>> On Wed, Jul 24, 2019 at 08:25:54PM +0300, Ilya Maximets wrote: >>>>> On 24.07.2019 20:05, Ben Pfaff wrote: >>>>>> Signed-off-by: Ben Pfaff <blp@ovn.org> >>>>>> --- >>>>>> Based on an off-lst discussion with Ilya. This is just my first, off-hand >>>>>> thought on the topic. Feedback welcome! >>>>> >>>>> We could also consider suggesting a git hook like that: >>>>> --- >>>>> $ cat .git/hooks/pre-push >>>>> #!/bin/bash >>>>> >>>>> remote=$1 >>>>> protected_remote='upstream' >>>>> >>>>> if [ $protected_remote != $remote ]; then exit 0; fi >>>>> >>>>> echo "You're about to push to $protected_remote." >>>>> >>>>> read -p "Do you want to proceed? [y|n] " reply < /dev/tty >>>>> if echo $reply | grep -E '^[Yy]$' > /dev/null; then exit 0; fi >>>>> >>>>> exit 1 >>>>> --- >>>> >>>> Have you tried this? How does it work for you in practice? >>> >>> Yes. It works for me. >>> 'exit 1' from the script fails the hook and disallows further push processing. >> >> OK. Do you want to submit a patch? I'm not sure how to include this into repository and it's probably better to get this along with the docs pointing to it. So, feel free to use/modify in your patch. > > I've now installed something similar locally. I wanted to make sure > that my script for automatic crossporting didn't prompt me, so I > modified it to be selective about the branch name: > > #!/bin/bash > > remote=$1 > protected_remote='ovs' > > if [ $protected_remote != $remote ]; then exit 0; fi > > prompt=no > while read local_ref local_sha1 remote_ref remote_sha1; do > case $remote_ref in > refs/heads/master) prompt=yes ;; > esac > done > if test $prompt = no; then exit 0; fi > > echo "You're about to push to a protected branch on $protected_remote." > > read -p "Do you want to proceed? [y|n] " reply < /dev/tty > if echo $reply | grep -E '^[Yy]$' > /dev/null; then exit 0; fi > > exit 1 Looks good. But since I don't have any fully automated scripts for applying patches I'd rather forbid all branches for myself.
diff --git a/Documentation/internals/committer-responsibilities.rst b/Documentation/internals/committer-responsibilities.rst index 4d10c3980875..b34dddaa99fd 100644 --- a/Documentation/internals/committer-responsibilities.rst +++ b/Documentation/internals/committer-responsibilities.rst @@ -94,3 +94,9 @@ Use Reported-by: and Tested-by: tags in commit messages to indicate the source of a bug report. Keep the ``AUTHORS.rst`` file up to date. + +If you mistakenly push a commit that should not have been applied, it's OK to +use a Git "force-push" to remove it from the tree, as long as you do it within +a few minutes and no one has pushed other commits to the tree in the meantime. +Otherwise, it's necessary to revert the commit. (If you do this frequently, +more than once or twice a year, then maybe you should be more careful.)
Signed-off-by: Ben Pfaff <blp@ovn.org> --- Based on an off-lst discussion with Ilya. This is just my first, off-hand thought on the topic. Feedback welcome!