diff mbox series

[ovs-dev] committer-responsibilities: Add guidelines for fixing up mistakes.

Message ID 20190724170541.19548-1-blp@ovn.org
State New
Headers show
Series [ovs-dev] committer-responsibilities: Add guidelines for fixing up mistakes. | expand

Commit Message

Ben Pfaff July 24, 2019, 5:05 p.m. UTC
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!

Comments

Ilya Maximets July 24, 2019, 5:25 p.m. UTC | #1
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.)
>
Ben Pfaff July 25, 2019, 3:09 a.m. UTC | #2
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?
Ilya Maximets July 25, 2019, 8:21 a.m. UTC | #3
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.
Ben Pfaff July 25, 2019, 3:39 p.m. UTC | #4
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?
William Tu July 25, 2019, 6:50 p.m. UTC | #5
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
Ben Pfaff July 25, 2019, 8:25 p.m. UTC | #6
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
Ilya Maximets July 26, 2019, 1:12 p.m. UTC | #7
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 mbox series

Patch

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.)