Re: [PATCH v2] commit: support commit.verbose and --no-verbose

On Sun, May 25, 2014 at 2:24 AM, Caleb Thompson [off-list ref] wrote:

Incorporated changes from Duy Nguyen and Jeremiah Mahler.

As a courtesy to reviewers, it is helpful to provide a pointer to the
previous submission to give context for the new submission. For
instance, like this [1].

[1]: http://git.661346.n2.nabble.com/commit-support-commit-verbose-and-no-verbose-td7611617.html

Jeremiah, I didn't make the changes about `<<-EOF` or `test_expect_success`
because I'm guessing that keeping the local style of the code intact is more
important than using those. Do you think it makes sense to refactor the rest of
the test file (t/t7507-commit-verbose.sh) to use those? I could also change the
other `git config` calls to use `test_config`.

Generally speaking, it is important to respect local style, however,
it is also appropriate to include one or more cleanup patches before
your primary changes in order to bring the code in line with current
practices. Conversion to test_config could be such a cleanup patch.

Duy, you were right about `-V`. Do you know of a simple way to add that
shortened flag? `OPT_BOOL('v', "verbose", ...)` gives me `-v`, `--verbose`, and
`--no-verbose`, but no `-V` as a shortened form of `--no-verbose`.

At this point, after your email commentary but before the actual
patch, you should have a scissor line -->8-- so that "git am" can
extract your patch automatically from the email.

commit 1a49356b87c9028e68e731f34790c11a3075f736

Drop this line. It has no meaning outside of your local repository.

Author: Caleb Thompson [off-list ref]
Date:   Fri May 23 11:47:44 2014 -0500

Ditto for the date.

    commit: support commit.verbose and --no-verbose

    Add a new configuration variable commit.verbose to implicitly pass
    `--verbose` to `git-commit`. Add `--no-verbose` to commit to negate that
    setting.

The commit message would read just as well or better without the backquotes.

    Signed-off-by: Caleb Thompson [off-list ref]
    Reviewed-by: Duy Nguyen [off-list ref]
    Reviewed-by: Jeremiah Mahler [off-list ref]

Considering that the code in this patch has changed since v1, it's
probably not appropriate to add these Reviewed-by: lines.

Already mentioned by Jeremiah: s/diff\s+/diff /

+       template.  This is the default behavior, but can be used to override
+       the`commit.verbose` configuration variable.

Also already mentioned: s/the/the /

This is not a new problem, but since you copied and modified the
test_set_editor invocation for your own test (below), it can be
mentioned that $(pwd) should be used rather than $PWD. See discussion
of $(pwd) in t/README. A preparatory patch which fixes this would not
be unwelcome.

+cat >check-for-no-diff <<EOF
+#!$SHELL_PATH
+exec grep -v '^diff --git' "\$1"
+EOF
+chmod +x check-for-no-diff

write_script (from test-lib-functions.sh) would be a more appropriate
and modern way to compose this script. If you're concerned about style
consistency, a cleanup patch before this one could employ write_script
for the check-for-diff script, as well.

Also, since this script is used by only the one test, current practice
suggests that script creation should be done within the test itself.

Is your intention to add more content to 'file'? If so, use '>>'.

+       git add file &&
+       test_config commit.verbose true &&
+       check_message message

As Jeremiah pointed out, this test is not actually testing if
commit.verbose=true worked since it's not invoking git-commit. In
fact, check_message is testing something unrelated. You probably meant
"git commit --amend" rather than "check_message message"

+'
+
+test_expect_success 'commit does not show verbose diff with --no-verbose' '

As this is the only test which needs check-for-no-diff, it would be
appropriate to move script creation (via write_script) here into the
test itself (unless you plan on adding more tests which invoke the
script).

+       echo morecontent >file &&
+       git add file &&

Again, since you're using '>' rather than '>>', you haven't actually
changed the content of the file since the last test, so this code
serves no purpose.

+       test_config commit.verbose true &&
+       test_set_editor "$PWD/check-for-no-diff" &&

As noted above, use $(pwd) rather than $PWD.

This invocation of test_set_editor potentially breaks tests following
this one (including tests which may be added in the future) since it
changes the global state established by test_set_editor near the top
of the script. To avoid such a problem, you could invoke
test_set_editor and git-commit in a subshell.

Alternately, current practice would suggest that each test which
requires a particular editor should be responsible for setting it. As
such, a preparatory patch could drop the global test_set_editor and
invoke it instead in each test which requires it. (In fact, there are
a couple tests which are still setting EDITOR manually, and these
could be converted to test_set_editor.)

+       git commit --amend --no-verbose
+'

You're missing some potential tests, such as:

commit.verbose = <unset> (optional)
commit.verbose = false
--verbose overrides commit.verbose=false

 cat >diff <<'EOF'
 This is an example commit message that contains a diff.