Re: [PATCH v5 4/4] commit: Add commit.verbose configuration
Caleb Thompson writes: >> Again, what are we testing, exactly? >> >> We do not want to see "^diff --git" in the output file, in other >> words, we want to make sure "^diff --git" does not appear in the >> output. >> >> So >> >> write_script check-for-no-diff <<-\EOF >> ! grep '^diff --git' "$@" >> EOF >> >> should be the most natural way to express what we are testing, no? > > I did consider that. The reason I didn't propose that is that it doesn't catch > the unlikely case that the $1 only contains a "diff --git" line or that $1 is > empty. > > Those are rather unreasonable concerns, so I'm happy to use the much more > readable version as you propose. If it only has "diff --git", then the grep will find a hit, exits with success, the script yields the opposite and "git commit" will fail, which is what we want, so that is OK. "$1 is empty" may or may not be an error, depending on your settings, I guess (i.e. can't we squelch the "# helpful instruction" lines altogether?)? If the editor input is expected to be very stable, we could even do something like: write_script check-editor-input <<-\EOF diff expect "$1" >&2 EOF and then catch any deviation from the norm with something like: cat >expect <<-\EOF && ... expected editor input comes here ... EOF test_set_editor "$PWD/check-editor-input && git commit --amend but if the editor input may easily be affected by volatile things like blob object names given in the diff output by "commit -v" or untracked cruft in the working tree listed in the status output, then it would add unnecessary maintenance burden (every time earlier parts of the test scripts are updated, the expected output may have to change to adjust to these non-essential details), so it is a judgment call. Thanks. -- 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: [PATCH v5 4/4] commit: Add commit.verbose configuration
On Mon, Jun 16, 2014 at 01:06:45PM -0700, Junio C Hamano wrote: > Caleb Thompson writes: > > > On Fri, Jun 13, 2014 at 10:48:55AM -0700, Junio C Hamano wrote: > >> Caleb Thompson writes: > >> > >> > diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh > >> > index 35a4d06..402d6a1 100755 > >> > --- a/t/t7507-commit-verbose.sh > >> > +++ b/t/t7507-commit-verbose.sh > >> > @@ -7,6 +7,10 @@ write_script check-for-diff <<-'EOF' > >> > exec grep '^diff --git' "$1" > >> > EOF > >> > > >> > +write_script check-for-no-diff <<-'EOF' > >> > +exec grep -v '^diff --git' "$1" > >> > +EOF > >> > >> This lets grep show all lines that are not "diff --git" in the > >> input, and as usual grep exits success if it has any line in the > >> output. > >> > >> $ grep -v '^diff --git' <<\EOF ; echo $? > >> diff --git > >> a > >> EOF > >> a > >> 0 > >> $ exit > >> > >> What are we testing, exactly? > > > > Good catch. It worked when I switched check-for-diff from > > check-for-no-diff, but I didn't try to make check-for-no-diff fail > > independently, so I apologize. > > No need to apologize at all. None of us (including this reviewer) > is perfect and that is why we review patches by each other. > > > This version removes the the beginning of a line starting with > > "diff --git" from the string,... > > Again, what are we testing, exactly? > > We do not want to see "^diff --git" in the output file, in other > words, we want to make sure "^diff --git" does not appear in the > output. > > So > > write_script check-for-no-diff <<-\EOF > ! grep '^diff --git' "$@" > EOF > > should be the most natural way to express what we are testing, no? I did consider that. The reason I didn't propose that is that it doesn't catch the unlikely case that the $1 only contains a "diff --git" line or that $1 is empty. Those are rather unreasonable concerns, so I'm happy to use the much more readable version as you propose. Caleb Thompson pgpuRfoSf3aXR.pgp Description: PGP signature
Re: [PATCH v5 4/4] commit: Add commit.verbose configuration
Caleb Thompson writes: > On Fri, Jun 13, 2014 at 10:48:55AM -0700, Junio C Hamano wrote: >> Caleb Thompson writes: >> >> > diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh >> > index 35a4d06..402d6a1 100755 >> > --- a/t/t7507-commit-verbose.sh >> > +++ b/t/t7507-commit-verbose.sh >> > @@ -7,6 +7,10 @@ write_script check-for-diff <<-'EOF' >> >exec grep '^diff --git' "$1" >> > EOF >> > >> > +write_script check-for-no-diff <<-'EOF' >> > + exec grep -v '^diff --git' "$1" >> > +EOF >> >> This lets grep show all lines that are not "diff --git" in the >> input, and as usual grep exits success if it has any line in the >> output. >> >> $ grep -v '^diff --git' <<\EOF ; echo $? >> diff --git >> a >> EOF >> a >> 0 >> $ exit >> >> What are we testing, exactly? > > Good catch. It worked when I switched check-for-diff from > check-for-no-diff, but I didn't try to make check-for-no-diff fail > independently, so I apologize. No need to apologize at all. None of us (including this reviewer) is perfect and that is why we review patches by each other. > This version removes the the beginning of a line starting with > "diff --git" from the string,... Again, what are we testing, exactly? We do not want to see "^diff --git" in the output file, in other words, we want to make sure "^diff --git" does not appear in the output. So write_script check-for-no-diff <<-\EOF ! grep '^diff --git' "$@" EOF should be the most natural way to express what we are testing, no? -- 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: [PATCH v5 4/4] commit: Add commit.verbose configuration
On Mon, Jun 16, 2014 at 02:50:57PM -0500, Caleb Thompson wrote:
> On Fri, Jun 13, 2014 at 10:48:55AM -0700, Junio C Hamano wrote:
> > Caleb Thompson writes:
> >
> > > diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> > > index 35a4d06..402d6a1 100755
> > > --- a/t/t7507-commit-verbose.sh
> > > +++ b/t/t7507-commit-verbose.sh
> > > @@ -7,6 +7,10 @@ write_script check-for-diff <<-'EOF'
> > > exec grep '^diff --git' "$1"
> > > EOF
> > >
> > > +write_script check-for-no-diff <<-'EOF'
> > > + exec grep -v '^diff --git' "$1"
> > > +EOF
> >
> > This lets grep show all lines that are not "diff --git" in the
> > input, and as usual grep exits success if it has any line in the
> > output.
> >
> > $ grep -v '^diff --git' <<\EOF ; echo $?
> > diff --git
> > a
> > EOF
> > a
> > 0
> > $ exit
> >
> > What are we testing, exactly?
>
> Good catch. It worked when I switched check-for-diff from
> check-for-no-diff, but I didn't try to make check-for-no-diff fail
> independently, so I apologize.
>
> This version removes the the beginning of a line starting with
> "diff --git" from the string, then checks that the result and the
> original string are not the same. Switching the != logic to = makes the
> tests using check-for-no-diff fail.
>
> write_script check-for-no-diff <<-'EOF'
> exec test "${1#*^diff --git} != $1
> EOF
>
> Another option is to replace the parameter substitution with a call to
> grep:
>
> write_script check-for-no-diff <<-'EOF'
> exec test "`grep -v '^diff --git' \"$1\"` != "$1
> EOF
>
> I think that the former reads nicer, and requires less escaping, but I'm
> open to feedback.
Correction: the first variant does not work, only the second. Sorry for the
confustion.
write_script check-for-no-diff <<-'EOF'
exec test "`grep -v '^diff --git' \"$1\"`" != "$1"
EOF
> > > @@ -48,6 +52,38 @@ test_expect_success 'verbose diff is stripped out
> > > (mnemonicprefix)' '
> > > check_message message
> > > '
> > >
> > > +test_expect_success 'commit shows verbose diff with commit.verbose true'
> > > '
> > > + echo morecontent >>file &&
> > > + git add file &&
> > > + test_config commit.verbose true &&
> > > + test_set_editor "$PWD/check-for-diff" &&
> > > + git commit --amend
> > > +'
> > > +
> > > +test_expect_success 'commit --verbose overrides commit.verbose false' '
> > > + echo evenmorecontent >>file &&
> > > + git add file &&
> > > + test_config commit.verbose false &&
> > > + test_set_editor "$PWD/check-for-diff" &&
> > > + git commit --amend --verbose
> > > +'
> > > +
> > > +test_expect_success 'commit does not show verbose diff with
> > > commit.verbose false' '
> > > + echo evenmorecontent >>file &&
> > > + git add file &&
> > > + test_config commit.verbose false &&
> > > + test_set_editor "$PWD/check-for-no-diff" &&
> > > + git commit --amend
> > > +'
> > > +
> > > +test_expect_success 'commit --no-verbose overrides commit.verbose true' '
> > > + echo evenmorecontent >>file &&
> > > + git add file &&
> > > + test_config commit.verbose true &&
> > > + test_set_editor "$PWD/check-for-no-diff" &&
> > > + git commit --amend --no-verbose
> > > +'
> > > +
> > > cat >diff <<'EOF'
> > > This is an example commit message that contains a diff.
> > >
> > > --
> > > 2.0.0
>
> Caleb Thompson
pgpbyvFQ4c6wi.pgp
Description: PGP signature
Re: [PATCH v5 4/4] commit: Add commit.verbose configuration
On Fri, Jun 13, 2014 at 10:48:55AM -0700, Junio C Hamano wrote:
> Caleb Thompson writes:
>
> > diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> > index 35a4d06..402d6a1 100755
> > --- a/t/t7507-commit-verbose.sh
> > +++ b/t/t7507-commit-verbose.sh
> > @@ -7,6 +7,10 @@ write_script check-for-diff <<-'EOF'
> > exec grep '^diff --git' "$1"
> > EOF
> >
> > +write_script check-for-no-diff <<-'EOF'
> > + exec grep -v '^diff --git' "$1"
> > +EOF
>
> This lets grep show all lines that are not "diff --git" in the
> input, and as usual grep exits success if it has any line in the
> output.
>
> $ grep -v '^diff --git' <<\EOF ; echo $?
> diff --git
> a
> EOF
> a
> 0
> $ exit
>
> What are we testing, exactly?
Good catch. It worked when I switched check-for-diff from
check-for-no-diff, but I didn't try to make check-for-no-diff fail
independently, so I apologize.
This version removes the the beginning of a line starting with
"diff --git" from the string, then checks that the result and the
original string are not the same. Switching the != logic to = makes the
tests using check-for-no-diff fail.
write_script check-for-no-diff <<-'EOF'
exec test "${1#*^diff --git} != $1
EOF
Another option is to replace the parameter substitution with a call to
grep:
write_script check-for-no-diff <<-'EOF'
exec test "`grep -v '^diff --git' \"$1\"` != "$1
EOF
I think that the former reads nicer, and requires less escaping, but I'm
open to feedback.
> > @@ -48,6 +52,38 @@ test_expect_success 'verbose diff is stripped out
> > (mnemonicprefix)' '
> > check_message message
> > '
> >
> > +test_expect_success 'commit shows verbose diff with commit.verbose true' '
> > + echo morecontent >>file &&
> > + git add file &&
> > + test_config commit.verbose true &&
> > + test_set_editor "$PWD/check-for-diff" &&
> > + git commit --amend
> > +'
> > +
> > +test_expect_success 'commit --verbose overrides commit.verbose false' '
> > + echo evenmorecontent >>file &&
> > + git add file &&
> > + test_config commit.verbose false &&
> > + test_set_editor "$PWD/check-for-diff" &&
> > + git commit --amend --verbose
> > +'
> > +
> > +test_expect_success 'commit does not show verbose diff with commit.verbose
> > false' '
> > + echo evenmorecontent >>file &&
> > + git add file &&
> > + test_config commit.verbose false &&
> > + test_set_editor "$PWD/check-for-no-diff" &&
> > + git commit --amend
> > +'
> > +
> > +test_expect_success 'commit --no-verbose overrides commit.verbose true' '
> > + echo evenmorecontent >>file &&
> > + git add file &&
> > + test_config commit.verbose true &&
> > + test_set_editor "$PWD/check-for-no-diff" &&
> > + git commit --amend --no-verbose
> > +'
> > +
> > cat >diff <<'EOF'
> > This is an example commit message that contains a diff.
> >
> > --
> > 2.0.0
Caleb Thompson
pgpS_n1HfoIeB.pgp
Description: PGP signature
Re: [PATCH v5 4/4] commit: Add commit.verbose configuration
Caleb Thompson writes: > diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh > index 35a4d06..402d6a1 100755 > --- a/t/t7507-commit-verbose.sh > +++ b/t/t7507-commit-verbose.sh > @@ -7,6 +7,10 @@ write_script check-for-diff <<-'EOF' > exec grep '^diff --git' "$1" > EOF > > +write_script check-for-no-diff <<-'EOF' > + exec grep -v '^diff --git' "$1" > +EOF This lets grep show all lines that are not "diff --git" in the input, and as usual grep exits success if it has any line in the output. $ grep -v '^diff --git' <<\EOF ; echo $? diff --git a EOF a 0 $ exit What are we testing, exactly? > @@ -48,6 +52,38 @@ test_expect_success 'verbose diff is stripped out > (mnemonicprefix)' ' > check_message message > ' > > +test_expect_success 'commit shows verbose diff with commit.verbose true' ' > + echo morecontent >>file && > + git add file && > + test_config commit.verbose true && > + test_set_editor "$PWD/check-for-diff" && > + git commit --amend > +' > + > +test_expect_success 'commit --verbose overrides commit.verbose false' ' > + echo evenmorecontent >>file && > + git add file && > + test_config commit.verbose false && > + test_set_editor "$PWD/check-for-diff" && > + git commit --amend --verbose > +' > + > +test_expect_success 'commit does not show verbose diff with commit.verbose > false' ' > + echo evenmorecontent >>file && > + git add file && > + test_config commit.verbose false && > + test_set_editor "$PWD/check-for-no-diff" && > + git commit --amend > +' > + > +test_expect_success 'commit --no-verbose overrides commit.verbose true' ' > + echo evenmorecontent >>file && > + git add file && > + test_config commit.verbose true && > + test_set_editor "$PWD/check-for-no-diff" && > + git commit --amend --no-verbose > +' > + > cat >diff <<'EOF' > This is an example commit message that contains a diff. > > -- > 2.0.0 -- 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
[PATCH v5 4/4] commit: Add commit.verbose configuration
Add a new configuration variable commit.verbose to implicitly pass
--verbose to git-commit. Ensure that --no-verbose to git-commit
negates that setting.
Signed-off-by: Caleb Thompson
---
Documentation/config.txt | 5 +
Documentation/git-commit.txt | 8 +++-
builtin/commit.c | 4
contrib/completion/git-completion.bash | 1 +
t/t7507-commit-verbose.sh | 36 ++
5 files changed, 53 insertions(+), 1 deletion(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index cd2d651..ec51e1c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1017,6 +1017,11 @@ commit.template::
"`~/`" is expanded to the value of `$HOME` and "`~user/`" to the
specified user's home directory.
+commit.verbose::
+ A boolean to enable/disable inclusion of diff information in the
+ commit message template when using an editor to prepare the commit
+ message. Defaults to false.
+
credential.helper::
Specify an external helper to be called when a username or
password credential is needed; the helper may consult external
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 0bbc8f5..8cb3439 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -282,7 +282,13 @@ configuration variable documented in linkgit:git-config[1].
Show unified diff between the HEAD commit and what
would be committed at the bottom of the commit message
template. Note that this diff output doesn't have its
- lines prefixed with '#'.
+ lines prefixed with '#'. The `commit.verbose` configuration
+ variable can be set to true to implicitly send this option.
+
+--no-verbose::
+ Do not show the unified diff at the bottom of the commit message
+ template. This is the default behavior, but can be used to override
+ the `commit.verbose` configuration variable.
-q::
--quiet::
diff --git a/builtin/commit.c b/builtin/commit.c
index 99c2044..c782388 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1489,6 +1489,10 @@ static int git_commit_config(const char *k, const char
*v, void *cb)
sign_commit = git_config_bool(k, v) ? "" : NULL;
return 0;
}
+ if (!strcmp(k, "commit.verbose")) {
+ verbose = git_config_bool(k, v);
+ return 0;
+ }
status = git_gpg_config(k, v, NULL);
if (status)
diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index 2c59a76..b8f4b94 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1976,6 +1976,7 @@ _git_config ()
color.ui
commit.status
commit.template
+ commit.verbose
core.abbrev
core.askpass
core.attributesfile
diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index 35a4d06..402d6a1 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -7,6 +7,10 @@ write_script check-for-diff <<-'EOF'
exec grep '^diff --git' "$1"
EOF
+write_script check-for-no-diff <<-'EOF'
+ exec grep -v '^diff --git' "$1"
+EOF
+
cat >message <<'EOF'
subject
@@ -48,6 +52,38 @@ test_expect_success 'verbose diff is stripped out
(mnemonicprefix)' '
check_message message
'
+test_expect_success 'commit shows verbose diff with commit.verbose true' '
+ echo morecontent >>file &&
+ git add file &&
+ test_config commit.verbose true &&
+ test_set_editor "$PWD/check-for-diff" &&
+ git commit --amend
+'
+
+test_expect_success 'commit --verbose overrides commit.verbose false' '
+ echo evenmorecontent >>file &&
+ git add file &&
+ test_config commit.verbose false &&
+ test_set_editor "$PWD/check-for-diff" &&
+ git commit --amend --verbose
+'
+
+test_expect_success 'commit does not show verbose diff with commit.verbose
false' '
+ echo evenmorecontent >>file &&
+ git add file &&
+ test_config commit.verbose false &&
+ test_set_editor "$PWD/check-for-no-diff" &&
+ git commit --amend
+'
+
+test_expect_success 'commit --no-verbose overrides commit.verbose true' '
+ echo evenmorecontent >>file &&
+ git add file &&
+ test_config commit.verbose true &&
+ test_set_editor "$PWD/check-for-no-diff" &&
+ git commit --amend --no-verbose
+'
+
cat >diff <<'EOF'
This is an example commit message that contains a diff.
--
2.0.0
--
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
