| Message ID | YXpZddOixrJDd//s@pflmari (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove negation from the commit and merge option "--no-verify" | expand |
Hi Alex On 28/10/2021 09:04, Alex Riesen wrote: > From: Alex Riesen <raa.lkml@gmail.com> > > This allows re-enabling of the hooks disabled by an earlier "--no-verify" > in command-line and makes the interface more consistent. Thanks for working on this. Since 0f1930c587 ("parse-options: allow positivation of options starting, with no-", 2012-02-25) merge and commit have accepted "--verify" but it is undocumented. The documentation updates and fix to pull in this patch are very welcome, but I'm not sure we need the other changes. I've left a couple of comments below. [As an aside we should probably improve the documentation in parse-options.h if both Peff and Junio did not know how it handles "--no-foo" but that is outside the scope of this patch] > Incidentally, this also fixes unexpected calling of the hooks by "git > pull" when "--no-verify" was specified, where it was incorrectly > translated to "--no-verify-signatures". This caused the unexpected > effect of the hooks being called. And an even more unexpected effect of > disabling verification of signatures. Ouch! > Signed-off-by: Alexander Riesen <raa.lkml@gmail.com> >[...] > diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt > index a3baea32ae..ba66209274 100644 > --- a/Documentation/git-commit.txt > +++ b/Documentation/git-commit.txt > @@ -11,7 +11,7 @@ SYNOPSIS > 'git commit' [-a | --interactive | --patch] [-s] [-v] [-u<mode>] [--amend] > [--dry-run] [(-c | -C | --fixup | --squash) <commit>] > [-F <file> | -m <msg>] [--reset-author] [--allow-empty] > - [--allow-empty-message] [--no-verify] [-e] [--author=<author>] > + [--allow-empty-message] [--[no-]verify] [-e] [--author=<author>] I think for the synopsis it is fine just to list the most common options. Having --no-verify without the [no-] makes it clear that --verify is the default so is not a commonly used option. > [--date=<date>] [--cleanup=<mode>] [--[no-]status] > [-i | -o] [--pathspec-from-file=<file> [--pathspec-file-nul]] > [-S[<keyid>]] [--] [<pathspec>...] > @@ -174,7 +174,13 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`. > > -n:: > --no-verify:: > - This option bypasses the pre-commit and commit-msg hooks. > + By default, pre-merge and commit-msg hooks are run. When one of these I think saying "the pre-merge and commit-msg hooks" would be clearer as you do below. > + options is given, these are bypassed. > + See also linkgit:githooks[5]. > + > +--verify:: > + This option re-enables running of the pre-commit and commit-msg hooks > + after an earlier `-n` or `--no-verify`. > See also linkgit:githooks[5]. Some of the existing documentation describes the "--no-foo" option with "--foo" (e.g --[no-]signoff) but in other places we list the two options separately (e.g. --[no-]edit), I'd lean towards combining them as you have done for the merge documentation but I don't feel strongly about it. > --allow-empty:: > diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt > index 3819fadac1..324ae879d2 100644 > --- a/Documentation/git-merge.txt > +++ b/Documentation/git-merge.txt > @@ -10,7 +10,7 @@ SYNOPSIS > -------- > [verse] > 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit] > - [--no-verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]] > + [--[no-]verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]] Again I'm not sure changing the synopsis makes things clearer. > [--[no-]allow-unrelated-histories] > [--[no-]rerere-autoupdate] [-m <msg>] [-F <file>] [<commit>...] > 'git merge' (--continue | --abort | --quit) > diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt > index 80d4831662..f8016b0f7b 100644 > --- a/Documentation/merge-options.txt > +++ b/Documentation/merge-options.txt > @@ -112,8 +112,9 @@ option can be used to override --squash. > + > With --squash, --commit is not allowed, and will fail. > > ---no-verify:: > - This option bypasses the pre-merge and commit-msg hooks. > +--[no-]verify:: > + By default, pre-merge and commit-msg hooks are run. When `--no-verify` I think "the pre-merge ..." would be better here as well. > + is given, these are bypassed. > See also linkgit:githooks[5]. >[...] > diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh > index db1a381cd9..7d3a8ae0d3 100755 > --- a/t/t5521-pull-options.sh > +++ b/t/t5521-pull-options.sh > @@ -225,4 +225,16 @@ test_expect_success 'git pull --no-signoff flag cancels --signoff flag' ' > test_must_be_empty actual > ' > > +test_expect_success 'git pull --no-verify flag passed to merge' ' > + test_when_finished "rm -fr src dst actual" && > + git init src && > + test_commit -C src one && > + git clone src dst && > + write_script dst/.git/hooks/commit-msg <<-\EOF && > + false > + EOF > + test_commit -C src two && > + git -C dst pull --no-ff --no-verify > +' Thanks for adding a test > test_done > diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh > index 31b9c6a2c1..166ff5fb26 100755 > --- a/t/t7504-commit-msg-hook.sh > +++ b/t/t7504-commit-msg-hook.sh > @@ -130,6 +130,14 @@ test_expect_success '--no-verify with failing hook' ' > > ' > > +test_expect_success '-n with failing hook' ' > + > + echo "more" >> file && > + git add file && > + git commit -n -m "more" > + > +' Is this to check that "-n" works like "--no-verify"? I think it would be very useful to add another test that checks "--verify" overrides "--no-verify". Best Wishes Phillip
From: Alex Riesen <raa.lkml@gmail.com> This documents re-enabling of the hooks disabled by an earlier "--no-verify" in command-line. Signed-off-by: Alexander Riesen <raa.lkml@gmail.com> --- Hi Phillip, Phillip Wood, Thu, Oct 28, 2021 15:57:58 +0200: > On 28/10/2021 09:04, Alex Riesen wrote: > > From: Alex Riesen <raa.lkml@gmail.com> > > > > This allows re-enabling of the hooks disabled by an earlier "--no-verify" > > in command-line and makes the interface more consistent. > > Thanks for working on this. Since 0f1930c587 ("parse-options: allow > positivation of options starting, with no-", 2012-02-25) merge and commit > have accepted "--verify" but it is undocumented. The documentation updates > and fix to pull in this patch are very welcome, but I'm not sure we need the > other changes. I've left a couple of comments below. > > [As an aside we should probably improve the documentation in parse-options.h > if both Peff and Junio did not know how it handles "--no-foo" but that is > outside the scope of this patch] Interesting feature. It is unfortunate it was so well hidden. You're right, of course, and the newly added tests in t7504-commit-msg-hook.sh pass without any changes to the "builtin/commit.c". Removal of double-negation in the code was an improvement to its readability, but I like small patches more. Also, the series has no conflicts with 2.33.0 anymore and the "git pull" can be applied independently. > > diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt > > index a3baea32ae..ba66209274 100644 > > --- a/Documentation/git-commit.txt > > +++ b/Documentation/git-commit.txt > > @@ -11,7 +11,7 @@ SYNOPSIS > > 'git commit' [-a | --interactive | --patch] [-s] [-v] [-u<mode>] [--amend] > > [--dry-run] [(-c | -C | --fixup | --squash) <commit>] > > [-F <file> | -m <msg>] [--reset-author] [--allow-empty] > > - [--allow-empty-message] [--no-verify] [-e] [--author=<author>] > > + [--allow-empty-message] [--[no-]verify] [-e] [--author=<author>] > > I think for the synopsis it is fine just to list the most common options. > Having --no-verify without the [no-] makes it clear that --verify is the > default so is not a commonly used option. Yep, makes sense. > > [--date=<date>] [--cleanup=<mode>] [--[no-]status] > > [-i | -o] [--pathspec-from-file=<file> [--pathspec-file-nul]] > > [-S[<keyid>]] [--] [<pathspec>...] > > @@ -174,7 +174,13 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`. > > -n:: > > --no-verify:: > > - This option bypasses the pre-commit and commit-msg hooks. > > + By default, pre-merge and commit-msg hooks are run. When one of these > > I think saying "the pre-merge and commit-msg hooks" would be clearer as you > do below. > > > + options is given, these are bypassed. > > + See also linkgit:githooks[5]. > > + > > +--verify:: > > + This option re-enables running of the pre-commit and commit-msg hooks > > + after an earlier `-n` or `--no-verify`. > > See also linkgit:githooks[5]. > > Some of the existing documentation describes the "--no-foo" option with > "--foo" (e.g --[no-]signoff) but in other places we list the two options > separately (e.g. --[no-]edit), I'd lean towards combining them as you have > done for the merge documentation but I don't feel strongly about it. How about this instead: -n:: --no-verify:: By default, pre-commit and commit-msg hooks are run. When one of these options is given, the hooks will be bypassed. See also linkgit:githooks[5]. --verify:: This option re-enables running of the pre-commit and commit-msg hooks after an earlier `-n` or `--no-verify`. > > diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt > > index 3819fadac1..324ae879d2 100644 > > --- a/Documentation/git-merge.txt > > +++ b/Documentation/git-merge.txt > > @@ -10,7 +10,7 @@ SYNOPSIS > > -------- > > [verse] > > 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit] > > - [--no-verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]] > > + [--[no-]verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]] > > Again I'm not sure changing the synopsis makes things clearer. Removed. > > [--[no-]allow-unrelated-histories] > > [--[no-]rerere-autoupdate] [-m <msg>] [-F <file>] [<commit>...] > > 'git merge' (--continue | --abort | --quit) > > diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt > > index 80d4831662..f8016b0f7b 100644 > > --- a/Documentation/merge-options.txt > > +++ b/Documentation/merge-options.txt > > @@ -112,8 +112,9 @@ option can be used to override --squash. > > + > > With --squash, --commit is not allowed, and will fail. > > ---no-verify:: > > - This option bypasses the pre-merge and commit-msg hooks. > > +--[no-]verify:: > > + By default, pre-merge and commit-msg hooks are run. When `--no-verify` > > I think "the pre-merge ..." would be better here as well. Like this? --[no-]verify:: By default, the pre-merge and commit-msg hooks are run. When `--no-verify` is given, these are bypassed. See also linkgit:githooks[5]. > > diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh > > index 31b9c6a2c1..166ff5fb26 100755 > > --- a/t/t7504-commit-msg-hook.sh > > +++ b/t/t7504-commit-msg-hook.sh > > @@ -130,6 +130,14 @@ test_expect_success '--no-verify with failing hook' ' > > ' > > +test_expect_success '-n with failing hook' ' > > + > > + echo "more" >> file && > > + git add file && > > + git commit -n -m "more" > > + > > +' > > Is this to check that "-n" works like "--no-verify"? Frankly, it was to check that the separate "-n" option works as I supposed it would. I never used parse-options before. > I think it would be very useful to add another test that checks "--verify" > overrides "--no-verify". Replaced the test with one which has "-n --verify". Thanks! Documentation/git-commit.txt | 7 ++++++- Documentation/merge-options.txt | 5 +++-- t/t7504-commit-msg-hook.sh | 8 ++++++++ 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt index a3baea32ae..2268787483 100644 --- a/Documentation/git-commit.txt +++ b/Documentation/git-commit.txt @@ -174,9 +174,14 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`. -n:: --no-verify:: - This option bypasses the pre-commit and commit-msg hooks. + By default, pre-commit and commit-msg hooks are run. When one of these + options is given, the hooks will be bypassed. See also linkgit:githooks[5]. +--verify:: + This option re-enables running of the pre-commit and commit-msg hooks + after an earlier `-n` or `--no-verify`. + --allow-empty:: Usually recording a commit that has the exact same tree as its sole parent commit is a mistake, and the command prevents you diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt index 80d4831662..80267008af 100644 --- a/Documentation/merge-options.txt +++ b/Documentation/merge-options.txt @@ -112,8 +112,9 @@ option can be used to override --squash. + With --squash, --commit is not allowed, and will fail. ---no-verify:: - This option bypasses the pre-merge and commit-msg hooks. +--[no-]verify:: + By default, the pre-merge and commit-msg hooks are run. + When `--no-verify` is given, these are bypassed. See also linkgit:githooks[5]. -s <strategy>:: diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh index 31b9c6a2c1..67fcc19637 100755 --- a/t/t7504-commit-msg-hook.sh +++ b/t/t7504-commit-msg-hook.sh @@ -130,6 +130,14 @@ test_expect_success '--no-verify with failing hook' ' ' +test_expect_success '-n followed by --verify with failing hook' ' + + echo "even more" >> file && + git add file && + test_must_fail git commit -n --verify -m "even more" + +' + test_expect_success '--no-verify with failing hook (editor)' ' echo "more stuff" >> file &&
Subject of this messages was supposed to be
Document positive variant of commit and merge option "--no-verify"
But I hand-edited it beyond all repair :-(
Hi Alex On 28/10/2021 16:44, Alex Riesen wrote: > From: Alex Riesen <raa.lkml@gmail.com> > > This documents re-enabling of the hooks disabled by an earlier > "--no-verify" in command-line. > > Signed-off-by: Alexander Riesen <raa.lkml@gmail.com> > --- > > Hi Phillip, > > Phillip Wood, Thu, Oct 28, 2021 15:57:58 +0200: >> On 28/10/2021 09:04, Alex Riesen wrote: >>> From: Alex Riesen <raa.lkml@gmail.com> >>> >>> This allows re-enabling of the hooks disabled by an earlier "--no-verify" >>> in command-line and makes the interface more consistent. >> >> Thanks for working on this. Since 0f1930c587 ("parse-options: allow >> positivation of options starting, with no-", 2012-02-25) merge and commit >> have accepted "--verify" but it is undocumented. The documentation updates >> and fix to pull in this patch are very welcome, but I'm not sure we need the >> other changes. I've left a couple of comments below. >> >> [As an aside we should probably improve the documentation in parse-options.h >> if both Peff and Junio did not know how it handles "--no-foo" but that is >> outside the scope of this patch] > > Interesting feature. It is unfortunate it was so well hidden. You're right, of > course, and the newly added tests in t7504-commit-msg-hook.sh pass without any > changes to the "builtin/commit.c". > > Removal of double-negation in the code was an improvement to its readability, > but I like small patches more. > > Also, the series has no conflicts with 2.33.0 anymore and the "git pull" can > be applied independently. > >>> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt >>> index a3baea32ae..ba66209274 100644 >>> --- a/Documentation/git-commit.txt >>> +++ b/Documentation/git-commit.txt >>> @@ -11,7 +11,7 @@ SYNOPSIS >>> 'git commit' [-a | --interactive | --patch] [-s] [-v] [-u<mode>] [--amend] >>> [--dry-run] [(-c | -C | --fixup | --squash) <commit>] >>> [-F <file> | -m <msg>] [--reset-author] [--allow-empty] >>> - [--allow-empty-message] [--no-verify] [-e] [--author=<author>] >>> + [--allow-empty-message] [--[no-]verify] [-e] [--author=<author>] >> >> I think for the synopsis it is fine just to list the most common options. >> Having --no-verify without the [no-] makes it clear that --verify is the >> default so is not a commonly used option. > > Yep, makes sense. > >>> [--date=<date>] [--cleanup=<mode>] [--[no-]status] >>> [-i | -o] [--pathspec-from-file=<file> [--pathspec-file-nul]] >>> [-S[<keyid>]] [--] [<pathspec>...] >>> @@ -174,7 +174,13 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`. >>> -n:: >>> --no-verify:: >>> - This option bypasses the pre-commit and commit-msg hooks. >>> + By default, pre-merge and commit-msg hooks are run. When one of these >> >> I think saying "the pre-merge and commit-msg hooks" would be clearer as you >> do below. >> >>> + options is given, these are bypassed. >>> + See also linkgit:githooks[5]. >>> + >>> +--verify:: >>> + This option re-enables running of the pre-commit and commit-msg hooks >>> + after an earlier `-n` or `--no-verify`. >>> See also linkgit:githooks[5]. >> >> Some of the existing documentation describes the "--no-foo" option with >> "--foo" (e.g --[no-]signoff) but in other places we list the two options >> separately (e.g. --[no-]edit), I'd lean towards combining them as you have >> done for the merge documentation but I don't feel strongly about it. > > How about this instead: > > -n:: > --no-verify:: > By default, pre-commit and commit-msg hooks are run. When one of these > options is given, the hooks will be bypassed. > See also linkgit:githooks[5]. > > --verify:: > This option re-enables running of the pre-commit and commit-msg hooks > after an earlier `-n` or `--no-verify`. > >>> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt >>> index 3819fadac1..324ae879d2 100644 >>> --- a/Documentation/git-merge.txt >>> +++ b/Documentation/git-merge.txt >>> @@ -10,7 +10,7 @@ SYNOPSIS >>> -------- >>> [verse] >>> 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit] >>> - [--no-verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]] >>> + [--[no-]verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]] >> >> Again I'm not sure changing the synopsis makes things clearer. > > Removed. > >>> [--[no-]allow-unrelated-histories] >>> [--[no-]rerere-autoupdate] [-m <msg>] [-F <file>] [<commit>...] >>> 'git merge' (--continue | --abort | --quit) >>> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt >>> index 80d4831662..f8016b0f7b 100644 >>> --- a/Documentation/merge-options.txt >>> +++ b/Documentation/merge-options.txt >>> @@ -112,8 +112,9 @@ option can be used to override --squash. >>> + >>> With --squash, --commit is not allowed, and will fail. >>> ---no-verify:: >>> - This option bypasses the pre-merge and commit-msg hooks. >>> +--[no-]verify:: >>> + By default, pre-merge and commit-msg hooks are run. When `--no-verify` >> >> I think "the pre-merge ..." would be better here as well. > > Like this? > > --[no-]verify:: > By default, the pre-merge and commit-msg hooks are run. > When `--no-verify` is given, these are bypassed. > See also linkgit:githooks[5]. > >>> diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh >>> index 31b9c6a2c1..166ff5fb26 100755 >>> --- a/t/t7504-commit-msg-hook.sh >>> +++ b/t/t7504-commit-msg-hook.sh >>> @@ -130,6 +130,14 @@ test_expect_success '--no-verify with failing hook' ' >>> ' >>> +test_expect_success '-n with failing hook' ' >>> + >>> + echo "more" >> file && >>> + git add file && >>> + git commit -n -m "more" >>> + >>> +' >> >> Is this to check that "-n" works like "--no-verify"? > > Frankly, it was to check that the separate "-n" option works as I supposed it > would. I never used parse-options before. > >> I think it would be very useful to add another test that checks "--verify" >> overrides "--no-verify". > > Replaced the test with one which has "-n --verify". > > Thanks! > > > Documentation/git-commit.txt | 7 ++++++- > Documentation/merge-options.txt | 5 +++-- > t/t7504-commit-msg-hook.sh | 8 ++++++++ > 3 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt > index a3baea32ae..2268787483 100644 > --- a/Documentation/git-commit.txt > +++ b/Documentation/git-commit.txt > @@ -174,9 +174,14 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`. > > -n:: > --no-verify:: > - This option bypasses the pre-commit and commit-msg hooks. > + By default, pre-commit and commit-msg hooks are run. When one of these As I suggested yesterday I think this would be better if it kept the "the" from the original text as you do below for the merge documentation - s/default, /&the / > + options is given, the hooks will be bypassed. > See also linkgit:githooks[5]. > > +--verify:: > + This option re-enables running of the pre-commit and commit-msg hooks > + after an earlier `-n` or `--no-verify`. > + > --allow-empty:: > Usually recording a commit that has the exact same tree as its > sole parent commit is a mistake, and the command prevents you > diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt > index 80d4831662..80267008af 100644 > --- a/Documentation/merge-options.txt > +++ b/Documentation/merge-options.txt > @@ -112,8 +112,9 @@ option can be used to override --squash. > + > With --squash, --commit is not allowed, and will fail. > > ---no-verify:: > - This option bypasses the pre-merge and commit-msg hooks. > +--[no-]verify:: > + By default, the pre-merge and commit-msg hooks are run. > + When `--no-verify` is given, these are bypassed. > See also linkgit:githooks[5]. This text looks good. It would be nice to be consistent when documenting "--verify" and "--no-verify" so that documentation for commit and merge both have either a separate entry for each option as you have for commit or a shared entry as you have here for merge. I'd be tempted to use this form in the commit documentation. > -s <strategy>:: > diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh > index 31b9c6a2c1..67fcc19637 100755 > --- a/t/t7504-commit-msg-hook.sh > +++ b/t/t7504-commit-msg-hook.sh > @@ -130,6 +130,14 @@ test_expect_success '--no-verify with failing hook' ' > > ' > > +test_expect_success '-n followed by --verify with failing hook' ' > + > + echo "even more" >> file && > + git add file && > + test_must_fail git commit -n --verify -m "even more" > + > +' Thanks, having the new test is very helpful. Best Wishes Phillip
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt index a3baea32ae..ba66209274 100644 --- a/Documentation/git-commit.txt +++ b/Documentation/git-commit.txt @@ -11,7 +11,7 @@ SYNOPSIS 'git commit' [-a | --interactive | --patch] [-s] [-v] [-u<mode>] [--amend] [--dry-run] [(-c | -C | --fixup | --squash) <commit>] [-F <file> | -m <msg>] [--reset-author] [--allow-empty] - [--allow-empty-message] [--no-verify] [-e] [--author=<author>] + [--allow-empty-message] [--[no-]verify] [-e] [--author=<author>] [--date=<date>] [--cleanup=<mode>] [--[no-]status] [-i | -o] [--pathspec-from-file=<file> [--pathspec-file-nul]] [-S[<keyid>]] [--] [<pathspec>...] @@ -174,7 +174,13 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`. -n:: --no-verify:: - This option bypasses the pre-commit and commit-msg hooks. + By default, pre-merge and commit-msg hooks are run. When one of these + options is given, these are bypassed. + See also linkgit:githooks[5]. + +--verify:: + This option re-enables running of the pre-commit and commit-msg hooks + after an earlier `-n` or `--no-verify`. See also linkgit:githooks[5]. --allow-empty:: diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt index 3819fadac1..324ae879d2 100644 --- a/Documentation/git-merge.txt +++ b/Documentation/git-merge.txt @@ -10,7 +10,7 @@ SYNOPSIS -------- [verse] 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit] - [--no-verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]] + [--[no-]verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]] [--[no-]allow-unrelated-histories] [--[no-]rerere-autoupdate] [-m <msg>] [-F <file>] [<commit>...] 'git merge' (--continue | --abort | --quit) diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt index 80d4831662..f8016b0f7b 100644 --- a/Documentation/merge-options.txt +++ b/Documentation/merge-options.txt @@ -112,8 +112,9 @@ option can be used to override --squash. + With --squash, --commit is not allowed, and will fail. ---no-verify:: - This option bypasses the pre-merge and commit-msg hooks. +--[no-]verify:: + By default, pre-merge and commit-msg hooks are run. When `--no-verify` + is given, these are bypassed. See also linkgit:githooks[5]. -s <strategy>:: diff --git a/builtin/commit.c b/builtin/commit.c index 1dfd799ec5..714722b0cd 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -108,7 +108,7 @@ static char *edit_message, *use_message; static char *fixup_message, *squash_message; static int all, also, interactive, patch_interactive, only, amend, signoff; static int edit_flag = -1; /* unspecified */ -static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship; +static int quiet, verbose, verify = 1, allow_empty, dry_run, renew_authorship; static int config_commit_verbose = -1; /* unspecified */ static int no_post_rewrite, allow_empty_message, pathspec_file_nul; static char *untracked_files_arg, *force_date, *ignore_submodule_arg, *ignored_arg; @@ -164,6 +164,16 @@ static int opt_parse_m(const struct option *opt, const char *arg, int unset) return 0; } +static int opt_parse_n(const struct option *opt, const char *arg, int unset) +{ + int *value = opt->value; + + BUG_ON_OPT_NEG(unset); + + *value = 0; + return 0; +} + static int opt_parse_rename_score(const struct option *opt, const char *arg, int unset) { const char **value = opt->value; @@ -699,7 +709,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, /* This checks and barfs if author is badly specified */ determine_author_info(author_ident); - if (!no_verify && run_commit_hook(use_editor, index_file, "pre-commit", NULL)) + if (verify && run_commit_hook(use_editor, index_file, "pre-commit", NULL)) return 0; if (squash_message) { @@ -983,7 +993,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, return 0; } - if (!no_verify && find_hook("pre-commit")) { + if (verify && find_hook("pre-commit")) { /* * Re-read the index as pre-commit hook could have updated it, * and write it out as a tree. We must do this before we invoke @@ -1014,7 +1024,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, strvec_clear(&env); } - if (!no_verify && + if (verify && run_commit_hook(use_editor, index_file, "commit-msg", git_path_commit_editmsg(), NULL)) { return 0; } @@ -1522,7 +1532,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix) OPT_BOOL(0, "interactive", &interactive, N_("interactively add files")), OPT_BOOL('p', "patch", &patch_interactive, N_("interactively add changes")), OPT_BOOL('o', "only", &only, N_("commit only specified files")), - OPT_BOOL('n', "no-verify", &no_verify, N_("bypass pre-commit and commit-msg hooks")), + OPT_CALLBACK_F('n', NULL, &verify, "", N_("bypass pre-commit and commit-msg hooks"), + PARSE_OPT_NOARG|PARSE_OPT_NONEG, opt_parse_n), + OPT_BOOL(0, "verify", &verify, N_("control use of pre-commit and commit-msg hooks")), OPT_BOOL(0, "dry-run", &dry_run, N_("show what would be committed")), OPT_SET_INT(0, "short", &status_format, N_("show status concisely"), STATUS_FORMAT_SHORT), diff --git a/builtin/merge.c b/builtin/merge.c index 9d5359edc2..ab5c221234 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -83,7 +83,7 @@ static int default_to_upstream = 1; static int signoff; static const char *sign_commit; static int autostash; -static int no_verify; +static int verify = 1; static struct strategy all_strategy[] = { { "recursive", DEFAULT_TWOHEAD | NO_TRIVIAL }, @@ -290,7 +290,7 @@ static struct option builtin_merge_options[] = { OPT_AUTOSTASH(&autostash), OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")), OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")), - OPT_BOOL(0, "no-verify", &no_verify, N_("bypass pre-merge-commit and commit-msg hooks")), + OPT_BOOL(0, "verify", &verify, N_("control use of pre-merge-commit and commit-msg hooks")), OPT_END() }; @@ -822,7 +822,7 @@ static void prepare_to_commit(struct commit_list *remoteheads) struct strbuf msg = STRBUF_INIT; const char *index_file = get_index_file(); - if (!no_verify && run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL)) + if (verify && run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL)) abort_commit(remoteheads, NULL); /* * Re-read the index as pre-merge-commit hook could have updated it, @@ -858,9 +858,9 @@ static void prepare_to_commit(struct commit_list *remoteheads) abort_commit(remoteheads, NULL); } - if (!no_verify && run_commit_hook(0 < option_edit, get_index_file(), - "commit-msg", - git_path_merge_msg(the_repository), NULL)) + if (verify && run_commit_hook(0 < option_edit, get_index_file(), + "commit-msg", + git_path_merge_msg(the_repository), NULL)) abort_commit(remoteheads, NULL); read_merge_msg(&msg); diff --git a/builtin/pull.c b/builtin/pull.c index 425950f469..e783da10b2 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -84,6 +84,7 @@ static char *opt_edit; static char *cleanup_arg; static char *opt_ff; static char *opt_verify_signatures; +static char *opt_verify; static int opt_autostash = -1; static int config_autostash; static int check_trust_level = 1; @@ -160,6 +161,9 @@ static struct option pull_options[] = { OPT_PASSTHRU(0, "ff-only", &opt_ff, NULL, N_("abort if fast-forward is not possible"), PARSE_OPT_NOARG | PARSE_OPT_NONEG), + OPT_PASSTHRU(0, "verify", &opt_verify, NULL, + N_("control use of pre-merge-commit and commit-msg hooks"), + PARSE_OPT_NOARG), OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL, N_("verify that the named commit has a valid GPG signature"), PARSE_OPT_NOARG), @@ -688,6 +692,8 @@ static int run_merge(void) strvec_pushf(&args, "--cleanup=%s", cleanup_arg); if (opt_ff) strvec_push(&args, opt_ff); + if (opt_verify) + strvec_push(&args, opt_verify); if (opt_verify_signatures) strvec_push(&args, opt_verify_signatures); strvec_pushv(&args, opt_strategies.v); diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh index db1a381cd9..7d3a8ae0d3 100755 --- a/t/t5521-pull-options.sh +++ b/t/t5521-pull-options.sh @@ -225,4 +225,16 @@ test_expect_success 'git pull --no-signoff flag cancels --signoff flag' ' test_must_be_empty actual ' +test_expect_success 'git pull --no-verify flag passed to merge' ' + test_when_finished "rm -fr src dst actual" && + git init src && + test_commit -C src one && + git clone src dst && + write_script dst/.git/hooks/commit-msg <<-\EOF && + false + EOF + test_commit -C src two && + git -C dst pull --no-ff --no-verify +' + test_done diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh index 31b9c6a2c1..166ff5fb26 100755 --- a/t/t7504-commit-msg-hook.sh +++ b/t/t7504-commit-msg-hook.sh @@ -130,6 +130,14 @@ test_expect_success '--no-verify with failing hook' ' ' +test_expect_success '-n with failing hook' ' + + echo "more" >> file && + git add file && + git commit -n -m "more" + +' + test_expect_success '--no-verify with failing hook (editor)' ' echo "more stuff" >> file &&