On Sun, Mar 20, 2016 at 9:26 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Fri, Mar 18, 2016 at 5:19 PM, Pranit Bauva <pranit.bauva@xxxxxxxxx> wrote:
>> Add commit.verbose configuration variable as a convenience for those
>> who always prefer --verbose.
>>
>> Signed-off-by: Pranit Bauva <pranit.bauva@xxxxxxxxx>
>> ---
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> @@ -1654,6 +1661,14 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>> + if (verbose < 0) {
>> + if (config_verbose > -1)
>> + verbose = config_verbose;
>> + else
>> + verbose = 0;
>> + }
>
> I think it's more common in this codebase to compare against -1
> directly rather than <0, so:
>
> if (verbose == -1) {
> if (config_verbose != -1)
> verbose = config_verbose;
> else
> verbose = 0;
> }
>
> Or, this might be easier to read:
>
> if (verbose == -1)
> verbose = config_verbose;
>
> if (verbose == -1)
> verbose = 0;
>
> But, this likely isn't better:
>
> if (verbose == -1)
> verbose = config_verbose == -1 ? 0 : config_verbose;
>
> Anyhow, probably not worth a re-roll.
I will note this for future patches.
>
>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
>> @@ -96,4 +96,59 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' '
>> +test_expect_success 'commit.verbose true and --verbose' '
>> + (
>> + GIT_EDITOR=cat &&
>> + export GIT_EDITOR &&
>> + git -c commit.verbose=true commit --amend --verbose
>
> Easier would be to write this as:
>
> GIT_EDITOR=cat git -c commit.verbose=true commit --amend --verbose
>
> and then you wouldn't need the subhsell.
True. I will update this.
> However, more intuitive would probably be to create another "editor"
> similar to the 'check-for-diff' editor this script already uses. (The
> 'check-for-diff' editor is an obvious example about how to go about
> such an undertaking.) You would need to invoke 'test_set_editor' in a
> subshell for this particular test in order to avoid clobbering the
> global editor used by this script. Or, have a preparatory patch which
> ditches the global setting of the editor and has each test invoke
> 'test_set_editor' as needed (and only if needed).
I guess it would complicate things as sometimes I need to check
whether it has 1 line and sometimes 2 lines.
> Same comments apply to the other new tests which use a custom "editor".
>
>> + ) &&
>> + grep "^diff --git" .git/COMMIT_EDITMSG >out &&
>> + wc -l out | grep "1"
>> +'
>> +
>> +test_expect_success 'commit.verbose true and -v -v' '
>> + (
>> + GIT_EDITOR=cat &&
>> + export GIT_EDITOR &&
>> + git -c commit.verbose=true commit --amend -v -v
>> + ) &&
>> + grep "# Changes not staged for commit" .git/COMMIT_EDITMSG >out &&
>> + wc -l out | grep "2"
>> +'
>> +
>> +test_expect_success 'commit.verbose true and --no-verbose' '
>> + test_must_fail git -c commit.verbose=true commit --amend --no-verbose
>> +'
>> +
>> +test_expect_success 'commit.verbose false and --verbose' '
>> + git -c commit.verbose=false commit --amend --verbose
>> +'
>> +
>> +test_expect_success 'commit.verbose false and -v -v' '
>> + (
>> + GIT_EDITOR=cat &&
>> + export GIT_EDITOR &&
>> + git -c commit.verbose=false commit --amend -v -v
>> + ) &&
>> + grep "# Changes not staged for commit" .git/COMMIT_EDITMSG >out &&
>> + wc -l out | grep "2"
>> +'
>> +
>> +test_expect_success 'commit.verbose false and --verbose omitted' '
>> + test_must_fail git -c commit.verbose=false commit --amend
>> +'
>> +
>> +test_expect_success 'commit.verbose false and --no-verbose' '
>> + test_must_fail git -c commit.verbose=false commit --amend --no-verbose
>> +'
>> +
>> +test_expect_success 'status ignores commit.verbose=true' '
>> + git -c commit.verbose=true status >actual &&
>> + ! grep "^diff --git" actual
>> +'
>> +
>> test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html