Add support for post-dcommit hooks

Add support for post-dcommit/post-push hooks.

Patch contributed by sadrul@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71097

[M-A Ruel, 2010-12-22 20:28:21]

I wonder what others think about it. I wouldn't use it personally because it
doesn't fit my workflow.

http://codereview.chromium.org/5972005/diff/1/git_cl.py
File git_cl.py (right):

http://codereview.chromium.org/5972005/diff/1/git_cl.py#newcode222
git_cl.py:222: error_ok=True)
align either at ( or at +4 from previous line.

http://codereview.chromium.org/5972005/diff/1/git_cl.py#newcode981
git_cl.py:981: success = False
You could use instead;

retcode = 0

and check for retcode at line 1006.

[sadrul, 2010-12-22 20:39:49]

I like to keep the work-history around, so I do not delete branches after
committing. But the 'git branch' list gets way too huge relatively quickly. Some
way to filter the branches that have already been checked in helps me find the
list of tasks I am currently working on. I am hoping this would be useful for
other folks too.

http://codereview.chromium.org/5972005/diff/1/git_cl.py
File git_cl.py (right):

http://codereview.chromium.org/5972005/diff/1/git_cl.py#newcode222
git_cl.py:222: error_ok=True)
On 2010/12/22 20:28:21, Marc-Antoine Ruel wrote:
> align either at ( or at +4 from previous line.

Done.

http://codereview.chromium.org/5972005/diff/1/git_cl.py#newcode981
git_cl.py:981: success = False
On 2010/12/22 20:28:21, Marc-Antoine Ruel wrote:
> You could use instead;
> 
> retcode = 0
> 
> and check for retcode at line 1006.

I set retcode = 1 here so that the branch doesn't get renamed in case the
'merge' or 'commit' commands fail (is that possible?) in the try block before
either 'push' or 'dcommit' happens.

[Evan Martin, 2010-12-22 20:47:36]

FYI, conflict warning: I guess you might need to wait for Chase to move this to
SVN and then recreate your patch.

I also find the leftover branches a problem.  I'm not sure what to do about it,
though.  I feel renaming them mostly moves the mess around (there's no "git
branch -d COMMITTED_*" command, right?  or does that work?).

I wonder if a better approach would be:
1) leave the branch associated with the issue on commit
2) make "git cl status" check whether the issue has been closed or not
3) provide a "git cl cleanup" command or something that kills off closed
branches

Now that I've written that out, I still find it unsatisfactory...

Ok, here's another idea:
Always delete the branch on dcommit.  print out "if you still wanted this
branch, run 'git checkout -b thebranchname hash'" (where we fill in
thebranchname and HASH).

(I think the more obvious approach adding a y/n prompt will just cause yes/no
blindness.)

[Mandeep Singh Baines, 2010-12-22 20:54:30]

Can't this be done with a post-commit hook.

I'd prefer not to bloat git-cl.

sadrul@chromium.org (sadrul@chromium.org) wrote:
> I like to keep the work-history around, so I do not delete branches after
> committing. But the 'git branch' list gets way too huge relatively
> quickly. Some
> way to filter the branches that have already been checked in helps
> me find the
> list of tasks I am currently working on. I am hoping this would be
> useful for
> other folks too.
> 
> 
> http://codereview.chromium.org/5972005/diff/1/git_cl.py
> File git_cl.py (right):
> 
> http://codereview.chromium.org/5972005/diff/1/git_cl.py#newcode222
> git_cl.py:222: error_ok=True)
> On 2010/12/22 20:28:21, Marc-Antoine Ruel wrote:
> >align either at ( or at +4 from previous line.
> 
> Done.
> 
> http://codereview.chromium.org/5972005/diff/1/git_cl.py#newcode981
> git_cl.py:981: success = False
> On 2010/12/22 20:28:21, Marc-Antoine Ruel wrote:
> >You could use instead;
> 
> >retcode = 0
> 
> >and check for retcode at line 1006.
> 
> I set retcode = 1 here so that the branch doesn't get renamed in case
> the 'merge' or 'commit' commands fail (is that possible?) in the try
> block before either 'push' or 'dcommit' happens.
> 
> http://codereview.chromium.org/5972005/

[sadrul, 2010-12-22 21:27:57]

[snip]
> Always delete the branch on dcommit.  print out "if you still wanted this
> branch, run 'git checkout -b thebranchname hash'" (where we fill in
> thebranchname and HASH).

Uh, I would much rather the delete happened on a command-line switch! (I
personally don't want my branches deleted, ever)

> Can't this be done with a post-commit hook.

A post-commit hook would be most awesome! I believe it will be a hook for 'git
svn dcommit'? Any TFM I should R for that? :-)

[Evan Martin, 2010-12-22 21:36:21]

On 2010/12/22 21:27:57, sadrul wrote:
> A post-commit hook would be most awesome! I believe it will be a hook for 'git
> svn dcommit'? Any TFM I should R for that? :-)

We have hooks in git-cl.  If you can make this a post-dcommit hook, I think a
patch to git-cl to run post-dcommit hooks would be nice (we already have
pre-dcommit hooks).

[sadrul, 2010-12-22 21:54:20]

On 2010/12/22 21:36:21, Evan Martin wrote:
> On 2010/12/22 21:27:57, sadrul wrote:
> > A post-commit hook would be most awesome! I believe it will be a hook for
'git
> > svn dcommit'? Any TFM I should R for that? :-)
> 
> We have hooks in git-cl.  If you can make this a post-dcommit hook, I think a
> patch to git-cl to run post-dcommit hooks would be nice (we already have
> pre-dcommit hooks).

Done! PTAL!

[Evan Martin, 2010-12-22 22:14:56]

http://codereview.chromium.org/5972005/diff/12001/git_cl.py
File git_cl.py (right):

http://codereview.chromium.org/5972005/diff/12001/git_cl.py#newcode996
git_cl.py:996: if retcode == 0 and os.path.isfile(POSTDCOMMIT_HOOK):
It's common in the git world for hooks like these to exist but to not have +x
permissions (indicating they shouldn't ber run).  I guess the RunHook would fail
below in that case, hopefully silently, and on Windows we can't believe +x
anyway.

http://codereview.chromium.org/5972005/diff/12001/git_cl.py#newcode999
git_cl.py:999: if cl.GetIssue():
Should we not run all of the remainder of this code when retcode != 0?

[Evan Martin, 2010-12-22 22:15:11]

(PS please update the review description)

[Mandeep Singh Baines, 2010-12-22 22:15:56]

Code LGTM. But please update the issue (click 'Edit Issue') to reflect what the
code does now. Also, please add a test.

[sadrul, 2010-12-22 22:46:29]

Updated the issue description.

http://codereview.chromium.org/5972005/diff/12001/git_cl.py
File git_cl.py (right):

http://codereview.chromium.org/5972005/diff/12001/git_cl.py#newcode996
git_cl.py:996: if retcode == 0 and os.path.isfile(POSTDCOMMIT_HOOK):
On 2010/12/22 22:14:57, Evan Martin wrote:
> It's common in the git world for hooks like these to exist but to not have +x
> permissions (indicating they shouldn't ber run).  I guess the RunHook would
fail
> below in that case, hopefully silently, and on Windows we can't believe +x
> anyway.

Looks like if the pre-dcommit hook is -x, it's not handled properly, i.e. 'cl
presubmit' crashes with a child_exception. Do we want to handle the post-dcommit
hook differently?

http://codereview.chromium.org/5972005/diff/12001/git_cl.py#newcode999
git_cl.py:999: if cl.GetIssue():
On 2010/12/22 22:14:57, Evan Martin wrote:
> Should we not run all of the remainder of this code when retcode != 0?

I think so, because I have noticed an issue getting closed when dcommit failed
(once, maybe twice). But I am not familiar enough with this code to be sure.

[Evan Martin, 2010-12-22 23:05:44]

http://codereview.chromium.org/5972005/diff/12001/git_cl.py
File git_cl.py (right):

http://codereview.chromium.org/5972005/diff/12001/git_cl.py#newcode999
git_cl.py:999: if cl.GetIssue():
On 2010/12/22 22:46:29, sadrul wrote:
> On 2010/12/22 22:14:57, Evan Martin wrote:
> > Should we not run all of the remainder of this code when retcode != 0?
> 
> I think so, because I have noticed an issue getting closed when dcommit failed
> (once, maybe twice). But I am not familiar enough with this code to be sure.

Oh actually, since we don't pass error_ok=True above, maybe we'll never get here
on error.

So maybe you want to move your new code outside of the "finally" block and
remove the retcode test.

[sadrul, 2010-12-22 23:17:24]

Added a test. Please take a look.

http://codereview.chromium.org/5972005/diff/12001/git_cl.py
File git_cl.py (right):

http://codereview.chromium.org/5972005/diff/12001/git_cl.py#newcode999
git_cl.py:999: if cl.GetIssue():
On 2010/12/22 23:05:44, Evan Martin wrote:
> On 2010/12/22 22:46:29, sadrul wrote:
> > On 2010/12/22 22:14:57, Evan Martin wrote:
> > > Should we not run all of the remainder of this code when retcode != 0?
> > 
> > I think so, because I have noticed an issue getting closed when dcommit
failed
> > (once, maybe twice). But I am not familiar enough with this code to be sure.
> 
> Oh actually, since we don't pass error_ok=True above, maybe we'll never get
here
> on error.
> 
> So maybe you want to move your new code outside of the "finally" block and
> remove the retcode test.

I don't think I understand. By 'above', do you mean with RunGitWithCode?

[Evan Martin, 2010-12-22 23:22:53]

On 2010/12/22 23:17:24, sadrul wrote:
> > So maybe you want to move your new code outside of the "finally" block and
> > remove the retcode test.
> 
> I don't think I understand. By 'above', do you mean with RunGitWithCode?

Yeah.  When the dcommit fails, the RunGit throws an exception.  The "finally"
cleans up the branches from that exception but doesn't handle the exception, so
the function stops there.

Since you want your thing to only run on dcommit success, just moving the code
out of the finally block (to like L999) will be sufficient.

[sadrul, 2010-12-23 00:27:37]

On 2010/12/22 23:22:53, Evan Martin wrote:
> On 2010/12/22 23:17:24, sadrul wrote:
> > > So maybe you want to move your new code outside of the "finally" block and
> > > remove the retcode test.
> > 
> > I don't think I understand. By 'above', do you mean with RunGitWithCode?
> 
> Yeah.  When the dcommit fails, the RunGit throws an exception.  The "finally"
> cleans up the branches from that exception but doesn't handle the exception,
so
> the function stops there.
> 
> Since you want your thing to only run on dcommit success, just moving the code
> out of the finally block (to like L999) will be sufficient.

Done.

[Evan Martin, 2010-12-23 00:32:07]

http://codereview.chromium.org/5972005/diff/23001/git_cl.py
File git_cl.py (right):

http://codereview.chromium.org/5972005/diff/23001/git_cl.py#newcode997
git_cl.py:997: if retcode == 0 and os.path.isfile(POSTDCOMMIT_HOOK):
My point was that you don't need to check retcode anymore; you'll only get to
here if retcode == 0.

[sadrul, 2010-12-23 00:41:31]

http://codereview.chromium.org/5972005/diff/23001/git_cl.py
File git_cl.py (right):

http://codereview.chromium.org/5972005/diff/23001/git_cl.py#newcode997
git_cl.py:997: if retcode == 0 and os.path.isfile(POSTDCOMMIT_HOOK):
On 2010/12/23 00:32:07, Evan Martin wrote:
> My point was that you don't need to check retcode anymore; you'll only get to
> here if retcode == 0.

Ah. I wasn't sure if that would always be the case, since retcode is used later
in the code (line 1003).

Done.

[Evan Martin, 2010-12-23 00:55:39]

On 2010/12/23 00:41:31, sadrul wrote:
> Ah. I wasn't sure if that would always be the case, since retcode is used
later
> in the code (line 1003).

Oh, suck.  I see there's a code path where it is used.  Apparently
RunGitWithCode doesn't call through to RunCommand and, bleh.

I guess you should also consider whether this should happen after "push" or not,
and adjust accordingly :(

[sadrul, 2010-12-23 01:23:18]

On 2010/12/23 00:55:39, Evan Martin wrote:
> On 2010/12/23 00:41:31, sadrul wrote:
> > Ah. I wasn't sure if that would always be the case, since retcode is used
> later
> > in the code (line 1003).
> 
> Oh, suck.  I see there's a code path where it is used.  Apparently
> RunGitWithCode doesn't call through to RunCommand and, bleh.
> 
> I guess you should also consider whether this should happen after "push" or
not,
> and adjust accordingly :(

I made a change to allow separate post-hooks for both push and dcommit. Also,
use RunGitWithCode for both the commands, and check for retcode after finally.
Using this may still not be necessary, but I am sure at least it doesn't hurt
anything.

[M-A Ruel, 2010-12-23 01:31:22]

For the record, you will want to sync depot_tools and recreate the patch against
depot_tools/git_cl/git_cl.py.

[sadrul, 2010-12-23 05:17:59]

On 2010/12/23 01:31:22, Marc-Antoine Ruel wrote:
> For the record, you will want to sync depot_tools and recreate the patch
against
> depot_tools/git_cl/git_cl.py.

Please ignore patchsets 8 and 9. I was figuring out gcl. Patchset 10 contains
the change to run post-hooks for dcommit and/or push, and a test. Please take
another look.

[Evan Martin, 2010-12-23 19:45:14]

LGTM

[sadrul, 2011-01-10 19:10:37]

I moved the dcommit hook execution a bit lower, after the issue is closed.
Otherwise if the post-dcommit/push hook renames/removes the branch, then it gets
an error. Please take another look.

[M-A Ruel, 2011-01-10 19:22:08]

Please fix the change description.

http://codereview.chromium.org/5972005/diff/42001/git_cl/git_cl.py
File git_cl/git_cl.py (right):

http://codereview.chromium.org/5972005/diff/42001/git_cl/git_cl.py#newcode32
git_cl/git_cl.py:32: POSTUPSTREAM_HOOK_PATTERN = '.git/hooks/post-cl-%s'
I think you should remove POSTDCOMMIT_HOOK and POSTPUSH_HOO since they are not
used.

[sadrul, 2011-01-10 19:27:40]

On 2011/01/10 19:22:08, Marc-Antoine Ruel wrote:
> Please fix the change description.

Oops! Didn't realize it had changed. Fixed!

[sadrul, 2011-01-10 19:27:50]

http://codereview.chromium.org/5972005/diff/42001/git_cl/git_cl.py
File git_cl/git_cl.py (right):

http://codereview.chromium.org/5972005/diff/42001/git_cl/git_cl.py#newcode32
git_cl/git_cl.py:32: POSTUPSTREAM_HOOK_PATTERN = '.git/hooks/post-cl-%s'
On 2011/01/10 19:22:08, Marc-Antoine Ruel wrote:
> I think you should remove POSTDCOMMIT_HOOK and POSTPUSH_HOO since they are not
> used.

Done.

[Evan Martin, 2011-01-10 19:35:23]

LGTM, maybe wait for M-A too

[Mandeep Singh Baines, 2011-01-10 21:21:41]

http://codereview.chromium.org/5972005/diff/49001/git_cl/git_cl.py
File git_cl/git_cl.py (right):

http://codereview.chromium.org/5972005/diff/49001/git_cl/git_cl.py#newcode1016
git_cl/git_cl.py:1016: if os.path.isfile(hook):
Nit. This check is redundant. RunHook already checks for file existence. Also,
none of the other call sites are doing this test.

[sadrul, 2011-01-10 21:45:08]

http://codereview.chromium.org/5972005/diff/49001/git_cl/git_cl.py
File git_cl/git_cl.py (right):

http://codereview.chromium.org/5972005/diff/49001/git_cl/git_cl.py#newcode1016
git_cl/git_cl.py:1016: if os.path.isfile(hook):
On 2011/01/10 21:21:41, Mandeep Singh Baines wrote:
> Nit. This check is redundant. RunHook already checks for file existence. Also,
> none of the other call sites are doing this test.

Done.

[Mandeep Singh Baines, 2011-01-10 21:53:05]

sadrul@chromium.org (sadrul@chromium.org) wrote:
> 
> http://codereview.chromium.org/5972005/diff/49001/git_cl/git_cl.py
> File git_cl/git_cl.py (right):
> 
> http://codereview.chromium.org/5972005/diff/49001/git_cl/git_cl.py#newcode1016
> git_cl/git_cl.py:1016: if os.path.isfile(hook):
> On 2011/01/10 21:21:41, Mandeep Singh Baines wrote:
> >Nit. This check is redundant. RunHook already checks for file
> existence. Also,
> >none of the other call sites are doing this test.
> 
> Done.
> 

LGTM

> http://codereview.chromium.org/5972005/

[commit-bot: I haz the power, 2011-01-11 22:11:07]

Can't process patch: Can't apply svn property svn:executable at line 86