Re: [RFC/PATCH 0/3] Teach revert/cherry-pick the --no-verify option

2014-09-08 Thread Johan Herland
On Fri, Sep 5, 2014 at 11:05 PM, Fabian Ruch baf...@gmail.com wrote:
 neither git-cherry-pick nor git-revert execute the pre-commit or
 commit-msg hooks at the moment. The underlying rationale can be found in
 the log message of commit 9fa4db5 (Do not verify
 reverted/cherry-picked/rebased patches.). Indeed, the sequencer uses
 git-commit internally which executes the two verify hooks by default.
 However, the particular command line being used implicitly specifies the
 --no-verify option. This behaviour is implemented in
 sequencer.c#run_git_commit as well, right before the configurable
 git-commit options are handled. I guess that's easily overlooked since
 the documentation doesn't mention it and the implementation uses the
 short version -n of --no-verify.

Damn. You're obviously correct, and my patch series is seriously
misguided. Please drop it, Junio, and I'm very sorry for the noise.
Hopefully, I will learn not to blindly follow my assumptions.

...Johan


-- 
Johan Herland, jo...@herland.net
www.herland.net
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC/PATCH 0/3] Teach revert/cherry-pick the --no-verify option

2014-09-05 Thread Fabian Ruch
Hi Johan,

Johan Herland writes:
 A colleague of mine noticed that cherry-pick does not accept the
 --no-verify option to skip running the pre-commit/commit-msg hooks.

neither git-cherry-pick nor git-revert execute the pre-commit or
commit-msg hooks at the moment. The underlying rationale can be found in
the log message of commit 9fa4db5 (Do not verify
reverted/cherry-picked/rebased patches.). Indeed, the sequencer uses
git-commit internally which executes the two verify hooks by default.
However, the particular command line being used implicitly specifies the
--no-verify option. This behaviour is implemented in
sequencer.c#run_git_commit as well, right before the configurable
git-commit options are handled. I guess that's easily overlooked since
the documentation doesn't mention it and the implementation uses the
short version -n of --no-verify.

The reasons why the new test cases succeed nonetheless are manifold. I
hope they're still understandable even though I don't put the comments
next to the code.

The revert with failing hook test case fails if run in isolation,
which can be achieved by using the very cool --run option of test-lib.
More specifically, git-revert does not fail because it executes the
failing hook but because the preceding test case leaves behind an
uncommitted index.

In the cherry-pick with failing hook test case, git-cherry-pick really
fails because it doesn't know the --no-verify option yet, which
presumably ended up there only by accident. This test case is
meaningless if run in isolation because it assumes that revert with
failing hook creates a commit (else HEAD^ points nowhere).

I like your patchset for that it makes it explicit in both the
documentation and the tests whether the commits resulting from
cherry-picks are being verified or not.

Kind regards,
   Fabian

 Here's a first attempt at adding --no-verify to the revert/cherry-pick.
 
 Have fun! :)
 
 ...Johan
 
 Johan Herland (3):
   t7503/4: Add failing testcases for revert/cherry-pick --no-verify
   revert/cherry-pick: Add --no-verify option, and pass it on to commit
   revert/cherry-pick --no-verify: Update documentation
 
  Documentation/git-cherry-pick.txt |  4 
  Documentation/git-revert.txt  |  4 
  Documentation/githooks.txt| 20 ++--
  builtin/revert.c  |  1 +
  sequencer.c   |  7 +++
  sequencer.h   |  1 +
  t/t7503-pre-commit-hook.sh| 24 
  t/t7504-commit-msg-hook.sh| 24 
  8 files changed, 75 insertions(+), 10 deletions(-)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html