v6, 4 partscommit test: Use write_script

This patch allows people to set commit.verbose to implicitly send --verbose to git-commit.

It introduces several cleanup patches to t/t7505-commit-verbose.sh to bring it closer to the current state of the tests as they have been explained to me, then adds the verbose config and --no-verbose flag.

Since the last version of this patch (http://thread.gmane.org/gmane.comp.version-control.git/251486), there have been a couple of commit message clarifications and the body of the check-for-no-diff script in the last patch was changed to simply negate check-for-diff.

Caleb Thompson

------------------------------------------------------

Caleb Thompson (4):
  commit test: Use test_config instead of git-config
  commit test: Use write_script
  commit test: test_set_editor in each test
  commit: Add commit.verbose configuration

 Documentation/config.txt               |  5 +++
 Documentation/git-commit.txt           |  8 ++++-
 builtin/commit.c                       |  4 +++
 contrib/completion/git-completion.bash |  1 +
 t/t7507-commit-verbose.sh              | 64 +++++++++++++++++++++++++---------
 5 files changed, 64 insertions(+), 18 deletions(-)

-- 2.0.0

Some of the tests in t/t7507-commit-verbose.sh were still using git-config to set configuration. Change them to use the test_config helper.

Signed-off-by: Caleb Thompson <caleb@calebthompson.io>
Reviewed-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 t/t7507-commit-verbose.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Caleb Thompson <caleb@calebthompson.io> writes:

"were still using" is only a half of the story, and we need to be more careful than that, though.

By switching to test_config, you unconfigure the diff.mnemonicprefix configuration after this test piece is done. The next one, "diff in message is retained with -v", used to show the change using c/ and i/ as prefixes in the log editor, but now it should be showing a/ and b/.

Have you thought about the reason why the log message used in that test uses c/ and i/ prefixes as a sample patch to be retained, instead of a/ and b/ prefixes? It is to make sure that this in-message patch looks similar to the diff for "--verbose" option, and we used to do the latter under mnemonicprefix. That way, we would be more sure that an incorrect implementation that cuts the result given from the editor at "diff --git $srcprefix/" will fail the test.

This is the kind of thing I mean by "we need to be more careful".

If somebody who is doing "git config to test_config" conversion understands what he is doing, I would have expected to see that these c/ and i/ prefixes in the sample log message are replaced to use a/ and b/ prefixes.

Can we make a similar reasoning on possible fallout from this change? An expected answer would be something like:

    The remaining test pieces after this one does not make any
    change before running "commit -a -v", so having diff.submodule
    set to log or unset does not make any difference to them.

Junio C Hamano <gitster@pobox.com> writes:

I'd rephrase it like so if I were doing this patch myself:

    Subject: t7507: use test_config instead of git-config
    
    Some of the tests in t/t7507-commit-verbose.sh uses git-config
    to set configuration, but the updated variable setting does not
    have to be used in later tests.
    
    Change them to use the test_config helper.

Use write_script from t/test-lib-functions.sh instead of cat, shebang, and chmod. This aids in readability for creating the script by using the named function and allows us to turn off interpolation in the heredoc of the script body to avoid extra escaping, since $SHELL_PATH is handled for us.

Signed-off-by: Caleb Thompson <caleb@calebthompson.io>
Reviewed-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 t/t7507-commit-verbose.sh | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

t/t7507-commit-verbose.sh was using a global test_set_editor call to build its environment. The $EDITOR being used was not necessary for all tests, and was in fact circumvented using subshells in some cases.

To improve robustness against global state changes and avoid the use of subshells to temporarily switch the editor as some were with GIT_EDITOR=cat, set the editor explicitly wherever it will be important.

Specifically, in tests that need to check for the presence of a diff in the editor, make calls to set_test_editor to set $EDITOR to check-for-diff rather than relying on that editor being configured globally. This also helps readers grok the tests as the setup is closer to the verification.

Signed-off-by: Caleb Thompson <caleb@calebthompson.io>
Reviewed-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 t/t7507-commit-verbose.sh | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

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 <caleb@calebthompson.io>
---
 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(-)

On Tue, Jun 17, 2014 at 02:39:00PM -0500, Caleb Thompson wrote:

Don't you need "$PWD/" or similar here, since check-for-diff is not in our path?

Otherwise, your tests will always pass ("! check-for-diff" is always true, since we cannot find it). I get (running with "-v"):

expecting success: 
        echo evenmorecontent >>file &&
        git add file &&
        test_config commit.verbose true &&
        test_set_editor "$PWD/check-for-no-diff" &&
        git commit --amend --no-verbose

/home/peff/compile/git/t/trash directory.t7507-commit-verbose/check-for-no-diff: 2: /home/peff/compile/git/t/trash directory.t7507-commit-verbose/check-for-no-diff: check-for-diff: not found

Other than that, the whole series looks OK to me.

-Peff

Jeff King <peff@peff.net> writes:

I actually would prefer a simpler

	! grep '^diff --git' "$1"

that does not depend on what other parts of the tests have done.

On Tue, Jun 17, 2014 at 02:37:45PM -0700, Junio C Hamano wrote:

Yeah, if it is that simple, I would agree (I did not even look at the definition of check-for-diff, and assumed it was more complicated :) ).

-Peff

On Tue, Jun 17, 2014 at 14:38:56 -0500, Caleb Thompson wrote:

Hi Caleb,

Do you intend to work further on this topic? I have been using a similar (but less polished) patch, and would like to see this included in Git.

If you don't have time to work on it, I can incorporate the comments on this version and submit a v7.

Torstein

Hey Torstein.

I'd planned to come back with a heavily simplified version of this that didn't include any of the related changes, just the feature and the new test, since a lot of that seemed controversial.

That said, I haven't done so and you're welcome to take a whack at this feedback if you'd like.

Caleb Thompson

On Fri, Feb 27, 2015 at 10:13:36AM +0100, Torstein Hegge wrote: