* [PATCH 1/9] t7006: GIT_DIR/config should be honored in subdirs of toplevel
2010-04-13 2:11 [PATCH/RFC 0/9] Setup cleanup, chapter one Jonathan Nieder
@ 2010-04-13 2:13 ` Jonathan Nieder
2010-04-14 20:50 ` Junio C Hamano
2010-04-13 2:17 ` [PATCH 2/9] t7006: test pager configuration for several git commands Jonathan Nieder
` (9 subsequent siblings)
10 siblings, 1 reply; 30+ messages in thread
From: Jonathan Nieder @ 2010-04-13 2:13 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy, Jeff King, Junio C Hamano
When git is passed the --paginate option, starting up a pager
requires deciding what pager to start, which requires access to the
core.pager configuration.
Unfortunately, the --paginate option is handled before git has a
chance to search for a git directory. The effect is that with
--paginate and only with --paginate, a repository-local core.pager
setting does not take effect [*].
[*] unless the git directory is simply .git in the cwd or GIT_DIR
or GIT_CONFIG was set explicitly.
Add a test to demonstrate this counterintuitive behavior.
Noticed while reading over a patch by Nguyễn Thái Ngọc Duy that
fixes it.
Cc: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
| 32 ++++++++++++++++++++++++++++++++
1 files changed, 32 insertions(+), 0 deletions(-)
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index d9202d5..4f804ed 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -164,6 +164,38 @@ test_expect_success TTY 'core.pager overrides PAGER' '
test -e core.pager_used
'
+unset GIT_PAGER
+rm -f core.pager_used
+rm -fr sub
+test_expect_success TTY 'core.pager in subdir' '
+ PAGER=wc &&
+ stampname=$(pwd)/core.pager_used &&
+ export PAGER stampname &&
+ git config core.pager "wc > \"\$stampname\"" &&
+ mkdir sub &&
+ (
+ cd sub &&
+ test_terminal git log
+ ) &&
+ test -e "$stampname"
+'
+
+unset GIT_PAGER
+rm -f core.pager_used
+rm -fr sub
+test_expect_failure TTY 'core.pager in subdir with --paginate' '
+ PAGER=wc &&
+ stampname=$(pwd)/core.pager_used &&
+ export PAGER stampname &&
+ git config core.pager "wc > \"\$stampname\"" &&
+ mkdir sub &&
+ (
+ cd sub &&
+ test_terminal git --paginate log
+ ) &&
+ test -e "$stampname"
+'
+
rm -f GIT_PAGER_used
test_expect_success TTY 'GIT_PAGER overrides core.pager' '
git config core.pager wc &&
--
1.7.0.4
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 1/9] t7006: GIT_DIR/config should be honored in subdirs of toplevel
2010-04-13 2:13 ` [PATCH 1/9] t7006: GIT_DIR/config should be honored in subdirs of toplevel Jonathan Nieder
@ 2010-04-14 20:50 ` Junio C Hamano
2010-04-15 0:38 ` [PATCH] t7006: guard cleanup with test_expect_success Jonathan Nieder
0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2010-04-14 20:50 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Nguyễn Thái Ngọc Duy, Jeff King
Jonathan Nieder <jrnieder@gmail.com> writes:
> +unset GIT_PAGER
> +rm -f core.pager_used
> +rm -fr sub
Why do you have these outside the test itself?
> +test_expect_success TTY 'core.pager in subdir' '
> + PAGER=wc &&
> + stampname=$(pwd)/core.pager_used &&
> + export PAGER stampname &&
> + git config core.pager "wc > \"\$stampname\"" &&
> + mkdir sub &&
> + (
> + cd sub &&
> + test_terminal git log
> + ) &&
> + test -e "$stampname"
> +'
They are reasonable clean-up steps to start the real test (starting from
setting and exporting the pager) in a known good state, and as long as you
write them not to fail I don't see any reason to have them outside the
test. You _might_ want to "unset GIT_PAGER" immediately after the test
that does "export PAGER", so that it won't contaminate the environment the
test script itself runs, but even for that purpose, it would probably be
better to run the parts that depend on the exported value in a subshell.
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH] t7006: guard cleanup with test_expect_success
2010-04-14 20:50 ` Junio C Hamano
@ 2010-04-15 0:38 ` Jonathan Nieder
2010-04-15 0:56 ` Junio C Hamano
0 siblings, 1 reply; 30+ messages in thread
From: Jonathan Nieder @ 2010-04-15 0:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Nguyễn Thái Ngọc Duy, Jeff King
Most of these tests are removing files, environment variables, and
configuration that might interfere outside the test. Putting these
clean-up commands in the test (in the same spirit as v1.7.1-rc0~59,
2010-03-20) means that errors during setup will be caught quickly and
non-error text will be suppressed without -v.
While at it, apply some other minor fixes:
- do not rely on the shell to export variables defined with the same
command as a function call
- avoid whitespace immediately after the > redirection operator, for
consistency with the style of other tests
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Junio C Hamano wrote:
> They are reasonable clean-up steps to start the real test (starting from
> setting and exporting the pager) in a known good state, and as long as you
> write them not to fail I don't see any reason to have them outside the
> test.
Good catch, thanks. Here’s a patch for the existing test (against master).
| 149 ++++++++++++++++++++++++++++++++++++-----------------
t/test-lib.sh | 16 ++++++
2 files changed, 117 insertions(+), 48 deletions(-)
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index d9202d5..62595ab 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -4,17 +4,24 @@ test_description='Test automatic use of a pager.'
. ./test-lib.sh
-rm -f stdout_is_tty
+cleanup_fail() {
+ echo >&2 cleanup failed
+ exit 1
+}
+
test_expect_success 'set up terminal for tests' '
+ rm -f stdout_is_tty ||
+ cleanup_fail &&
+
if test -t 1
then
- : > stdout_is_tty
+ >stdout_is_tty
elif
test_have_prereq PERL &&
"$PERL_PATH" "$TEST_DIRECTORY"/t7006/test-terminal.perl \
sh -c "test -t 1"
then
- : > test_terminal_works
+ >test_terminal_works
fi
'
@@ -32,53 +39,68 @@ else
say no usable terminal, so skipping some tests
fi
-unset GIT_PAGER GIT_PAGER_IN_USE
-git config --unset core.pager
-PAGER='cat > paginated.out'
-export PAGER
-
test_expect_success 'setup' '
+ unset GIT_PAGER GIT_PAGER_IN_USE &&
+ test_might_fail git config --unset core.pager &&
+
+ PAGER="cat >paginated.out" &&
+ export PAGER &&
+
test_commit initial
'
-rm -f paginated.out
test_expect_success TTY 'some commands use a pager' '
+ rm -f paginated.out ||
+ cleanup_fail &&
+
test_terminal git log &&
test -e paginated.out
'
-rm -f paginated.out
test_expect_success TTY 'some commands do not use a pager' '
+ rm -f paginated.out ||
+ cleanup_fail &&
+
test_terminal git rev-list HEAD &&
! test -e paginated.out
'
-rm -f paginated.out
test_expect_success 'no pager when stdout is a pipe' '
+ rm -f paginated.out ||
+ cleanup_fail &&
+
git log | cat &&
! test -e paginated.out
'
-rm -f paginated.out
test_expect_success 'no pager when stdout is a regular file' '
- git log > file &&
+ rm -f paginated.out ||
+ cleanup_fail &&
+
+ git log >file &&
! test -e paginated.out
'
-rm -f paginated.out
test_expect_success TTY 'git --paginate rev-list uses a pager' '
+ rm -f paginated.out ||
+ cleanup_fail &&
+
test_terminal git --paginate rev-list HEAD &&
test -e paginated.out
'
-rm -f file paginated.out
test_expect_success 'no pager even with --paginate when stdout is a pipe' '
+ rm -f file paginated.out ||
+ cleanup_fail &&
+
git --paginate log | cat &&
! test -e paginated.out
'
-rm -f paginated.out
test_expect_success TTY 'no pager with --no-pager' '
+ rm -f paginated.out ||
+ cleanup_fail &&
+
test_terminal git --no-pager log &&
! test -e paginated.out
'
@@ -86,88 +108,119 @@ test_expect_success TTY 'no pager with --no-pager' '
# A colored commit log will begin with an appropriate ANSI escape
# for the first color; the text "commit" comes later.
colorful() {
- read firstline < $1
+ read firstline <$1
! expr "$firstline" : "^[a-zA-Z]" >/dev/null
}
-rm -f colorful.log colorless.log
test_expect_success 'tests can detect color' '
- git log --no-color > colorless.log &&
- git log --color > colorful.log &&
+ rm -f colorful.log colorless.log ||
+ cleanup_fail &&
+
+ git log --no-color >colorless.log &&
+ git log --color >colorful.log &&
! colorful colorless.log &&
colorful colorful.log
'
-rm -f colorless.log
-git config color.ui auto
test_expect_success 'no color when stdout is a regular file' '
- git log > colorless.log &&
+ rm -f colorless.log &&
+ git config color.ui auto ||
+ cleanup_fail &&
+
+ git log >colorless.log &&
! colorful colorless.log
'
-rm -f paginated.out
-git config color.ui auto
test_expect_success TTY 'color when writing to a pager' '
- TERM=vt100 test_terminal git log &&
+ rm -f paginated.out &&
+ git config color.ui auto ||
+ cleanup_fail &&
+
+ (
+ TERM=vt100 &&
+ export TERM &&
+ test_terminal git log
+ ) &&
colorful paginated.out
'
-rm -f colorful.log
-git config color.ui auto
test_expect_success 'color when writing to a file intended for a pager' '
- TERM=vt100 GIT_PAGER_IN_USE=true git log > colorful.log &&
+ rm -f colorful.log &&
+ git config color.ui auto ||
+ cleanup_fail &&
+
+ (
+ TERM=vt100 &&
+ GIT_PAGER_IN_USE=true &&
+ export TERM GIT_PAGER_IN_USE &&
+ git log >colorful.log
+ ) &&
colorful colorful.log
'
-unset PAGER GIT_PAGER
-git config --unset core.pager
test_expect_success 'determine default pager' '
+ unset PAGER GIT_PAGER &&
+ test_might_fail git config --unset core.pager ||
+ cleanup_fail &&
+
less=$(git var GIT_PAGER) &&
test -n "$less"
'
-if expr "$less" : '^[a-z]*$' > /dev/null && test_have_prereq TTY
+if expr "$less" : '^[a-z][a-z]*$' >/dev/null && test_have_prereq TTY
then
test_set_prereq SIMPLEPAGER
fi
-unset PAGER GIT_PAGER
-git config --unset core.pager
-rm -f default_pager_used
test_expect_success SIMPLEPAGER 'default pager is used by default' '
- cat > $less <<-EOF &&
- #!$SHELL_PATH
- wc > default_pager_used
+ unset PAGER GIT_PAGER &&
+ test_might_fail git config --unset core.pager &&
+ rm -f default_pager_used ||
+ cleanup_fail &&
+
+ cat >$less <<-\EOF &&
+ #!/bin/sh
+ wc >default_pager_used
EOF
chmod +x $less &&
- PATH=.:$PATH test_terminal git log &&
+ (
+ PATH=.:$PATH &&
+ export PATH &&
+ test_terminal git log
+ ) &&
test -e default_pager_used
'
-unset GIT_PAGER
-git config --unset core.pager
-rm -f PAGER_used
test_expect_success TTY 'PAGER overrides default pager' '
- PAGER="wc > PAGER_used" &&
+ unset GIT_PAGER &&
+ test_might_fail git config --unset core.pager &&
+ rm -f PAGER_used ||
+ cleanup_fail &&
+
+ PAGER="wc >PAGER_used" &&
export PAGER &&
test_terminal git log &&
test -e PAGER_used
'
-unset GIT_PAGER
-rm -f core.pager_used
test_expect_success TTY 'core.pager overrides PAGER' '
+ unset GIT_PAGER &&
+ rm -f core.pager_used ||
+ cleanup_fail &&
+
PAGER=wc &&
export PAGER &&
- git config core.pager "wc > core.pager_used" &&
+ git config core.pager "wc >core.pager_used" &&
test_terminal git log &&
test -e core.pager_used
'
-rm -f GIT_PAGER_used
test_expect_success TTY 'GIT_PAGER overrides core.pager' '
+ rm -f GIT_PAGER_used ||
+ cleanup_fail &&
+
git config core.pager wc &&
- GIT_PAGER="wc > GIT_PAGER_used" &&
+ GIT_PAGER="wc >GIT_PAGER_used" &&
export GIT_PAGER &&
test_terminal git log &&
test -e GIT_PAGER_used
diff --git a/t/test-lib.sh b/t/test-lib.sh
index c582964..f807625 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -516,6 +516,22 @@ test_must_fail () {
test $? -gt 0 -a $? -le 129 -o $? -gt 192
}
+# Similar to test_must_fail, but tolerates success, too. This is
+# meant to be used in contexts like:
+#
+# test_expect_success 'some command works without configuration' '
+# test_might_fail git config --unset all.configuration &&
+# do something
+# '
+#
+# Writing "git config --unset all.configuration || :" would be wrong,
+# because we want to notice if it fails due to segv.
+
+test_might_fail () {
+ "$@"
+ test $? -ge 0 -a $? -le 129 -o $? -gt 192
+}
+
# test_cmp is a helper function to compare actual and expected output.
# You can use it like:
#
--
1.7.1.rc1.18.g08771.dirty
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH] t7006: guard cleanup with test_expect_success
2010-04-15 0:38 ` [PATCH] t7006: guard cleanup with test_expect_success Jonathan Nieder
@ 2010-04-15 0:56 ` Junio C Hamano
2010-04-15 1:27 ` Jonathan Nieder
0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2010-04-15 0:56 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Nguyễn Thái Ngọc Duy, Jeff King
Jonathan Nieder <jrnieder@gmail.com> writes:
> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> index d9202d5..62595ab 100755
> --- a/t/t7006-pager.sh
> +++ b/t/t7006-pager.sh
> @@ -4,17 +4,24 @@ test_description='Test automatic use of a pager.'
>
> . ./test-lib.sh
>
> -rm -f stdout_is_tty
> +cleanup_fail() {
> + echo >&2 cleanup failed
> + exit 1
> +}
I think you meant to say "false" or "(exit 1)" here. To see why...
> test_expect_success 'set up terminal for tests' '
> + rm -f stdout_is_tty ||
> + cleanup_fail &&
> +
... try your patch with "rm -f stdout_is_tty" replaced with "(exit 1)" to
see how your cleanup_fail behaves.
Other than that I like the general idea, especially "might-fail" is cute.
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] t7006: guard cleanup with test_expect_success
2010-04-15 0:56 ` Junio C Hamano
@ 2010-04-15 1:27 ` Jonathan Nieder
0 siblings, 0 replies; 30+ messages in thread
From: Jonathan Nieder @ 2010-04-15 1:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Nguyễn Thái Ngọc Duy, Jeff King
Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
[...]
>> +cleanup_fail() {
>> + echo >&2 cleanup failed
>> + exit 1
>> +}
>
> I think you meant to say "false" or "(exit 1)" here. To see why...
[...]
> ... try your patch with "rm -f stdout_is_tty" replaced with "(exit 1)" to
> see how your cleanup_fail behaves.
Good catch; thanks. Here’s a patch for squashing in case you would
like one.
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 62595ab..a3d0210 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -6,7 +6,7 @@ test_description='Test automatic use of a pager.'
cleanup_fail() {
echo >&2 cleanup failed
- exit 1
+ return 1
}
test_expect_success 'set up terminal for tests' '
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/9] t7006: test pager configuration for several git commands
2010-04-13 2:11 [PATCH/RFC 0/9] Setup cleanup, chapter one Jonathan Nieder
2010-04-13 2:13 ` [PATCH 1/9] t7006: GIT_DIR/config should be honored in subdirs of toplevel Jonathan Nieder
@ 2010-04-13 2:17 ` Jonathan Nieder
2010-04-13 6:34 ` Johannes Sixt
2010-04-14 1:26 ` [PATCH 2/9 v2] " Jonathan Nieder
2010-04-13 2:24 ` [PATCH 3/9] builtins: do not commit pager choice early Jonathan Nieder
` (8 subsequent siblings)
10 siblings, 2 replies; 30+ messages in thread
From: Jonathan Nieder @ 2010-04-13 2:17 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy, Jeff King, Junio C Hamano
Test choice of pager at several stages of repository setup. This
way we can have some (admittedly uninteresting) examples in mind when
considering future changes to the setup procedure.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
| 151 ++++++++++++++++++++++++++++-------------------------
1 files changed, 80 insertions(+), 71 deletions(-)
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 4f804ed..403c260 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -131,78 +131,87 @@ then
test_set_prereq SIMPLEPAGER
fi
-unset PAGER GIT_PAGER
-git config --unset core.pager
-rm -f default_pager_used
-test_expect_success SIMPLEPAGER 'default pager is used by default' '
- cat > $less <<-EOF &&
- #!$SHELL_PATH
- wc > default_pager_used
- EOF
- chmod +x $less &&
- PATH=.:$PATH test_terminal git log &&
- test -e default_pager_used
-'
-
-unset GIT_PAGER
-git config --unset core.pager
-rm -f PAGER_used
-test_expect_success TTY 'PAGER overrides default pager' '
- PAGER="wc > PAGER_used" &&
- export PAGER &&
- test_terminal git log &&
- test -e PAGER_used
-'
-
-unset GIT_PAGER
-rm -f core.pager_used
-test_expect_success TTY 'core.pager overrides PAGER' '
- PAGER=wc &&
- export PAGER &&
- git config core.pager "wc > core.pager_used" &&
- test_terminal git log &&
- test -e core.pager_used
-'
-
-unset GIT_PAGER
-rm -f core.pager_used
-rm -fr sub
-test_expect_success TTY 'core.pager in subdir' '
- PAGER=wc &&
- stampname=$(pwd)/core.pager_used &&
- export PAGER stampname &&
- git config core.pager "wc > \"\$stampname\"" &&
- mkdir sub &&
- (
- cd sub &&
- test_terminal git log
- ) &&
- test -e "$stampname"
-'
+test_pager_choice() {
+ if test $# = 1
+ then
+ cmd=$1
+ full_cmd="test_terminal $cmd"
+ else
+ cmd=$2
+ full_cmd="test_must_fail test_terminal $cmd"
+ fi
-unset GIT_PAGER
-rm -f core.pager_used
-rm -fr sub
-test_expect_failure TTY 'core.pager in subdir with --paginate' '
- PAGER=wc &&
- stampname=$(pwd)/core.pager_used &&
- export PAGER stampname &&
- git config core.pager "wc > \"\$stampname\"" &&
- mkdir sub &&
- (
- cd sub &&
- test_terminal git --paginate log
- ) &&
- test -e "$stampname"
-'
+ case "$cmd" in
+ *-p*)
+ test_expect_expected=test_expect_failure
+ ;;
+ *)
+ test_expect_expected=test_expect_success
+ ;;
+ esac
+
+ unset PAGER GIT_PAGER
+ git config --unset core.pager
+ rm -f default_pager_used
+ test_expect_success SIMPLEPAGER "$cmd - default pager is used by default" "
+ cat > $less <<-\EOF &&
+ #!$SHELL_PATH
+ wc > default_pager_used
+ EOF
+ chmod +x $less &&
+ PATH=.:\$PATH $full_cmd &&
+ test -e default_pager_used
+ "
+
+ unset GIT_PAGER
+ git config --unset core.pager
+ rm -f PAGER_used
+ test_expect_success TTY "$cmd - PAGER overrides default pager" "
+ PAGER='wc > PAGER_used' &&
+ export PAGER &&
+ $full_cmd &&
+ test -e PAGER_used
+ "
+
+ unset GIT_PAGER
+ rm -f core.pager_used
+ test_expect_success TTY "$cmd - core.pager overrides PAGER" "
+ PAGER=wc &&
+ export PAGER &&
+ git config core.pager 'wc > core.pager_used' &&
+ $full_cmd &&
+ test -e core.pager_used
+ "
+
+ unset GIT_PAGER
+ rm -f core.pager_used
+ rm -fr sub
+ $test_expect_expected TTY "$cmd - core.pager in subdir" "
+ PAGER=wc &&
+ stampname=\$(pwd)/core.pager_used &&
+ export PAGER stampname &&
+ git config core.pager 'wc > \"\$stampname\"' &&
+ mkdir sub &&
+ (
+ cd sub &&
+ $full_cmd
+ ) &&
+ test -e \"\$stampname\"
+ "
+
+ rm -f GIT_PAGER_used
+ test_expect_success TTY "$cmd - GIT_PAGER overrides core.pager" "
+ git config core.pager wc &&
+ GIT_PAGER='wc > GIT_PAGER_used' &&
+ export GIT_PAGER &&
+ $full_cmd &&
+ test -e GIT_PAGER_used
+ "
+}
-rm -f GIT_PAGER_used
-test_expect_success TTY 'GIT_PAGER overrides core.pager' '
- git config core.pager wc &&
- GIT_PAGER="wc > GIT_PAGER_used" &&
- export GIT_PAGER &&
- test_terminal git log &&
- test -e GIT_PAGER_used
-'
+test_pager_choice 'git log'
+test_pager_choice 'git -p log'
+test_pager_choice test_must_fail 'git -p'
+test_pager_choice test_must_fail 'git -p nonsense'
test_done
--
1.7.0.4
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 2/9] t7006: test pager configuration for several git commands
2010-04-13 2:17 ` [PATCH 2/9] t7006: test pager configuration for several git commands Jonathan Nieder
@ 2010-04-13 6:34 ` Johannes Sixt
2010-04-13 22:07 ` Jonathan Nieder
2010-04-14 1:26 ` [PATCH 2/9 v2] " Jonathan Nieder
1 sibling, 1 reply; 30+ messages in thread
From: Johannes Sixt @ 2010-04-13 6:34 UTC (permalink / raw)
To: Jonathan Nieder
Cc: git, Nguyễn Thái Ngọc Duy, Jeff King,
Junio C Hamano
Am 4/13/2010 4:17, schrieb Jonathan Nieder:
> +test_pager_choice() {
> +...
> + test_expect_success SIMPLEPAGER "$cmd - default pager is used by default" "
> + cat > $less <<-\EOF &&
> + #!$SHELL_PATH
This shell script is simple enough that you can use '#!/bin/sh' and avoid
$ expansion where one must ask whether it needs quoting or not.
> + wc > default_pager_used
> + EOF
> +...
> + "
> +...
> + test_expect_success TTY "$cmd - PAGER overrides default pager" "
> +...
> + $test_expect_expected TTY "$cmd - core.pager in subdir" "
> +...
> + test_expect_success TTY "$cmd - GIT_PAGER overrides core.pager" "
> +...
>
> +test_pager_choice 'git log'
> +test_pager_choice 'git -p log'
> +test_pager_choice test_must_fail 'git -p'
> +test_pager_choice test_must_fail 'git -p nonsense'
The intents of the test cases would be much easier to follow and to review
if this were written as, for example:
test_default_pager 'git log'
test_PAGER_overrides 'git log'
test_core_pager_overrides 'git log'
test_core_pager_subdir 'git log'
test_GIT_PAGER_overrides 'git log'
test_default_pager 'git -p log'
test_PAGER_overrides 'git -p log'
test_core_pager_overrides 'git -p log'
test_core_pager_subdir 'git -p log' fails
test_GIT_PAGER_overrides 'git -p log'
test_default_pager 'test_must_fail git -p'
test_PAGER_overrides 'test_must_fail git -p'
test_core_pager_overrides 'test_must_fail git -p'
test_core_pager_subdir 'test_must_fail git -p' fails
test_GIT_PAGER_overrides 'test_must_fail git -p'
test_default_pager 'test_must_fail git -p nonsense'
test_PAGER_overrides 'test_must_fail git -p nonsense'
test_core_pager_overrides 'test_must_fail git -p nonsense'
test_core_pager_subdir 'test_must_fail git -p nonsense' fails
test_GIT_PAGER_overrides 'test_must_fail git -p nonsense'
the point being that it is less factorized (not the exact syntax).
-- Hannes
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 2/9] t7006: test pager configuration for several git commands
2010-04-13 6:34 ` Johannes Sixt
@ 2010-04-13 22:07 ` Jonathan Nieder
0 siblings, 0 replies; 30+ messages in thread
From: Jonathan Nieder @ 2010-04-13 22:07 UTC (permalink / raw)
To: Johannes Sixt
Cc: git, Nguyễn Thái Ngọc Duy, Jeff King,
Junio C Hamano
Johannes Sixt wrote:
> The intents of the test cases would be much easier to follow and to review
> if this were written as, for example:
>
> test_default_pager 'git log'
> test_PAGER_overrides 'git log'
> test_core_pager_overrides 'git log'
> test_core_pager_subdir 'git log'
> test_GIT_PAGER_overrides 'git log'
> test_default_pager 'git -p log'
[...]
Good idea. This would also make the expected result much easier to
change test-by-test. Will do, thanks.
Jonathan
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 2/9 v2] t7006: test pager configuration for several git commands
2010-04-13 2:17 ` [PATCH 2/9] t7006: test pager configuration for several git commands Jonathan Nieder
2010-04-13 6:34 ` Johannes Sixt
@ 2010-04-14 1:26 ` Jonathan Nieder
1 sibling, 0 replies; 30+ messages in thread
From: Jonathan Nieder @ 2010-04-14 1:26 UTC (permalink / raw)
To: git
Cc: Nguyễn Thái Ngọc Duy, Jeff King, Junio C Hamano,
Johannes Sixt
Test choice of pager at several stages of repository setup. Some
patches under consideration will produce different results for
different commands, so it should be useful to have examples like this
in mind to illustrate their effect.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Improved-by: Johannes Sixt <j6t@kdbg.org>
---
Changes from v1:
- Use /bin/sh instead of $SHELL_PATH to simplify quoting issues
- Split the test_pager_choice function into single-test pieces to
make the test easier to understand.
I’ll resend the patches 3 and 9 with corresponding adjustments in a
moment.
Thanks to Hannes for the feedback.
| 192 +++++++++++++++++++++++++++++++++++-------------------
1 files changed, 124 insertions(+), 68 deletions(-)
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 4f804ed..d5f8a18 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -131,78 +131,134 @@ then
test_set_prereq SIMPLEPAGER
fi
-unset PAGER GIT_PAGER
-git config --unset core.pager
-rm -f default_pager_used
-test_expect_success SIMPLEPAGER 'default pager is used by default' '
- cat > $less <<-EOF &&
- #!$SHELL_PATH
- wc > default_pager_used
- EOF
- chmod +x $less &&
- PATH=.:$PATH test_terminal git log &&
- test -e default_pager_used
-'
+# Tests for pager choice.
-unset GIT_PAGER
-git config --unset core.pager
-rm -f PAGER_used
-test_expect_success TTY 'PAGER overrides default pager' '
- PAGER="wc > PAGER_used" &&
- export PAGER &&
- test_terminal git log &&
- test -e PAGER_used
-'
+# Usage:
+# parse_args expect_(success|failure) [test_must_fail] git whatever
+# $test_expectation "$cmd - behaves well" "
+# ...
+# $full_command &&
+# ...
+# "
+parse_args() {
+ test_expectation="test_$1"
+ shift
+ if test "$1" = test_must_fail
+ then
+ full_command="test_must_fail test_terminal "
+ shift
+ else
+ full_command="test_terminal "
+ fi
+ cmd=$1
+ full_command="$full_command $1"
+}
-unset GIT_PAGER
-rm -f core.pager_used
-test_expect_success TTY 'core.pager overrides PAGER' '
- PAGER=wc &&
- export PAGER &&
- git config core.pager "wc > core.pager_used" &&
- test_terminal git log &&
- test -e core.pager_used
-'
+# usage: test_default_pager expect_(success|failure) [test_must_fail] command
+test_default_pager() {
+ parse_args "$@"
-unset GIT_PAGER
-rm -f core.pager_used
-rm -fr sub
-test_expect_success TTY 'core.pager in subdir' '
- PAGER=wc &&
- stampname=$(pwd)/core.pager_used &&
- export PAGER stampname &&
- git config core.pager "wc > \"\$stampname\"" &&
- mkdir sub &&
- (
- cd sub &&
- test_terminal git log
- ) &&
- test -e "$stampname"
-'
+ unset PAGER GIT_PAGER
+ git config --unset core.pager
+ rm -f default_pager_used
+ $test_expectation SIMPLEPAGER "$cmd - default pager is used by default" "
+ cat >$less <<-\EOF &&
+ #!/bin/sh
+ wc > default_pager_used
+ EOF
+ chmod +x $less &&
+ (
+ PATH=.:$PATH &&
+ export PATH &&
+ $full_command
+ ) &&
+ test -e default_pager_used
+ "
+}
-unset GIT_PAGER
-rm -f core.pager_used
-rm -fr sub
-test_expect_failure TTY 'core.pager in subdir with --paginate' '
- PAGER=wc &&
- stampname=$(pwd)/core.pager_used &&
- export PAGER stampname &&
- git config core.pager "wc > \"\$stampname\"" &&
- mkdir sub &&
- (
- cd sub &&
- test_terminal git --paginate log
- ) &&
- test -e "$stampname"
-'
+test_PAGER_overrides() {
+ parse_args "$@"
-rm -f GIT_PAGER_used
-test_expect_success TTY 'GIT_PAGER overrides core.pager' '
- git config core.pager wc &&
- GIT_PAGER="wc > GIT_PAGER_used" &&
- export GIT_PAGER &&
- test_terminal git log &&
- test -e GIT_PAGER_used
-'
+ unset GIT_PAGER
+ git config --unset core.pager
+ rm -f PAGER_used
+ $test_expectation TTY "$cmd - PAGER overrides default pager" "
+ PAGER='wc > PAGER_used' &&
+ export PAGER &&
+ $full_command &&
+ test -e PAGER_used
+ "
+}
+
+test_core_pager_overrides() {
+ parse_args "$@"
+
+ unset GIT_PAGER
+ rm -f core.pager_used
+ $test_expectation TTY "$cmd - core.pager overrides PAGER" "
+ PAGER=wc &&
+ export PAGER &&
+ git config core.pager 'wc > core.pager_used' &&
+ $full_command &&
+ test -e core.pager_used
+ "
+}
+
+test_core_pager_subdir() {
+ parse_args "$@"
+
+ unset GIT_PAGER
+ rm -f core.pager_used
+ rm -fr sub
+ $test_expectation TTY "$cmd - core.pager from subdirectory" "
+ stampname=\$(pwd)/core.pager_used &&
+ PAGER=wc &&
+ export PAGER stampname &&
+ git config core.pager 'wc > \"\$stampname\"' &&
+ mkdir sub &&
+ (
+ cd sub &&
+ $full_command
+ ) &&
+ test -e \"\$stampname\"
+ "
+}
+
+test_GIT_PAGER_overrides() {
+ parse_args "$@"
+
+ rm -f GIT_PAGER_used
+ $test_expectation TTY "$cmd - GIT_PAGER overrides core.pager" "
+ git config core.pager wc &&
+ GIT_PAGER='wc > GIT_PAGER_used' &&
+ export GIT_PAGER &&
+ $full_command &&
+ test -e GIT_PAGER_used
+ "
+}
+
+test_default_pager expect_success 'git log'
+test_PAGER_overrides expect_success 'git log'
+test_core_pager_overrides expect_success 'git log'
+test_core_pager_subdir expect_success 'git log'
+test_GIT_PAGER_overrides expect_success 'git log'
+
+test_default_pager expect_success 'git -p log'
+test_PAGER_overrides expect_success 'git -p log'
+test_core_pager_overrides expect_success 'git -p log'
+test_core_pager_subdir expect_failure 'git -p log'
+test_GIT_PAGER_overrides expect_success 'git -p log'
+
+test_default_pager expect_success test_must_fail 'git -p'
+test_PAGER_overrides expect_success test_must_fail 'git -p'
+test_core_pager_overrides expect_success test_must_fail 'git -p'
+test_core_pager_subdir expect_failure test_must_fail 'git -p'
+test_GIT_PAGER_overrides expect_success test_must_fail 'git -p'
+
+test_default_pager expect_success test_must_fail 'git -p nonsense'
+test_PAGER_overrides expect_success test_must_fail 'git -p nonsense'
+test_core_pager_overrides expect_success test_must_fail 'git -p nonsense'
+test_core_pager_subdir expect_failure test_must_fail 'git -p nonsense'
+test_GIT_PAGER_overrides expect_success test_must_fail 'git -p nonsense'
test_done
--
1.7.0.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 3/9] builtins: do not commit pager choice early
2010-04-13 2:11 [PATCH/RFC 0/9] Setup cleanup, chapter one Jonathan Nieder
2010-04-13 2:13 ` [PATCH 1/9] t7006: GIT_DIR/config should be honored in subdirs of toplevel Jonathan Nieder
2010-04-13 2:17 ` [PATCH 2/9] t7006: test pager configuration for several git commands Jonathan Nieder
@ 2010-04-13 2:24 ` Jonathan Nieder
2010-04-14 2:17 ` [PATCH 3/9 v2] " Jonathan Nieder
2010-04-13 2:25 ` [PATCH 4/9] t7006: test pager.<cmd> configuration Jonathan Nieder
` (7 subsequent siblings)
10 siblings, 1 reply; 30+ messages in thread
From: Jonathan Nieder @ 2010-04-13 2:24 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy, Jeff King, Junio C Hamano
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
If git is passed the --paginate option, committing the pager choice
will require setting up a pager, which requires access to repository
for the core.pager configuration.
After handle_options() is called, the repository has not been searched
for yet. Unless GIT_DIR or GIT_CONFIG is set, attempting to access
the configuration at this point results in git_dir being set to .git,
which is almost certainly not what was wanted. If a git command is
run from a subdirectory of the toplevel directory of a git repository
and the --paginate option is supplied, the core.pager setting from
that repository is not being respected.
There are several possible code paths after handle_options() and
commit_pager_choice() are called:
1. list_common_cmds_help() is a printout of 29 lines. This can
benefit from pagination, so do commit the pager choice before
writing this output.
Since ‘git’ without subcommand has no other reason to access the
repository, ideally this would not involve accessing the
repository-specific configuration file. But for now, it is simpler
to commit the pager choice using the funny code that would notice
whatever repository happens to be at .git. That this accesses a
repository if it happens to be very convenient to is a bug but not
a very important one.
2. run_argv() already commits pager choice inside run_builtin() if a
command is found. If the relevant git subcommand searches for a
git directory, not commiting to a pager before then means the
core.pager setting from the correct $GIT_DIR/config will be
respected.
3. help_unknown_cmd() prints out a few lines to stderr. It is not
important to paginate this, so don’t.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Duy’s version did not allow paginating list_common_cmds_help() output.
I have an idea for fixing the setup procedure for that later, but one
thing at a time. In other words, this is analogous to a lock pushdown
in a way: the immediate goal is not to eliminate problems but to reduce
their scope.
git.c | 2 +-
| 65 +++++++++++++++++++++++++++++++++++++----------------
2 files changed, 46 insertions(+), 21 deletions(-)
diff --git a/git.c b/git.c
index 6bae305..32720e5 100644
--- a/git.c
+++ b/git.c
@@ -502,12 +502,12 @@ int main(int argc, const char **argv)
argv++;
argc--;
handle_options(&argv, &argc, NULL);
- commit_pager_choice();
if (argc > 0) {
if (!prefixcmp(argv[0], "--"))
argv[0] += 2;
} else {
/* The user didn't specify a command; give them help */
+ commit_pager_choice();
printf("usage: %s\n\n", git_usage_string);
list_common_cmds_help();
printf("\n%s\n", git_more_info_string);
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 403c260..944e830 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -132,23 +132,28 @@ then
fi
test_pager_choice() {
- if test $# = 1
+ wrapper=test_terminal
+
+ if test "$1" = test_must_fail
then
- cmd=$1
- full_cmd="test_terminal $cmd"
- else
- cmd=$2
- full_cmd="test_must_fail test_terminal $cmd"
+ wrapper="test_must_fail $wrapper"
+ shift
fi
- case "$cmd" in
- *-p*)
- test_expect_expected=test_expect_failure
- ;;
- *)
- test_expect_expected=test_expect_success
- ;;
- esac
+ cmd=$1
+ toplevel_expectation=${2:-success}
+ use_repository_config="${3:-yes}"
+
+ full_cmd="$wrapper $cmd"
+ test_expectation="test_expect_$toplevel_expectation"
+ if test "$use_repository_config" = no
+ then
+ if_want_local_config='! '
+ used_if_wanted='is not used'
+ else
+ if_want_local_config=
+ used_if_wanted='overrides PAGER'
+ fi
unset PAGER GIT_PAGER
git config --unset core.pager
@@ -175,18 +180,18 @@ test_pager_choice() {
unset GIT_PAGER
rm -f core.pager_used
- test_expect_success TTY "$cmd - core.pager overrides PAGER" "
+ $test_expectation TTY "$cmd - core.pager ${used_if_wanted}" "
PAGER=wc &&
export PAGER &&
git config core.pager 'wc > core.pager_used' &&
$full_cmd &&
- test -e core.pager_used
+ ${if_want_local_config}test -e core.pager_used
"
unset GIT_PAGER
rm -f core.pager_used
rm -fr sub
- $test_expect_expected TTY "$cmd - core.pager in subdir" "
+ test_expect_success TTY "$cmd - core.pager ${used_if_wanted} from subdirectory" "
PAGER=wc &&
stampname=\$(pwd)/core.pager_used &&
export PAGER stampname &&
@@ -196,7 +201,7 @@ test_pager_choice() {
cd sub &&
$full_cmd
) &&
- test -e \"\$stampname\"
+ ${if_want_local_config}test -e \"\$stampname\"
"
rm -f GIT_PAGER_used
@@ -209,9 +214,29 @@ test_pager_choice() {
"
}
+doesnt_paginate() {
+ if test $# = 1
+ then
+ cmd=$1
+ full_cmd="test_terminal $cmd"
+ else
+ cmd=$2
+ full_cmd="test_must_fail test_terminal $cmd"
+ fi
+
+ rm -f GIT_PAGER_used
+ test_expect_success TTY "no pager for '$cmd'" "
+ GIT_PAGER='wc > GIT_PAGER_used' &&
+ export GIT_PAGER
+ $full_cmd &&
+ ! test -e GIT_PAGER_used
+ "
+}
+
test_pager_choice 'git log'
test_pager_choice 'git -p log'
-test_pager_choice test_must_fail 'git -p'
-test_pager_choice test_must_fail 'git -p nonsense'
+test_pager_choice test_must_fail 'git -p' failure no
+
+doesnt_paginate test_must_fail 'git -p nonsense'
test_done
--
1.7.0.4
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 3/9 v2] builtins: do not commit pager choice early
2010-04-13 2:24 ` [PATCH 3/9] builtins: do not commit pager choice early Jonathan Nieder
@ 2010-04-14 2:17 ` Jonathan Nieder
0 siblings, 0 replies; 30+ messages in thread
From: Jonathan Nieder @ 2010-04-14 2:17 UTC (permalink / raw)
To: git
Cc: Nguyễn Thái Ngọc Duy, Jeff King, Junio C Hamano,
Johannes Sixt
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
If git is passed the --paginate option, committing the pager choice
will require setting up a pager, which requires access to repository
for the core.pager configuration.
After handle_options() is called, the repository has not been searched
for yet. Unless GIT_DIR or GIT_CONFIG is set, attempting to access
the configuration at this point results in git_dir being set to .git,
which is almost certainly not what was wanted. If a git command is
run from a subdirectory of the toplevel directory of a git repository
and the --paginate option is supplied, the core.pager setting from
that repository is not being respected.
There are several possible code paths after handle_options() and
commit_pager_choice() are called:
1. list_common_cmds_help() is a printout of 29 lines. This can
benefit from pagination, so do commit the pager choice before
writing this output.
Since ‘git’ without subcommand has no other reason to access the
repository, ideally this would not involve accessing the
repository-specific configuration file. But for now, it is simpler
to commit the pager choice using the funny code that would notice
whatever repository happens to be at .git. That this accesses a
repository if it happens to be very convenient is a bug but not
a very important one.
2. run_argv() already commits pager choice inside run_builtin() if a
command is found. If the relevant git subcommand searches for a
git directory, not commiting to a pager before then means the
core.pager setting from the correct $GIT_DIR/config will be
respected.
3. help_unknown_cmd() prints out a few lines to stderr. It is not
important to paginate this, so don’t.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Rebased on top of patch 2 v2
git.c | 2 +-
| 63 +++++++++++++++++++++++++++++++++++++++++++----------
2 files changed, 52 insertions(+), 13 deletions(-)
diff --git a/git.c b/git.c
index 6bae305..32720e5 100644
--- a/git.c
+++ b/git.c
@@ -502,12 +502,12 @@ int main(int argc, const char **argv)
argv++;
argc--;
handle_options(&argv, &argc, NULL);
- commit_pager_choice();
if (argc > 0) {
if (!prefixcmp(argv[0], "--"))
argv[0] += 2;
} else {
/* The user didn't specify a command; give them help */
+ commit_pager_choice();
printf("usage: %s\n\n", git_usage_string);
list_common_cmds_help();
printf("\n%s\n", git_more_info_string);
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index d5f8a18..a240b15 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -190,27 +190,58 @@ test_PAGER_overrides() {
"
}
+parse_ifwanted_arg() {
+ if test "$1" = yes
+ then
+ if_want_local_config=
+ used_if_wanted='overrides PAGER'
+ else
+ if_want_local_config='! '
+ used_if_wanted='is not used'
+ fi
+}
+
test_core_pager_overrides() {
+ test_core_pager yes "$@"
+}
+
+test_local_config_ignored() {
+ test_core_pager no "$@"
+}
+
+test_core_pager() {
+ parse_ifwanted_arg "$1"
+ shift
parse_args "$@"
unset GIT_PAGER
rm -f core.pager_used
- $test_expectation TTY "$cmd - core.pager overrides PAGER" "
+ $test_expectation TTY "$cmd - core.pager $used_if_wanted" "
PAGER=wc &&
export PAGER &&
git config core.pager 'wc > core.pager_used' &&
$full_command &&
- test -e core.pager_used
+ ${if_want_local_config}test -e core.pager_used
"
}
test_core_pager_subdir() {
+ test_pager_subdir_helper yes "$@"
+}
+
+test_no_local_config_subdir() {
+ test_pager_subdir_helper no "$@"
+}
+
+test_pager_subdir_helper() {
+ parse_ifwanted_arg "$1"
+ shift
parse_args "$@"
unset GIT_PAGER
rm -f core.pager_used
rm -fr sub
- $test_expectation TTY "$cmd - core.pager from subdirectory" "
+ $test_expectation TTY "$cmd - core.pager $used_if_wanted from subdirectory" "
stampname=\$(pwd)/core.pager_used &&
PAGER=wc &&
export PAGER stampname &&
@@ -220,7 +251,7 @@ test_core_pager_subdir() {
cd sub &&
$full_command
) &&
- test -e \"\$stampname\"
+ ${if_want_local_config}test -e \"\$stampname\"
"
}
@@ -237,6 +268,18 @@ test_GIT_PAGER_overrides() {
"
}
+test_doesnt_paginate() {
+ parse_args "$@"
+
+ rm -f GIT_PAGER_used
+ $test_expectation TTY "no pager for '$cmd'" "
+ GIT_PAGER='wc > GIT_PAGER_used' &&
+ export GIT_PAGER &&
+ $full_command &&
+ ! test -e GIT_PAGER_used
+ "
+}
+
test_default_pager expect_success 'git log'
test_PAGER_overrides expect_success 'git log'
test_core_pager_overrides expect_success 'git log'
@@ -246,19 +289,15 @@ test_GIT_PAGER_overrides expect_success 'git log'
test_default_pager expect_success 'git -p log'
test_PAGER_overrides expect_success 'git -p log'
test_core_pager_overrides expect_success 'git -p log'
-test_core_pager_subdir expect_failure 'git -p log'
+test_core_pager_subdir expect_success 'git -p log'
test_GIT_PAGER_overrides expect_success 'git -p log'
test_default_pager expect_success test_must_fail 'git -p'
test_PAGER_overrides expect_success test_must_fail 'git -p'
-test_core_pager_overrides expect_success test_must_fail 'git -p'
-test_core_pager_subdir expect_failure test_must_fail 'git -p'
+test_local_config_ignored expect_failure test_must_fail 'git -p'
+test_no_local_config_subdir expect_success test_must_fail 'git -p'
test_GIT_PAGER_overrides expect_success test_must_fail 'git -p'
-test_default_pager expect_success test_must_fail 'git -p nonsense'
-test_PAGER_overrides expect_success test_must_fail 'git -p nonsense'
-test_core_pager_overrides expect_success test_must_fail 'git -p nonsense'
-test_core_pager_subdir expect_failure test_must_fail 'git -p nonsense'
-test_GIT_PAGER_overrides expect_success test_must_fail 'git -p nonsense'
+test_doesnt_paginate expect_success test_must_fail 'git -p nonsense'
test_done
--
1.7.0.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 4/9] t7006: test pager.<cmd> configuration
2010-04-13 2:11 [PATCH/RFC 0/9] Setup cleanup, chapter one Jonathan Nieder
` (2 preceding siblings ...)
2010-04-13 2:24 ` [PATCH 3/9] builtins: do not commit pager choice early Jonathan Nieder
@ 2010-04-13 2:25 ` Jonathan Nieder
2010-04-14 2:19 ` [PATCH 4/9 v2] " Jonathan Nieder
2010-04-13 2:27 ` [PATCH 5/9] builtin: introduce startup_info struct Jonathan Nieder
` (6 subsequent siblings)
10 siblings, 1 reply; 30+ messages in thread
From: Jonathan Nieder @ 2010-04-13 2:25 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy, Jeff King, Junio C Hamano
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
| 20 ++++++++++++++++++++
1 files changed, 20 insertions(+), 0 deletions(-)
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 944e830..237a689 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -71,6 +71,22 @@ test_expect_success TTY 'git --paginate rev-list uses a pager' '
test -e paginated.out
'
+rm -f paginated.out
+test_expect_success TTY 'git rev-list uses a pager if configured to' '
+ git config pager.rev-list true &&
+ test_terminal git rev-list HEAD &&
+ test -e paginated.out
+'
+git config --unset pager.rev-list
+
+rm -f paginated.out
+test_expect_failure TTY 'git config uses a pager if configured to' '
+ git config pager.config true &&
+ test_terminal git config --list &&
+ test -e paginated.out
+'
+git --no-pager config --unset pager.config
+
rm -f file paginated.out
test_expect_success 'no pager even with --paginate when stdout is a pipe' '
git --paginate log | cat &&
@@ -239,4 +255,8 @@ test_pager_choice test_must_fail 'git -p' failure no
doesnt_paginate test_must_fail 'git -p nonsense'
+doesnt_paginate 'git rev-list HEAD'
+git config pager.rev-list true
+test_pager_choice 'git rev-list HEAD'
+
test_done
--
1.7.0.4
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 4/9 v2] t7006: test pager.<cmd> configuration
2010-04-13 2:25 ` [PATCH 4/9] t7006: test pager.<cmd> configuration Jonathan Nieder
@ 2010-04-14 2:19 ` Jonathan Nieder
0 siblings, 0 replies; 30+ messages in thread
From: Jonathan Nieder @ 2010-04-14 2:19 UTC (permalink / raw)
To: git
Cc: Nguyễn Thái Ngọc Duy, Jeff King, Junio C Hamano,
Johannes Sixt
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Rebased against patch 3 v2.
| 26 ++++++++++++++++++++++++++
1 files changed, 26 insertions(+), 0 deletions(-)
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index a240b15..97ca5d6 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -71,6 +71,22 @@ test_expect_success TTY 'git --paginate rev-list uses a pager' '
test -e paginated.out
'
+rm -f paginated.out
+test_expect_success TTY 'git rev-list uses a pager if configured to' '
+ git config pager.rev-list true &&
+ test_terminal git rev-list HEAD &&
+ test -e paginated.out
+'
+git config --unset pager.rev-list
+
+rm -f paginated.out
+test_expect_failure TTY 'git config uses a pager if configured to' '
+ git config pager.config true &&
+ test_terminal git config --list &&
+ test -e paginated.out
+'
+git --no-pager config --unset pager.config
+
rm -f file paginated.out
test_expect_success 'no pager even with --paginate when stdout is a pipe' '
git --paginate log | cat &&
@@ -300,4 +316,14 @@ test_GIT_PAGER_overrides expect_success test_must_fail 'git -p'
test_doesnt_paginate expect_success test_must_fail 'git -p nonsense'
+test_doesnt_paginate expect_success 'git rev-list HEAD'
+test_expect_success 'set up rev-list to use a pager' '
+ git config pager.rev-list true
+'
+test_default_pager expect_success 'git rev-list HEAD'
+test_PAGER_overrides expect_success 'git rev-list HEAD'
+test_core_pager_overrides expect_success 'git rev-list HEAD'
+test_core_pager_subdir expect_success 'git rev-list HEAD'
+test_GIT_PAGER_overrides expect_success 'git rev-list HEAD'
+
test_done
--
1.7.0.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 5/9] builtin: introduce startup_info struct
2010-04-13 2:11 [PATCH/RFC 0/9] Setup cleanup, chapter one Jonathan Nieder
` (3 preceding siblings ...)
2010-04-13 2:25 ` [PATCH 4/9] t7006: test pager.<cmd> configuration Jonathan Nieder
@ 2010-04-13 2:27 ` Jonathan Nieder
2010-04-13 2:28 ` [PATCH 6/9] builtin: remember whether repository was found Jonathan Nieder
` (5 subsequent siblings)
10 siblings, 0 replies; 30+ messages in thread
From: Jonathan Nieder @ 2010-04-13 2:27 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy, Jeff King, Junio C Hamano
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
The startup_info struct will collect information managed by the git
setup code, such as the prefix for relative paths passed on the
command line (i.e., path to the starting cwd from the toplevel of
the work tree) and whether a git repository has been found.
In other words, startup_info is intended to be a collection of global
variables representing results that were previously returned from
setup functions. Letting these values persist means there is more
flexibility in deciding when to run setup.
For now, the struct is empty.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Creation of struct split off from its population, mostly for
reviewability. Feel free to merge this patch with the next one
if you prefer.
cache.h | 5 +++++
environment.c | 1 +
git.c | 4 ++++
3 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/cache.h b/cache.h
index 5eb0573..1479824 100644
--- a/cache.h
+++ b/cache.h
@@ -1058,6 +1058,11 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix);
char *alias_lookup(const char *alias);
int split_cmdline(char *cmdline, const char ***argv);
+/* git.c */
+struct startup_info {
+};
+extern struct startup_info *startup_info;
+
/* builtin/merge.c */
int checkout_fast_forward(const unsigned char *from, const unsigned char *to);
diff --git a/environment.c b/environment.c
index 876c5e5..c36c902 100644
--- a/environment.c
+++ b/environment.c
@@ -52,6 +52,7 @@ enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
char *notes_ref_name;
int grafts_replace_parents = 1;
int core_apply_sparse_checkout;
+struct startup_info *startup_info;
/* Parallel index stat data preload? */
int core_preload_index = 0;
diff --git a/git.c b/git.c
index 32720e5..b1d51c9 100644
--- a/git.c
+++ b/git.c
@@ -13,6 +13,7 @@ const char git_usage_string[] =
const char git_more_info_string[] =
"See 'git help COMMAND' for more information on a specific command.";
+static struct startup_info git_startup_info;
static int use_pager = -1;
struct pager_config {
const char *cmd;
@@ -477,6 +478,9 @@ int main(int argc, const char **argv)
{
const char *cmd;
+ memset(&git_startup_info, 0, sizeof(git_startup_info));
+ startup_info = &git_startup_info;
+
cmd = git_extract_argv0_path(argv[0]);
if (!cmd)
cmd = "git-help";
--
1.7.0.4
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 6/9] builtin: remember whether repository was found
2010-04-13 2:11 [PATCH/RFC 0/9] Setup cleanup, chapter one Jonathan Nieder
` (4 preceding siblings ...)
2010-04-13 2:27 ` [PATCH 5/9] builtin: introduce startup_info struct Jonathan Nieder
@ 2010-04-13 2:28 ` Jonathan Nieder
2010-04-13 2:29 ` [PATCH 7/9] builtin: Support RUN_SETUP_GENTLY to set up repository early if found Jonathan Nieder
` (4 subsequent siblings)
10 siblings, 0 replies; 30+ messages in thread
From: Jonathan Nieder @ 2010-04-13 2:28 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy, Jeff King, Junio C Hamano
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Attempts to access gitdir are everywhere, even before cmd_*() is
called (in particular for alias handling, pager configuration, work
tree, and other core configuration). Because the repository has not
been found, repository-specific information (GIT_DIR/config) can not
be read. This can lead to obscure bugs.
The sooner we set up gitdir, the less trouble we may have to deal with.
Currently, the cmd_*() functions learn whether a repository was
found through the *nongit_ok return value to
setup_git_directory_gently(). If run_builtin() is to take care of
this sooner, then they will have to get it from somewhere else.
startup_info is a good place for this.
As a bonus, this information can be used to teach functions such as
git_config() to avoid trying to access a repository if none is
present.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
cache.h | 1 +
setup.c | 12 +++++++++++-
2 files changed, 12 insertions(+), 1 deletions(-)
diff --git a/cache.h b/cache.h
index 1479824..030ba01 100644
--- a/cache.h
+++ b/cache.h
@@ -1060,6 +1060,7 @@ int split_cmdline(char *cmdline, const char ***argv);
/* git.c */
struct startup_info {
+ int have_repository;
};
extern struct startup_info *startup_info;
diff --git a/setup.c b/setup.c
index 5716d90..3b4ff09 100644
--- a/setup.c
+++ b/setup.c
@@ -315,7 +315,7 @@ const char *read_gitfile_gently(const char *path)
* We cannot decide in this function whether we are in the work tree or
* not, since the config can only be read _after_ this function was called.
*/
-const char *setup_git_directory_gently(int *nongit_ok)
+static const char *setup_git_directory_gently_1(int *nongit_ok)
{
const char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);
const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);
@@ -443,6 +443,16 @@ const char *setup_git_directory_gently(int *nongit_ok)
return cwd + offset;
}
+const char *setup_git_directory_gently(int *nongit_ok)
+{
+ const char *prefix;
+
+ prefix = setup_git_directory_gently_1(nongit_ok);
+ if (startup_info)
+ startup_info->have_repository = !nongit_ok || !*nongit_ok;
+ return prefix;
+}
+
int git_config_perm(const char *var, const char *value)
{
int i;
--
1.7.0.4
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 7/9] builtin: Support RUN_SETUP_GENTLY to set up repository early if found
2010-04-13 2:11 [PATCH/RFC 0/9] Setup cleanup, chapter one Jonathan Nieder
` (5 preceding siblings ...)
2010-04-13 2:28 ` [PATCH 6/9] builtin: remember whether repository was found Jonathan Nieder
@ 2010-04-13 2:29 ` Jonathan Nieder
2010-04-13 2:30 ` [PATCH 8/9] builtin: check pager.<cmd> configuration if RUN_SETUP_GENTLY is used Jonathan Nieder
` (3 subsequent siblings)
10 siblings, 0 replies; 30+ messages in thread
From: Jonathan Nieder @ 2010-04-13 2:29 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy, Jeff King, Junio C Hamano
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Attempts to access gitdir are everywhere, even before cmd_*() is
called (examples: alias handling, pager configuration, work tree).
Because the repository has not been found, repository-specific
information (i.e., $GIT_DIR/config) can not be read. This leads to
obscure bugs.
So the sooner we setup gitdir, the less trouble we may have to deal with.
This patch sets up the infrastructure. Later patches will teach
particular commands to use it where applicable.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
git.c | 11 ++++++++---
1 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/git.c b/git.c
index b1d51c9..20e9bfc 100644
--- a/git.c
+++ b/git.c
@@ -220,13 +220,14 @@ static int handle_alias(int *argcp, const char ***argv)
const char git_version_string[] = GIT_VERSION;
-#define RUN_SETUP (1<<0)
-#define USE_PAGER (1<<1)
+#define RUN_SETUP (1<<0)
+#define USE_PAGER (1<<1)
/*
* require working tree to be present -- anything uses this needs
* RUN_SETUP for reading from the configuration file.
*/
-#define NEED_WORK_TREE (1<<2)
+#define NEED_WORK_TREE (1<<2)
+#define RUN_SETUP_GENTLY (1<<3)
struct cmd_struct {
const char *cmd;
@@ -245,6 +246,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
if (!help) {
if (p->option & RUN_SETUP)
prefix = setup_git_directory();
+ if (p->option & RUN_SETUP_GENTLY) {
+ int nongit_ok;
+ prefix = setup_git_directory_gently(&nongit_ok);
+ }
if (use_pager == -1 && p->option & RUN_SETUP)
use_pager = check_pager_config(p->cmd);
--
1.7.0.4
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 8/9] builtin: check pager.<cmd> configuration if RUN_SETUP_GENTLY is used
2010-04-13 2:11 [PATCH/RFC 0/9] Setup cleanup, chapter one Jonathan Nieder
` (6 preceding siblings ...)
2010-04-13 2:29 ` [PATCH 7/9] builtin: Support RUN_SETUP_GENTLY to set up repository early if found Jonathan Nieder
@ 2010-04-13 2:30 ` Jonathan Nieder
2010-04-13 5:29 ` Nguyen Thai Ngoc Duy
2010-04-13 10:12 ` Nguyen Thai Ngoc Duy
2010-04-13 2:31 ` [PATCH 9/9] config: run setup before commiting pager choice Jonathan Nieder
` (2 subsequent siblings)
10 siblings, 2 replies; 30+ messages in thread
From: Jonathan Nieder @ 2010-04-13 2:30 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy, Jeff King, Junio C Hamano
Eventually, all git commands should check their configuration at
start-up. For now, reading configuration before repository discovery
is dangerous because it could cause a pager.<cmd> setting from the
current repository to be neglected.
But for commands with RUN_SETUP or RUN_SETUP_GENTLY set, it is safe.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
git.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/git.c b/git.c
index 20e9bfc..56e93cf 100644
--- a/git.c
+++ b/git.c
@@ -251,7 +251,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
prefix = setup_git_directory_gently(&nongit_ok);
}
- if (use_pager == -1 && p->option & RUN_SETUP)
+ if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))
use_pager = check_pager_config(p->cmd);
if (use_pager == -1 && p->option & USE_PAGER)
use_pager = 1;
--
1.7.0.4
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 8/9] builtin: check pager.<cmd> configuration if RUN_SETUP_GENTLY is used
2010-04-13 2:30 ` [PATCH 8/9] builtin: check pager.<cmd> configuration if RUN_SETUP_GENTLY is used Jonathan Nieder
@ 2010-04-13 5:29 ` Nguyen Thai Ngoc Duy
2010-04-14 4:38 ` Jonathan Nieder
2010-04-13 10:12 ` Nguyen Thai Ngoc Duy
1 sibling, 1 reply; 30+ messages in thread
From: Nguyen Thai Ngoc Duy @ 2010-04-13 5:29 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Jeff King, Junio C Hamano
2010/4/13 Jonathan Nieder <jrnieder@gmail.com>:
> Eventually, all git commands should check their configuration at
> start-up. For now, reading configuration before repository discovery
> is dangerous because it could cause a pager.<cmd> setting from the
> current repository to be neglected.
>
> But for commands with RUN_SETUP or RUN_SETUP_GENTLY set, it is safe.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> git.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/git.c b/git.c
> index 20e9bfc..56e93cf 100644
> --- a/git.c
> +++ b/git.c
> @@ -251,7 +251,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
> prefix = setup_git_directory_gently(&nongit_ok);
> }
>
> - if (use_pager == -1 && p->option & RUN_SETUP)
> + if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))
> use_pager = check_pager_config(p->cmd);
> if (use_pager == -1 && p->option & USE_PAGER)
> use_pager = 1;
This still leaves a chance of going wrong: when user explicitly gives
"--paginate", use_pager will be 1, but commands like "git init" does
not have RUN_SETUP*. So when setup_pager is called later on, it will
mess things up. This could be solved completely (indeed I have a patch
under testing), but it would require unset_git_directory(), making
this series a bit longer :(
--
Duy
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 8/9] builtin: check pager.<cmd> configuration if RUN_SETUP_GENTLY is used
2010-04-13 5:29 ` Nguyen Thai Ngoc Duy
@ 2010-04-14 4:38 ` Jonathan Nieder
0 siblings, 0 replies; 30+ messages in thread
From: Jonathan Nieder @ 2010-04-14 4:38 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git, Jeff King, Junio C Hamano, Johannes Sixt
Nguyen Thai Ngoc Duy wrote:
> 2010/4/13 Jonathan Nieder <jrnieder@gmail.com>:
>> @@ -251,7 +251,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
[...]
>> - if (use_pager == -1 && p->option & RUN_SETUP)
>> + if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))
>> use_pager = check_pager_config(p->cmd);
>> if (use_pager == -1 && p->option & USE_PAGER)
>> use_pager = 1;
>
> This still leaves a chance of going wrong: when user explicitly gives
> "--paginate", use_pager will be 1, but commands like "git init" does
> not have RUN_SETUP*. So when setup_pager is called later on, it will
> mess things up.
I decided not to check_pager_config() unconditionally to avoid
breaking ‘git diff’ (especially ‘git diff --exit-code’). Maybe that
is too conservative; not sure. The commit message should be more
explicit, something like:
Eventually, all git commands should check their configuration at
start-up. For now, reading configuration before repository discovery
is dangerous because it could cause a pager.<cmd> setting from the
current repository to be neglected.
But for commands with RUN_SETUP or RUN_SETUP_GENTLY set, it is safe.
This will not affect commands like "git init" that cannot have
RUN_SETUP* set; they will have to be helped separately later. In
particular, commands such as "git diff" that use command-specific
options to decide whether to search for a repository and whether to
paginate output are outside the scope of this change.
Guiding principle: better to leave some bugs for later than risk
regressions.
> This could be solved completely (indeed I have a patch
> under testing), but it would require unset_git_directory(), making
> this series a bit longer :(
I agree; that’s the right way to do it. run_builtin() ought to be
relatively sure about whether a repo is going to be searched for. For
commands like diff and grep, I think it should be okay to search for a
repo unless --no-index is the first argument.
Thanks for your thoughtfulness.
Jonathan
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 8/9] builtin: check pager.<cmd> configuration if RUN_SETUP_GENTLY is used
2010-04-13 2:30 ` [PATCH 8/9] builtin: check pager.<cmd> configuration if RUN_SETUP_GENTLY is used Jonathan Nieder
2010-04-13 5:29 ` Nguyen Thai Ngoc Duy
@ 2010-04-13 10:12 ` Nguyen Thai Ngoc Duy
2010-04-14 5:06 ` Jonathan Nieder
1 sibling, 1 reply; 30+ messages in thread
From: Nguyen Thai Ngoc Duy @ 2010-04-13 10:12 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Jeff King, Junio C Hamano
2010/4/13 Jonathan Nieder <jrnieder@gmail.com>:
> diff --git a/git.c b/git.c
> index 20e9bfc..56e93cf 100644
> --- a/git.c
> +++ b/git.c
> @@ -251,7 +251,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
> prefix = setup_git_directory_gently(&nongit_ok);
> }
>
> - if (use_pager == -1 && p->option & RUN_SETUP)
> + if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))
> use_pager = check_pager_config(p->cmd);
> if (use_pager == -1 && p->option & USE_PAGER)
> use_pager = 1;
On the second look, there's another case, when RUN_SETUP_GENTLY is
set, but no repository is found. git_config() will pick
$(cwd)/.git/config if it exists. I guess it's OK for this series
because the true fix will require more changes.
--
Duy
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 8/9] builtin: check pager.<cmd> configuration if RUN_SETUP_GENTLY is used
2010-04-13 10:12 ` Nguyen Thai Ngoc Duy
@ 2010-04-14 5:06 ` Jonathan Nieder
2010-04-15 8:33 ` Nguyen Thai Ngoc Duy
0 siblings, 1 reply; 30+ messages in thread
From: Jonathan Nieder @ 2010-04-14 5:06 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git, Jeff King, Junio C Hamano, Johannes Sixt
Nguyen Thai Ngoc Duy wrote:
> there's another case, when RUN_SETUP_GENTLY is
> set, but no repository is found. git_config() will pick
> $(cwd)/.git/config if it exists. I guess it's OK for this series
> because the true fix will require more changes.
I have claimed before that it’s better to fix a few bugs now and
provide a solid foundation to build on than to try to do everything
right away. Sounds good in principle, but in practice we have to look
to the future to decide whether the foundation is strong enough.
So here are some ideas for future work (not necessarily in order).
- Teach remaining commands that need to search for a repository to
use RUN_SETUP_GENTLY, with appropriate exceptions where needed
(for --no-index, for example).
- Introduce unset_git_directory and the RUN_UNSETUP option (yes,
this needs a better name). Teach commands that work without a
git directory to use it (this should fix the init poisoned by
parent repository and aliased init problems).
- Teach git_config() to ignore the repository-specific configuration
if have_run_setup is true but have_repository is false.
- Teach git_attr() to ignore .git/info/attributes if have_run_setup
is true but have_repository is false.
- Teach git_config() to optionally die if have_run_setup is not true
and the setup_git_dir* to optionally die if have_run_setup is true.
test-lib.sh would enable this option.
Sane?
Jonathan
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 8/9] builtin: check pager.<cmd> configuration if RUN_SETUP_GENTLY is used
2010-04-14 5:06 ` Jonathan Nieder
@ 2010-04-15 8:33 ` Nguyen Thai Ngoc Duy
[not found] ` <20100415084925.GA14660@progeny.tock>
0 siblings, 1 reply; 30+ messages in thread
From: Nguyen Thai Ngoc Duy @ 2010-04-15 8:33 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Jeff King, Junio C Hamano, Johannes Sixt
On Wed, Apr 14, 2010 at 7:06 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> So here are some ideas for future work (not necessarily in order).
>
> - Teach remaining commands that need to search for a repository to
> use RUN_SETUP_GENTLY, with appropriate exceptions where needed
> (for --no-index, for example).
- Two patches "worktree setup: calculate prefix.." and "index-pack:
trust the prefix" may form a separate series. That would unblock
RUN_SETUP_GENTLY series.
> - Introduce unset_git_directory and the RUN_UNSETUP option (yes,
> this needs a better name). Teach commands that work without a
> git directory to use it (this should fix the init poisoned by
> parent repository and aliased init problems).
- Two patches that move enter_repo() to setup.c and fix it up can go
first. They should have no impact on system.
>
> - Teach git_config() to ignore the repository-specific configuration
> if have_run_setup is true but have_repository is false.
>
> - Teach git_attr() to ignore .git/info/attributes if have_run_setup
> is true but have_repository is false.
>
> - Teach git_config() to optionally die if have_run_setup is not true
> and the setup_git_dir* to optionally die if have_run_setup is true.
> test-lib.sh would enable this option.
>
- Introduce startup_info->prefix. I think that's a good change, but
it's independent to this series. The reason is, prefix can change
after setup_git_dir is called (i.e. setup_work_tree()).
- The fix for "git cmd -h" can make a separate series (4 patches)
- Finally the soft guard patch and assorted fixups to avoid warnings.
I'll reorder my tree and see if it looks good. Patches of this 9-patch
series will be untouched.
--
Duy
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 9/9] config: run setup before commiting pager choice
2010-04-13 2:11 [PATCH/RFC 0/9] Setup cleanup, chapter one Jonathan Nieder
` (7 preceding siblings ...)
2010-04-13 2:30 ` [PATCH 8/9] builtin: check pager.<cmd> configuration if RUN_SETUP_GENTLY is used Jonathan Nieder
@ 2010-04-13 2:31 ` Jonathan Nieder
2010-04-14 2:23 ` [PATCH 9/9 v2] " Jonathan Nieder
2010-04-13 3:08 ` [RFC/PATCH 00/46] nd/setup remainder for convenient reference Jonathan Nieder
2010-04-14 20:54 ` [PATCH/RFC 0/9] Setup cleanup, chapter one Junio C Hamano
10 siblings, 1 reply; 30+ messages in thread
From: Jonathan Nieder @ 2010-04-13 2:31 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy, Jeff King, Junio C Hamano
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
For the pager choice (and the choice to paginate in the first place)
to reflect the current repository configuration, the repository
needs to be searched for first.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
That’s the end of the mini-series. I hope you enjoyed reading it as
much as I did adapting it.
Looking forward to your thoughts,
Jonathan
builtin/config.c | 6 ++----
git.c | 4 ++--
| 16 ++++++++++------
3 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 4bc46b1..ecc8f87 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -326,11 +326,9 @@ static int get_colorbool(int print)
return get_colorbool_found ? 0 : 1;
}
-int cmd_config(int argc, const char **argv, const char *unused_prefix)
+int cmd_config(int argc, const char **argv, const char *prefix)
{
- int nongit;
char *value;
- const char *prefix = setup_git_directory_gently(&nongit);
config_exclusive_filename = getenv(CONFIG_ENVIRONMENT);
@@ -409,7 +407,7 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
}
else if (actions == ACTION_EDIT) {
check_argc(argc, 0, 0);
- if (!config_exclusive_filename && nongit)
+ if (!config_exclusive_filename && !startup_info->have_repository)
die("not in a git directory");
git_config(git_default_config, NULL);
launch_editor(config_exclusive_filename ?
diff --git a/git.c b/git.c
index 56e93cf..0a0d9eb 100644
--- a/git.c
+++ b/git.c
@@ -309,7 +309,7 @@ static void handle_internal_command(int argc, const char **argv)
{ "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE },
{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
{ "commit-tree", cmd_commit_tree, RUN_SETUP },
- { "config", cmd_config },
+ { "config", cmd_config, RUN_SETUP_GENTLY },
{ "count-objects", cmd_count_objects, RUN_SETUP },
{ "describe", cmd_describe, RUN_SETUP },
{ "diff", cmd_diff },
@@ -366,7 +366,7 @@ static void handle_internal_command(int argc, const char **argv)
{ "reflog", cmd_reflog, RUN_SETUP },
{ "remote", cmd_remote, RUN_SETUP },
{ "replace", cmd_replace, RUN_SETUP },
- { "repo-config", cmd_config },
+ { "repo-config", cmd_config, RUN_SETUP_GENTLY },
{ "rerere", cmd_rerere, RUN_SETUP },
{ "reset", cmd_reset, RUN_SETUP },
{ "rev-list", cmd_rev_list, RUN_SETUP },
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 237a689..e58b14d 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -80,7 +80,7 @@ test_expect_success TTY 'git rev-list uses a pager if configured to' '
git config --unset pager.rev-list
rm -f paginated.out
-test_expect_failure TTY 'git config uses a pager if configured to' '
+test_expect_success TTY 'git config uses a pager if configured to' '
git config pager.config true &&
test_terminal git config --list &&
test -e paginated.out
@@ -172,7 +172,7 @@ test_pager_choice() {
fi
unset PAGER GIT_PAGER
- git config --unset core.pager
+ git --no-pager config --unset core.pager
rm -f default_pager_used
test_expect_success SIMPLEPAGER "$cmd - default pager is used by default" "
cat > $less <<-\EOF &&
@@ -185,7 +185,7 @@ test_pager_choice() {
"
unset GIT_PAGER
- git config --unset core.pager
+ git --no-pager config --unset core.pager
rm -f PAGER_used
test_expect_success TTY "$cmd - PAGER overrides default pager" "
PAGER='wc > PAGER_used' &&
@@ -199,7 +199,7 @@ test_pager_choice() {
$test_expectation TTY "$cmd - core.pager ${used_if_wanted}" "
PAGER=wc &&
export PAGER &&
- git config core.pager 'wc > core.pager_used' &&
+ git --no-pager config core.pager 'wc > core.pager_used' &&
$full_cmd &&
${if_want_local_config}test -e core.pager_used
"
@@ -211,7 +211,7 @@ test_pager_choice() {
PAGER=wc &&
stampname=\$(pwd)/core.pager_used &&
export PAGER stampname &&
- git config core.pager 'wc > \"\$stampname\"' &&
+ git --no-pager config core.pager 'wc > \"\$stampname\"' &&
mkdir sub &&
(
cd sub &&
@@ -222,7 +222,7 @@ test_pager_choice() {
rm -f GIT_PAGER_used
test_expect_success TTY "$cmd - GIT_PAGER overrides core.pager" "
- git config core.pager wc &&
+ git --no-pager config core.pager wc &&
GIT_PAGER='wc > GIT_PAGER_used' &&
export GIT_PAGER &&
$full_cmd &&
@@ -259,4 +259,8 @@ doesnt_paginate 'git rev-list HEAD'
git config pager.rev-list true
test_pager_choice 'git rev-list HEAD'
+doesnt_paginate 'git config -l'
+git config pager.config true
+test_pager_choice 'git config -l'
+
test_done
--
1.7.0.4
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 9/9 v2] config: run setup before commiting pager choice
2010-04-13 2:31 ` [PATCH 9/9] config: run setup before commiting pager choice Jonathan Nieder
@ 2010-04-14 2:23 ` Jonathan Nieder
0 siblings, 0 replies; 30+ messages in thread
From: Jonathan Nieder @ 2010-04-14 2:23 UTC (permalink / raw)
To: git
Cc: Nguyễn Thái Ngọc Duy, Jeff King, Junio C Hamano,
Johannes Sixt
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
For the pager choice (and the choice to paginate in the first place)
to reflect the current repository configuration, the repository
needs to be searched for first.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Rebased for changes to t7006 in patches 2, 3, 4 v2.
builtin/config.c | 6 ++----
git.c | 4 ++--
| 22 ++++++++++++++++------
3 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 4bc46b1..ecc8f87 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -326,11 +326,9 @@ static int get_colorbool(int print)
return get_colorbool_found ? 0 : 1;
}
-int cmd_config(int argc, const char **argv, const char *unused_prefix)
+int cmd_config(int argc, const char **argv, const char *prefix)
{
- int nongit;
char *value;
- const char *prefix = setup_git_directory_gently(&nongit);
config_exclusive_filename = getenv(CONFIG_ENVIRONMENT);
@@ -409,7 +407,7 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
}
else if (actions == ACTION_EDIT) {
check_argc(argc, 0, 0);
- if (!config_exclusive_filename && nongit)
+ if (!config_exclusive_filename && !startup_info->have_repository)
die("not in a git directory");
git_config(git_default_config, NULL);
launch_editor(config_exclusive_filename ?
diff --git a/git.c b/git.c
index 56e93cf..0a0d9eb 100644
--- a/git.c
+++ b/git.c
@@ -309,7 +309,7 @@ static void handle_internal_command(int argc, const char **argv)
{ "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE },
{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
{ "commit-tree", cmd_commit_tree, RUN_SETUP },
- { "config", cmd_config },
+ { "config", cmd_config, RUN_SETUP_GENTLY },
{ "count-objects", cmd_count_objects, RUN_SETUP },
{ "describe", cmd_describe, RUN_SETUP },
{ "diff", cmd_diff },
@@ -366,7 +366,7 @@ static void handle_internal_command(int argc, const char **argv)
{ "reflog", cmd_reflog, RUN_SETUP },
{ "remote", cmd_remote, RUN_SETUP },
{ "replace", cmd_replace, RUN_SETUP },
- { "repo-config", cmd_config },
+ { "repo-config", cmd_config, RUN_SETUP_GENTLY },
{ "rerere", cmd_rerere, RUN_SETUP },
{ "reset", cmd_reset, RUN_SETUP },
{ "rev-list", cmd_rev_list, RUN_SETUP },
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 97ca5d6..d649f55 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -80,7 +80,7 @@ test_expect_success TTY 'git rev-list uses a pager if configured to' '
git config --unset pager.rev-list
rm -f paginated.out
-test_expect_failure TTY 'git config uses a pager if configured to' '
+test_expect_success TTY 'git config uses a pager if configured to' '
git config pager.config true &&
test_terminal git config --list &&
test -e paginated.out
@@ -175,7 +175,7 @@ test_default_pager() {
parse_args "$@"
unset PAGER GIT_PAGER
- git config --unset core.pager
+ git --no-pager config --unset core.pager
rm -f default_pager_used
$test_expectation SIMPLEPAGER "$cmd - default pager is used by default" "
cat >$less <<-\EOF &&
@@ -196,7 +196,7 @@ test_PAGER_overrides() {
parse_args "$@"
unset GIT_PAGER
- git config --unset core.pager
+ git --no-pager config --unset core.pager
rm -f PAGER_used
$test_expectation TTY "$cmd - PAGER overrides default pager" "
PAGER='wc > PAGER_used' &&
@@ -235,7 +235,7 @@ test_core_pager() {
$test_expectation TTY "$cmd - core.pager $used_if_wanted" "
PAGER=wc &&
export PAGER &&
- git config core.pager 'wc > core.pager_used' &&
+ git --no-pager config core.pager 'wc > core.pager_used' &&
$full_command &&
${if_want_local_config}test -e core.pager_used
"
@@ -261,7 +261,7 @@ test_pager_subdir_helper() {
stampname=\$(pwd)/core.pager_used &&
PAGER=wc &&
export PAGER stampname &&
- git config core.pager 'wc > \"\$stampname\"' &&
+ git --no-pager config core.pager 'wc > \"\$stampname\"' &&
mkdir sub &&
(
cd sub &&
@@ -276,7 +276,7 @@ test_GIT_PAGER_overrides() {
rm -f GIT_PAGER_used
$test_expectation TTY "$cmd - GIT_PAGER overrides core.pager" "
- git config core.pager wc &&
+ git --no-pager config core.pager wc &&
GIT_PAGER='wc > GIT_PAGER_used' &&
export GIT_PAGER &&
$full_command &&
@@ -326,4 +326,14 @@ test_core_pager_overrides expect_success 'git rev-list HEAD'
test_core_pager_subdir expect_success 'git rev-list HEAD'
test_GIT_PAGER_overrides expect_success 'git rev-list HEAD'
+test_doesnt_paginate expect_success 'git config -l'
+test_expect_success "set up 'git config' to use a pager" '
+ git config pager.config true
+'
+test_default_pager expect_success 'git config -l'
+test_PAGER_overrides expect_success 'git config -l'
+test_core_pager_overrides expect_success 'git config -l'
+test_core_pager_subdir expect_success 'git config -l'
+test_GIT_PAGER_overrides expect_success 'git config -l'
+
test_done
--
1.7.0.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [RFC/PATCH 00/46] nd/setup remainder for convenient reference
2010-04-13 2:11 [PATCH/RFC 0/9] Setup cleanup, chapter one Jonathan Nieder
` (8 preceding siblings ...)
2010-04-13 2:31 ` [PATCH 9/9] config: run setup before commiting pager choice Jonathan Nieder
@ 2010-04-13 3:08 ` Jonathan Nieder
2010-04-14 7:59 ` Nguyen Thai Ngoc Duy
2010-04-14 20:54 ` [PATCH/RFC 0/9] Setup cleanup, chapter one Junio C Hamano
10 siblings, 1 reply; 30+ messages in thread
From: Jonathan Nieder @ 2010-04-13 3:08 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy, Jeff King, Junio C Hamano
I was thinking of sending the remainder of nd/setup after this to save
others the trouble of rebasing, but I fear the traffic would be too
disruptive. So I am putting it up by git.
Caveats: maybe these patches make no sense. After I last rebased
them, I walked away without looking. Still, maybe they can save
you some time.
Good night,
Jonathan
The following changes since commit e0cc2322c409c6cfa897f395f6feb058e2d53d4a:
Nguyễn Thái Ngọc Duy (1):
config: run setup before commiting pager choice
are available in the git repository at:
git://repo.or.cz/git/jrn.git/ nd/setup
Jonathan Nieder (5):
t0001: test git init when run via an alias
t5512: test that ls-remote does not require a git repository
t7002: test git grep --no-index from a bare repository
t7006: expose test_terminal() for use by other tests
help: note why it is appropriate for this command not to use RUN_SETUP_GENTLY
Nguyễn Thái Ngọc Duy (41):
t1300: test that scripted commands do not respect stray .git/config
setup: save prefix (original cwd relative to toplevel) in startup_info
builtin: remove prefix variable from run_builtin()
run_builtin(): save "-h" detection result for later use
builtin: remember whether repository has been searched for
builtin: USE_PAGER should not be used without RUN_SETUP*
hash-object: use RUN_SETUP_GENTLY
shortlog: use RUN_SETUP_GENTLY
grep: use RUN_SETUP_GENTLY
archive: use RUN_SETUP_GENTLY
mailinfo: use RUN_SETUP_GENTLY
check-ref-format: use RUN_SETUP_GENTLY
verify-pack: use RUN_SETUP_GENTLY
apply: use RUN_SETUP_GENTLY
bundle: use RUN_SETUP_GENTLY
diff: use RUN_SETUP_GENTLY
ls-remote: use RUN_SETUP_GENTLY
var: use RUN_SETUP_GENTLY
merge-file: use RUN_SETUP_GENTLY
worktree setup: calculate prefix even if no worktree is found
index-pack: trust the prefix returned by setup_git_directory_gently()
index-pack: use RUN_SETUP_GENTLY
Move enter_repo() to setup.c
enter_repo(): initialize other variables as setup_git_directory_gently() does
rev-parse --git-dir: print relative gitdir correctly
worktree setup: call set_git_dir explicitly
Add git_config_early()
Use git_config_early() instead of git_config() during repo setup
worktree setup: restore original state when things go wrong
init/clone: turn on startup->have_repository properly
git_config(): do not read .git/config if there is no repository
Do not read .git/info/exclude if there is no repository
Do not read .git/info/attributes if there is no repository
apply: do not check sha1 if there is no repository
config: do not read .git/config if there is no repository
builtins: utilize startup_info->help where possible
builtins: check for startup_info->help, print and exit early
Allow to undo setup_git_directory_gently() gracefully (and fix alias code)
alias: keep repository found while collecting aliases as long as possible
Guard unallowed access to repository when it's not set up
builtins: setup repository before print unknown command error
attr.c | 5 +-
builtin/apply.c | 9 +-
builtin/archive.c | 2 +-
builtin/branch.c | 3 +
builtin/bundle.c | 6 +-
builtin/check-ref-format.c | 2 +-
builtin/checkout-index.c | 3 +
builtin/clone.c | 3 +-
builtin/commit.c | 6 +
builtin/config.c | 9 +-
builtin/diff.c | 6 +-
builtin/gc.c | 3 +
builtin/grep.c | 11 +-
builtin/hash-object.c | 9 +-
builtin/help.c | 5 +
builtin/index-pack.c | 21 +--
builtin/init-db.c | 10 +-
builtin/log.c | 6 +-
builtin/ls-files.c | 3 +
builtin/ls-remote.c | 3 -
builtin/mailinfo.c | 3 -
builtin/merge-file.c | 4 +-
builtin/merge-ours.c | 2 +-
builtin/merge.c | 3 +
builtin/pack-redundant.c | 2 +-
builtin/rev-parse.c | 8 +
builtin/shortlog.c | 4 +-
builtin/show-ref.c | 2 +-
builtin/update-index.c | 3 +
builtin/upload-archive.c | 7 +-
builtin/var.c | 2 -
cache.h | 8 +-
config.c | 22 +++-
dir.c | 8 +-
environment.c | 31 ++++-
git.c | 84 +++++++-----
help.c | 4 +
path.c | 91 ------------
setup.c | 213 ++++++++++++++++++++++++++---
t/lib-pager.sh | 37 +++++
t/{t7006 => lib-pager}/test-terminal.perl | 0
t/t0001-init.sh | 56 ++++++++
t/t1300-repo-config.sh | 15 ++
t/t1302-repo-version.sh | 2 +-
t/t1500-rev-parse.sh | 2 +-
t/t1501-worktree.sh | 5 +
t/t5512-ls-remote.sh | 14 ++
t/t7002-grep.sh | 116 ++++++++++++++++
t/t7006-pager.sh | 29 +----
t/t7020-help.sh | 18 +++
50 files changed, 660 insertions(+), 260 deletions(-)
create mode 100644 t/lib-pager.sh
rename t/{t7006 => lib-pager}/test-terminal.perl (100%)
create mode 100755 t/t7020-help.sh
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC/PATCH 00/46] nd/setup remainder for convenient reference
2010-04-13 3:08 ` [RFC/PATCH 00/46] nd/setup remainder for convenient reference Jonathan Nieder
@ 2010-04-14 7:59 ` Nguyen Thai Ngoc Duy
0 siblings, 0 replies; 30+ messages in thread
From: Nguyen Thai Ngoc Duy @ 2010-04-14 7:59 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Jeff King, Junio C Hamano
2010/4/13 Jonathan Nieder <jrnieder@gmail.com>:
> I was thinking of sending the remainder of nd/setup after this to save
> others the trouble of rebasing, but I fear the traffic would be too
> disruptive. So I am putting it up by git.
>
> Caveats: maybe these patches make no sense. After I last rebased
> them, I walked away without looking. Still, maybe they can save
> you some time.
Will go through them, but not now.
> are available in the git repository at:
>
> git://repo.or.cz/git/jrn.git/ nd/setup
I took your tree (5d46d95), dropped "builtin: USE_PAGER should not be
used without RUN_SETUP*" (it's useless), lightly modified "alias: keep
repository found while collecting aliases as long as possible" and
replaced "Guard unallowed access to repository when it's not set up".
Available at:
git://repo.or.cz/git/pclouds.git tp/setup
I will split the "Guard..." patch for easier review, later today. I
couldn't because it was a die() patch. Now all it does is warning(),
so it's safe to split up. One more good point of warning().
--
Duy
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH/RFC 0/9] Setup cleanup, chapter one
2010-04-13 2:11 [PATCH/RFC 0/9] Setup cleanup, chapter one Jonathan Nieder
` (9 preceding siblings ...)
2010-04-13 3:08 ` [RFC/PATCH 00/46] nd/setup remainder for convenient reference Jonathan Nieder
@ 2010-04-14 20:54 ` Junio C Hamano
2010-04-15 0:05 ` Jonathan Nieder
10 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2010-04-14 20:54 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Nguyễn Thái Ngọc Duy, Jeff King
Jonathan Nieder <jrnieder@gmail.com> writes:
> These patches are designed to make people who want the pager.<cmd>
> configuration to be more reliable a little happier. More importantly,
> they bring the setup procedure closer to doing the Right Thing™.
Thanks for looking into this.
By the way, the other day I had an interesting experience after running:
git -p help -m cat-file
If you try this yourself, you probably need "reset" or "stty sane" to
recover.
Does this series address it?
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH/RFC 0/9] Setup cleanup, chapter one
2010-04-14 20:54 ` [PATCH/RFC 0/9] Setup cleanup, chapter one Junio C Hamano
@ 2010-04-15 0:05 ` Jonathan Nieder
0 siblings, 0 replies; 30+ messages in thread
From: Jonathan Nieder @ 2010-04-15 0:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Nguyễn Thái Ngọc Duy, Jeff King
Junio C Hamano wrote:
> By the way, the other day I had an interesting experience after running:
>
> git -p help -m cat-file
>
> If you try this yourself, you probably need "reset" or "stty sane" to
> recover.
>
> Does this series address it?
No, it doesn’t.
The bug seems to be timing-related. The two copies of ‘less’ fight over
initialization of the terminal and nobody wins.
What is the right thing to do in this case? Can we assume ‘man’ comes
with a pager and suppress our own? Should we ask ‘man’ to suppress its
pager, losing the formatting and status line in the process, but setting
MANPAGER=cat? Probably the right thing to do would be to suppress our
pager and set MANPAGER according to git_pager() so ‘man’ can use it.
Maybe something roughly like this (on top of the series)? This does
not help backends that do not know how to use MANPAGER.
diff --git a/builtin/help.c b/builtin/help.c
index 3182a2b..d9a0307 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -316,6 +316,40 @@ static const char *cmd_to_page(const char *git_cmd)
return prepend("git", git_cmd);
}
+static void commit_pager_choice(void) {
+ switch (startup_info->use_pager) {
+ case 0:
+ setenv("GIT_PAGER", "cat", 1);
+ break;
+ case 1:
+ setup_pager();
+ break;
+ default:
+ break;
+ }
+}
+
+static void setup_man_pager(void) {
+ const char *pager;
+
+ switch (startup_info->use_pager) {
+ case 0:
+ setenv("MANPAGER", "cat", 1);
+ break;
+ case 1:
+ pager = git_pager(isatty(1));
+ if (!pager) {
+ setenv("MANPAGER", "cat", 1);
+ } else {
+ setenv("LESS", "FRSX", 0);
+ setenv("MANPAGER", pager, 1);
+ }
+ break;
+ default:
+ break;
+ }
+}
+
static void setup_man_path(void)
{
struct strbuf new_path = STRBUF_INIT;
@@ -358,6 +392,7 @@ static void show_man_page(const char *git_cmd)
const char *fallback = getenv("GIT_MAN_VIEWER");
setup_man_path();
+ setup_man_pager();
for (viewer = man_viewer_list; viewer; viewer = viewer->next)
{
exec_viewer(viewer->name, page); /* will return when unable */
@@ -423,20 +458,17 @@ int cmd_help(int argc, const char **argv, const char *prefix)
builtin_help_usage, 0);
parsed_help_format = help_format;
- if (show_all) {
+ if (show_all || !argv[0]) {
+ commit_pager_choice();
printf("usage: %s\n\n", git_usage_string);
- list_commands("git commands", &main_cmds, &other_cmds);
+ if (show_all)
+ list_commands("git commands", &main_cmds, &other_cmds);
+ else
+ list_common_cmds_help();
printf("%s\n", git_more_info_string);
return 0;
}
- if (!argv[0]) {
- printf("usage: %s\n\n", git_usage_string);
- list_common_cmds_help();
- printf("\n%s\n", git_more_info_string);
- return 0;
- }
-
setup_git_directory_gently(&nongit);
git_config(git_help_config, NULL);
diff --git a/cache.h b/cache.h
index 030ba01..8ba5727 100644
--- a/cache.h
+++ b/cache.h
@@ -1061,6 +1061,7 @@ int split_cmdline(char *cmdline, const char ***argv);
/* git.c */
struct startup_info {
int have_repository;
+ int use_pager;
};
extern struct startup_info *startup_info;
diff --git a/git.c b/git.c
index 0a0d9eb..a81726e 100644
--- a/git.c
+++ b/git.c
@@ -14,7 +14,6 @@ const char git_more_info_string[] =
"See 'git help COMMAND' for more information on a specific command.";
static struct startup_info git_startup_info;
-static int use_pager = -1;
struct pager_config {
const char *cmd;
int val;
@@ -39,7 +38,7 @@ int check_pager_config(const char *cmd)
}
static void commit_pager_choice(void) {
- switch (use_pager) {
+ switch (startup_info->use_pager) {
case 0:
setenv("GIT_PAGER", "cat", 1);
break;
@@ -86,9 +85,9 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
puts(system_path(GIT_HTML_PATH));
exit(0);
} else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
- use_pager = 1;
+ startup_info->use_pager = 1;
} else if (!strcmp(cmd, "--no-pager")) {
- use_pager = 0;
+ startup_info->use_pager = 0;
if (envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "--no-replace-objects")) {
@@ -221,13 +220,15 @@ static int handle_alias(int *argcp, const char ***argv)
const char git_version_string[] = GIT_VERSION;
#define RUN_SETUP (1<<0)
-#define USE_PAGER (1<<1)
/*
* require working tree to be present -- anything uses this needs
* RUN_SETUP for reading from the configuration file.
*/
-#define NEED_WORK_TREE (1<<2)
-#define RUN_SETUP_GENTLY (1<<3)
+#define NEED_WORK_TREE (1<<1)
+#define RUN_SETUP_GENTLY (1<<2)
+#define USE_PAGER (1<<3)
+/* do not commit_pager_choice() -- the builtin will take care of it */
+#define NO_PAGER_SETUP (1<<4)
struct cmd_struct {
const char *cmd;
@@ -244,6 +245,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
prefix = NULL;
help = argc == 2 && !strcmp(argv[1], "-h");
if (!help) {
+ int use_pager = startup_info->use_pager;
+
if (p->option & RUN_SETUP)
prefix = setup_git_directory();
if (p->option & RUN_SETUP_GENTLY) {
@@ -255,8 +258,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
use_pager = check_pager_config(p->cmd);
if (use_pager == -1 && p->option & USE_PAGER)
use_pager = 1;
+ startup_info->use_pager = use_pager;
}
- commit_pager_choice();
+ if (!(p->option & NO_PAGER_SETUP))
+ commit_pager_choice();
if (!help && p->option & NEED_WORK_TREE)
setup_work_tree();
@@ -328,7 +333,7 @@ static void handle_internal_command(int argc, const char **argv)
{ "get-tar-commit-id", cmd_get_tar_commit_id },
{ "grep", cmd_grep, USE_PAGER },
{ "hash-object", cmd_hash_object },
- { "help", cmd_help },
+ { "help", cmd_help, NO_PAGER_SETUP },
{ "index-pack", cmd_index_pack },
{ "init", cmd_init_db },
{ "init-db", cmd_init_db },
@@ -484,6 +489,7 @@ int main(int argc, const char **argv)
const char *cmd;
memset(&git_startup_info, 0, sizeof(git_startup_info));
+ git_startup_info.use_pager = -1;
startup_info = &git_startup_info;
cmd = git_extract_argv0_path(argv[0]);
^ permalink raw reply related [flat|nested] 30+ messages in thread