Re: What's cooking in git.git (May 2016, #02; Fri, 6)

[Johannes Schindelin]

Hi Junio,

On Fri, 6 May 2016, Junio C Hamano wrote:

> * jk/submodule-c-credential (2016-05-06) 6 commits
>  - submodule: stop sanitizing config options
>  - submodule: use prepare_submodule_repo_env consistently
>  - submodule--helper: move config-sanitizing to submodule.c
>  - submodule: export sanitized GIT_CONFIG_PARAMETERS
>  - t5550: break submodule config test into multiple sub-tests
>  - t5550: fix typo in $HTTPD_URL
> 
>  An earlier addition of "sanitize_submodule_env" with 14111fc4 (git:
>  submodule honor -c credential.* from command line, 2016-02-29)
>  turned out to be a convoluted no-op; implement what it wanted to do
>  correctly.
> 
>  Everybody happy?

I cannot speak for everybody. I am happy, though. In particular with this
topic branch ;-)

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (May 2016, #03; Mon, 9)

[Pranit Bauva]

On Tue, May 10, 2016 at 4:30 AM, Junio C Hamano  wrote:

> * pb/commit-verbose-config (2016-05-05) 8 commits
>  - SQUASH???
>  - commit: add a commit.verbose config variable
>  - t7507-commit-verbose: improve test coverage by testing number of diffs
>  - parse-options.c: make OPTION_COUNTUP respect "unspecified" values
>  - t/t7507: improve test coverage
>  - t0040-parse-options: improve test coverage
>  - test-parse-options: print quiet as integer
>  - t0040-test-parse-options.sh: fix style issues
>  (this branch is used by jc/test-parse-options-expect.)
>
>  "git commit" learned to pay attention to "commit.verbose"
>  configuration variable and act as if "--verbose" option was
>  given from the command line.
>
>  Almost there.
>  ($gmane/293663).

I like the change you have made so you could squash it in. Thanks.

Regards,
Pranit Bauva
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/6] t1500: reduce dependence upon global state

[Eric Sunshine]

This script pollutes its global state by changing its working directory
and capturing that state via $(pwd) in the GIT_CONFIG environment
variable. This makes it difficult to modernize the script since tests
ideally should be self-contained and not rely upon such transient global
state.

With the ultimate goal of eliminating working directory changes, as a
first step, avoid capturing global state by instead taking advantage of
$ROOT which captured $(pwd) prior to any working directory changes, and
compose GIT_CONFIG from it and local knowledge of the working directory.

Subsequent patches will eliminate changes of working directory and drop
GIT_CONFIG altogether.

Signed-off-by: Eric Sunshine 
---
 t/t1500-rev-parse.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 03f22fe..3d79568 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -49,7 +49,7 @@ test_rev_parse 'core.bare undefined' false false true
 mkdir work || exit 1
 cd work || exit 1
 GIT_DIR=../.git
-GIT_CONFIG="$(pwd)"/../.git/config
+GIT_CONFIG="$ROOT/work/../.git/config"
 export GIT_DIR GIT_CONFIG
 
 git config core.bare false
@@ -63,7 +63,7 @@ test_rev_parse 'GIT_DIR=../.git, core.bare undefined' false 
false true ''
 
 mv ../.git ../repo.git || exit 1
 GIT_DIR=../repo.git
-GIT_CONFIG="$(pwd)"/../repo.git/config
+GIT_CONFIG="$ROOT/work/../repo.git/config"
 
 git config core.bare false
 test_rev_parse 'GIT_DIR=../repo.git, core.bare = false' false false true ''
-- 
2.8.2.530.g51d527d

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/6] t1500: avoid setting environment variables outside of tests

[Eric Sunshine]

Ideally, each test should be responsible for setting up state it needs
rather than relying upon transient global state. Toward this end, teach
test_rev_parse() to accept a "-g " option to allow callers to
specify the value of the GIT_DIR environment variable explicitly, and
take advantage of this new option to avoid polluting the global scope
with GIT_DIR assignments.

Regarding the implementation: Typically, tests avoid polluting the
global state by wrapping transient environment variable assignments
within a subshell, however, this technique can not be used here since
test_config() and test_unconfig() need to know GIT_DIR, as well, but
neither function can be used within a subshell. Consequently, GIT_DIR is
cleared manually via sane_unset().

Signed-off-by: Eric Sunshine 
---
 t/t1500-rev-parse.sh | 19 ++-
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index c058aa4..525e6d3 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -7,11 +7,13 @@ test_description='test git rev-parse'
 test_rev_parse () {
dir=
bare=
+   env=
while :
do
case "$1" in
-C) dir="-C $2"; shift; shift ;;
-b) bare="$2"; shift; shift ;;
+   -g) env="GIT_DIR=$2; export GIT_DIR"; shift; shift ;;
-*) error "test_rev_parse: unrecognized option '$1'" ;;
*) break ;;
esac
@@ -36,6 +38,8 @@ test_rev_parse () {
do
expect="$1"
test_expect_success "$name: $o" '
+   test_when_finished "sane_unset GIT_DIR" &&
+   eval $env &&
$bare &&
echo "$expect" >expect &&
git $dir rev-parse --$o >actual &&
@@ -61,22 +65,19 @@ test_rev_parse -b t 'core.bare = true' true false false
 test_rev_parse -b u 'core.bare undefined' false false true
 
 test_expect_success 'setup non-local database ../.git' 'mkdir work'
-GIT_DIR=../.git
-export GIT_DIR
 
-test_rev_parse -C work -b f 'GIT_DIR=../.git, core.bare = false' false false 
true ''
+test_rev_parse -C work -g ../.git -b f 'GIT_DIR=../.git, core.bare = false' 
false false true ''
 
-test_rev_parse -C work -b t 'GIT_DIR=../.git, core.bare = true' true false 
false ''
+test_rev_parse -C work -g ../.git -b t 'GIT_DIR=../.git, core.bare = true' 
true false false ''
 
-test_rev_parse -C work -b u 'GIT_DIR=../.git, core.bare undefined' false false 
true ''
+test_rev_parse -C work -g ../.git -b u 'GIT_DIR=../.git, core.bare undefined' 
false false true ''
 
 test_expect_success 'setup non-local database ../repo.git' 'mv .git repo.git'
-GIT_DIR=../repo.git
 
-test_rev_parse -C work -b f 'GIT_DIR=../repo.git, core.bare = false' false 
false true ''
+test_rev_parse -C work -g ../repo.git -b f 'GIT_DIR=../repo.git, core.bare = 
false' false false true ''
 
-test_rev_parse -C work -b t 'GIT_DIR=../repo.git, core.bare = true' true false 
false ''
+test_rev_parse -C work -g ../repo.git -b t 'GIT_DIR=../repo.git, core.bare = 
true' true false false ''
 
-test_rev_parse -C work -b u 'GIT_DIR=../repo.git, core.bare undefined' false 
false true ''
+test_rev_parse -C work -g ../repo.git -b u 'GIT_DIR=../repo.git, core.bare 
undefined' false false true ''
 
 test_done
-- 
2.8.2.530.g51d527d

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/6] t1500: avoid changing working directory outside of tests

[Eric Sunshine]

Ideally, each test should be responsible for setting up state it needs
rather than relying upon transient global state. Toward this end, teach
test_rev_parse() to accept a "-C " option to allow callers to
instruct it explicitly in which directory its tests should be run, and
take advantage of this new option to avoid changing the working
directory outside of tests.

Signed-off-by: Eric Sunshine 
---
 t/t1500-rev-parse.sh | 44 
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 3d79568..f294ecc 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -3,8 +3,18 @@
 test_description='test git rev-parse'
 . ./test-lib.sh
 
-# usage: label is-bare is-inside-git is-inside-work prefix git-dir
+# usage: [options] label is-bare is-inside-git is-inside-work prefix git-dir
 test_rev_parse () {
+   dir=
+   while :
+   do
+   case "$1" in
+   -C) dir="-C $2"; shift; shift ;;
+   -*) error "test_rev_parse: unrecognized option '$1'" ;;
+   *) break ;;
+   esac
+   done
+
name=$1
shift
 
@@ -17,7 +27,7 @@ test_rev_parse () {
expect="$1"
test_expect_success "$name: $o" '
echo "$expect" >expect &&
-   git rev-parse --$o >actual &&
+   git $dir rev-parse --$o >actual &&
test_cmp expect actual
'
shift
@@ -29,16 +39,11 @@ ROOT=$(pwd)
 
 test_rev_parse toplevel false false true '' .git
 
-cd .git || exit 1
-test_rev_parse .git/ false true false '' .
-cd objects || exit 1
-test_rev_parse .git/objects/ false true false '' "$ROOT/.git"
-cd ../.. || exit 1
+test_rev_parse -C .git .git/ false true false '' .
+test_rev_parse -C .git/objects .git/objects/ false true false '' "$ROOT/.git"
 
-mkdir -p sub/dir || exit 1
-cd sub/dir || exit 1
-test_rev_parse subdirectory false false true sub/dir/ "$ROOT/.git"
-cd ../.. || exit 1
+test_expect_success 'setup untracked sub/dir' 'mkdir -p sub/dir'
+test_rev_parse -C sub/dir subdirectory false false true sub/dir/ "$ROOT/.git"
 
 git config core.bare true
 test_rev_parse 'core.bare = true' true false false
@@ -46,32 +51,31 @@ test_rev_parse 'core.bare = true' true false false
 git config --unset core.bare
 test_rev_parse 'core.bare undefined' false false true
 
-mkdir work || exit 1
-cd work || exit 1
+test_expect_success 'setup non-local database ../.git' 'mkdir work'
 GIT_DIR=../.git
 GIT_CONFIG="$ROOT/work/../.git/config"
 export GIT_DIR GIT_CONFIG
 
 git config core.bare false
-test_rev_parse 'GIT_DIR=../.git, core.bare = false' false false true ''
+test_rev_parse -C work 'GIT_DIR=../.git, core.bare = false' false false true ''
 
 git config core.bare true
-test_rev_parse 'GIT_DIR=../.git, core.bare = true' true false false ''
+test_rev_parse -C work 'GIT_DIR=../.git, core.bare = true' true false false ''
 
 git config --unset core.bare
-test_rev_parse 'GIT_DIR=../.git, core.bare undefined' false false true ''
+test_rev_parse -C work 'GIT_DIR=../.git, core.bare undefined' false false true 
''
 
-mv ../.git ../repo.git || exit 1
+test_expect_success 'setup non-local database ../repo.git' 'mv .git repo.git'
 GIT_DIR=../repo.git
 GIT_CONFIG="$ROOT/work/../repo.git/config"
 
 git config core.bare false
-test_rev_parse 'GIT_DIR=../repo.git, core.bare = false' false false true ''
+test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare = false' false false 
true ''
 
 git config core.bare true
-test_rev_parse 'GIT_DIR=../repo.git, core.bare = true' true false false ''
+test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare = true' true false 
false ''
 
 git config --unset core.bare
-test_rev_parse 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
+test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare undefined' false false 
true ''
 
 test_done
-- 
2.8.2.530.g51d527d

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/6] t1500: be considerate to future potential tests

[Eric Sunshine]

The final batch of git-rev-parse tests work against a non-local object
database named ../repo.git rather than the typically-named ../.git. It
prepares by renaming .git/ to repo.git/ and pointing GIT_DIR at
../repo.git, but never restores the name to .git/, which can be
problematic for tests added in the future. Be more friendly by instead
making repo.git/ a copy of .git/.

Signed-off-by: Eric Sunshine 
---
 t/t1500-rev-parse.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 525e6d3..af223ed 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -72,7 +72,7 @@ test_rev_parse -C work -g ../.git -b t 'GIT_DIR=../.git, 
core.bare = true' true
 
 test_rev_parse -C work -g ../.git -b u 'GIT_DIR=../.git, core.bare undefined' 
false false true ''
 
-test_expect_success 'setup non-local database ../repo.git' 'mv .git repo.git'
+test_expect_success 'setup non-local database ../repo.git' 'cp -R .git 
repo.git'
 
 test_rev_parse -C work -g ../repo.git -b f 'GIT_DIR=../repo.git, core.bare = 
false' false false true ''
 
-- 
2.8.2.530.g51d527d

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/6] t1500: test_rev_parse: facilitate future test enhancements

[Eric Sunshine]

Tests run by test_rev_parse() are nearly identical; each invokes
git-rev-parse with a single option and compares the result against an
expected value. Such duplication makes it onerous to extend the tests
since any change needs to be repeated in each test. Reduce the
duplication by parameterizing the test and driving it via a for-loop.

Signed-off-by: Eric Sunshine 
---
 t/t1500-rev-parse.sh | 44 +---
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 48ee077..03f22fe 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -3,38 +3,28 @@
 test_description='test git rev-parse'
 . ./test-lib.sh
 
-test_rev_parse() {
+# usage: label is-bare is-inside-git is-inside-work prefix git-dir
+test_rev_parse () {
name=$1
shift
 
-   test_expect_success "$name: is-bare-repository" \
-   "test '$1' = \"\$(git rev-parse --is-bare-repository)\""
-   shift
-   [ $# -eq 0 ] && return
-
-   test_expect_success "$name: is-inside-git-dir" \
-   "test '$1' = \"\$(git rev-parse --is-inside-git-dir)\""
-   shift
-   [ $# -eq 0 ] && return
-
-   test_expect_success "$name: is-inside-work-tree" \
-   "test '$1' = \"\$(git rev-parse --is-inside-work-tree)\""
-   shift
-   [ $# -eq 0 ] && return
-
-   test_expect_success "$name: prefix" \
-   "test '$1' = \"\$(git rev-parse --show-prefix)\""
-   shift
-   [ $# -eq 0 ] && return
-
-   test_expect_success "$name: git-dir" \
-   "test '$1' = \"\$(git rev-parse --git-dir)\""
-   shift
-   [ $# -eq 0 ] && return
+   for o in is-bare-repository \
+is-inside-git-dir \
+is-inside-work-tree \
+show-prefix \
+git-dir
+   do
+   expect="$1"
+   test_expect_success "$name: $o" '
+   echo "$expect" >expect &&
+   git rev-parse --$o >actual &&
+   test_cmp expect actual
+   '
+   shift
+   test $# -eq 0 && break
+   done
 }
 
-# label is-bare is-inside-git is-inside-work prefix git-dir
-
 ROOT=$(pwd)
 
 test_rev_parse toplevel false false true '' .git
-- 
2.8.2.530.g51d527d

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/6] modernize t1500

[Eric Sunshine]

This series modernizes t1500; it takes an entirely different approach
than [1][2] and is intended to replace that series. Whereas [1][2]
dropped the systematic function-driven testing of git-rev-parse in favor
of dozens of nearly identical copy/paste tests, this series retains the
structure of the existing script and instead updates the tests to set up
their own needed state rather than relying upon transient and fragile
manipulation of global state.

Due to its systematic function-driven approach, the original script is
small at 87 lines, and easily understood. When [1] dropped the
systematic approach and replaced it with individual copy/paste tests,
the script ballooned to a whopping 573 lines; its v2 successor, while
smaller, still inflated it to 322 lines. Writing that amount of code
correctly (even when primarily copy/paste) is difficult; reviewing it
for correctness is tedious, mind-numbing, and error-prone, as evidenced
by [3].

This series, on the other hand, values the concision of the original and
only makes changes necessary to modernize it; the script size remains
about the same; it drops from 87 to 83 lines.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/291087/focus=291088
[2]: http://thread.gmane.org/gmane.comp.version-control.git/291729/focus=291731
[3]: http://thread.gmane.org/gmane.comp.version-control.git/291729/focus=291745

Eric Sunshine (6):
  t1500: test_rev_parse: facilitate future test enhancements
  t1500: reduce dependence upon global state
  t1500: avoid changing working directory outside of tests
  t1500: avoid setting configuration options outside of tests
  t1500: avoid setting environment variables outside of tests
  t1500: be considerate to future potential tests

 t/t1500-rev-parse.sh | 116 +--
 1 file changed, 56 insertions(+), 60 deletions(-)

-- 
2.8.2.530.g51d527d

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/6] t1500: avoid setting configuration options outside of tests

[Eric Sunshine]

Ideally, each test should be responsible for setting up state it needs
rather than relying upon transient global state. Toward this end, teach
test_rev_parse() to accept a "-b " option to allow callers to set
"core.bare" explicitly or undefine it, and take advantage of this new
option to avoid setting "core.bare" outside of tests.

Under the hood, "-b " invokes "test_config -C " (or
"test_unconfig -C "), which means that git-config knows explicitly
where to find its configuration file. Consequently, the global
GIT_CONFIG environment variable, which was needed by the manual
git-config invocations outside of tests, is no longer needed, and is
thus dropped.

Signed-off-by: Eric Sunshine 
---
 t/t1500-rev-parse.sh | 39 ---
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index f294ecc..c058aa4 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -6,15 +6,25 @@ test_description='test git rev-parse'
 # usage: [options] label is-bare is-inside-git is-inside-work prefix git-dir
 test_rev_parse () {
dir=
+   bare=
while :
do
case "$1" in
-C) dir="-C $2"; shift; shift ;;
+   -b) bare="$2"; shift; shift ;;
-*) error "test_rev_parse: unrecognized option '$1'" ;;
*) break ;;
esac
done
 
+   case "$bare" in
+   '') ;;
+   t*) bare="test_config $dir core.bare true" ;;
+   f*) bare="test_config $dir core.bare false" ;;
+   u*) bare="test_unconfig $dir core.bare" ;;
+   *) error "test_rev_parse: unrecognized core.bare value '$bare'"
+   esac
+
name=$1
shift
 
@@ -26,6 +36,7 @@ test_rev_parse () {
do
expect="$1"
test_expect_success "$name: $o" '
+   $bare &&
echo "$expect" >expect &&
git $dir rev-parse --$o >actual &&
test_cmp expect actual
@@ -45,37 +56,27 @@ test_rev_parse -C .git/objects .git/objects/ false true 
false '' "$ROOT/.git"
 test_expect_success 'setup untracked sub/dir' 'mkdir -p sub/dir'
 test_rev_parse -C sub/dir subdirectory false false true sub/dir/ "$ROOT/.git"
 
-git config core.bare true
-test_rev_parse 'core.bare = true' true false false
+test_rev_parse -b t 'core.bare = true' true false false
 
-git config --unset core.bare
-test_rev_parse 'core.bare undefined' false false true
+test_rev_parse -b u 'core.bare undefined' false false true
 
 test_expect_success 'setup non-local database ../.git' 'mkdir work'
 GIT_DIR=../.git
-GIT_CONFIG="$ROOT/work/../.git/config"
-export GIT_DIR GIT_CONFIG
+export GIT_DIR
 
-git config core.bare false
-test_rev_parse -C work 'GIT_DIR=../.git, core.bare = false' false false true ''
+test_rev_parse -C work -b f 'GIT_DIR=../.git, core.bare = false' false false 
true ''
 
-git config core.bare true
-test_rev_parse -C work 'GIT_DIR=../.git, core.bare = true' true false false ''
+test_rev_parse -C work -b t 'GIT_DIR=../.git, core.bare = true' true false 
false ''
 
-git config --unset core.bare
-test_rev_parse -C work 'GIT_DIR=../.git, core.bare undefined' false false true 
''
+test_rev_parse -C work -b u 'GIT_DIR=../.git, core.bare undefined' false false 
true ''
 
 test_expect_success 'setup non-local database ../repo.git' 'mv .git repo.git'
 GIT_DIR=../repo.git
-GIT_CONFIG="$ROOT/work/../repo.git/config"
 
-git config core.bare false
-test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare = false' false false 
true ''
+test_rev_parse -C work -b f 'GIT_DIR=../repo.git, core.bare = false' false 
false true ''
 
-git config core.bare true
-test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare = true' true false 
false ''
+test_rev_parse -C work -b t 'GIT_DIR=../repo.git, core.bare = true' true false 
false ''
 
-git config --unset core.bare
-test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare undefined' false false 
true ''
+test_rev_parse -C work -b u 'GIT_DIR=../repo.git, core.bare undefined' false 
false true ''
 
 test_done
-- 
2.8.2.530.g51d527d

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] t6302: simplify non-gpg cases

[Eric Sunshine]

On Mon, May 9, 2016 at 10:40 PM, Jeff King  wrote:
> On Mon, May 09, 2016 at 01:47:35PM -0400, Eric Sunshine wrote:
>> > -   test_prepare_expect <<-\EOF | sed -e "s/Z$//" >expect &&
>> > +   cat <<-\EOF | sed -e "s/Z$//" >expect &&
>>
>> To make this as close to a reversion as possible, this could be
>> restored to the original (sans 'cat'):
>>
>> sed -e "s/Z$//" >expect <<-\EOF &&
>
> Thanks, I did the reversion with s/test_prepare_expect/cat/ rather than
> reverting 618310a, but I agree dropping this useless-use-of-cat is worth
> doing. Here's v2 with that change and your reviewed-by.

Looks good, thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t6044 broken on pu

[Jeff King]

On Mon, May 09, 2016 at 11:36:09AM -0700, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > Yes, I think the comment should just go.  Nobody used that alphabet
> > form since it was written in d17cf5f3 (tests: Introduce test_seq,
> > 2012-08-04).
> >
> >> I don't really care either way whether it is replaced or not (at one
> >> point there were some people really interested in NO_PERL not even using
> >> one-liners in the test suite, but I am not one of them).
> >
> > Neither am I, but I think it is prudent to drop that "letters".  The
> > comment even says the letter form is not portable already, so the
> > mention of GNU seq(1) is not helping at all.
> 
> -- >8 --
> Subject: test-lib-functions.sh: remove misleading comment on test_seq

Thanks, this (and the actual implementation change) both look good to
me, and I'm happy with either or both being applied.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] t6302: simplify non-gpg cases

[Jeff King]

[+cc Junio as this should be the final version]

On Mon, May 09, 2016 at 01:47:35PM -0400, Eric Sunshine wrote:

> > Subject: [PATCH] t6302: simplify non-gpg cases
> >
> > When commit 618310a taught t6302 to run without the GPG
> 
> 618310a (t6302: skip only signed tags rather than all tests when GPG
> is missing, 2016-03-06)

I sometimes intentionally avoid using that longer form when the title of
the commit does not convey what I want to communicate, and I have to
summarize the change in my own words anyway (in this case the
interesting thing is not _what_ it did, but _how_ it chose to do it). So
I find including the original subject line just bloats the sentence and
makes the point harder to find.

But I'm curious whether other people run into that problem, or if
readers would prefer an unconditional full-citation. If I were writing
a book, I would probably footnote a case like this (to give extra
context if somebody wants it, but not break the flow of the text). But
"618310a" is already a footnote of sorts. So I dunno.

> Should we cite bc9acea (ref-filter: implement %(if), %(then), and
> %(else) atoms, 2016-04-25) here as an example of a commit for which
> this was problematic (and which indeed broke the tests when GPG is
> unavailable)?

Nope, as Karthik mentioned, we don't know the sha1 of that commit yet.
:(

> Looks good. With or without the minor change below, this patch is:
> 
> Reviewed-by: Eric Sunshine 

Thanks.

> > -   test_prepare_expect <<-\EOF | sed -e "s/Z$//" >expect &&
> > +   cat <<-\EOF | sed -e "s/Z$//" >expect &&
> 
> To make this as close to a reversion as possible, this could be
> restored to the original (sans 'cat'):
> 
> sed -e "s/Z$//" >expect <<-\EOF &&

Thanks, I did the reversion with s/test_prepare_expect/cat/ rather than
reverting 618310a, but I agree dropping this useless-use-of-cat is worth
doing. Here's v2 with that change and your reviewed-by.

-- >8 --
Subject: t6302: simplify non-gpg cases

When commit 618310a taught t6302 to run without the GPG
prerequisite, it did so by conditionally creating the signed
tags only when gpg is available. As a result, further tests
need to take this into account, which they can do with the
test_prepare_expect helper. This is a minor hassle, though,
as the helper cannot easily cover all cases (it just matches
"signed" in the output, so all output must include the
actual refname).

Instead, let's take a different approach. We'll always
create the tags, and only conditionally sign them. This does
mean our tag-names are a minor lie, but it lets the tests
which do not care about signing easily behave the same in
all settings. We'll include a comment to document our lie
and avoid confusing further test-writers.

Reviewed-by: Eric Sunshine 
Signed-off-by: Jeff King 
---
 t/t6302-for-each-ref-filter.sh | 45 +-
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 70afb44..d0ab09f 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -5,15 +5,6 @@ test_description='test for-each-refs usage of ref-filter APIs'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-gpg.sh
 
-test_prepare_expect () {
-   if test_have_prereq GPG
-   then
-   cat
-   else
-   sed '/signed/d'
-   fi
-}
-
 test_expect_success 'setup some history and refs' '
test_commit one &&
test_commit two &&
@@ -22,11 +13,19 @@ test_expect_success 'setup some history and refs' '
test_commit four &&
git tag -m "An annotated tag" annotated-tag &&
git tag -m "Annonated doubly" doubly-annotated-tag annotated-tag &&
+
+   # Note that these "signed" tags might not actually be signed.
+   # Tests which care about the distinction should be marked
+   # with the GPG prereq.
if test_have_prereq GPG
then
-   git tag -s -m "A signed tag" signed-tag &&
-   git tag -s -m "Signed doubly" doubly-signed-tag signed-tag
+   sign=-s
+   else
+   sign=
fi &&
+   git tag $sign -m "A signed tag" signed-tag &&
+   git tag $sign -m "Signed doubly" doubly-signed-tag signed-tag &&
+
git checkout master &&
git update-ref refs/odd/spot master
 '
@@ -42,7 +41,7 @@ test_expect_success 'filtering with --points-at' '
 '
 
 test_expect_success 'check signed tags with --points-at' '
-   test_prepare_expect <<-\EOF | sed -e "s/Z$//" >expect &&
+   sed -e "s/Z$//" >expect <<-\EOF &&
refs/heads/side Z
refs/tags/annotated-tag four
refs/tags/four Z
@@ -65,7 +64,7 @@ test_expect_success 'filtering with --merged' '
 '
 
 test_expect_success 'filtering with --no-merged' '
-   test_prepare_expect >expect <<-\EOF &&
+   cat >expect <<-\EOF &&
refs/heads/side

Re: [PATCH v9 00/19] index-helper/watchman

[Ramsay Jones]

On 10/05/16 00:12, David Turner wrote:
> On Mon, 2016-05-09 at 15:32 -0700, Junio C Hamano wrote:
>> Junio C Hamano  writes:
>>
>>> David Turner  writes:
>>>
 On Mon, 2016-05-09 at 14:40 -0700, Junio C Hamano wrote:
> Hmmm, I seem to be getting
>
> $ cat t/trash*7900*/err
> fatal: Already running
>
> after running t7900 and it fails at #5, after applying
> "index-helper: optionally automatically run"
>>
>> The symptom looks pretty similar to $gmane/293461 reported earlier.
>> Here is how "t7900-index-helper.sh -i -v -x -d" ends.
>>
>>
>> expecting success:
>> test_when_finished "git index-helper --kill" &&
>> rm -f .git/index-helper.sock &&
>> git status &&
>> test_path_is_missing .git/index-helper.sock &&
>> test_config indexhelper.autorun true &&
>> git status &&
>> test -S .git/index-helper.sock &&
>> git status 2>err &&
>> test -S .git/index-helper.sock &&
>> test_must_be_empty err &&
>> git index-helper --kill &&
>> test_config indexhelper.autorun false &&
>> git status &&
>> test_path_is_missing .git/index-helper.sock
>>
>> + test_when_finished git index-helper --kill
>> + test 0 = 0
>> + test_cleanup={ git index-helper --kill
>> } && (exit "$eval_ret"); eval_ret=$?; :
>> + rm -f .git/index-helper.sock
>> + git status
>> On branch master
>> Untracked files:
>>   (use "git add ..." to include in what will be committed)
>>
>> err
>>
>> nothing added to commit but untracked files present (use "git add" to
>> track)
>> + test_path_is_missing .git/index-helper.sock
>> + test -e .git/index-helper.sock
>> + test_config indexhelper.autorun true
>> + config_dir=
>> + test indexhelper.autorun = -C
>> + test_when_finished test_unconfig  'indexhelper.autorun'
>> + test 0 = 0
>> + test_cleanup={ test_unconfig  'indexhelper.autorun'
>> } && (exit "$eval_ret"); eval_ret=$?; { git index
>> -helper --kill
>> } && (exit "$eval_ret"); eval_ret=$?; :
>> + git config indexhelper.autorun true
>> + git status
>> error: last command exited with $?=141
> 
> I think that's a SIGPIPE on the first git status.  Weird, since I just
> added sigpipe-avoidance code (in v8).  Does anyone have any idea why
> the sigchain stuff isn't doing what I think it is?

Sorry for a late report (I've been a bit busy last couple of days), but
I've been seeing exactly the same on v8 of this series.

Note that the above 'git status' is actually the second git-status in the
test.

I haven't been able to debug it too much, but I can tell you that it is
not failing at exactly the same place every time (so it may be time
sensitive). However, it often fails in poke_and_wait_for_reply() at the
first packet_flush() (which in turn calls write_or_die()  which calls
check_pipe() with an EPIPE(32)). At other times it fails when issuing
a flush after a refresh packet. For example, on one run with packet
tracing enabled, I got this for the trace:

  trace: built-in: git 'status'
  packet:  git> poke 3221 
  packet:  git> 
  packet:  git< OK
  packet:  git> refresh
  packet:  git> 

So, its getting the EPIPE for the refresh in this case, even though
the index-helper is still running, the unix socket is in .git/
(and so is a shm-* file BTW).

I didn't get any further than that I'm afraid.

ATB,
Ramsay Jones



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 00/19] index-helper/watchman

[David Turner]

On Mon, 2016-05-09 at 15:32 -0700, Junio C Hamano wrote:
> Junio C Hamano  writes:
> 
> > David Turner  writes:
> > 
> > > On Mon, 2016-05-09 at 14:40 -0700, Junio C Hamano wrote:
> > > > Hmmm, I seem to be getting
> > > > 
> > > > $ cat t/trash*7900*/err
> > > > fatal: Already running
> > > > 
> > > > after running t7900 and it fails at #5, after applying
> > > > "index-helper: optionally automatically run"
> 
> The symptom looks pretty similar to $gmane/293461 reported earlier.
> Here is how "t7900-index-helper.sh -i -v -x -d" ends.
> 
> 
> expecting success:
> test_when_finished "git index-helper --kill" &&
> rm -f .git/index-helper.sock &&
> git status &&
> test_path_is_missing .git/index-helper.sock &&
> test_config indexhelper.autorun true &&
> git status &&
> test -S .git/index-helper.sock &&
> git status 2>err &&
> test -S .git/index-helper.sock &&
> test_must_be_empty err &&
> git index-helper --kill &&
> test_config indexhelper.autorun false &&
> git status &&
> test_path_is_missing .git/index-helper.sock
> 
> + test_when_finished git index-helper --kill
> + test 0 = 0
> + test_cleanup={ git index-helper --kill
> } && (exit "$eval_ret"); eval_ret=$?; :
> + rm -f .git/index-helper.sock
> + git status
> On branch master
> Untracked files:
>   (use "git add ..." to include in what will be committed)
> 
> err
> 
> nothing added to commit but untracked files present (use "git add" to
> track)
> + test_path_is_missing .git/index-helper.sock
> + test -e .git/index-helper.sock
> + test_config indexhelper.autorun true
> + config_dir=
> + test indexhelper.autorun = -C
> + test_when_finished test_unconfig  'indexhelper.autorun'
> + test 0 = 0
> + test_cleanup={ test_unconfig  'indexhelper.autorun'
> } && (exit "$eval_ret"); eval_ret=$?; { git index
> -helper --kill
> } && (exit "$eval_ret"); eval_ret=$?; :
> + git config indexhelper.autorun true
> + git status
> error: last command exited with $?=141

I think that's a SIGPIPE on the first git status.  Weird, since I just
added sigpipe-avoidance code (in v8).  Does anyone have any idea why
the sigchain stuff isn't doing what I think it is?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

What's cooking in git.git (May 2016, #03; Mon, 9)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

The 'master' branch now has the tenth batch of topics of this cycle.
On the 'maint' front, 2.8.2 is out and fixes that have been in
'master' accumulates on it for 2.8.3.

Ones with questionable status has a '?' character in their comments.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[New Topics]

* ak/t4151-ls-files-could-be-empty (2016-05-09) 1 commit
 - t4151: make sure argument to 'test -z' is given

 Test fix.

 Will merge to 'next'.


* es/test-gpg-tags (2016-05-09) 1 commit
 - t6302: simplify non-gpg cases

 Test fix.

 Will merge to 'next'.


* jc/test-seq (2016-05-09) 2 commits
 - test-lib-functions.sh: rewrite test_seq without Perl
 - test-lib-functions.sh: remove misleading comment on test_seq

 Test fix.

 Will merge to 'next'.


* js/windows-dotgit (2016-05-09) 2 commits
 - mingw: remove unnecessary definition
 - mingw: introduce the 'core.hideDotFiles' setting

 On Windows, .git and optionally any files whose name starts with a
 dot are now marked as hidden, with a core.hideDotFiles knob to
 customize this behaviour.


* nd/error-errno (2016-05-09) 41 commits
 - wrapper.c: use warning_errno()
 - vcs-svn: use error_errno()
 - upload-pack.c: use error_errno()
 - unpack-trees.c: use error_errno()
 - transport-helper.c: use error_errno()
 - sha1_file.c: use {error,die,warning}_errno()
 - server-info.c: use error_errno()
 - sequencer.c: use error_errno()
 - run-command.c: use error_errno()
 - rerere.c: use error_errno() and warning_errno()
 - reachable.c: use error_errno()
 - mailmap.c: use error_errno()
 - ident.c: use warning_errno()
 - http.c: use error_errno() and warning_errno()
 - grep.c: use error_errno()
 - gpg-interface.c: use error_errno()
 - fast-import.c: use error_errno()
 - entry.c: use error_errno()
 - editor.c: use error_errno()
 - diff-no-index.c: use error_errno()
 - credential-cache--daemon.c: use warning_errno()
 - copy.c: use error_errno()
 - connected.c: use error_errno()
 - config.c: use error_errno()
 - compat/win32/syslog.c: use warning_errno()
 - combine-diff.c: use error_errno()
 - check-racy.c: use error_errno()
 - builtin/worktree.c: use error_errno()
 - builtin/upload-archive.c: use error_errno()
 - builtin/update-index.c: prefer "err" to "errno" in process_lstat_error
 - builtin/rm.c: use warning_errno()
 - builtin/pack-objects.c: use die_errno() and warning_errno()
 - builtin/merge-file.c: use error_errno()
 - builtin/mailsplit.c: use error_errno()
 - builtin/help.c: use warning_errno()
 - builtin/fetch.c: use error_errno()
 - builtin/branch.c: use error_errno()
 - builtin/am.c: use error_errno()
 - bisect.c: use die_errno() and warning_errno()
 - usage.c: add warning_errno() and error_errno()
 - usage.c: move format processing out of die_errno()

 The code for warning_errno/die_errno has been refactored and a new
 error_errno() reporting helper is introduced.

 Will merge to 'next'.


* nd/remote-plural-ours-plus-theirs (2016-05-06) 1 commit
 - remote.c: specify correct plural form in "commit diverge" message

 Message fix.

 Will merge to 'next'.


* nd/test-helpers (2016-05-09) 1 commit
 - wrap-for-bin.sh: handle t/helper/ paths internally

 Switching between 'master' and 'next', between which the paths to
 test helper binaries have changed, did not update bin-wrappers/*
 scripts used in tests, causing false test failures.

 Will merge to 'next'.


* tb/core-eol-fix (2016-04-25) 4 commits
 - convert.c: ident + core.autocrlf didn't work
 - t0027: test cases for combined attributes
 - convert: allow core.autocrlf=input and core.eol=crlf
 - t0027: make commit_chk_wrnNNO() reliable

 A couple of bugs around core.autocrlf have been fixed.

 Will merge to 'next'.


* tb/t5601-sed-fix (2016-05-09) 1 commit
 - t5601: Remove trailing space in sed expression

 Test fix.

 Will merge to 'next'.


* va/i18n-remote-comment-to-align (2016-05-09) 1 commit
 - i18n: remote: add comment for translators

 Message fix.

 Will merge to 'next'.


* jc/linkgit-fix (2016-05-09) 1 commit
 - Documentation: fix linkgit references

 Many 'linkgit:' references were broken,
 which are all fixed with this.

 Will merge to 'next'.


* js/http-custom-headers (2016-05-09) 2 commits
 - t5551: make the test for extra HTTP headers more robust
 - tests: adjust the configuration for Apache 2.2

 Update tests for "http.extraHeaders=" to be portable back
 to Apache 2.2 (the original depended on  which is a
 more recent feature).

 Will merge to 'next'.


--
[Stalled]

* sb/bisect (2016-04-15) 22 commits
 - SQUASH???
 - bisect: 

Re: [PATCH v9 00/19] index-helper/watchman

[Junio C Hamano]

Junio C Hamano  writes:

> David Turner  writes:
>
>> On Mon, 2016-05-09 at 14:40 -0700, Junio C Hamano wrote:
>>> Hmmm, I seem to be getting
>>> 
>>> $ cat t/trash*7900*/err
>>> fatal: Already running
>>> 
>>> after running t7900 and it fails at #5, after applying
>>> "index-helper: optionally automatically run"

The symptom looks pretty similar to $gmane/293461 reported earlier.
Here is how "t7900-index-helper.sh -i -v -x -d" ends.


expecting success:
test_when_finished "git index-helper --kill" &&
rm -f .git/index-helper.sock &&
git status &&
test_path_is_missing .git/index-helper.sock &&
test_config indexhelper.autorun true &&
git status &&
test -S .git/index-helper.sock &&
git status 2>err &&
test -S .git/index-helper.sock &&
test_must_be_empty err &&
git index-helper --kill &&
test_config indexhelper.autorun false &&
git status &&
test_path_is_missing .git/index-helper.sock

+ test_when_finished git index-helper --kill
+ test 0 = 0
+ test_cleanup={ git index-helper --kill
} && (exit "$eval_ret"); eval_ret=$?; :
+ rm -f .git/index-helper.sock
+ git status
On branch master
Untracked files:
  (use "git add ..." to include in what will be committed)

err

nothing added to commit but untracked files present (use "git add" to track)
+ test_path_is_missing .git/index-helper.sock
+ test -e .git/index-helper.sock
+ test_config indexhelper.autorun true
+ config_dir=
+ test indexhelper.autorun = -C
+ test_when_finished test_unconfig  'indexhelper.autorun'
+ test 0 = 0
+ test_cleanup={ test_unconfig  'indexhelper.autorun'
} && (exit "$eval_ret"); eval_ret=$?; { git index-helper --kill
} && (exit "$eval_ret"); eval_ret=$?; :
+ git config indexhelper.autorun true
+ git status
error: last command exited with $?=141
not ok 5 - index-helper autorun works
#
#   test_when_finished "git index-helper --kill" &&
#   rm -f .git/index-helper.sock &&
#   git status &&
#   test_path_is_missing .git/index-helper.sock &&
#   test_config indexhelper.autorun true &&
#   git status &&
#   test -S .git/index-helper.sock &&
#   git status 2>err &&
#   test -S .git/index-helper.sock &&
#   test_must_be_empty err &&
#   git index-helper --kill &&
#   test_config indexhelper.autorun false &&
#   git status &&
#   test_path_is_missing .git/index-helper.sock
#
:
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 00/19] index-helper/watchman

[Junio C Hamano]

David Turner  writes:

> On Mon, 2016-05-09 at 14:40 -0700, Junio C Hamano wrote:
>> Hmmm, I seem to be getting
>> 
>> $ cat t/trash*7900*/err
>> fatal: Already running
>> 
>> after running t7900 and it fails at #5, after applying
>> "index-helper: optionally automatically run"
>
> It still passes for me (with or without USE_WATCHMAN set).  I'm testing
> on Ubuntu 15.10 with "Linux ubuntu 4.2.0-35-generic", on an ext4
> filesystem.

It passes on one box and fails on another.  They both run the same
Ubuntu 14.04 derivative, with same ext3 filesystem.  The failing one
is on a VM.

> Does it fail before or after git status 2>err ?  (If before, then those
> "err" contents are from the previous test).

I didn't dig and I do not know (yet).

> Is index-helper in fact running after the test fails? 

"ps ax | grep index-helper" showed a process, which I had to "kill"
manually.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 00/19] index-helper/watchman

[David Turner]

On Mon, 2016-05-09 at 14:40 -0700, Junio C Hamano wrote:
> Hmmm, I seem to be getting
> 
> $ cat t/trash*7900*/err
> fatal: Already running
> 
> after running t7900 and it fails at #5, after applying
> "index-helper: optionally automatically run"

It still passes for me (with or without USE_WATCHMAN set).  I'm testing
on Ubuntu 15.10 with "Linux ubuntu 4.2.0-35-generic", on an ext4
filesystem.

Does it fail before or after git status 2>err ?  (If before, then those
"err" contents are from the previous test).

Is index-helper in fact running after the test fails? 

Can you tell me a little about your system?  I'm not even sure what
would be useful here, but if I could set up a similarly-configured vm,
I could try to repro.

Thanks, and sorry for the failures.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 00/33] Yet more preparation for reference backends

[Junio C Hamano]

Michael Haggerty  writes:

>> The "demonstrate an existing breakage first" order makes it slightly
>> easier to review and follow a long series, as it forces the reviewer
>> to see the issue first and think about possible avenues to solve it
>> for themselves, before seeing a paticular solution.  For a trivial
>> single-issue fix, it is not necessary (including a fix and a test to
>> protect the fix from future breakage in the same patch is a norm).
>
> I find it useful to add the broken test in a separate patch, because it
> is then easy to cherry pick that patch to other versions of Git to
> discover which ones are also affected by the problem. If the addition of
> the test is combined with the fix, then the patch would more often fail
> to apply to other versions due to conflicts at the locations of the fix,
> and even if it applied, you wouldn't learn whether that version of git
> was broken but the breakage was fixed by the same fix, or whether it
> wasn't broken in the first place and the fix was unnecessary.

Note that I only said "it is not necessary", and did not say "it is
not desirable".  I wouldn't automatically reject a two patch series
with demonstration followed by fix, only because they are not in a
single patch.

Even when I know the maintenance track a particular fix and test
targets at, I'd do the "only try to test to see how it is broken
currently" step manually anyway as part of the initial "acceptance"
check when applying [*1*], so trying the same procedure for older
maintenance tracks is no big burden for me.  Having these as two
separate patches is easier to split them apart, which unfortunately
makes it easier to lose one of them while cherry-picking.

This is of course subjective.


[Footnote]

*1* There is a bit of white-lie here.  Instead of applying only t/
part, I tend to just do "git am" the whole thing, and then pipe
"git show" to "git apply -R" to in-work-tree revert only the
code that fixes it.  But the result I get is the same.

And the "cherry-picking" would also involve "show only the t/
part and pipe that to "git apply", which is even simpler than
actually cherry-picking and creating a commit (I do not have to
be very careful not to leave such a "test cherry-pick" commit in
the real history).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 00/33] Yet more preparation for reference backends

[Michael Haggerty]

On 05/09/2016 11:05 PM, Junio C Hamano wrote:
> David Turner  writes:
>> [...]
>> I generally like to put the bug fixes before the tests for those fixes
>> (so that bisect on the complete suite works).  But maybe the git policy
>> is different.
> 
> The Git policy only asks not to break bisection.
> 
> As long as patch that adds a new test that comes before a patch that
> fixes the issue marks the new test with test_expect_failure, and a
> later patch that fixes the issue turns it into test_expect_success,
> bisection would not break.
> 
> The "demonstrate an existing breakage first" order makes it slightly
> easier to review and follow a long series, as it forces the reviewer
> to see the issue first and think about possible avenues to solve it
> for themselves, before seeing a paticular solution.  For a trivial
> single-issue fix, it is not necessary (including a fix and a test to
> protect the fix from future breakage in the same patch is a norm).

I find it useful to add the broken test in a separate patch, because it
is then easy to cherry pick that patch to other versions of Git to
discover which ones are also affected by the problem. If the addition of
the test is combined with the fix, then the patch would more often fail
to apply to other versions due to conflicts at the locations of the fix,
and even if it applied, you wouldn't learn whether that version of git
was broken but the breakage was fixed by the same fix, or whether it
wasn't broken in the first place and the fix was unnecessary.

Michael

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 00/19] index-helper/watchman

[Junio C Hamano]

Hmmm, I seem to be getting

$ cat t/trash*7900*/err
fatal: Already running

after running t7900 and it fails at #5, after applying
"index-helper: optionally automatically run".


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t6302: simplify non-gpg cases

[Eric Sunshine]

On Mon, May 9, 2016 at 4:24 PM, Karthik Nayak  wrote:
> On Mon, May 9, 2016 at 11:17 PM, Eric Sunshine 
> wrote:
>> Should we cite bc9acea (ref-filter: implement %(if), %(then), and
>> %(else) atoms, 2016-04-25) here as an example of a commit for which
>> this was problematic (and which indeed broke the tests when GPG is
>> unavailable)?
>
> But it's still in pu and I'll be re-rolling it, would that be acceptable?

Ah right, therefore, no reason to cite that particular commit. Peff's
description is fine as-is.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t4151 missing quotes

[Eric Sunshine]

On Mon, May 9, 2016 at 4:45 PM, Junio C Hamano  wrote:
> Eric Sunshine  writes:
>> On Mon, May 9, 2016 at 2:56 PM, Junio C Hamano  wrote:
>>> Something like this follows Documentation/SubmittingPatches [...]
>>>
>>> -- >8 --
>>> From: Armin Kunaschik 
>>> Subject: t4151: make sure argument to 'test -z' is given
>>>
>>> 88d50724 (am --skip: revert changes introduced by failed 3way merge,
>>> 2015-06-06), unlike all the other patches in the series, forgot to
>>> quote the output from "$(git ls-files -u)" when using it as the
>>> argument to "test -z", leading to a syntax error.
>>
>> To make it clear that this was not a syntax error in the typical case,
>> it might make sense to say:
>>
>> ...potentially leading to a syntax error if some earlier tests failed.
>
> Hmph, do we have a broken &&-chain?

I don't know. Unfortunately, Armin didn't provide much information in
his initial email, saying only "skipping through some failed tests",
which doesn't necessarily indicate if those tests failed or if he
somehow manually skipped them.

> If an earlier test fails and leaves an unmerged path, "ls-files -u"
> would give some output, so "test -z" would get one or more non-empty
> strings; if we feed multiple, this would fail.  But we would not have
> even run "test -z" as long as we properly &&-chain these tests.
>
> I think the real issue is when the earlier step succeeds and does
> not leave any unmerged path.  In that case, we would run "test -z"
> without anything else on the command line, which would lead to an
> syntax error.
>
> Side Note: /usr/bin/test and test (built into bash and dash)
> seem not to care about the lack of string in "test -z "
> and "test -n ".  It appears to me that they just take
> "-z" and "-n" without "" as a special case of "test
> " that is fed "-z" or "-n" as .  Apparently, the
> platform Armin is working on doesn't.

I also tested on Mac OS X and BSD, and they happily accept bare "test
-n", as well (though, I don't doubt that there are old shells which
complain).

> Perhaps
>
> ... leading to a syntax error on some platforms whose "test"
> does not interpret "test -z" (no other arguments) as testing if
> a string "-z" is the null string (which GNU test and test that
> is built into bash and dash seem to do).
>
> would be an improvement?

Yes, that sounds good.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t6044 broken on pu

[Eric Sunshine]

On Mon, May 9, 2016 at 5:08 PM, Junio C Hamano  wrote:
> Subject: [PATCH] test-lib-functions.sh: rewrite test_seq without Perl
>
> Rewrite the 'seq' imitation only with commands and features
> that are typically found as built-in in modern POSIX shells,
> instead of relying on Perl to run a single-liner.
>
> Signed-off-by: Junio C Hamano 
> ---
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> @@ -679,7 +679,12 @@ test_seq () {
> 2)  ;;
> *)  error "bug in the test script: not 1 or 2 parameters to 
> test_seq" ;;
> esac
> -   perl -le 'print for $ARGV[0]..$ARGV[1]' -- "$@"
> +   test_seq_counter__=$1
> +   while test "$test_seq_counter__" -le "$2"
> +   do
> +   echo "$test_seq_counter__"
> +   test_seq_counter__=$(( $test_seq_counter__ + 1 ))
> +   done
>  }

Looks (obviously) correct and works as expected on Mac and BSD.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t6044 broken on pu

[Junio C Hamano]

Junio C Hamano  writes:

> Stop promising that we may allow using "letters"; this would open an
> easier reimplementation that does not rely on $PERL, if somebody
> later wants to.
>
> Signed-off-by: Junio C Hamano 

And I am not the one who particularly wants to, but here is the
previous patch sent elsewhere in the thread.

-- >8 --
Subject: [PATCH] test-lib-functions.sh: rewrite test_seq without Perl

Rewrite the 'seq' imitation only with commands and features
that are typically found as built-in in modern POSIX shells,
instead of relying on Perl to run a single-liner.

Signed-off-by: Junio C Hamano 
---
 t/test-lib-functions.sh | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 39b8151..9734e32 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -679,7 +679,12 @@ test_seq () {
2)  ;;
*)  error "bug in the test script: not 1 or 2 parameters to 
test_seq" ;;
esac
-   perl -le 'print for $ARGV[0]..$ARGV[1]' -- "$@"
+   test_seq_counter__=$1
+   while test "$test_seq_counter__" -le "$2"
+   do
+   echo "$test_seq_counter__"
+   test_seq_counter__=$(( $test_seq_counter__ + 1 ))
+   done
 }
 
 # This function can be used to schedule some commands to be run
-- 
2.8.2-557-gee41d5e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 00/33] Yet more preparation for reference backends

[Junio C Hamano]

David Turner  writes:

> On Fri, 2016-05-06 at 18:13 +0200, Michael Haggerty wrote:
>> Thanks to David, Junio, and Peff for their comments on v1 of this
>> patch series [1]. I think I have addressed all of the points that
>> were
>> brought up. Plus I fixed a pre-existing bug that I noticed myself
>> while adding some more tests; see the first bullet point below for
>> more information.
>> 
>> Changes between v1 and v2:
>> 
>> * Prefixed the patch series with three new patches that demonstrate
>>   and fix some bugs in reference resolution that I noticed while
>>   inspecting this series:
>> 
>>   * "t1404: demonstrate a bug resolving references" -- this
>> demonstrates a bug that has existed since at least 2.5.0.
>>   * "read_raw_ref(): don't get confused by an empty directory"
>>   * "commit_ref(): if there is an empty dir in the way, delete it"
>
> I generally like to put the bug fixes before the tests for those fixes
> (so that bisect on the complete suite works).  But maybe the git policy
> is different.

The Git policy only asks not to break bisection.

As long as patch that adds a new test that comes before a patch that
fixes the issue marks the new test with test_expect_failure, and a
later patch that fixes the issue turns it into test_expect_success,
bisection would not break.

The "demonstrate an existing breakage first" order makes it slightly
easier to review and follow a long series, as it forces the reviewer
to see the issue first and think about possible avenues to solve it
for themselves, before seeing a paticular solution.  For a trivial
single-issue fix, it is not necessary (including a fix and a test to
protect the fix from future breakage in the same patch is a norm).

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 05/19] index-helper: log warnings

[David Turner]

Instead of writing warnings to stderr, write them to a log.  Later, we'll
probably be daemonized, so writing to stderr will be pointless.

Signed-off-by: David Turner 
---
 Documentation/git-index-helper.txt |  3 +++
 index-helper.c | 32 
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-index-helper.txt 
b/Documentation/git-index-helper.txt
index 1f92c89..9c2f5eb 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -54,6 +54,9 @@ command. The following commands are used to control the 
daemon:
 
 All commands and replies are terminated by a NUL byte.
 
+In the event of an error, messages may be written to
+$GIT_DIR/index-helper.log.
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/index-helper.c b/index-helper.c
index bc7e110..21fb431 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -19,6 +19,29 @@ static struct shm shm_index;
 static struct shm shm_base_index;
 static int to_verify = 1;
 
+static void log_warning(const char *warning, ...)
+{
+   va_list ap;
+   FILE *fp;
+
+   fp = fopen(git_path("index-helper.log"), "a");
+   if (!fp)
+   /*
+* Probably nobody will see this, but it's the best
+* we can do.
+*/
+   die("Failed to open log for warnings");
+
+   fprintf(fp, "%"PRIuMAX"\t", (uintmax_t)time(NULL));
+
+   va_start(ap, warning);
+   vfprintf(fp, warning, ap);
+   va_end(ap);
+
+   fputc('\n', fp);
+   fclose(fp);
+}
+
 static void release_index_shm(struct shm *is)
 {
if (!is->shm)
@@ -93,7 +116,8 @@ static void share_index(struct index_state *istate, struct 
shm *is)
if (shared_mmap_create(istate->mmap_size, _mmap,
   git_path("shm-index-%s",
sha1_to_hex(istate->sha1))) < 0) {
-   die("Failed to create shm-index file");
+   log_warning("Failed to create shm-index file");
+   exit(1);
}
 
 
@@ -135,7 +159,7 @@ static int verify_shm(void)
ce = istate.cache[i];
if (ce->ce_namelen != base->ce_namelen ||
strcmp(ce->name, base->name)) {
-   warning("mismatch at entry %d", i);
+   log_warning("mismatch at entry %d", i);
break;
}
ce_flags = ce->ce_flags;
@@ -149,7 +173,7 @@ static int verify_shm(void)
ce->ce_flags = ce_flags;
base->ce_flags = base_flags;
if (ret) {
-   warning("mismatch at entry %d", i);
+   log_warning("mismatch at entry %d", i);
break;
}
}
@@ -255,7 +279,7 @@ static void loop(int fd, int idle_in_seconds)
 * alive, nothing to do.
 */
} else {
-   warning("BUG: Bogus command %s", buf);
+   log_warning("BUG: Bogus command %s", buf);
}
} else {
/*
-- 
2.4.2.767.g62658d5-twtrsrc

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 19/19] untracked-cache: config option

[David Turner]

Add a config option to populate the untracked cache.

For installations that have centrally-managed configuration, it's
easier to set a config once than to run update-index on every
repository.

Signed-off-by: David Turner 
---
 Documentation/config.txt | 4 
 read-cache.c | 7 ++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 385ea66..c7b76ef 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1848,6 +1848,10 @@ imap::
The configuration variables in the 'imap' section are described
in linkgit:git-imap-send[1].
 
+index.adduntrackedcache::
+   Automatically populate the untracked cache whenever the index
+   is written.
+
 index.addwatchmanextension::
Automatically add the watchman extension to the index whenever
it is written.
diff --git a/read-cache.c b/read-cache.c
index c547b0b..7e735e2 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2470,7 +2470,7 @@ static int do_write_index(struct index_state *istate, int 
newfd,
int entries = istate->cache_nr;
struct stat st;
struct strbuf previous_name_buf = STRBUF_INIT, *previous_name;
-   int watchman = 0;
+   int watchman = 0, untracked = 0;
uint64_t start = getnanotime();
 
for (i = removed = extended = 0; i < entries; i++) {
@@ -2500,6 +2500,11 @@ static int do_write_index(struct index_state *istate, 
int newfd,
!the_index.last_update)
the_index.last_update = xstrdup("");
 
+   if (!git_config_get_bool("index.adduntrackedcache", ) &&
+   untracked &&
+   !istate->untracked)
+   add_untracked_cache(_index);
+
hdr_version = istate->version;
 
hdr.hdr_signature = htonl(CACHE_SIGNATURE);
-- 
2.4.2.767.g62658d5-twtrsrc

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 17/19] index-helper: optionally automatically run

[David Turner]

Introduce a new config option, indexhelper.autorun, to automatically
run git index-helper before starting up a builtin git command.  This
enables users to keep index-helper running without manual
intervention.

Signed-off-by: David Turner 
---
 Documentation/config.txt |  4 
 read-cache.c | 37 +++--
 t/t7900-index-helper.sh  | 20 
 3 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 15001ce..385ea66 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1856,6 +1856,10 @@ index.version::
Specify the version with which new index files should be
initialized.  This does not affect existing repositories.
 
+indexhelper.autorun::
+   Automatically run git index-helper when any builtin git
+   command is run inside a repository.
+
 init.templateDir::
Specify the directory from which templates will be copied.
(See the "TEMPLATE DIRECTORY" section of linkgit:git-init[1].)
diff --git a/read-cache.c b/read-cache.c
index 7b7cb39..ebbcb7f 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -22,6 +22,7 @@
 #include "pkt-line.h"
 #include "sigchain.h"
 #include "ewah/ewok.h"
+#include "run-command.h"
 
 static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,
   unsigned int options);
@@ -1723,6 +1724,33 @@ static void post_read_index_from(struct index_state 
*istate)
tweak_untracked_cache(istate);
 }
 
+static int want_auto_index_helper(void)
+{
+   int want = -1;
+   const char *value = NULL;
+   const char *key = "indexhelper.autorun";
+
+   if (git_config_key_is_valid(key) &&
+   !git_config_get_value(key, )) {
+   int b = git_config_maybe_bool(key, value);
+   want = b >= 0 ? b : 0;
+   }
+   return want;
+}
+
+static void autorun_index_helper(void)
+{
+   const char *argv[] = {"git-index-helper", "--detach", "--autorun", 
NULL};
+   if (want_auto_index_helper() <= 0)
+   return;
+
+   trace_argv_printf(argv, "trace: auto index-helper:");
+
+   if (run_command_v_opt(argv,
+ RUN_SILENT_EXEC_FAILURE | RUN_CLEAN_ON_EXIT))
+   warning(_("You specified indexhelper.autorun, but running 
git-index-helper failed."));
+}
+
 /* in ms */
 #define WATCHMAN_TIMEOUT 1000
 
@@ -1796,6 +1824,7 @@ static int poke_daemon(struct index_state *istate,
if (fd < 0) {
warning("Failed to connect to index-helper socket");
unlink(git_path("index-helper.sock"));
+   autorun_index_helper();
return -1;
}
sigchain_push(SIGPIPE, SIG_IGN);
@@ -1835,9 +1864,13 @@ static int try_shm(struct index_state *istate)
int fd = -1;
 
if (!is_main_index(istate) ||
-   old_size <= 20 ||
-   stat(git_path("index-helper.sock"), ))
+   old_size <= 20)
return -1;
+
+   if (stat(git_path("index-helper.sock"), )) {
+   autorun_index_helper();
+   return -1;
+   }
if (poke_daemon(istate, , 0))
return -1;
sha1 = (unsigned char *)istate->mmap + old_size - 20;
diff --git a/t/t7900-index-helper.sh b/t/t7900-index-helper.sh
index aa6e5fc..3cfdf63 100755
--- a/t/t7900-index-helper.sh
+++ b/t/t7900-index-helper.sh
@@ -16,6 +16,9 @@ test -n "$NO_MMAP" && {
 }
 
 test_expect_success 'index-helper smoke test' '
+   # We need an existing commit so that the index exists (otherwise,
+   # the index-helper will not be autostarted)
+   test_commit x &&
git index-helper --exit-after 1 &&
test_path_is_missing .git/index-helper.sock
 '
@@ -46,4 +49,21 @@ test_expect_success 'index-helper is quiet with --autorun' '
git index-helper --autorun
 '
 
+test_expect_success 'index-helper autorun works' '
+   test_when_finished "git index-helper --kill" &&
+   rm -f .git/index-helper.sock &&
+   git status &&
+   test_path_is_missing .git/index-helper.sock &&
+   test_config indexhelper.autorun true &&
+   git status &&
+   test -S .git/index-helper.sock &&
+   git status 2>err &&
+   test -S .git/index-helper.sock &&
+   test_must_be_empty err &&
+   git index-helper --kill &&
+   test_config indexhelper.autorun false &&
+   git status &&
+   test_path_is_missing .git/index-helper.sock
+'
+
 test_done
-- 
2.4.2.767.g62658d5-twtrsrc

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 18/19] trace: measure where the time is spent in the index-heavy operations

[David Turner]

From: Nguyễn Thái Ngọc Duy 

All the known heavy code blocks are measured (except object database
access). This should help identify if an optimization is effective or
not. An unoptimized git-status would give something like below (92% of
time is accounted). To sum up the effort of making git scale better:

 - read cache line is being addressed by index-helper
 - preload/refresh index by watchman
 - read packed refs by lmdb backend
 - diff-index by rebuilding cache-tree
 - read directory by untracked cache and watchman
 - write index by split index
 - name hash potientially by index-helper

read-cache.c:2075 performance: 0.004058570 s: read cache .../index
preload-index.c:104   performance: 0.004419864 s: preload index
read-cache.c:1265 performance: 0.000185224 s: refresh index
refs/files-backend.c:1100 performance: 0.001935788 s: read packed refs
diff-lib.c:240performance: 0.000144132 s: diff-files
diff-lib.c:506performance: 0.013592000 s: diff-index
name-hash.c:128   performance: 0.000614177 s: initialize name hash
dir.c:2030performance: 0.015814103 s: read directory
read-cache.c:2565 performance: 0.004052343 s: write index, changed mask 
= 2
trace.c:420   performance: 0.048365509 s: git command: './git' 
'status'

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: David Turner 
---
 diff-lib.c   |  4 
 dir.c|  2 ++
 name-hash.c  |  2 ++
 preload-index.c  |  2 ++
 read-cache.c | 11 +++
 refs/files-backend.c |  2 ++
 6 files changed, 23 insertions(+)

diff --git a/diff-lib.c b/diff-lib.c
index bc49c70..7af7f9a 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -90,6 +90,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
int diff_unmerged_stage = revs->max_count;
unsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED)
  ? CE_MATCH_RACY_IS_DIRTY : 0);
+   uint64_t start = getnanotime();
 
diff_set_mnemonic_prefix(>diffopt, "i/", "w/");
 
@@ -236,6 +237,7 @@ int run_diff_files(struct rev_info *revs, unsigned int 
option)
}
diffcore_std(>diffopt);
diff_flush(>diffopt);
+   trace_performance_since(start, "diff-files");
return 0;
 }
 
@@ -491,6 +493,7 @@ static int diff_cache(struct rev_info *revs,
 int run_diff_index(struct rev_info *revs, int cached)
 {
struct object_array_entry *ent;
+   uint64_t start = getnanotime();
 
ent = revs->pending.objects;
if (diff_cache(revs, ent->item->oid.hash, ent->name, cached))
@@ -500,6 +503,7 @@ int run_diff_index(struct rev_info *revs, int cached)
diffcore_fix_diff_index(>diffopt);
diffcore_std(>diffopt);
diff_flush(>diffopt);
+   trace_performance_since(start, "diff-index");
return 0;
 }
 
diff --git a/dir.c b/dir.c
index 5058b29..c56a8b9 100644
--- a/dir.c
+++ b/dir.c
@@ -2183,6 +2183,7 @@ int read_directory(struct dir_struct *dir, const char 
*path, int len, const stru
 {
struct path_simplify *simplify;
struct untracked_cache_dir *untracked;
+   uint64_t start = getnanotime();
 
/*
 * Check out create_simplify()
@@ -2224,6 +2225,7 @@ int read_directory(struct dir_struct *dir, const char 
*path, int len, const stru
free_simplify(simplify);
qsort(dir->entries, dir->nr, sizeof(struct dir_entry *), cmp_name);
qsort(dir->ignored, dir->ignored_nr, sizeof(struct dir_entry *), 
cmp_name);
+   trace_performance_since(start, "read directory %.*s", len, path);
if (dir->untracked) {
static struct trace_key trace_untracked_stats = 
TRACE_KEY_INIT(UNTRACKED_STATS);
trace_printf_key(_untracked_stats,
diff --git a/name-hash.c b/name-hash.c
index 6d9f23e..b3966d8 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -115,6 +115,7 @@ static int cache_entry_cmp(const struct cache_entry *ce1,
 static void lazy_init_name_hash(struct index_state *istate)
 {
int nr;
+   uint64_t start = getnanotime();
 
if (istate->name_hash_initialized)
return;
@@ -124,6 +125,7 @@ static void lazy_init_name_hash(struct index_state *istate)
for (nr = 0; nr < istate->cache_nr; nr++)
hash_index_entry(istate, istate->cache[nr]);
istate->name_hash_initialized = 1;
+   trace_performance_since(start, "initialize name hash");
 }
 
 void add_name_hash(struct index_state *istate, struct cache_entry *ce)
diff --git a/preload-index.c b/preload-index.c
index c1fe3a3..7bb4809 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -72,6 +72,7 @@ static void preload_index(struct index_state *index,
 {
int threads, i, work, offset;
struct thread_data data[MAX_PARALLEL];
+   uint64_t start = getnanotime();
 
if (!core_preload_index)
return;
@@ -100,6 

[PATCH v9 04/19] index-helper: add --strict

[David Turner]

From: Nguyễn Thái Ngọc Duy 

There are "holes" in the index-helper approach because the shared
memory is not verified again by git. If $USER is compromised, shared
memory could be modified. But anyone who could do this could already
modify $GIT_DIR/index. A more realistic risk is some bugs in
index-helper that produce corrupt shared memory. --strict is added to
avoid that.

Strictly speaking there's still a very small gap where corrupt shared
memory could still be read by git: after we write the trailing SHA-1 in
the shared memory (thus signaling "this shm is ready") and before
verify_shm() detects an error.

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: David Turner 
---
 Documentation/git-index-helper.txt |  9 +++
 cache.h|  1 +
 index-helper.c | 48 ++
 read-cache.c   |  9 ---
 4 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-index-helper.txt 
b/Documentation/git-index-helper.txt
index f892184..1f92c89 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -25,6 +25,15 @@ OPTIONS
Exit if the cached index is not accessed for ``
seconds. Specify 0 to wait forever. Default is 600.
 
+--strict::
+--no-strict::
+   Strict mode makes index-helper verify the shared memory after
+   it's created. If the result does not match what's read from
+   $GIT_DIR/index, the shared memory is destroyed. This makes
+   index-helper take more than double the amount of time required
+   for reading an index, but because it will happen in the
+   background, it's not noticable. `--strict` is enabled by default.
+
 NOTES
 -
 
diff --git a/cache.h b/cache.h
index 2d7af6f..6cb0d02 100644
--- a/cache.h
+++ b/cache.h
@@ -345,6 +345,7 @@ struct index_state {
  * on it.
  */
 to_shm : 1,
+always_verify_trailing_sha1 : 1,
 initialized : 1;
struct hashmap name_hash;
struct hashmap dir_hash;
diff --git a/index-helper.c b/index-helper.c
index b9443f4..bc7e110 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -17,6 +17,7 @@ struct shm {
 
 static struct shm shm_index;
 static struct shm shm_base_index;
+static int to_verify = 1;
 
 static void release_index_shm(struct shm *is)
 {
@@ -114,11 +115,56 @@ static void share_index(struct index_state *istate, 
struct shm *is)
hashcpy((unsigned char *)new_mmap + istate->mmap_size - 20, is->sha1);
 }
 
+static int verify_shm(void)
+{
+   int i;
+   struct index_state istate;
+   memset(, 0, sizeof(istate));
+   istate.always_verify_trailing_sha1 = 1;
+   istate.to_shm = 1;
+   i = read_index();
+   if (i != the_index.cache_nr)
+   goto done;
+   for (; i < the_index.cache_nr; i++) {
+   struct cache_entry *base, *ce;
+   /* namelen is checked separately */
+   const unsigned int ondisk_flags =
+   CE_STAGEMASK | CE_VALID | CE_EXTENDED_FLAGS;
+   unsigned int ce_flags, base_flags, ret;
+   base = the_index.cache[i];
+   ce = istate.cache[i];
+   if (ce->ce_namelen != base->ce_namelen ||
+   strcmp(ce->name, base->name)) {
+   warning("mismatch at entry %d", i);
+   break;
+   }
+   ce_flags = ce->ce_flags;
+   base_flags = base->ce_flags;
+   /* only on-disk flags matter */
+   ce->ce_flags   &= ondisk_flags;
+   base->ce_flags &= ondisk_flags;
+   ret = memcmp(>ce_stat_data, >ce_stat_data,
+offsetof(struct cache_entry, name) -
+offsetof(struct cache_entry, ce_stat_data));
+   ce->ce_flags = ce_flags;
+   base->ce_flags = base_flags;
+   if (ret) {
+   warning("mismatch at entry %d", i);
+   break;
+   }
+   }
+done:
+   discard_index();
+   return i == the_index.cache_nr;
+}
+
 static void share_the_index(void)
 {
if (the_index.split_index && the_index.split_index->base)
share_index(the_index.split_index->base, _base_index);
share_index(_index, _index);
+   if (to_verify && !verify_shm())
+   cleanup_shm();
discard_index(_index);
 }
 
@@ -247,6 +293,8 @@ int main(int argc, char **argv)
struct option options[] = {
OPT_INTEGER(0, "exit-after", _in_seconds,
N_("exit if not used after some seconds")),
+   OPT_BOOL(0, "strict", _verify,
+"verify shared memory after creating"),
OPT_END()
};
 
diff --git 

[PATCH v9 11/19] update-index: enable/disable watchman support

[David Turner]

From: Nguyễn Thái Ngọc Duy 

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: David Turner 
---
 Documentation/git-index-helper.txt |  3 +++
 Documentation/git-update-index.txt |  6 ++
 builtin/update-index.c | 16 
 3 files changed, 25 insertions(+)

diff --git a/Documentation/git-index-helper.txt 
b/Documentation/git-index-helper.txt
index cce00cb..55a8a5a 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -18,6 +18,9 @@ each with a submodule, you might need four index-helpers.  
(In practice,
 this is only worthwhile for large indexes, so only use it if you notice
 that git status is slow).
 
+If you want the index-helper to accelerate untracked file checking,
+run git update-index --watchman before using it.
+
 OPTIONS
 ---
 
diff --git a/Documentation/git-update-index.txt 
b/Documentation/git-update-index.txt
index c6cbed1..6736487 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -19,6 +19,7 @@ SYNOPSIS
 [--ignore-submodules]
 [--[no-]split-index]
 [--[no-|test-|force-]untracked-cache]
+[--[no-]watchman]
 [--really-refresh] [--unresolve] [--again | -g]
 [--info-only] [--index-info]
 [-z] [--stdin] [--index-version ]
@@ -176,6 +177,11 @@ may not support it yet.
 --no-untracked-cache::
Enable or disable untracked cache feature. Please use
`--test-untracked-cache` before enabling it.
+
+--watchman::
+--no-watchman::
+   Enable or disable watchman support. This is, at present,
+   only useful with git index-helper.
 +
 These options take effect whatever the value of the `core.untrackedCache`
 configuration variable (see linkgit:git-config[1]). But a warning is
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 1c94ca5..a3b4b5d 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -914,6 +914,7 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
 {
int newfd, entries, has_errors = 0, nul_term_line = 0;
enum uc_mode untracked_cache = UC_UNSPECIFIED;
+   int use_watchman = -1;
int read_from_stdin = 0;
int prefix_length = prefix ? strlen(prefix) : 0;
int preferred_index_format = 0;
@@ -1012,6 +1013,8 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
N_("test if the filesystem supports untracked 
cache"), UC_TEST),
OPT_SET_INT(0, "force-untracked-cache", _cache,
N_("enable untracked cache without testing the 
filesystem"), UC_FORCE),
+   OPT_BOOL(0, "watchman", _watchman,
+   N_("use or not use watchman to reduce refresh cost")),
OPT_END()
};
 
@@ -1149,6 +1152,19 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
die("Bug: bad untracked_cache value: %d", untracked_cache);
}
 
+   if (use_watchman > 0) {
+   the_index.last_update= xstrdup("");
+   the_index.cache_changed |= WATCHMAN_CHANGED;
+#ifndef USE_WATCHMAN
+   warning("git was built without watchman support -- I'm "
+   "adding the extension here, but it probably won't "
+   "do you any good.");
+#endif
+   } else if (!use_watchman) {
+   the_index.last_update= NULL;
+   the_index.cache_changed |= WATCHMAN_CHANGED;
+   }
+
if (active_cache_changed) {
if (newfd < 0) {
if (refresh_args.flags & REFRESH_QUIET)
-- 
2.4.2.767.g62658d5-twtrsrc

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 13/19] watchman: add a config option to enable the extension

[David Turner]

For installations that have centrally-managed configuration, it's
easier to set a config once than to run update-index on every
repository.

Signed-off-by: David Turner 
---
 .gitignore|  1 +
 Documentation/config.txt  |  4 
 Makefile  |  1 +
 read-cache.c  |  6 ++
 t/t1701-watchman-extension.sh | 37 +
 test-dump-watchman.c  | 16 
 6 files changed, 65 insertions(+)
 create mode 100755 t/t1701-watchman-extension.sh
 create mode 100644 test-dump-watchman.c

diff --git a/.gitignore b/.gitignore
index b92f122..e6a5b2c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -188,6 +188,7 @@
 /test-dump-cache-tree
 /test-dump-split-index
 /test-dump-untracked-cache
+/test-dump-watchman
 /test-fake-ssh
 /test-scrap-cache-tree
 /test-genrandom
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2cd6bdd..15001ce 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1848,6 +1848,10 @@ imap::
The configuration variables in the 'imap' section are described
in linkgit:git-imap-send[1].
 
+index.addwatchmanextension::
+   Automatically add the watchman extension to the index whenever
+   it is written.
+
 index.version::
Specify the version with which new index files should be
initialized.  This does not affect existing repositories.
diff --git a/Makefile b/Makefile
index 65ab0f4..5f0a954 100644
--- a/Makefile
+++ b/Makefile
@@ -599,6 +599,7 @@ TEST_PROGRAMS_NEED_X += test-delta
 TEST_PROGRAMS_NEED_X += test-dump-cache-tree
 TEST_PROGRAMS_NEED_X += test-dump-split-index
 TEST_PROGRAMS_NEED_X += test-dump-untracked-cache
+TEST_PROGRAMS_NEED_X += test-dump-watchman
 TEST_PROGRAMS_NEED_X += test-fake-ssh
 TEST_PROGRAMS_NEED_X += test-genrandom
 TEST_PROGRAMS_NEED_X += test-hashmap
diff --git a/read-cache.c b/read-cache.c
index 76b65c2..7b7cb39 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2428,6 +2428,7 @@ static int do_write_index(struct index_state *istate, int 
newfd,
int entries = istate->cache_nr;
struct stat st;
struct strbuf previous_name_buf = STRBUF_INIT, *previous_name;
+   int watchman = 0;
 
for (i = removed = extended = 0; i < entries; i++) {
if (cache[i]->ce_flags & CE_REMOVE)
@@ -2451,6 +2452,11 @@ static int do_write_index(struct index_state *istate, 
int newfd,
if (istate->version == 3 || istate->version == 2)
istate->version = extended ? 3 : 2;
 
+   if (!git_config_get_bool("index.addwatchmanextension", ) &&
+   watchman &&
+   !the_index.last_update)
+   the_index.last_update = xstrdup("");
+
hdr_version = istate->version;
 
hdr.hdr_signature = htonl(CACHE_SIGNATURE);
diff --git a/t/t1701-watchman-extension.sh b/t/t1701-watchman-extension.sh
new file mode 100755
index 000..71f1d46
--- /dev/null
+++ b/t/t1701-watchman-extension.sh
@@ -0,0 +1,37 @@
+#!/bin/sh
+
+test_description='watchman extension smoke tests'
+
+# These don't actually test watchman interaction -- just the
+# index extension
+
+. ./test-lib.sh
+
+test_expect_success 'enable watchman' '
+   test_commit a &&
+   test-dump-watchman .git/index >actual &&
+   echo "last_update: (null)" >expect &&
+   test_cmp expect actual &&
+   git update-index --watchman &&
+   test-dump-watchman .git/index >actual &&
+   echo "last_update: " >expect &&
+   test_cmp expect actual
+'
+
+test_expect_success 'disable watchman' '
+   git update-index --no-watchman &&
+   test-dump-watchman .git/index >actual &&
+   echo "last_update: (null)" >expect &&
+   test_cmp expect actual
+'
+
+test_expect_success 'auto-enable watchman' '
+   test_config index.addwatchmanextension true &&
+   test_commit c &&
+   test-dump-watchman .git/index >actual &&
+   echo "last_update: " >expect &&
+   test_cmp expect actual
+'
+
+
+test_done
diff --git a/test-dump-watchman.c b/test-dump-watchman.c
new file mode 100644
index 000..0314fa5
--- /dev/null
+++ b/test-dump-watchman.c
@@ -0,0 +1,16 @@
+#include "cache.h"
+#include "ewah/ewok.h"
+
+int main(int argc, char **argv)
+{
+   do_read_index(_index, argv[1], 1);
+   printf("last_update: %s\n", the_index.last_update ?
+  the_index.last_update : "(null)");
+
+   /*
+* For now, we just dump last_update, since it is not reasonable
+* to populate the extension itself in tests.
+*/
+
+   return 0;
+}
-- 
2.4.2.767.g62658d5-twtrsrc

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 10/19] index-helper: use watchman to avoid refreshing index with lstat()

[David Turner]

From: Nguyễn Thái Ngọc Duy 

Watchman is hidden behind index-helper. Before git tries to read the
index from shm, it notifies index-helper through the socket and waits
for index-helper to prepare a file for sharing memory (with
MAP_SHARED). index-helper then contacts watchman, updates 'WAMA'
extension and put it in a separate file and wakes git up with a reply
to git's socket.

Git uses this extension to not lstat unchanged entries. Git only
trusts the 'WAMA' extension when it's received from the separate file,
not from disk. Unmarked entries are "clean". Marked entries are dirty
from watchman point of view. If it finds out some entries are
'watchman-dirty', but are really unchanged (e.g. the file was changed,
then reverted back), then Git will clear the marking in 'WAMA' before
writing it down.

Hiding watchman behind index-helper means you need both daemons. You
can't run watchman alone. Not so good. But on the other hand, 'git'
binary is not linked to watchman/json libraries, which is good for
packaging. Core git package will run fine without watchman-related
packages. If they need watchman, they can install git-index-helper and
dependencies.

This also lets us trust anything in the untracked cache that we haven't
marked invalid, saving those stat() calls.

Another reason for tying watchman to index-helper is, when used with
untracked cache, we need to keep track of $GIT_WORK_TREE file
listing. That kind of list can be kept in index-helper.

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: David Turner 
---
 Documentation/git-index-helper.txt |   6 +
 cache.h|   2 +
 dir.c  |  23 +++-
 dir.h  |   3 +
 index-helper.c | 107 ++--
 read-cache.c   | 243 ++---
 6 files changed, 357 insertions(+), 27 deletions(-)

diff --git a/Documentation/git-index-helper.txt 
b/Documentation/git-index-helper.txt
index e144752..cce00cb 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -55,6 +55,12 @@ command. The following commands are used to control the 
daemon:
Let the daemon know the index is to be read. It keeps the
daemon alive longer, unless `--exit-after=0` is used.
 
+"poke ":
+   Like "poke", but replies with "OK".  If the index has the
+   watchman extension, index-helper queries watchman, then
+   prepares a shared memory object with the watchman index
+   extension before replying.
+
 All commands and replies are terminated by a NUL byte.
 
 In the event of an error, messages may be written to
diff --git a/cache.h b/cache.h
index 452aea2..633e1dd 100644
--- a/cache.h
+++ b/cache.h
@@ -567,6 +567,7 @@ extern int daemonize(int *);
 
 /* Initialize and use the cache information */
 struct lock_file;
+extern int verify_index(const struct index_state *);
 extern int read_index(struct index_state *);
 extern int read_index_preload(struct index_state *, const struct pathspec 
*pathspec);
 extern int do_read_index(struct index_state *istate, const char *path,
@@ -574,6 +575,7 @@ extern int do_read_index(struct index_state *istate, const 
char *path,
 extern int read_index_from(struct index_state *, const char *path);
 extern int is_index_unborn(struct index_state *);
 extern int read_index_unmerged(struct index_state *);
+extern void write_watchman_ext(struct strbuf *sb, struct index_state *istate);
 #define COMMIT_LOCK(1 << 0)
 #define CLOSE_LOCK (1 << 1)
 extern int write_locked_index(struct index_state *, struct lock_file *lock, 
unsigned flags);
diff --git a/dir.c b/dir.c
index 69e0be6..5058b29 100644
--- a/dir.c
+++ b/dir.c
@@ -597,9 +597,9 @@ static void trim_trailing_spaces(char *buf)
  *
  * If "name" has the trailing slash, it'll be excluded in the search.
  */
-static struct untracked_cache_dir *lookup_untracked(struct untracked_cache *uc,
-   struct untracked_cache_dir 
*dir,
-   const char *name, int len)
+struct untracked_cache_dir *lookup_untracked(struct untracked_cache *uc,
+struct untracked_cache_dir *dir,
+const char *name, int len)
 {
int first, last;
struct untracked_cache_dir *d;
@@ -1726,6 +1726,17 @@ static int valid_cached_dir(struct dir_struct *dir,
if (!untracked)
return 0;
 
+   if (dir->untracked->use_watchman) {
+   /*
+* With watchman, we can trust the untracked cache's
+* valid field.
+*/
+   if (untracked->valid)
+   goto skip_stat;
+   else
+   invalidate_directory(dir->untracked, untracked);
+   }
+
if 

[PATCH v9 16/19] index-helper: autorun mode

[David Turner]

Soon, we'll want to automatically start index-helper, so we need
a mode that silently exits if it can't start up (either because
it's not in a git dir, or because another one is already running).

Signed-off-by: David Turner 
---
 Documentation/git-index-helper.txt |  4 
 index-helper.c | 29 +++--
 t/t7900-index-helper.sh|  8 
 3 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-index-helper.txt 
b/Documentation/git-index-helper.txt
index addf694..0466296 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -43,6 +43,10 @@ OPTIONS
 --kill::
Kill any running index-helper and clean up the socket
 
+--autorun::
+   If index-helper is not already running, start it.  Else, do
+   nothing.
+
 NOTES
 -
 
diff --git a/index-helper.c b/index-helper.c
index b275f6e..9743481 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -427,8 +427,9 @@ static void request_kill(void)
 int main(int argc, char **argv)
 {
const char *prefix;
-   int idle_in_seconds = 600, detach = 0, kill = 0;
+   int idle_in_seconds = 600, detach = 0, kill = 0, autorun = 0;
int fd;
+   int nongit;
struct strbuf socket_path = STRBUF_INIT;
struct option options[] = {
OPT_INTEGER(0, "exit-after", _in_seconds,
@@ -437,6 +438,7 @@ int main(int argc, char **argv)
 "verify shared memory after creating"),
OPT_BOOL(0, "detach", , "detach the process"),
OPT_BOOL(0, "kill", , "request that existing index helper 
processes exit"),
+   OPT_BOOL(0, "autorun", , "this is an automatic run of 
git index-helper, so certain errors can be solved by silently exiting"),
OPT_END()
};
 
@@ -446,7 +448,14 @@ int main(int argc, char **argv)
if (argc == 2 && !strcmp(argv[1], "-h"))
usage_with_options(usage_text, options);
 
-   prefix = setup_git_directory();
+   prefix = setup_git_directory_gently();
+   if (nongit) {
+   if (autorun)
+   exit(0);
+   else
+   die(_("not a git repository"));
+   }
+
if (parse_options(argc, (const char **)argv, prefix,
  options, usage_text, 0))
die(_("too many arguments"));
@@ -460,10 +469,18 @@ int main(int argc, char **argv)
 
/* check that no other copy is running */
fd = unix_stream_connect(git_path("index-helper.sock"));
-   if (fd > 0)
-   die(_("Already running"));
-   if (errno != ECONNREFUSED && errno != ENOENT)
-   die_errno(_("Unexpected error checking socket"));
+   if (fd > 0) {
+   if (autorun)
+   exit(0);
+   else
+   die(_("Already running"));
+   }
+   if (errno != ECONNREFUSED && errno != ENOENT) {
+   if (autorun)
+   return 0;
+   else
+   die_errno(_("Unexpected error checking socket"));
+   }
 
atexit(cleanup);
sigchain_push_common(cleanup_on_signal);
diff --git a/t/t7900-index-helper.sh b/t/t7900-index-helper.sh
index 7159971..aa6e5fc 100755
--- a/t/t7900-index-helper.sh
+++ b/t/t7900-index-helper.sh
@@ -38,4 +38,12 @@ test_expect_success 'index-helper will not start if already 
running' '
grep "Already running" err
 '
 
+test_expect_success 'index-helper is quiet with --autorun' '
+   test_when_finished "git index-helper --kill" &&
+   git index-helper --kill &&
+   git index-helper --detach &&
+   test -S .git/index-helper.sock &&
+   git index-helper --autorun
+'
+
 test_done
-- 
2.4.2.767.g62658d5-twtrsrc

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 03/19] index-helper: new daemon for caching index and related stuff

[David Turner]

From: Nguyễn Thái Ngọc Duy 

Instead of reading the index from disk and worrying about disk
corruption, the index is cached in memory (memory bit-flips happen
too, but hopefully less often). The result is faster read. Read time
is reduced by 70%.

The biggest gain is not having to verify the trailing SHA-1, which
takes lots of time especially on large index files. But this also
opens doors for further optimizations:

 - we could create an in-memory format that's essentially the memory
   dump of the index to eliminate most of parsing/allocation
   overhead. The mmap'd memory can be used straight away. Experiment
   [1] shows we could reduce read time by 88%.

 - we could cache non-index info such as name hash

Shared memory is done by storing files in a per-repository temporary
directory.  This is more portable than shm (which requires
posix-realtime and has various quirks on OS X).  It might even work on
Windows, although this has not been tested. The shared memory file's
name follows the template "shm--" where  is the
trailing SHA-1 of the index file.  is "index" for cached index
files (and might later be "name-hash" for name-hash cache). If such
shared memory exists, it contains the same index content as on
disk. The content is already validated by the daemon and git won't
validate it again (except comparing the trailing SHA-1s).

We also add some bits to the index (to_shm and from_shm) to track
when an index came from shared memory or is going to shared memory.

We keep this daemon's logic as thin as possible. The "brain" stays in
git. So the daemon can read and validate stuff, but that's all it's
allowed to do. It does not add/create new information. It doesn't even
accept direct updates from git.

Git can poke the daemon via unix domain sockets to tell it to refresh
the index cache, or to keep it alive some more minutes. It can't give
any real index data directly to the daemon. Real data goes to disk
first, then the daemon reads and verifies it from there. The daemon
only handles $GIT_DIR/index, not temporary index files; it only gets
poked for the former.

$GIT_DIR/index-helper.sock is the socket for the daemon process. The
daemon reads from the socket and executes commands.

Named pipes were considered for portability reasons, but then commands
that need replies from the daemon would have to open their own pipes,
since a named pipe should only have one reader.  Unix domain sockets
don't have this problem.

On webkit.git with index format v2, duplicating 8 times to 1.5m
entries and 236MB in size:

(vanilla)  0.50 s: read_index_from .git/index
(index-helper) 0.18 s: read_index_from .git/index

Interestingly with index v4, we get less out of index-helper. It makes
sense as v4 requires more processing after loading the index:

(vanilla)  0.37 s: read_index_from .git/index
(index-helper) 0.22 s: read_index_from .git/index

[1] http://thread.gmane.org/gmane.comp.version-control.git/247268/focus=248771

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: David Turner 
Signed-off-by: Ramsay Jones 
---
 .gitignore |   1 +
 Documentation/git-index-helper.txt |  50 +++
 Makefile   |   5 +
 cache.h|  11 ++
 git-compat-util.h  |   1 +
 index-helper.c | 277 +
 read-cache.c   | 125 +++--
 t/t7900-index-helper.sh|  23 +++
 8 files changed, 484 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/git-index-helper.txt
 create mode 100644 index-helper.c
 create mode 100755 t/t7900-index-helper.sh

diff --git a/.gitignore b/.gitignore
index 5087ce1..b92f122 100644
--- a/.gitignore
+++ b/.gitignore
@@ -71,6 +71,7 @@
 /git-http-fetch
 /git-http-push
 /git-imap-send
+/git-index-helper
 /git-index-pack
 /git-init
 /git-init-db
diff --git a/Documentation/git-index-helper.txt 
b/Documentation/git-index-helper.txt
new file mode 100644
index 000..f892184
--- /dev/null
+++ b/Documentation/git-index-helper.txt
@@ -0,0 +1,50 @@
+git-index-helper(1)
+===
+
+NAME
+
+git-index-helper - A simple cache daemon for speeding up index file access
+
+SYNOPSIS
+
+[verse]
+'git index-helper' [options]
+
+DESCRIPTION
+---
+Keep the index file in memory for faster access. This daemon is per
+repository and per working tree.  So if you have two working trees
+each with a submodule, you might need four index-helpers.  (In practice,
+this is only worthwhile for large indexes, so only use it if you notice
+that git status is slow).
+
+OPTIONS
+---
+
+--exit-after=::
+   Exit if the cached index is not accessed for ``
+   seconds. Specify 0 to wait forever. Default is 600.
+
+NOTES
+-
+
+$GIT_DIR/index-helper.sock a Unix domain socket that the daemon reads
+commands from.  The directory 

[PATCH v9 15/19] index-helper: don't run if already running

[David Turner]

Signed-off-by: David Turner 
---
 index-helper.c  | 7 +++
 t/t7900-index-helper.sh | 9 +
 2 files changed, 16 insertions(+)

diff --git a/index-helper.c b/index-helper.c
index 4ed1610..b275f6e 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -458,6 +458,13 @@ int main(int argc, char **argv)
return 0;
}
 
+   /* check that no other copy is running */
+   fd = unix_stream_connect(git_path("index-helper.sock"));
+   if (fd > 0)
+   die(_("Already running"));
+   if (errno != ECONNREFUSED && errno != ENOENT)
+   die_errno(_("Unexpected error checking socket"));
+
atexit(cleanup);
sigchain_push_common(cleanup_on_signal);
 
diff --git a/t/t7900-index-helper.sh b/t/t7900-index-helper.sh
index e71b5af..7159971 100755
--- a/t/t7900-index-helper.sh
+++ b/t/t7900-index-helper.sh
@@ -29,4 +29,13 @@ test_expect_success 'index-helper creates usable path file 
and can be killed' '
test_path_is_missing .git/index-helper.sock
 '
 
+test_expect_success 'index-helper will not start if already running' '
+   test_when_finished "git index-helper --kill" &&
+   git index-helper --detach &&
+   test -S .git/index-helper.sock &&
+   test_must_fail git index-helper 2>err &&
+   test -S .git/index-helper.sock &&
+   grep "Already running" err
+'
+
 test_done
-- 
2.4.2.767.g62658d5-twtrsrc

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 09/19] watchman: support watchman to reduce index refresh cost

[David Turner]

From: Nguyễn Thái Ngọc Duy 

The previous patch has the logic to clear bits in 'WAMA' bitmap. This
patch has logic to set bits as told by watchman. The missing bit,
_using_ these bits, are not here yet.

A lot of this code is written by David Turner originally, mostly from
[1]. I'm just copying and polishing it a bit.

[1] http://article.gmane.org/gmane.comp.version-control.git/248006

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: David Turner 
---
 Makefile   |  12 +
 cache.h|   1 +
 config.c   |   5 ++
 configure.ac   |   8 
 environment.c  |   3 ++
 watchman-support.c | 135 +
 watchman-support.h |   7 +++
 7 files changed, 171 insertions(+)
 create mode 100644 watchman-support.c
 create mode 100644 watchman-support.h

diff --git a/Makefile b/Makefile
index c8be0e7..65ab0f4 100644
--- a/Makefile
+++ b/Makefile
@@ -451,6 +451,7 @@ MSGFMT = msgfmt
 CURL_CONFIG = curl-config
 PTHREAD_LIBS = -lpthread
 PTHREAD_CFLAGS =
+WATCHMAN_LIBS =
 GCOV = gcov
 
 export TCL_PATH TCLTK_PATH
@@ -1416,6 +1417,13 @@ else
LIB_OBJS += thread-utils.o
 endif
 
+ifdef USE_WATCHMAN
+   LIB_H += watchman-support.h
+   LIB_OBJS += watchman-support.o
+   WATCHMAN_LIBS = -lwatchman
+   BASIC_CFLAGS += -DUSE_WATCHMAN
+endif
+
 ifdef HAVE_PATHS_H
BASIC_CFLAGS += -DHAVE_PATHS_H
 endif
@@ -2025,6 +2033,9 @@ git-remote-testsvn$X: remote-testsvn.o GIT-LDFLAGS 
$(GITLIBS) $(VCSSVN_LIB)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) 
$(LIBS) \
$(VCSSVN_LIB)
 
+git-index-helper$X: index-helper.o GIT-LDFLAGS $(GITLIBS)
+   $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) 
$(LIBS) $(WATCHMAN_LIBS)
+
 $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY)
$(QUIET_LNCP)$(RM) $@ && \
ln $< $@ 2>/dev/null || \
@@ -2164,6 +2175,7 @@ GIT-BUILD-OPTIONS: FORCE
@echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@+
@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst 
','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
@echo NO_MMAP=\''$(subst ','\'',$(subst ','\'',$(NO_MMAP)))'\' >>$@+
+   @echo USE_WATCHMAN=\''$(subst ','\'',$(subst 
','\'',$(USE_WATCHMAN)))'\' >>$@+
 ifdef TEST_OUTPUT_DIRECTORY
@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst 
','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
 endif
diff --git a/cache.h b/cache.h
index f10992d..452aea2 100644
--- a/cache.h
+++ b/cache.h
@@ -696,6 +696,7 @@ extern char *git_replace_ref_base;
 
 extern int fsync_object_files;
 extern int core_preload_index;
+extern int core_watchman_sync_timeout;
 extern int core_apply_sparse_checkout;
 extern int precomposed_unicode;
 extern int protect_hfs;
diff --git a/config.c b/config.c
index 9ba40bc..e6dc141 100644
--- a/config.c
+++ b/config.c
@@ -882,6 +882,11 @@ static int git_default_core_config(const char *var, const 
char *value)
return 0;
}
 
+   if (!strcmp(var, "core.watchmansynctimeout")) {
+   core_watchman_sync_timeout = git_config_int(var, value);
+   return 0;
+   }
+
if (!strcmp(var, "core.createobject")) {
if (!strcmp(value, "rename"))
object_creation_mode = OBJECT_CREATION_USES_RENAMES;
diff --git a/configure.ac b/configure.ac
index 0cd9f46..334d63b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1099,6 +1099,14 @@ AC_COMPILE_IFELSE([BSD_SYSCTL_SRC],
HAVE_BSD_SYSCTL=])
 GIT_CONF_SUBST([HAVE_BSD_SYSCTL])
 
+#
+# Check for watchman client library
+
+AC_CHECK_LIB([watchman], [watchman_connect],
+   [USE_WATCHMAN=YesPlease],
+   [USE_WATCHMAN=])
+GIT_CONF_SUBST([USE_WATCHMAN])
+
 ## Other checks.
 # Define USE_PIC if you need the main git objects to be built with -fPIC
 # in order to build and link perl/Git.so.  x86-64 seems to need this.
diff --git a/environment.c b/environment.c
index 6dec9d0..35e03c7 100644
--- a/environment.c
+++ b/environment.c
@@ -94,6 +94,9 @@ int core_preload_index = 1;
  */
 int ignore_untracked_cache_config;
 
+int core_watchman_sync_timeout = 300;
+
+
 /* This is set by setup_git_dir_gently() and/or git_default_config() */
 char *git_work_tree_cfg;
 static char *work_tree;
diff --git a/watchman-support.c b/watchman-support.c
new file mode 100644
index 000..dc8cd51
--- /dev/null
+++ b/watchman-support.c
@@ -0,0 +1,135 @@
+#include "cache.h"
+#include "watchman-support.h"
+#include "strbuf.h"
+#include "dir.h"
+#include 
+
+static struct watchman_query *make_query(const char *last_update)
+{
+   struct watchman_query *query = watchman_query();
+   watchman_query_set_fields(query, WATCHMAN_FIELD_NAME |
+WATCHMAN_FIELD_EXISTS |
+WATCHMAN_FIELD_NEWER);
+   watchman_query_set_empty_on_fresh(query, 1);
+   

[PATCH v9 14/19] index-helper: kill mode

[David Turner]

Add a new command (and command-line arg) to allow index-helpers to
exit cleanly.

This is mainly useful for tests.

Signed-off-by: David Turner 
---
 Documentation/git-index-helper.txt |  3 +++
 index-helper.c | 31 ++-
 t/t7900-index-helper.sh|  9 +
 3 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-index-helper.txt 
b/Documentation/git-index-helper.txt
index 55a8a5a..addf694 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -40,6 +40,9 @@ OPTIONS
 --detach::
Detach from the shell.
 
+--kill::
+   Kill any running index-helper and clean up the socket
+
 NOTES
 -
 
diff --git a/index-helper.c b/index-helper.c
index 71c4f48..4ed1610 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -373,6 +373,8 @@ static void loop(int fd, int idle_in_seconds)
 * alive, nothing to do.
 */
}
+   } else if (!strcmp(buf, "die")) {
+   break;
} else {
log_warning("BUG: Bogus command %s", buf);
}
@@ -403,10 +405,29 @@ static const char * const usage_text[] = {
NULL
 };
 
+static void request_kill(void)
+{
+   int fd = unix_stream_connect(git_path("index-helper.sock"));
+
+   if (fd >= 0) {
+   write_in_full(fd, "die", 4);
+   close(fd);
+   }
+
+   /*
+* The child will try to do this anyway, but we want to be
+* ready to launch a new daemon immediately after this command
+* returns.
+*/
+
+   unlink(git_path("index-helper.sock"));
+   return;
+}
+
 int main(int argc, char **argv)
 {
const char *prefix;
-   int idle_in_seconds = 600, detach = 0;
+   int idle_in_seconds = 600, detach = 0, kill = 0;
int fd;
struct strbuf socket_path = STRBUF_INIT;
struct option options[] = {
@@ -415,6 +436,7 @@ int main(int argc, char **argv)
OPT_BOOL(0, "strict", _verify,
 "verify shared memory after creating"),
OPT_BOOL(0, "detach", , "detach the process"),
+   OPT_BOOL(0, "kill", , "request that existing index helper 
processes exit"),
OPT_END()
};
 
@@ -429,6 +451,13 @@ int main(int argc, char **argv)
  options, usage_text, 0))
die(_("too many arguments"));
 
+   if (kill) {
+   if (detach)
+   die(_("--kill doesn't want any other options"));
+   request_kill();
+   return 0;
+   }
+
atexit(cleanup);
sigchain_push_common(cleanup_on_signal);
 
diff --git a/t/t7900-index-helper.sh b/t/t7900-index-helper.sh
index 114c112..e71b5af 100755
--- a/t/t7900-index-helper.sh
+++ b/t/t7900-index-helper.sh
@@ -20,4 +20,13 @@ test_expect_success 'index-helper smoke test' '
test_path_is_missing .git/index-helper.sock
 '
 
+test_expect_success 'index-helper creates usable path file and can be killed' '
+   test_when_finished "git index-helper --kill" &&
+   test_path_is_missing .git/index-helper.sock &&
+   git index-helper --detach &&
+   test -S .git/index-helper.sock &&
+   git index-helper --kill &&
+   test_path_is_missing .git/index-helper.sock
+'
+
 test_done
-- 
2.4.2.767.g62658d5-twtrsrc

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 01/19] read-cache.c: fix constness of verify_hdr()

[David Turner]

From: Nguyễn Thái Ngọc Duy 

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: David Turner 
---
 read-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/read-cache.c b/read-cache.c
index d9fb78b..16cc487 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1345,7 +1345,7 @@ struct ondisk_cache_entry_extended {
ondisk_cache_entry_extended_size(ce_namelen(ce)) : \
ondisk_cache_entry_size(ce_namelen(ce)))
 
-static int verify_hdr(struct cache_header *hdr, unsigned long size)
+static int verify_hdr(const struct cache_header *hdr, unsigned long size)
 {
git_SHA_CTX c;
unsigned char sha1[20];
-- 
2.4.2.767.g62658d5-twtrsrc

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 02/19] read-cache: allow to keep mmap'd memory after reading

[David Turner]

From: Nguyễn Thái Ngọc Duy 

Later, we will introduce git index-helper to share this memory with
other git processes.

We only unmap it when we discard the index (although the kernel may of
course choose to page it out).

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: David Turner 
---
 cache.h  |  3 +++
 read-cache.c | 13 -
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/cache.h b/cache.h
index b829410..4180e2b 100644
--- a/cache.h
+++ b/cache.h
@@ -333,11 +333,14 @@ struct index_state {
struct split_index *split_index;
struct cache_time timestamp;
unsigned name_hash_initialized : 1,
+keep_mmap : 1,
 initialized : 1;
struct hashmap name_hash;
struct hashmap dir_hash;
unsigned char sha1[20];
struct untracked_cache *untracked;
+   void *mmap;
+   size_t mmap_size;
 };
 
 extern struct index_state the_index;
diff --git a/read-cache.c b/read-cache.c
index 16cc487..3cb0ec3 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1574,6 +1574,10 @@ int do_read_index(struct index_state *istate, const char 
*path, int must_exist)
mmap = xmmap(NULL, mmap_size, PROT_READ, MAP_PRIVATE, fd, 0);
if (mmap == MAP_FAILED)
die_errno("unable to map index file");
+   if (istate->keep_mmap) {
+   istate->mmap = mmap;
+   istate->mmap_size = mmap_size;
+   }
close(fd);
 
hdr = mmap;
@@ -1626,11 +1630,13 @@ int do_read_index(struct index_state *istate, const 
char *path, int must_exist)
src_offset += 8;
src_offset += extsize;
}
-   munmap(mmap, mmap_size);
+   if (!istate->keep_mmap)
+   munmap(mmap, mmap_size);
return istate->cache_nr;
 
 unmap:
munmap(mmap, mmap_size);
+   istate->mmap = NULL;
die("index file corrupt");
 }
 
@@ -1655,6 +1661,7 @@ int read_index_from(struct index_state *istate, const 
char *path)
discard_index(split_index->base);
else
split_index->base = xcalloc(1, sizeof(*split_index->base));
+   split_index->base->keep_mmap = istate->keep_mmap;
ret = do_read_index(split_index->base,
git_path("sharedindex.%s",
 sha1_to_hex(split_index->base_sha1)), 1);
@@ -1698,6 +1705,10 @@ int discard_index(struct index_state *istate)
free(istate->cache);
istate->cache = NULL;
istate->cache_alloc = 0;
+   if (istate->keep_mmap && istate->mmap) {
+   munmap(istate->mmap, istate->mmap_size);
+   istate->mmap = NULL;
+   }
discard_split_index(istate);
free_untracked_cache(istate->untracked);
istate->untracked = NULL;
-- 
2.4.2.767.g62658d5-twtrsrc

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 08/19] read-cache: add watchman 'WAMA' extension

[David Turner]

From: Nguyễn Thái Ngọc Duy 

The extension contains a bitmap, one bit for each entry in the
index. If the n-th bit is zero, the n-th entry is considered
unchanged, we can ce_mark_uptodate() it without refreshing. If the bit
is non-zero and we found out the corresponding file is clean after
refresh, we can clear the bit.

In addition, there's a list of directories in the untracked-cache
to invalidate (because they have new or modified entries).

The 'skipping refresh' bit is not in this patch yet as we would need
watchman. More details in later patches.

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: David Turner 
---
 Documentation/technical/index-format.txt |  22 ++
 cache.h  |   4 ++
 dir.h|   3 +
 read-cache.c | 117 ++-
 4 files changed, 144 insertions(+), 2 deletions(-)

diff --git a/Documentation/technical/index-format.txt 
b/Documentation/technical/index-format.txt
index ade0b0c..86ed3a6 100644
--- a/Documentation/technical/index-format.txt
+++ b/Documentation/technical/index-format.txt
@@ -295,3 +295,25 @@ The remaining data of each directory block is grouped by 
type:
 in the previous ewah bitmap.
 
   - One NUL.
+
+== Watchman cache
+
+  The watchman cache tracks files for which watchman has told us about
+  changes.  The signature for this extension is { 'W', 'A', 'M', 'A' }.
+
+  The extension starts with
+
+  - A NUL-terminated string: the watchman vector clock at the last
+time we heard from watchman.
+
+  - 32-bit bitmap size: the size of the CE_WATCHMAN_DIRTY bitmap
+
+  - 32-bit untracked cache entry count: the number of dirty untracked
+cache entries
+
+  - An ewah bitmap, the n-th bit indicates whether the n-th index entry
+is CE_WATCHMAN_DIRTY.
+
+  - a list of N NUL-terminated strings.  Each is a directory that should
+be marked dirty in the untracked cache because watchman has told us
+about an update to a file in it.
diff --git a/cache.h b/cache.h
index 4c1529a..f10992d 100644
--- a/cache.h
+++ b/cache.h
@@ -182,6 +182,8 @@ struct cache_entry {
 #define CE_VALID (0x8000)
 #define CE_STAGESHIFT 12
 
+#define CE_WATCHMAN_DIRTY  (0x0001)
+
 /*
  * Range 0x0FFF in ce_flags is divided into
  * two parts: in-memory flags and on-disk ones.
@@ -320,6 +322,7 @@ static inline unsigned int canon_mode(unsigned int mode)
 #define CACHE_TREE_CHANGED (1 << 5)
 #define SPLIT_INDEX_ORDERED(1 << 6)
 #define UNTRACKED_CHANGED  (1 << 7)
+#define WATCHMAN_CHANGED   (1 << 8)
 
 struct split_index;
 struct untracked_cache;
@@ -353,6 +356,7 @@ struct index_state {
struct untracked_cache *untracked;
void *mmap;
size_t mmap_size;
+   char *last_update;
 };
 
 extern struct index_state the_index;
diff --git a/dir.h b/dir.h
index 3ec3fb0..3d540de 100644
--- a/dir.h
+++ b/dir.h
@@ -142,6 +142,9 @@ struct untracked_cache {
int gitignore_invalidated;
int dir_invalidated;
int dir_opened;
+   /* watchman invalidation data */
+   unsigned int use_watchman : 1;
+   struct string_list invalid_untracked;
 };
 
 struct dir_struct {
diff --git a/read-cache.c b/read-cache.c
index d7849d6..9399a81 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -21,6 +21,7 @@
 #include "unix-socket.h"
 #include "pkt-line.h"
 #include "sigchain.h"
+#include "ewah/ewok.h"
 
 static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,
   unsigned int options);
@@ -43,11 +44,13 @@ static struct cache_entry *refresh_cache_entry(struct 
cache_entry *ce,
 #define CACHE_EXT_RESOLVE_UNDO 0x52455543 /* "REUC" */
 #define CACHE_EXT_LINK 0x6c696e6b/* "link" */
 #define CACHE_EXT_UNTRACKED 0x554E5452   /* "UNTR" */
+#define CACHE_EXT_WATCHMAN 0x57414D41/* "WAMA" */
 
 /* changes that can be kept in $GIT_DIR/index (basically all extensions) */
 #define EXTMASK (RESOLVE_UNDO_CHANGED | CACHE_TREE_CHANGED | \
 CE_ENTRY_ADDED | CE_ENTRY_REMOVED | CE_ENTRY_CHANGED | \
-SPLIT_INDEX_ORDERED | UNTRACKED_CHANGED)
+SPLIT_INDEX_ORDERED | UNTRACKED_CHANGED | \
+WATCHMAN_CHANGED)
 
 struct index_state the_index;
 static const char *alternate_index_output;
@@ -1222,8 +1225,13 @@ int refresh_index(struct index_state *istate, unsigned 
int flags,
continue;
 
new = refresh_cache_ent(istate, ce, options, _errno, 
);
-   if (new == ce)
+   if (new == ce) {
+   if (ce->ce_flags & CE_WATCHMAN_DIRTY) {
+   ce->ce_flags  &= ~CE_WATCHMAN_DIRTY;
+   istate->cache_changed |= WATCHMAN_CHANGED;
+   }
continue;
+   }
if (!new) {
   

[PATCH v9 12/19] unpack-trees: preserve index extensions

[David Turner]

Make git checkout (and other unpack_tree operations) preserve the
untracked cache and watchman status. This is valuable for two reasons:

1. Often, an unpack_tree operation will not touch large parts of the
working tree, and thus most of the untracked cache will continue to be
valid.

2. Even if the untracked cache were entirely invalidated by such an
operation, the user has signaled their intention to have such a cache,
and we don't want to throw it away.

The same logic applies to the watchman state.

Signed-off-by: David Turner 
---
 cache.h   |  1 +
 read-cache.c  |  8 
 t/t7063-status-untracked-cache.sh | 22 ++
 t/test-lib-functions.sh   |  4 
 unpack-trees.c|  1 +
 5 files changed, 36 insertions(+)

diff --git a/cache.h b/cache.h
index 633e1dd..1b372ed 100644
--- a/cache.h
+++ b/cache.h
@@ -580,6 +580,7 @@ extern void write_watchman_ext(struct strbuf *sb, struct 
index_state *istate);
 #define CLOSE_LOCK (1 << 1)
 extern int write_locked_index(struct index_state *, struct lock_file *lock, 
unsigned flags);
 extern int discard_index(struct index_state *);
+extern void move_index_extensions(struct index_state *dst, struct index_state 
*src);
 extern int unmerged_index(const struct index_state *);
 extern int verify_path(const char *path);
 extern int index_dir_exists(struct index_state *istate, const char *name, int 
namelen);
diff --git a/read-cache.c b/read-cache.c
index b4ed18e..76b65c2 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2769,3 +2769,11 @@ void stat_validity_update(struct stat_validity *sv, int 
fd)
fill_stat_data(sv->sd, );
}
 }
+
+void move_index_extensions(struct index_state *dst, struct index_state *src)
+{
+   dst->untracked = src->untracked;
+   src->untracked = NULL;
+   dst->last_update = src->last_update;
+   src->last_update = NULL;
+}
diff --git a/t/t7063-status-untracked-cache.sh 
b/t/t7063-status-untracked-cache.sh
index a971884..083516d 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -646,4 +646,26 @@ test_expect_success 'test ident field is working' '
test_cmp ../expect ../err
 '
 
+test_expect_success 'untracked cache survives a checkout' '
+   git commit --allow-empty -m empty &&
+   test-dump-untracked-cache >../before &&
+   test_when_finished  "git checkout master" &&
+   git checkout -b other_branch &&
+   test-dump-untracked-cache >../after &&
+   test_cmp ../before ../after &&
+   test_commit test &&
+   test-dump-untracked-cache >../before &&
+   git checkout master &&
+   test-dump-untracked-cache >../after &&
+   test_cmp ../before ../after
+'
+
+test_expect_success 'untracked cache survives a commit' '
+   test-dump-untracked-cache >../before &&
+   git add done/two &&
+   git commit -m commit &&
+   test-dump-untracked-cache >../after &&
+   test_cmp ../before ../after
+'
+
 test_done
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 8d99eb3..e974b5b 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -186,6 +186,10 @@ test_commit () {
test_tick
fi &&
git commit $signoff -m "$1" &&
+   if [ "$(git config core.bare)" = false ]
+   then
+   git update-index --force-untracked-cache
+   fi
git tag "${4:-$1}"
 }
 
diff --git a/unpack-trees.c b/unpack-trees.c
index 9f55cc2..fc90eb3 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1215,6 +1215,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
struct unpack_trees_options
  WRITE_TREE_SILENT |
  WRITE_TREE_REPAIR);
}
+   move_index_extensions(>result, o->dst_index);
discard_index(o->dst_index);
*o->dst_index = o->result;
} else {
-- 
2.4.2.767.g62658d5-twtrsrc

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 07/19] index-helper: add --detach

[David Turner]

From: Nguyễn Thái Ngọc Duy 

We detach after creating and opening the socket, because otherwise
we might return control to the shell before index-helper is ready to
accept commands.  This might lead to flaky tests.

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: David Turner 
---
 Documentation/git-index-helper.txt |  3 +++
 index-helper.c | 15 +--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-index-helper.txt 
b/Documentation/git-index-helper.txt
index 9c2f5eb..e144752 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -34,6 +34,9 @@ OPTIONS
for reading an index, but because it will happen in the
background, it's not noticable. `--strict` is enabled by default.
 
+--detach::
+   Detach from the shell.
+
 NOTES
 -
 
diff --git a/index-helper.c b/index-helper.c
index 21fb431..7df7a97 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -17,7 +17,7 @@ struct shm {
 
 static struct shm shm_index;
 static struct shm shm_base_index;
-static int to_verify = 1;
+static int daemonized, to_verify = 1;
 
 static void log_warning(const char *warning, ...)
 {
@@ -59,6 +59,8 @@ static void cleanup_shm(void)
 
 static void cleanup(void)
 {
+   if (daemonized)
+   return;
unlink(git_path("index-helper.sock"));
cleanup_shm();
 }
@@ -311,7 +313,7 @@ static const char * const usage_text[] = {
 int main(int argc, char **argv)
 {
const char *prefix;
-   int idle_in_seconds = 600;
+   int idle_in_seconds = 600, detach = 0;
int fd;
struct strbuf socket_path = STRBUF_INIT;
struct option options[] = {
@@ -319,6 +321,7 @@ int main(int argc, char **argv)
N_("exit if not used after some seconds")),
OPT_BOOL(0, "strict", _verify,
 "verify shared memory after creating"),
+   OPT_BOOL(0, "detach", , "detach the process"),
OPT_END()
};
 
@@ -342,6 +345,14 @@ int main(int argc, char **argv)
if (fd < 0)
die_errno(_("could not set up index-helper socket"));
 
+   if (!freopen("/dev/null", "w", stderr))
+   die_errno("failed to redirect stderr to /dev/null");
+
+   if (!freopen("/dev/null", "w", stdout))
+   die_errno("failed to redirect stdout to /dev/null");
+
+   if (detach && daemonize())
+   die_errno(_("unable to detach"));
loop(fd, idle_in_seconds);
 
close(fd);
-- 
2.4.2.767.g62658d5-twtrsrc

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 06/19] daemonize(): set a flag before exiting the main process

[David Turner]

From: Nguyễn Thái Ngọc Duy 

This allows signal handlers and atexit functions to realize this
situation and not clean up.

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: David Turner 
---
 builtin/gc.c | 2 +-
 cache.h  | 2 +-
 daemon.c | 2 +-
 setup.c  | 4 +++-
 4 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index c583aad..37180de 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -385,7 +385,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 * failure to daemonize is ok, we'll continue
 * in foreground
 */
-   daemonized = !daemonize();
+   daemonized = !daemonize(NULL);
}
} else
add_repack_all_option();
diff --git a/cache.h b/cache.h
index 6cb0d02..4c1529a 100644
--- a/cache.h
+++ b/cache.h
@@ -539,7 +539,7 @@ extern int set_git_dir_init(const char *git_dir, const char 
*real_git_dir, int);
 extern int init_db(const char *template_dir, unsigned int flags);
 
 extern void sanitize_stdfds(void);
-extern int daemonize(void);
+extern int daemonize(int *);
 
 #define alloc_nr(x) (((x)+16)*3/2)
 
diff --git a/daemon.c b/daemon.c
index 8d45c33..a5cf954 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1365,7 +1365,7 @@ int main(int argc, char **argv)
return execute();
 
if (detach) {
-   if (daemonize())
+   if (daemonize(NULL))
die("--detach not supported on this platform");
} else
sanitize_stdfds();
diff --git a/setup.c b/setup.c
index de1a2a7..9adf13f 100644
--- a/setup.c
+++ b/setup.c
@@ -1017,7 +1017,7 @@ void sanitize_stdfds(void)
close(fd);
 }
 
-int daemonize(void)
+int daemonize(int *daemonized)
 {
 #ifdef NO_POSIX_GOODIES
errno = ENOSYS;
@@ -1029,6 +1029,8 @@ int daemonize(void)
case -1:
die_errno("fork failed");
default:
+   if (daemonized)
+   *daemonized = 1;
exit(0);
}
if (setsid() == -1)
-- 
2.4.2.767.g62658d5-twtrsrc

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 00/19] index-helper/watchman

[David Turner]

Formatting and patch naming fixes from Junio.  No substantive changes.

David Turner (8):
  index-helper: log warnings
  unpack-trees: preserve index extensions
  watchman: add a config option to enable the extension
  index-helper: kill mode
  index-helper: don't run if already running
  index-helper: autorun mode
  index-helper: optionally automatically run
  untracked-cache: config option

Nguyễn Thái Ngọc Duy (11):
  read-cache.c: fix constness of verify_hdr()
  read-cache: allow to keep mmap'd memory after reading
  index-helper: new daemon for caching index and related stuff
  index-helper: add --strict
  daemonize(): set a flag before exiting the main process
  index-helper: add --detach
  read-cache: add watchman 'WAMA' extension
  watchman: support watchman to reduce index refresh cost
  index-helper: use watchman to avoid refreshing index with lstat()
  update-index: enable/disable watchman support
  trace: measure where the time is spent in the index-heavy operations

 .gitignore   |   2 +
 Documentation/config.txt |  12 +
 Documentation/git-index-helper.txt   |  81 +
 Documentation/git-update-index.txt   |   6 +
 Documentation/technical/index-format.txt |  22 ++
 Makefile |  18 ++
 builtin/gc.c |   2 +-
 builtin/update-index.c   |  16 +
 cache.h  |  25 +-
 config.c |   5 +
 configure.ac |   8 +
 daemon.c |   2 +-
 diff-lib.c   |   4 +
 dir.c|  25 +-
 dir.h|   6 +
 environment.c|   3 +
 git-compat-util.h|   1 +
 index-helper.c   | 506 +
 name-hash.c  |   2 +
 preload-index.c  |   2 +
 read-cache.c | 534 ++-
 refs/files-backend.c |   2 +
 setup.c  |   4 +-
 t/t1701-watchman-extension.sh|  37 +++
 t/t7063-status-untracked-cache.sh|  22 ++
 t/t7900-index-helper.sh  |  69 
 t/test-lib-functions.sh  |   4 +
 test-dump-watchman.c |  16 +
 unpack-trees.c   |   1 +
 watchman-support.c   | 135 
 watchman-support.h   |   7 +
 31 files changed, 1558 insertions(+), 21 deletions(-)
 create mode 100644 Documentation/git-index-helper.txt
 create mode 100644 index-helper.c
 create mode 100755 t/t1701-watchman-extension.sh
 create mode 100755 t/t7900-index-helper.sh
 create mode 100644 test-dump-watchman.c
 create mode 100644 watchman-support.c
 create mode 100644 watchman-support.h

-- 
2.4.2.767.g62658d5-twtrsrc

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t4151 missing quotes

[Junio C Hamano]

Eric Sunshine  writes:

> On Mon, May 9, 2016 at 2:56 PM, Junio C Hamano  wrote:
>> Something like this follows Documentation/SubmittingPatches [...]
>>
>> -- >8 --
>> From: Armin Kunaschik 
>> Subject: t4151: make sure argument to 'test -z' is given
>>
>> 88d50724 (am --skip: revert changes introduced by failed 3way merge,
>> 2015-06-06), unlike all the other patches in the series, forgot to
>> quote the output from "$(git ls-files -u)" when using it as the
>> argument to "test -z", leading to a syntax error.
>
> To make it clear that this was not a syntax error in the typical case,
> it might make sense to say:
>
> ...potentially leading to a syntax error if some earlier tests failed.

Hmph, do we have a broken &&-chain?

If an earlier test fails and leaves an unmerged path, "ls-files -u"
would give some output, so "test -z" would get one or more non-empty
strings; if we feed multiple, this would fail.  But we would not have
even run "test -z" as long as we properly &&-chain these tests.

I think the real issue is when the earlier step succeeds and does
not leave any unmerged path.  In that case, we would run "test -z"
without anything else on the command line, which would lead to an
syntax error.

Side Note: /usr/bin/test and test (built into bash and dash)
seem not to care about the lack of string in "test -z "
and "test -n ".  It appears to me that they just take
"-z" and "-n" without "" as a special case of "test
" that is fed "-z" or "-n" as .  Apparently, the
platform Armin is working on doesn't.

Perhaps

... leading to a syntax error on some platforms whose "test"
does not interpret "test -z" (no other arguments) as testing if
a string "-z" is the null string (which GNU test and test that
is built into bash and dash seem to do).

would be an improvement?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 2/6] convert.c: stream and early out

[Junio C Hamano]

tbo...@web.de writes:

> + if (stats->stat_bits & earlyout)
> + break; /* We found what we have been searching for */

Are we sure if our callers are only interested in just one bit at a
time?  Otherwise, if we want to ensure all of the given bits are
set,

if ((stats->stat_bits & earlyout) == earlyout)
break;

would be necessary.  Otherwise, the "only one bit" assumption on the
"earlyout" parameter somehow needs to be documented in the code.

> + ssize_t readlen = read(fd, buf, sizeof(buf));

xread() to automatically retry an interrupted read?

> @@ -309,11 +354,13 @@ static int crlf_to_worktree(const char *path, const 
> char *src, size_t len,
>  {
>   char *to_free = NULL;
>   struct text_stat stats;
> + unsigned earlyout = CONVERT_STAT_BITS_TXT_CRLF | CONVERT_STAT_BITS_BIN;
> +
>  
>   if (!len || output_eol(crlf_action) != EOL_CRLF)
>   return 0;
>  
> - gather_stats(src, len, );
> + gather_stats(src, len, , earlyout);

Oops, this answers my earlier question, no?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 00/33] Yet more preparation for reference backends

[David Turner]

On Fri, 2016-05-06 at 18:13 +0200, Michael Haggerty wrote:
> Thanks to David, Junio, and Peff for their comments on v1 of this
> patch series [1]. I think I have addressed all of the points that
> were
> brought up. Plus I fixed a pre-existing bug that I noticed myself
> while adding some more tests; see the first bullet point below for
> more information.
> 
> Changes between v1 and v2:
> 
> * Prefixed the patch series with three new patches that demonstrate
>   and fix some bugs in reference resolution that I noticed while
>   inspecting this series:
> 
>   * "t1404: demonstrate a bug resolving references" -- this
> demonstrates a bug that has existed since at least 2.5.0.
>   * "read_raw_ref(): don't get confused by an empty directory"
>   * "commit_ref(): if there is an empty dir in the way, delete it"

I generally like to put the bug fixes before the tests for those fixes
(so that bisect on the complete suite works).  But maybe the git policy
is different.

Other than that, these changes look good to me.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t6044 broken on pu

[Eric Sunshine]

On Mon, May 9, 2016 at 2:36 PM, Junio C Hamano  wrote:
> Subject: test-lib-functions.sh: remove misleading comment on test_seq
>
> We never used the "letters" form since we came up with "test_seq" to
> replace use of non-portable "seq" in our test script, which we
> introduced it at d17cf5f3 (tests: Introduce test_seq, 2012-08-04).
>
> We use this helper to either iterate for N times (i.e. the values on
> the lines do not even matter), or just to get N distinct strings
> (i.e. the values on the lines themselves do not really matter, but
> we care that they are different from each other and reproducible).
>
> Stop promising that we may allow using "letters"; this would open an
> easier reimplementation that does not rely on $PERL, if somebody
> later wants to.
>
> Signed-off-by: Junio C Hamano 
> ---
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> @@ -718,20 +718,13 @@ test_cmp_rev () {
> -# Print a sequence of numbers or letters in increasing order.  This is
> -# similar to GNU seq(1), but the latter might not be available
> -# everywhere (and does not do letters).  It may be used like:
> -#
> -#  for i in $(test_seq 100)
> -#  do
> -#  for j in $(test_seq 10 20)
> -#  do
> -#  for k in $(test_seq a z)
> -#  do
> -#  echo $i-$j-$k
> -#  done
> -#  done
> -#  done
> +# Print a sequence of integers in increasing order, either with
> +# two arguments (start and end):
> +#
> +# test_seq 1 5 -- outputs 1 2 3 4 5 one line at a time
> +#
> +# or with one argument (end), in which case it starts counting
> +# from 1.

This new documentation is quite readable. Thanks.

>  test_seq () {
> case $# in
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 1/6] read-cache: factor out get_sha1_from_index() helper

[Junio C Hamano]

tbo...@web.de writes:

> +#define get_sha1_from_cache(path)  get_sha1_from_index (_index, (path))
>  #endif

Micronit: lose the extra SP; i.e. "get_sha1_from_index(_index, (path))".

> diff --git a/read-cache.c b/read-cache.c
> index d9fb78b..a3ef967 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2263,13 +2263,27 @@ int index_name_is_other(const struct index_state 
> *istate, const char *name,
>  
>  void *read_blob_data_from_index(struct index_state *istate, const char 
> *path, unsigned long *size)
>  {
> - int pos, len;
> + const unsigned char *sha1;
>   unsigned long sz;
>   enum object_type type;
>   void *data;
>  
> - len = strlen(path);
> - pos = index_name_pos(istate, path, len);
> + sha1 = get_sha1_from_index(istate, path);
> + if (!sha1)
> + return NULL;
> + data = read_sha1_file(sha1, , );
> + if (!data || type != OBJ_BLOB) {
> + free(data);
> + return NULL;
> + }
> + if (size)
> + *size = sz;
> + return data;
> +}
> +
> +const unsigned char *get_sha1_from_index(struct index_state *istate, const 
> char *path)
> +{
> + int pos = index_name_pos(istate, path, strlen(path));
>   if (pos < 0) {
>   /*
>* We might be in the middle of a merge, in which
> @@ -2285,14 +2299,7 @@ void *read_blob_data_from_index(struct index_state 
> *istate, const char *path, un
>   }
>   if (pos < 0)
>   return NULL;
> + return (istate->cache[pos]->sha1);

Micronit: lose the extra () pair around what is returned.

I wondered if we can share more code with this helper and
get_sha1_with_context_1(), which is the canonical copy of this logic
used to parse ":$path" and get the object name at $path in the
index, but this is sufficiently low-level and such a refactoring
of small code would not be of great benefit, so this patch is OK.

Thanks.

>  }
>  
>  void stat_validity_clear(struct stat_validity *sv)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: /* compiler workaround */ - what was the issue?

[Randall S. Becker]

On May 9, 2016 3:40 PM Philip Oakley wrote:
> From: "Stefan Beller" 
> > On Fri, May 6, 2016 at 12:57 PM, Junio C Hamano 
> wrote:
> >> Marc Branchaud  writes:
> >>
> >>> On 2016-05-06 02:54 PM, Junio C Hamano wrote:
> 
>  I wonder if can we come up with a short and sweet notation to
>  remind futhre readers that this "initialization" is not
>  initializing but merely squelching warnings from stupid compilers,
>  and agree to use it consistently?
> >>>
> >>> Perhaps
> >>>
> >>>   #define COMPILER_UNINITIALIZED_WARNING_INITIALIZER 0
> >>>
> >>> or, for short-and-sweet
> >>>
> >
> >   /* Here could go a longer explanation than the 4 character
> > define :) */
> >>>   #define CUWI 0
> >>>
> >>> ?
> >>>
> >>> :)
> >>
> >> I get that smiley.
> >>
> >> I was hinting to keep the /* compiler workaround */ comment, but in a
> >> bit shorter form.
> >> --
> 
> For some background, I found $gmane/169098/focus=169104 which covers
> some of the issues (the focused msg is one from Junio). Hannes then notes
> ($gmane/169121) that the current xx = xx; could be seen as possible
> undefined behaviour - perhaps one of those 'no good solution' problems.
> 
> Perhaps a suitable name...
> 
> #define SUPPRESS_COMPILER_UNINITIALIZED_WARNING 0
> /* Use when some in-use compiler is unable to determine if the variable is
> used uninitialized, and no good default value is available */
> 
> Though, where best to put it?

I would suggest this type of approach should be detected in the configure 
script and added automagically (as best as possible) or as a hint supplied by 
the platform's own specific configuration files if necessary as a last gasp.

-- Brief whoami: NonStop developer since approximately 
UNIX(421664400)/NonStop(2112884442)
-- In my real life, I talk too much.



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: /* compiler workaround */ - what was the issue?

[Philip Oakley]

From: "Stefan Beller" 

On Fri, May 6, 2016 at 12:57 PM, Junio C Hamano  wrote:

Marc Branchaud  writes:


On 2016-05-06 02:54 PM, Junio C Hamano wrote:


I wonder if can we come up with a short and sweet notation to remind
futhre readers that this "initialization" is not initializing but
merely squelching warnings from stupid compilers, and agree to use
it consistently?


Perhaps

  #define COMPILER_UNINITIALIZED_WARNING_INITIALIZER 0

or, for short-and-sweet



  /* Here could go a longer explanation than the 4 character
define :) */

  #define CUWI 0

?

:)


I get that smiley.

I was hinting to keep the /* compiler workaround */ comment, but
in a bit shorter form.
--


For some background, I found $gmane/169098/focus=169104 which covers some of 
the issues (the focused msg is one from Junio). Hannes then notes 
($gmane/169121) that the current xx = xx; could be seen as possible 
undefined behaviour - perhaps one of those 'no good solution' problems.


Perhaps a suitable name...

#define SUPPRESS_COMPILER_UNINITIALIZED_WARNING 0
/* Use when some in-use compiler is unable to determine if the variable is 
used uninitialized, and no good default value is available */


Though, where best to put it?

--
Philip 


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t4151 missing quotes

[Stefan Beller]

On Mon, May 9, 2016 at 11:56 AM, Junio C Hamano  wrote:
> Something like this follows Documentation/SubmittingPatches, except
> that it further needs your Sign-off before mine, which I can forge
> if you say it is OK.

The sign-off is a simple line at the end of the explanation for
the patch, which certifies that you wrote it or otherwise have
the right to pass it on as a open-source patch.  The rules are
pretty simple: if you can certify the below:

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.


>
> Thanks for a report and an analysis of the issue.
>
> -- >8 --
> From: Armin Kunaschik 
> Subject: t4151: make sure argument to 'test -z' is given
>
> 88d50724 (am --skip: revert changes introduced by failed 3way merge,
> 2015-06-06), unlike all the other patches in the series, forgot to
> quote the output from "$(git ls-files -u)" when using it as the
> argument to "test -z", leading to a syntax error.
>
> Note that $(git ls-files -u | wc -l) are deliberately left unquoted,
> as some implementations of "wc -l" includes extra blank characters
> in its output and cannot be compared as string, i.e. "test 0 = $(...)".
>
> Signed-off-by:
> Signed-off-by: Junio C Hamano 
> ---
>  t/t4151-am-abort.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
> index 833e7b2..b878c21 100755
> --- a/t/t4151-am-abort.sh
> +++ b/t/t4151-am-abort.sh
> @@ -82,7 +82,7 @@ test_expect_success 'am -3 --abort removes otherfile-4' '
> test 4 = "$(cat otherfile-4)" &&
> git am --abort &&
> test_cmp_rev initial HEAD &&
> -   test -z $(git ls-files -u) &&
> +   test -z "$(git ls-files -u)" &&
> test_path_is_missing otherfile-4
>  '
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t6041: do not compress backup tar file

[Stefan Beller]

+ cc Jens as he authored both t6041 as well as t3513 in
the series leading to ad25da009e2a3730 (2014-07-21,
Merge branch 'jl/submodule-tests')

On Mon, May 9, 2016 at 11:46 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> diff --git a/t/t6041-bisect-submodule.sh b/t/t6041-bisect-submodule.sh
>> index c6b7aa6..62b8a2e 100755
>> --- a/t/t6041-bisect-submodule.sh
>> +++ b/t/t6041-bisect-submodule.sh
>> @@ -8,7 +8,7 @@ test_description='bisect can handle submodules'
>>  git_bisect () {
>>   git status -su >expect &&
>>   ls -1pR * >>expect &&
>> - tar czf "$TRASH_DIRECTORY/tmp.tgz" * &&
>> + tar cf "$TRASH_DIRECTORY/tmp.tar" * &&
>>   GOOD=$(git rev-parse --verify HEAD) &&
>>   git checkout "$1" &&
>>   echo "foo" >bar &&
>> @@ -20,7 +20,7 @@ git_bisect () {
>>   git bisect start &&
>>   git bisect good $GOOD &&
>>   rm -rf * &&
>> - tar xzf "$TRASH_DIRECTORY/tmp.tgz" &&
>> + tar xf "$TRASH_DIRECTORY/tmp.tar" &&
>>   git status -su >actual &&
>>   ls -1pR * >>actual &&
>>   test_cmp expect actual &&
>
> While I am fine with taking this (and the other) change, which are
> the only sensible response to these bug reports, this makes me
> wonder two things and a half.
>
>  * Why do we even run "tar", especially without having a
>test_when_finished clean-up?  Can't there be better ways to test
>this and the other one without creating a copy of everything in
>the test directory?

I think some of the submodule testing is a test machinery on its own.
Any submodule test that is not in t74* follows the pattern to
provide a short function for its testing and then call  test_submodule_switch
with some long option, e.g.
KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1

I haven't attempted to look into these tests yet as they seem to test
fundamentals ("submodules as files" in other commands), whereas
I am more interested in some new features currently.

>
>  * Are we sure the trash directory contents is kept manageable size?

The testing in lib-submodule-update.sh do not seem to take care of
these tarballs. And the small testing scripts do not either, but there
we could inject a

test_when_finished "rm ..."

snippet IIUC.

>I am primarily worried about this "we are not sure what we will
>be clobbering, so let's blindly tar up everything and restore it
>later" pattern spreading and people mistakenly use it in tests
>that simulate our behaviour on a huge blob with a sparse but
>gigantic file.
>
>  * Is an addition of 'test_snapshot $tarball *' and 'test_restore
>$tarball' pair too much unnecessary refactoring to cater to only
>two users of this "let's tar up everything" pattern, given that
>we want to _discourage_ use of this pattern in the longer term?

As said before, I did not yet dive into these test areas myself.

>From a birds eye, there was not a lot of discussion around that series
(as compared to submodule groups for example). Maybe I am missing
prior or later series though.

http://thread.gmane.org/gmane.comp.version-control.git/251682


>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t4151 missing quotes

[Junio C Hamano]

Something like this follows Documentation/SubmittingPatches, except
that it further needs your Sign-off before mine, which I can forge
if you say it is OK.

Thanks for a report and an analysis of the issue.

-- >8 --
From: Armin Kunaschik 
Subject: t4151: make sure argument to 'test -z' is given

88d50724 (am --skip: revert changes introduced by failed 3way merge,
2015-06-06), unlike all the other patches in the series, forgot to
quote the output from "$(git ls-files -u)" when using it as the
argument to "test -z", leading to a syntax error.

Note that $(git ls-files -u | wc -l) are deliberately left unquoted,
as some implementations of "wc -l" includes extra blank characters
in its output and cannot be compared as string, i.e. "test 0 = $(...)".

Signed-off-by: 
Signed-off-by: Junio C Hamano 
---
 t/t4151-am-abort.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
index 833e7b2..b878c21 100755
--- a/t/t4151-am-abort.sh
+++ b/t/t4151-am-abort.sh
@@ -82,7 +82,7 @@ test_expect_success 'am -3 --abort removes otherfile-4' '
test 4 = "$(cat otherfile-4)" &&
git am --abort &&
test_cmp_rev initial HEAD &&
-   test -z $(git ls-files -u) &&
+   test -z "$(git ls-files -u)" &&
test_path_is_missing otherfile-4
 '
 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t6041: do not compress backup tar file

[Junio C Hamano]

Stefan Beller  writes:

> diff --git a/t/t6041-bisect-submodule.sh b/t/t6041-bisect-submodule.sh
> index c6b7aa6..62b8a2e 100755
> --- a/t/t6041-bisect-submodule.sh
> +++ b/t/t6041-bisect-submodule.sh
> @@ -8,7 +8,7 @@ test_description='bisect can handle submodules'
>  git_bisect () {
>   git status -su >expect &&
>   ls -1pR * >>expect &&
> - tar czf "$TRASH_DIRECTORY/tmp.tgz" * &&
> + tar cf "$TRASH_DIRECTORY/tmp.tar" * &&
>   GOOD=$(git rev-parse --verify HEAD) &&
>   git checkout "$1" &&
>   echo "foo" >bar &&
> @@ -20,7 +20,7 @@ git_bisect () {
>   git bisect start &&
>   git bisect good $GOOD &&
>   rm -rf * &&
> - tar xzf "$TRASH_DIRECTORY/tmp.tgz" &&
> + tar xf "$TRASH_DIRECTORY/tmp.tar" &&
>   git status -su >actual &&
>   ls -1pR * >>actual &&
>   test_cmp expect actual &&

While I am fine with taking this (and the other) change, which are
the only sensible response to these bug reports, this makes me
wonder two things and a half.

 * Why do we even run "tar", especially without having a
   test_when_finished clean-up?  Can't there be better ways to test
   this and the other one without creating a copy of everything in
   the test directory?

 * Are we sure the trash directory contents is kept manageable size?
   I am primarily worried about this "we are not sure what we will
   be clobbering, so let's blindly tar up everything and restore it
   later" pattern spreading and people mistakenly use it in tests
   that simulate our behaviour on a huge blob with a sparse but
   gigantic file.

 * Is an addition of 'test_snapshot $tarball *' and 'test_restore
   $tarball' pair too much unnecessary refactoring to cater to only
   two users of this "let's tar up everything" pattern, given that
   we want to _discourage_ use of this pattern in the longer term?

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 1/1] t5601: Remove trailing space in sed expression

[Junio C Hamano]

tbo...@web.de writes:

> From: Torsten Bögershausen 
>
> The sed expression for IPv6, "Tested User And Host" or "tuah" used a wrong
> sed expression, which doesn't work under all versions of sed.
>
> Reported-By: Armin Kunaschik 
> Signed-off-by: Torsten Bögershausen 
> ---

Good.  Thanks.

>  t/t5601-clone.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index 150aeaf..a433394 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -466,7 +466,7 @@ test_expect_success 'clone ssh://host.xz:22/~repo' '
>  #IPv6
>  for tuah in ::1 [::1] [::1]: user@::1 user@[::1] user@[::1]: [user@::1] 
> [user@::1]:
>  do
> - ehost=$(echo $tuah | sed -e "s/1]:/1]/ "| tr -d "[]")
> + ehost=$(echo $tuah | sed -e "s/1]:/1]/" | tr -d "[]")
>   test_expect_success "clone ssh://$tuah/home/user/repo" "
> test_clone_url ssh://$tuah/home/user/repo $ehost /home/user/repo
>   "
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t6044 broken on pu

[Junio C Hamano]

Junio C Hamano  writes:

> Yes, I think the comment should just go.  Nobody used that alphabet
> form since it was written in d17cf5f3 (tests: Introduce test_seq,
> 2012-08-04).
>
>> I don't really care either way whether it is replaced or not (at one
>> point there were some people really interested in NO_PERL not even using
>> one-liners in the test suite, but I am not one of them).
>
> Neither am I, but I think it is prudent to drop that "letters".  The
> comment even says the letter form is not portable already, so the
> mention of GNU seq(1) is not helping at all.

-- >8 --
Subject: test-lib-functions.sh: remove misleading comment on test_seq

We never used the "letters" form since we came up with "test_seq" to
replace use of non-portable "seq" in our test script, which we
introduced it at d17cf5f3 (tests: Introduce test_seq, 2012-08-04).

We use this helper to either iterate for N times (i.e. the values on
the lines do not even matter), or just to get N distinct strings
(i.e. the values on the lines themselves do not really matter, but
we care that they are different from each other and reproducible).

Stop promising that we may allow using "letters"; this would open an
easier reimplementation that does not rely on $PERL, if somebody
later wants to.

Signed-off-by: Junio C Hamano 
---
 t/test-lib-functions.sh | 21 +++--
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 8d99eb3..cc9f61d 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -718,20 +718,13 @@ test_cmp_rev () {
test_cmp expect.rev actual.rev
 }
 
-# Print a sequence of numbers or letters in increasing order.  This is
-# similar to GNU seq(1), but the latter might not be available
-# everywhere (and does not do letters).  It may be used like:
-#
-#  for i in $(test_seq 100)
-#  do
-#  for j in $(test_seq 10 20)
-#  do
-#  for k in $(test_seq a z)
-#  do
-#  echo $i-$j-$k
-#  done
-#  done
-#  done
+# Print a sequence of integers in increasing order, either with
+# two arguments (start and end):
+#
+# test_seq 1 5 -- outputs 1 2 3 4 5 one line at a time
+#
+# or with one argument (end), in which case it starts counting 
+# from 1.
 
 test_seq () {
case $# in
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t6044 broken on pu

[Junio C Hamano]

Jeff King  writes:

> On Mon, May 09, 2016 at 12:02:45PM -0400, Eric Sunshine wrote:
>
>> > I had that thought, too, but I think it would be an error to do so.
>> > test_seq is supposed to be a replacement for "seq", which does not
>> > understand non-numeric sequences.
>> 
>> Although, the comment block just above test_seq() in
>> test-lib-functions.sh says otherwise:
>> 
>> Print a sequence of numbers or letters in increasing order.  This
>> is similar to GNU seq(1), but the latter might not be available
>> everywhere (and does not do letters).  It may be used like:
>> 
>> for i in $(test_seq 100)
>> do
>> for j in $(test_seq 10 20)
>> do
>> for k in $(test_seq a z)
>> do
>> echo $i-$j-$k
>> done
>> done
>> done
>
> Oh, indeed. I apparently even Acked that documentation once upon a time. :-/
>
> Anyway, I double-checked my earlier "grep" and I do not think anybody is
> using that functionality. So I think we'd be OK to change it as long as
> we updated the documentation to match.

Yes, I think the comment should just go.  Nobody used that alphabet
form since it was written in d17cf5f3 (tests: Introduce test_seq,
2012-08-04).

> I don't really care either way whether it is replaced or not (at one
> point there were some people really interested in NO_PERL not even using
> one-liners in the test suite, but I am not one of them).

Neither am I, but I think it is prudent to drop that "letters".  The
comment even says the letter form is not portable already, so the
mention of GNU seq(1) is not helping at all.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t6044 broken on pu

[Junio C Hamano]

Torsten Bögershausen  writes:

> On 08.05.16 20:20, Junio C Hamano wrote:
>> Torsten Bögershausen  writes:
>>
>>> May a  simple
>>>  printf "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n"
>>>
>>> be an option ?
>> If you were to do that, at least have the decency to make it more
>> readable by doing something like:
>>
>>  printf "%s\n" 1 2 3 4 5 6 7 8 9 10
>>
>> ;-)
>>
>> But as I said, as a response to "t6044 broken on pu" bug report,
>> s/seq/test_seq/ is the only sensible change.
>>
>> Improving "test_seq, the alternative to seq" is a separate topic.
>>
>> If you have aversion to $PERL, perhaps do them without using
>> anything what is not expected to be built-in in modern shells,
>> perhaps like this?
> Please don't get me wrong -

I don't.

> I wasn't really clear why:

So is it now clear?

The title of the bug report is "t6044 broken on pu". The analysis of
the issue is "We use 'test_seq 1 10' when we want one to ten on each
line in the output to use in test to make sure it is portable, but a
new commit forgot the portability issue and used seq instead".

The only sensible fix is "Use test_seq like everybody else".

Improving the implementation of test_seq is a totally separate
issue.  It may be a good thing to do independently to save external
program, and the effect of such change will not be limited to t6044
but all other existing users of test_seq.

And that is why it must be done as a separate topic.

Why you think it is a good idea to update t6044 with printf
"1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n" is beyond me--don't you want to
make sure that existing users of test_seq benefits the same way?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t4151 missing quotes

[Armin Kunaschik]

I'm currently concentrating on finding problems with my setup... this
is already a tough job :-)
I'm a git beginner, and Documentation/SubmittingPatches would keep me
busy for a week.
So anybody feel free to submit this thingy.

Armin
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t4151 missing quotes

[Eric Sunshine]

[please don't top-post on this list]

On Mon, May 9, 2016 at 12:35 PM, Armin Kunaschik
 wrote:
> Sorry, this was my first patch to the list. I'll do better :-)
> You are right about the "wc -l" parts. Maybe I was a bit over
> pessimistic. Throw away my last mail.

Done :-)

> In my case test 9 ran unsuccessful because of an empty "git ls-files -u"

Okay, that makes perfect sense and does indeed deserve to be fixed.

> This reduces the diff to this one (hopefully the right way now):

Perhaps you can turn this into a proper patch acceptable for inclusion
in the project. If you're interested in attempting this, take a look
at Documentation/Submitting.

> *** ./t4151-am-abort.sh.origFri Apr 29 23:37:00 2016
> --- ./t4151-am-abort.sh Mon May  9 18:28:18 2016
> ***
> *** 82,88 
> test 4 = "$(cat otherfile-4)" &&
> git am --abort &&
> test_cmp_rev initial HEAD &&
> !   test -z $(git ls-files -u) &&
> test_path_is_missing otherfile-4
>   '
>
> --- 82,88 
> test 4 = "$(cat otherfile-4)" &&
> git am --abort &&
> test_cmp_rev initial HEAD &&
> !   test -z "$(git ls-files -u)" &&
> test_path_is_missing otherfile-4
>   '

This fix looks fine. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: deadlock git upload-pack command when GIT_TRACE is enabled

[Stefan Beller]

On Mon, May 9, 2016 at 10:53 AM, Stefan Beller  wrote:
> On Mon, May 9, 2016 at 10:49 AM, Eugene Petrenko
>  wrote:
>> Hello,
>>
>> I stuck around the deadlock inside git when running git upload-pack .
>> command. A debugging shown that the bottom process (it starts several
>> processes to implement the task) hangs writing to stderr. I managed to
>> reproduce the issue with a tiny bash script. The repository and the
>> script is found here
>> https://github.com/jonnyzzz/git-upload-pack-deadlock
>>
>> I saw the issue reproducing both under Windows and Linux/Mac.
>
> GIT_TRACE=true GIT_TRACE_PACKET=true GIT_TRACE_PACK_ACCESS=true
>
> What do you mean by true?
>
>
>If this variable is set to "1", "2" or "true" (comparison
> is case insensitive), trace messages will be printed to stderr.

Oh dang. I should read what I quote.




>
>If the variable is set to an integer value greater than 2
> and lower than 10 (strictly) then Git will interpret this value as an
> open file descriptor and will try to write the trace
>messages into this file descriptor.
>
>Alternatively, if the variable is set to an absolute path
> (starting with a / character), Git will interpret this as a file path
> and will try to write the trace messages into it.
>
>Unsetting the variable, or setting it to empty, "0" or
> "false" (case insensitive) disables trace messages.
>
>>
>> Windows thread dumps are available here
>> https://github.com/jonnyzzz/git-upload-pack-deadlock/tree/master/debug
>>
>>
>> According to those thread dumps I see the following problem around
>> upload-pack.c line 129. There the pack_objects command is executed.
>> First the wants block is pushed to the command, next the stdout
>> processing is started. This means, that pack_objects process output is
>> not processed until all output is put there. In the case I have, the
>> pack_objects process writes TRACE logging into stderr and eventually
>> (on hug repo) the OS buffer runs-out deadlocking the execution.
>>
>>
>> Best regards,
>> Eugene Petrenko
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: deadlock git upload-pack command when GIT_TRACE is enabled

[Stefan Beller]

On Mon, May 9, 2016 at 10:49 AM, Eugene Petrenko
 wrote:
> Hello,
>
> I stuck around the deadlock inside git when running git upload-pack .
> command. A debugging shown that the bottom process (it starts several
> processes to implement the task) hangs writing to stderr. I managed to
> reproduce the issue with a tiny bash script. The repository and the
> script is found here
> https://github.com/jonnyzzz/git-upload-pack-deadlock
>
> I saw the issue reproducing both under Windows and Linux/Mac.

GIT_TRACE=true GIT_TRACE_PACKET=true GIT_TRACE_PACK_ACCESS=true

What do you mean by true?


   If this variable is set to "1", "2" or "true" (comparison
is case insensitive), trace messages will be printed to stderr.

   If the variable is set to an integer value greater than 2
and lower than 10 (strictly) then Git will interpret this value as an
open file descriptor and will try to write the trace
   messages into this file descriptor.

   Alternatively, if the variable is set to an absolute path
(starting with a / character), Git will interpret this as a file path
and will try to write the trace messages into it.

   Unsetting the variable, or setting it to empty, "0" or
"false" (case insensitive) disables trace messages.

>
> Windows thread dumps are available here
> https://github.com/jonnyzzz/git-upload-pack-deadlock/tree/master/debug
>
>
> According to those thread dumps I see the following problem around
> upload-pack.c line 129. There the pack_objects command is executed.
> First the wants block is pushed to the command, next the stdout
> processing is started. This means, that pack_objects process output is
> not processed until all output is put there. In the case I have, the
> pack_objects process writes TRACE logging into stderr and eventually
> (on hug repo) the OS buffer runs-out deadlocking the execution.
>
>
> Best regards,
> Eugene Petrenko
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

deadlock git upload-pack command when GIT_TRACE is enabled

[Eugene Petrenko]

Hello,

I stuck around the deadlock inside git when running git upload-pack .
command. A debugging shown that the bottom process (it starts several
processes to implement the task) hangs writing to stderr. I managed to
reproduce the issue with a tiny bash script. The repository and the
script is found here
https://github.com/jonnyzzz/git-upload-pack-deadlock

I saw the issue reproducing both under Windows and Linux/Mac.

Windows thread dumps are available here
https://github.com/jonnyzzz/git-upload-pack-deadlock/tree/master/debug


According to those thread dumps I see the following problem around
upload-pack.c line 129. There the pack_objects command is executed.
First the wants block is pushed to the command, next the stdout
processing is started. This means, that pack_objects process output is
not processed until all output is put there. In the case I have, the
pack_objects process writes TRACE logging into stderr and eventually
(on hug repo) the OS buffer runs-out deadlocking the execution.


Best regards,
Eugene Petrenko
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 1/1] t5601: Remove trailing space in sed expression

[tboegi]

From: Torsten Bögershausen 

The sed expression for IPv6, "Tested User And Host" or "tuah" used a wrong
sed expression, which doesn't work under all versions of sed.

Reported-By: Armin Kunaschik 
Signed-off-by: Torsten Bögershausen 
---
 t/t5601-clone.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 150aeaf..a433394 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -466,7 +466,7 @@ test_expect_success 'clone ssh://host.xz:22/~repo' '
 #IPv6
 for tuah in ::1 [::1] [::1]: user@::1 user@[::1] user@[::1]: [user@::1] 
[user@::1]:
 do
-   ehost=$(echo $tuah | sed -e "s/1]:/1]/ "| tr -d "[]")
+   ehost=$(echo $tuah | sed -e "s/1]:/1]/" | tr -d "[]")
test_expect_success "clone ssh://$tuah/home/user/repo" "
  test_clone_url ssh://$tuah/home/user/repo $ehost /home/user/repo
"
-- 
2.0.0.rc1.6318.g0c2c796

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2

[Junio C Hamano]

Jeff King  writes:

> On Mon, May 09, 2016 at 09:42:32AM -0700, Junio C Hamano wrote:
>
>> > Hopefully your patch to remove the -c ... sanitizing makes it to `master`
>> > soon, then I can submit my next iteration.
>> 
>> Or we can just merge that "do not sanitize" branch in, and then
>> queue the "next iteration" which I'd assume would only be the test
>> addition?
>
> I think we'd also want the change to the test script to make sure that
> it fails with only a single header (Dscho's patch 2).

I think so, too.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t6302: simplify non-gpg cases

[Eric Sunshine]

On Mon, May 9, 2016 at 12:49 PM, Jeff King  wrote:
> On Mon, May 09, 2016 at 12:30:43PM -0400, Eric Sunshine wrote:
> Since you as the author of 618310a seem to agree with this direction,
> here it is as a real patch.

Thanks for working on this.

> Subject: [PATCH] t6302: simplify non-gpg cases
>
> When commit 618310a taught t6302 to run without the GPG

618310a (t6302: skip only signed tags rather than all tests when GPG
is missing, 2016-03-06)

> prerequisite, it did so by conditionally creating the signed
> tags only when gpg is available. As a result, further tests
> need to take this into account, which they can do with the
> test_prepare_expect helper. This is a minor hassle, though,
> as the helper cannot easily cover all cases (it just matches
> "signed" in the output, so all output must include the
> actual refname).

Should we cite bc9acea (ref-filter: implement %(if), %(then), and
%(else) atoms, 2016-04-25) here as an example of a commit for which
this was problematic (and which indeed broke the tests when GPG is
unavailable)?

> Instead, let's take a different approach. We'll always
> create the tags, and only conditionally sign them. This does
> mean our tag-names are a minor lie, but it lets the tests
> which do not care about signing easily behave the same in
> all settings. We'll include a comment to document our lie
> and avoid confusing further test-writers.
>
> Signed-off-by: Jeff King 

Looks good. With or without the minor change below, this patch is:

Reviewed-by: Eric Sunshine 

> ---
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
>  test_expect_success 'check signed tags with --points-at' '
> -   test_prepare_expect <<-\EOF | sed -e "s/Z$//" >expect &&
> +   cat <<-\EOF | sed -e "s/Z$//" >expect &&

To make this as close to a reversion as possible, this could be
restored to the original (sans 'cat'):

sed -e "s/Z$//" >expect <<-\EOF &&

> refs/heads/side Z
> refs/tags/annotated-tag four
> refs/tags/four Z
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 80/83] run-command: make dup_devnull() non static

[Junio C Hamano]

Duy Nguyen  writes:

> I vote one step at a time, leave multi-thread support for future.
> There's a lot more shared state than file descriptors anyway, at least
> there are object db and index access and probably a couple of hidden
> static variables somewhere. And I'm not sure if multi-thread really
> helps here. Are we really CPU-bound? If object inflation causes that
> (wild guess), can we just inflate ahead in some separate process and
> pass the result back?

I do not particularly care about multi-thread issues, but I have to
agree with Dscho that the updated code that claims to be "libified"
that futz with the file descriptors like the way this patch does is
not a proper libification.

Unfortunately, the anticipated caller of this code that does "this
may fail and it is OK because it is merely one of the attempts, so
let's not show the errors" is not something we call only when we are
falling back, so "why not do this rare codepath via the usual
run_command() interface to spawn 'apply' as a separate process?",
which would be the most sensible "one step at a time" suggestion if
it were the case, would not apply.

As you will be passing the apply state structure throughout the
callchain, would it be a viable and reasonable endgame state to have
a strbuf in it that accumulates the errors?  That is, instead of
dup()ing the standard error stream out, you would accumulate the
errors for a caller that asks their errors not directly sent to
the standard error stream, so that it can choose to either show it
at the end, or ignore it altogether.

How far can you go with just set-error-routine?  Are there things,
other than the file descriptors, that you need to futz with in order
to covert that "we'd fallback, so this early round must be silent"
codepath?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Found possible branch point, then "Use of uninitialized value $u in substitution/concatenation"

[Thierry Suzanne]

On 7 May 2016 at 08:07, Christian Couder  wrote:
>
> On Fri, May 6, 2016 at 5:31 PM, Thierry Suzanne  
> wrote:
> > Sorry, can't help :(
>
> I just sent a patch. It would be nice if you could test it to confirm
> that you get a nicer error message.
>
> Also please reply below other people's comments, as customary on this list.


Hi,

Amended my local svn.pm file with your 2 lines.

First time:
--
C:\code\myproduct\trunk>git svn clone
https://mycompany.svn.beanstalkapp.com/myproduct --no-metadata -A
c:\temp\svn_to_git_users.txt
--trunk=https://mycompany.svn.beanstalkapp.com/myproduct
--tags=https://mycompany.svn.beanstalkapp.com/myproduct/tags
--branches=https://mycompany.svn.beanstalkapp.com/myproduct/branches
c:\code\Git_TestPatch
[]
W: +empty_dir: branches/20080918_DBDEPLOY/vendor/src/csharp/MS WCSF
Contrib/src/Extensions/Silverlight
W: +empty_dir: branches/20080918_DBDEPLOY/vendor/src/csharp/MS WCSF
Contrib/src/Services
W: +empty_dir: 
branches/20080918_DBDEPLOY/vendor/src/csharp/RealWorldControls/References
r530 = c276e3b039d8e38759c6fb17443349732552d7a2 (refs/remotes/origin/trunk)
Found possible branch point:
https://mycompany.svn.beanstalkapp.com/myproduct/trunk =>
https://mycompany.svn.beanstalkapp.com/myproduct/branches/20080918_DBDEPLOY,
529
Initializing parent: refs/remotes/origin/20080918_DBDEPLOY@529
W: +empty_dir: trunk/etc
W: +empty_dir: trunk/src/csharp
[...]
W: +empty_dir: trunk/test
W: +empty_dir: trunk/vendor
r5 = c71eabc20ff1f4e3fd728727470a2fa5a3802891
(refs/remotes/origin/20080918_DBDEPLOY@529)
A   src/database/tables/IllustrationRow.tbl
A   src/database/tables/LegalEntity.tbl
[...]
--

No error!
Ctrl+C to interrupt, ran exact same command again, this time the new error:
--
C:\code\myproduct\trunk>git svn clone
https://mycompany.svn.beanstalkapp.com/myproduct --no-metadata -A
c:\temp\svn_to_git_users.txt
--trunk=https://mycompany.svn.beanstalkapp.com/myproduct
--tags=https://mycompany.svn.beanstalkapp.com/myproduct/tags
--branches=https://mycompany.svn.beanstalkapp.com/myproduct/branches
c:\code\Git_TestPatch
Found possible branch point:
https://mycompany.svn.beanstalkapp.com/myproduct/trunk =>
https://mycompany.svn.beanstalkapp.com/myproduct/branches/20080918_DBDEPLOY,
529
refs/remotes/origin/20080918_DBDEPLOY@529: no associated commit metadata

C:\code\myproduct\trunk>
--

So the repro steps are potentially different. Anyway, thanks for
taking interest and handling the error nicely.

For the error message, as a beginner end user converting svn to git,
the message "no associated commit metadata" doesn't really make me
think my command line is wrong. But I'm sure with time this message
will appear on stackoverflow with its true meaning :-)

Thanks,
T
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Re: [RFD/PATCH] submodule doc: describe where we can configure them

[Stefan Beller]

>> Here is what I imagine
>> When B mirrors from A, B sets up this special ref for its repository,
>> e.g. refs/meta/submodule-B and have a symbolic ref pointing at that.
>> (e.g. SUBMODULE_CONFIG pointing at refs/meta/submodule-B,
>> which has a worktree which contains a .gitmodules files which
>> sets up
>>   "submodule.baz.url = http://B/baz;
>>   "submodule.relativeBase = http://A;
>>
>> That way anyone cloning from B would get
>> the superproject and the submodule baz from B while the
>> rest of the submodules are found at A.
>
> This sounds sensible. But my imagination of a conflict was in a
> different way. E.g. project A has a submodule B. And now A has a remote
> 1 where you publish and maybe another remote 2 where someone else (a
> colleague?) publishes. Which configuration do you use? Here the two
> remotes are independent instead of subsequent forks. In this case my
> solution would be to use the configuration branch from 1 for B when
> interacting with 1. I do not have completely checked whether we always
> have a remote at hand for such a resolution.

I think it is the responsibility of the pusher to make sure the
configuration is sane.
So if I were to push to remote 2 and you push to remote 1, we'd both configure
the special branch of our superprojects for these remotes for that submodule.

If the superproject has relative urls for the submodule, all we had to do was
unset (or overwrite) the submodule.baseConfig.

>
>> When C mirrors from A, they add another branch  refs/meta/submodule-C,
>> which can either be a fork of refs/meta/submodule-B with some changes on
>> top of it or it can add a reference to refs/meta/submodule-B, i.e. the
>> configuration
>> would be:
>>
>>   "submodule.baseConfig = refs/meta/submodule-B"
>>   "submodule.foo.url = ssh://C/foo"
>>
>> and SUBMODULE_CONFIG would point at refs/meta/submodule-C.
>>
>> When cloning from C, the user would get
>>
>>  * the superproject from C
>>  * submodule foo from C
>>  * submodule baz from B
>>  * all other submodules from A
>>
>> By the inheriting property of the branch of B there are no conflicting 
>> values.
>> C could just overwrite submodule.baseConfig for example.
>
> So that means in the default case we create a chain of all previous
> forks embedded in repository database.

Not necessarily. I was just pointing out that this was possible. The
intermediate
party could decide that their upstream is too unreliable and not point
to their upstream.

This would incur the cost of having to clone all submodules and
overwriting the absolute
urls. For the relative URLs this would just work as of now.

All I wanted with that example is to offer the flexibility to not have
to clone all the
submodule, but I can fork a mega-project with 100s of submodules and maybe
just fiddle with one of them and then publish that.

> I am not saying that this is
> necessarily a bad thing but I feel that it is a new property which we
> should think about. It helps because users will get updated values from
> sources that are in the chain. On the other hand it adds a lot of
> dependencies which are point of failures in case a remote disappears. I
> am undecided on this. I would prefer if we could let people play with it
> a little (maybe on pu?) and then decide if there are practical pitfalls
> with this.
>
>> > My suggested scheme above does not solve the currently quite typical use
>> > case where you might 'git fetch' without submodules first and then do
>> > the submodule fetches during a 'git submodule update'. On the other hand
>> > in the 'ideal future world' where submodules behave like "normal files" the
>> > fetch will be done during the superproject fetch so in that case we
>> > could solve such conflicts.
>> >
>> > The main thing which we could keep in mind is that we only allow certain
>> > values in such special refs. E.g. only the ones needed to support the
>> > fork workflow. BTW, do we actually need to change other values than the
>> > URL? Addtionally we ignore other values that are more related to the
>> > overall project structure. E.g. like submodule..ignore.
>>
>> Maybe we want to have a dedicated protocol field, eventually.
>> A,B,C may have different standards on what they use by default.
>> e.g. Use ssh at kernel.org, but http in a corporate mirror, because http is
>> the only protocol not blocked by firewall. So I could imagine that a
>> complete mirror of submodules with relative URLs wants to only replace
>> ssh by http.
>
> By this you mean 'submodule.relativeBase' that you introduced above
> right? Or something similar. These values I would still consider them
> URL'ish. But my question was more geared towards this direction: Are
> there other values than the ones used to assemble the URL that make
> sense to share?
>
> E.g.: Someone might want to fork a repository and might want to change
> the default set of submodules that are populated with 'git submodule
> update --init'. Is this something we should allow via these 

Re: [PATCH v2 1/2] mingw: introduce the 'core.hideDotFiles' setting

[Junio C Hamano]

Johannes Schindelin  writes:

> +core.hideDotFiles::
> + (Windows-only) If true, mark newly-created directories and files whose
> + name starts with a dot as hidden.  If 'dotGitOnly', only the `.git/`
> + directory is hidden, but no other files starting with a dot.  The
> + default mode is to mark only the `.git/` directory as hidden.

I think "The default mode is 'dotGitOnly'" is sufficient, given that
it is described just one sentence before, which is still likely to
be in readers' mind.  But I'll let it pass without tweaking.

> +enum hide_dotfiles_type {
> + HIDE_DOTFILES_FALSE = 0,
> + HIDE_DOTFILES_TRUE,
> + HIDE_DOTFILES_DOTGITONLY,

We allow ',' after the last array initializer, but not after the
last enum definition.  I'll tweak it out while queuing.

> @@ -319,6 +364,21 @@ int mingw_open (const char *filename, int oflags, ...)
>   if (attrs != INVALID_FILE_ATTRIBUTES && (attrs & 
> FILE_ATTRIBUTE_DIRECTORY))
>   errno = EISDIR;
>   }
> + if ((oflags & O_CREAT) && needs_hiding(filename)) {
> + /*
> +  * Internally, _wopen() uses the CreateFile() API which errors
> +  * out with an ERROR_ACCESS_DENIED if CREATE_ALWAYS was
> +  * specified and an already existing file's attributes do not
> +  * match *exactly*. As there is no mode or flag we can set that
> +  * would correspond to FILE_ATTRIBUTE_HIDDEN, let's just try
> +  * again *without* the O_CREAT flag (that corresponds to the
> +  * CREATE_ALWAYS flag of CreateFile()).
> +  */
> + if (fd < 0 && errno == EACCES)
> + fd = _wopen(wfilename, oflags & ~O_CREAT, mode);

This "retry if we got EACCESS" felt strange to me in two ways.  One
is explained well in the comment and you know what you are doing, as
opposed to me who is clueless with CreateFile() API.

The other is why you do not have to retry creation in a similar way
when !needs_hiding(filename).  I didn't see anything in the function
before reaching this point that does anything differently based on
needs_hiding().  Can't the same 'ERROR_ACCESS_DENIED' error trigger
if CREATE_ALWAYS was specified and file attributes of an existing
file match, and if it can, don't you want to retry that too, even if
you are not going to hide the filename?

That is, I am wondering, without knowing much about Windows API, why
the code is more like this:

fd = _wopen(...);
if (fd < 0 && ...) {
if (attrs != INVALID_...)
errno = ISDIR;
}
if ((oflags & O_CREAT) && fd < 0 && errno == EACCESS)
/* That big comment here ... */
fd = _open(wfilename, oflags & ~O_CREAT, mode);
if ((oflags & O_CREAT) && needs_hiding(filename)) {
if (fd >= 0 && set_hidden_flag(wfilename, 1))
warning("could not mark '%s' as hidden"...);
}

Obviously, I will *not* do this tweak myself ;-)


> + if (fd >= 0 && set_hidden_flag(wfilename, 1))
> + warning("Could not mark '%s' as hidden.", filename);
> + }

I'll tweak all new instances of "Could" with s/Could/could/ to save
Micheal trouble (cf. b846ae21daf).


Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t6041: do not compress backup tar file

[Armin Kunaschik]

Hi Stefan,

I'm currently in the process of skipping through the failed tests on my AIX box.
There are more tests which require GNU tools like mktemp
(t7610-mergetool.sh) or readlink (t7800-difftool.sh).
But I don't have a solution or workaround for these two.
But at least there are not more failing compressed tar tests :-)

Thanks,
Armin

On Mon, May 9, 2016 at 7:09 PM, Stefan Beller  wrote:
> The test uses the 'z' option, i.e. "compress the output while at
> it", which is GNUism and not portable.
>
> Reported-by: Armin Kunaschik 
> Signed-off-by: Stefan Beller 
> ---
>
>  Thanks Armin for reporting these GNUism!
>  Are there any more? (So we can do these patches as a
>  series instead of one by one:)
>
>  developed on top of origin/sb/z-is-gnutar-ism
>
>  Thanks,
>  Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

t5601-clone.sh failures

[Armin Kunaschik]

Hi list,

eight t5601 tests don't run with my version of sed.
The reason is a trailing space in the sed expression. See below:

#IPv6
for tuah in ::1 [::1] [::1]: user@::1 user@[::1] user@[::1]:
[user@::1] [user@::1]:
do
ehost=$(echo $tuah | sed -e "s/1]:/1]/ "| tr -d "[]")
test_expect_success "clone ssh://$tuah/home/user/repo" "
  test_clone_url ssh://$tuah/home/user/repo $ehost /home/user/repo
"
done

I can not imagine a reason why there should be a space character...
The tests run fine here without the space.
When there isn't any reason, the trailing space should be removed.

Armin
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] t6041: do not compress backup tar file

[Stefan Beller]

The test uses the 'z' option, i.e. "compress the output while at
it", which is GNUism and not portable.

Reported-by: Armin Kunaschik 
Signed-off-by: Stefan Beller 
---

 Thanks Armin for reporting these GNUism!
 Are there any more? (So we can do these patches as a
 series instead of one by one:)
 
 developed on top of origin/sb/z-is-gnutar-ism
 
 Thanks,
 Stefan

 t/t6041-bisect-submodule.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t6041-bisect-submodule.sh b/t/t6041-bisect-submodule.sh
index c6b7aa6..62b8a2e 100755
--- a/t/t6041-bisect-submodule.sh
+++ b/t/t6041-bisect-submodule.sh
@@ -8,7 +8,7 @@ test_description='bisect can handle submodules'
 git_bisect () {
git status -su >expect &&
ls -1pR * >>expect &&
-   tar czf "$TRASH_DIRECTORY/tmp.tgz" * &&
+   tar cf "$TRASH_DIRECTORY/tmp.tar" * &&
GOOD=$(git rev-parse --verify HEAD) &&
git checkout "$1" &&
echo "foo" >bar &&
@@ -20,7 +20,7 @@ git_bisect () {
git bisect start &&
git bisect good $GOOD &&
rm -rf * &&
-   tar xzf "$TRASH_DIRECTORY/tmp.tgz" &&
+   tar xf "$TRASH_DIRECTORY/tmp.tar" &&
git status -su >actual &&
ls -1pR * >>actual &&
test_cmp expect actual &&
-- 
2.8.0.37.g63b3e6f.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/2] Support marking .git/ (or all files) as hidden on Windows

[Junio C Hamano]

Johannes Schindelin  writes:

> This is a heavily version of patches we carried in Git for Windows for
> way too long without submitting them upstream.
>
> In this iteration, I also claim authorship for the patch because by now
> Kusma's changes were so contorted and mutilated beyond recognition by me
> that I do not want anybody to blame him for my sins.

OK, so what do you want me to do with this "heavily modified
version"?  Earlier you responded:

> I have a huge preference for a code that has been production for
> years over a new code that would cook at most two weeks in 'next'.

I agree. However, it does not fill me with confidence that we did not
catch those two bugs earlier. Even one round of reviews (including a
partial rewrite) was better than all that time since the regressions
were introduced.

So do we want to follow the regular "a few days in 'pu' in case
somebody finds 'oops this trivial change is needed', a week or two
in 'next' for simmering as everybody else, and finally down to
'master'" schedule?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2

[Jeff King]

On Mon, May 09, 2016 at 09:42:32AM -0700, Junio C Hamano wrote:

> > Hopefully your patch to remove the -c ... sanitizing makes it to `master`
> > soon, then I can submit my next iteration.
> 
> Or we can just merge that "do not sanitize" branch in, and then
> queue the "next iteration" which I'd assume would only be the test
> addition?

I think we'd also want the change to the test script to make sure that
it fails with only a single header (Dscho's patch 2).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

t6041-bisect-submodule.sh tar -z not portable

[Armin Kunaschik]

Hi,

similar to t3513, in t6041 tar is used with the -z flag which is not portable
and should be removed the same way as in t3513.

Regards,
Armin
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] t6302: simplify non-gpg cases

[Jeff King]

On Mon, May 09, 2016 at 12:30:43PM -0400, Eric Sunshine wrote:

> The latter seems very preferable, though perhaps it could be made more
> concise like this?
> 
> sign=
> test_have_prereq GPG && sign=-s
> 
> (But that's a minor issue.)

I agree that is nicer, but I wanted to keep the definition inside the
test_expect_success close to its point of use. And that means we to deal
with the existing &&-chain (you can get around it with a {} block, but
at that point you might as well if/then).

Since you as the author of 618310a seem to agree with this direction,
here it is as a real patch.

-- >8 --
Subject: [PATCH] t6302: simplify non-gpg cases

When commit 618310a taught t6302 to run without the GPG
prerequisite, it did so by conditionally creating the signed
tags only when gpg is available. As a result, further tests
need to take this into account, which they can do with the
test_prepare_expect helper. This is a minor hassle, though,
as the helper cannot easily cover all cases (it just matches
"signed" in the output, so all output must include the
actual refname).

Instead, let's take a different approach. We'll always
create the tags, and only conditionally sign them. This does
mean our tag-names are a minor lie, but it lets the tests
which do not care about signing easily behave the same in
all settings. We'll include a comment to document our lie
and avoid confusing further test-writers.

Signed-off-by: Jeff King 
---
 t/t6302-for-each-ref-filter.sh | 45 +-
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 70afb44..3225a0b 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -5,15 +5,6 @@ test_description='test for-each-refs usage of ref-filter APIs'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-gpg.sh
 
-test_prepare_expect () {
-   if test_have_prereq GPG
-   then
-   cat
-   else
-   sed '/signed/d'
-   fi
-}
-
 test_expect_success 'setup some history and refs' '
test_commit one &&
test_commit two &&
@@ -22,11 +13,19 @@ test_expect_success 'setup some history and refs' '
test_commit four &&
git tag -m "An annotated tag" annotated-tag &&
git tag -m "Annonated doubly" doubly-annotated-tag annotated-tag &&
+
+   # Note that these "signed" tags might not actually be signed.
+   # Tests which care about the distinction should be marked
+   # with the GPG prereq.
if test_have_prereq GPG
then
-   git tag -s -m "A signed tag" signed-tag &&
-   git tag -s -m "Signed doubly" doubly-signed-tag signed-tag
+   sign=-s
+   else
+   sign=
fi &&
+   git tag $sign -m "A signed tag" signed-tag &&
+   git tag $sign -m "Signed doubly" doubly-signed-tag signed-tag &&
+
git checkout master &&
git update-ref refs/odd/spot master
 '
@@ -42,7 +41,7 @@ test_expect_success 'filtering with --points-at' '
 '
 
 test_expect_success 'check signed tags with --points-at' '
-   test_prepare_expect <<-\EOF | sed -e "s/Z$//" >expect &&
+   cat <<-\EOF | sed -e "s/Z$//" >expect &&
refs/heads/side Z
refs/tags/annotated-tag four
refs/tags/four Z
@@ -65,7 +64,7 @@ test_expect_success 'filtering with --merged' '
 '
 
 test_expect_success 'filtering with --no-merged' '
-   test_prepare_expect >expect <<-\EOF &&
+   cat >expect <<-\EOF &&
refs/heads/side
refs/tags/annotated-tag
refs/tags/doubly-annotated-tag
@@ -78,7 +77,7 @@ test_expect_success 'filtering with --no-merged' '
 '
 
 test_expect_success 'filtering with --contains' '
-   test_prepare_expect >expect <<-\EOF &&
+   cat >expect <<-\EOF &&
refs/heads/master
refs/heads/side
refs/odd/spot
@@ -99,7 +98,7 @@ test_expect_success '%(color) must fail' '
 '
 
 test_expect_success 'left alignment is default' '
-   test_prepare_expect >expect <<-\EOF &&
+   cat >expect <<-\EOF &&
refname is refs/heads/master  |refs/heads/master
refname is refs/heads/side|refs/heads/side
refname is refs/odd/spot  |refs/odd/spot
@@ -117,7 +116,7 @@ test_expect_success 'left alignment is default' '
 '
 
 test_expect_success 'middle alignment' '
-   test_prepare_expect >expect <<-\EOF &&
+   cat >expect <<-\EOF &&
| refname is refs/heads/master |refs/heads/master
|  refname is refs/heads/side  |refs/heads/side
|   refname is refs/odd/spot   |refs/odd/spot
@@ -135,7 +134,7 @@ test_expect_success 'middle alignment' '
 '
 
 test_expect_success 'right alignment' '
-   test_prepare_expect >expect <<-\EOF &&
+   cat >expect <<-\EOF &&
|  refname is refs/heads/master|refs/heads/master
|refname is refs/heads/side|refs/heads/side
|  refname is 

Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2

[Junio C Hamano]

Johannes Schindelin  writes:

> Okay, I already force-pushed my extra-http-header branch and the next
> iteration will sport this paragraph.

The new explanation is well written and can and should also replace
the comment before the implementation in the configuration file to
help readers.

To be honest, I do not quite understand why you call it "ugly hack"
at all.  It is saying "The client is sending a satisfactory request
when these two headers are there; otherwise the request is not
good", which sounds quite natural way to express what is being
tested.  The rejection being part of "Rewrite" may be clever, but I
do not see it as ugly.

And it matches the spirit of the implementation for 2.3+ that uses
 quite well--you just do not need to say "Fail"
yourself over there, as that is implied.

> Hopefully your patch to remove the -c ... sanitizing makes it to `master`
> soon, then I can submit my next iteration.

Or we can just merge that "do not sanitize" branch in, and then
queue the "next iteration" which I'd assume would only be the test
addition?

Thanks.

-- >8 --
From: Johannes Schindelin 
Date: Mon, 9 May 2016 07:59:16 +0200
Subject: [PATCH] tests: adjust the configuration for Apache 2.2

Lars Schneider noticed that the configuration introduced to test the
extra HTTP headers cannot be used with Apache 2.2 (which is still
actively maintained, as pointed out by Junio Hamano).

To let the tests pass with Apache 2.2 again, let's substitute the
offending  and `expr` by using old school RewriteCond
statements.

As RewriteCond does not allow testing for *non*-matches, we simply match
the desired case first and let it pass by marking the RewriteRule as
'[L]' ("last rule, do not process any other matching RewriteRules after
this"), and then have another RewriteRule that matches all other cases
and lets them fail via '[F]' ("fail").

Signed-off-by: Johannes Schindelin 
Signed-off-by: Junio C Hamano 
---
 t/lib-httpd/apache.conf | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 838770c..2dcbb00 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -102,10 +102,6 @@ Alias /auth/dumb/ www/auth/dumb/
Header set Set-Cookie name=value
 
 
-   
-   Require expr %{HTTP:x-magic-one} == 'abra'
-   Require expr %{HTTP:x-magic-two} == 'cadabra'
-   
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
SetEnv GIT_HTTP_EXPORT_ALL
 
@@ -135,6 +131,18 @@ RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1 
[R=302]
 RewriteRule ^/loop-redir/x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-(.*) /$1 
[R=302]
 RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
 
+# Apache 2.2 does not understand , so we use RewriteCond.
+# And as RewriteCond does not allow testing for non-matches, we match
+# the desired case first (one has abra, two has cadabra), and let it
+# pass by marking the RewriteRule as [L], "last rule, do not process
+# any other matching RewriteRules after this"), and then have another
+# RewriteRule that matches all other cases and lets them fail via '[F]',
+# "fail the request".
+RewriteCond %{HTTP:x-magic-one} =abra
+RewriteCond %{HTTP:x-magic-two} =cadabra
+RewriteRule ^/smart_headers/.* - [L]
+RewriteRule ^/smart_headers/.* - [F]
+
 
 LoadModule ssl_module modules/mod_ssl.so
 
-- 
2.8.2-557-gee41d5e

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t4151 missing quotes

[Armin Kunaschik]

Sorry, this was my first patch to the list. I'll do better :-)
You are right about the "wc -l" parts. Maybe I was a bit over
pessimistic. Throw away my last mail.
In my case test 9 ran unsuccessful because of an empty "git ls-files -u"

This reduces the diff to this one (hopefully the right way now):
*** ./t4151-am-abort.sh.origFri Apr 29 23:37:00 2016
--- ./t4151-am-abort.sh Mon May  9 18:28:18 2016
***
*** 82,88 
test 4 = "$(cat otherfile-4)" &&
git am --abort &&
test_cmp_rev initial HEAD &&
!   test -z $(git ls-files -u) &&
test_path_is_missing otherfile-4
  '

--- 82,88 
test 4 = "$(cat otherfile-4)" &&
git am --abort &&
test_cmp_rev initial HEAD &&
!   test -z "$(git ls-files -u)" &&
test_path_is_missing otherfile-4
  '

All the other similar occurrences are correctly quoted.

On Mon, May 9, 2016 at 6:22 PM, Eric Sunshine  wrote:
> On Mon, May 9, 2016 at 12:09 PM, Armin Kunaschik
>  wrote:
>> skipping through some failed tests I found more (smaller) problems
>> inside the test... when test arguments are empty they need to be
>> quoted (quite a lot test in this sentence).
>>
>> Error is like
>> t4151-am-abort.sh[5]: test: argument expected
>>
>> My patch:
>>
>> *** t4151-am-abort.sh   Mon May  9 17:51:44 2016
>> --- t4151-am-abort.sh.orig  Fri Apr 29 23:37:00 2016
>> ***
>> *** 67,73 
>>   test_expect_success 'am -3 --skip removes otherfile-4' '
>> git reset --hard initial &&
>> test_must_fail git am -3 0003-*.patch &&
>> !   test 3 -eq "$(git ls-files -u | wc -l)" &&
>> test 4 = "$(cat otherfile-4)" &&
>> git am --skip &&
>> test_cmp_rev initial HEAD &&
>> --- 67,73 
>>   test_expect_success 'am -3 --skip removes otherfile-4' '
>> git reset --hard initial &&
>> test_must_fail git am -3 0003-*.patch &&
>> !   test 3 -eq $(git ls-files -u | wc -l) &&
>> test 4 = "$(cat otherfile-4)" &&
>> git am --skip &&
>> test_cmp_rev initial HEAD &&
>> ***
>
> Some comments:
>
> Quoting the output of 'wc -l' will break the tests on Mac OS X and BSD
> since the output contains leading whitespace which won't match the "3"
> on the other side of the '='.
>
> Your diff is backward, comparing 'current' against 'original', which
> makes it difficult to read. Reviewers on this list expect to see
> 'original' compared against 'current'.
>
> Use a unified format to make the diff easier to read; or just use
> git-diff or git-format patch, which is even simpler.
>
> It's not clear how the output of 'wc -l' could ever be the empty
> string. Perhaps git-ls-files is dying and causing the pipe to abort
> before 'wc -l' ever outputs anything? Without additional information
> about the problem you're experiencing, it's difficult to judge if this
> change is a good idea.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t6392 broken on pu (Mac OS X)

[Eric Sunshine]

On Mon, May 9, 2016 at 12:07 PM, Jeff King  wrote:
> On Sat, May 07, 2016 at 06:15:19PM +0200, Torsten Bögershausen wrote:
>> These tests fail here under Mac OS,
>> they pass under Linux:
>> commit ff3d9b660a4b6e9d3eeb664ce1febe717adff737
>> I haven't had a chance to dig further.
>
> I assume you mean t6302. It looks like the difference is not Mac OS, but
> rather that the GPG prerequisite is not fulfilled, so we are missing a
> few of the tags.
>
> Commit 618310a introduced a helper to munge the "expect" output. Using
> that fixes some of the cases, but not test 34. That one is expecting
> blank lines for tags, so test_prepare_expect doesn't know which lines
> are related to GPG.
>
> We could fix it by tweaking the test like this:
> [...snip...]
> However, I wonder if we could improve on the strategy in 618310a, and
> simply create non-signed versions of the "signed" tags when GPG is not
> available. That would make tests looking at the whole ref namespace
> more consistent. And any tests which wanted to look specifically at the
> signed attributes should be protected with the GPG prereq anyway (it
> doesn't look like there are any currently, though).
>
> I.e., something like:
> [...snip...]
>  test_expect_success 'setup some history and refs' '
> @@ -24,9 +20,12 @@ test_expect_success 'setup some history and refs' '
> git tag -m "Annonated doubly" doubly-annotated-tag annotated-tag &&
> if test_have_prereq GPG
> then
> -   git tag -s -m "A signed tag" signed-tag &&
> -   git tag -s -m "Signed doubly" doubly-signed-tag signed-tag
> +   sign=-s
> +   else
> +   sign=
> fi &&
> +   git tag $sign -m "A signed tag" signed-tag &&
> +   git tag $sign -m "Signed doubly" doubly-signed-tag signed-tag &&
> git checkout master &&
> git update-ref refs/odd/spot master
>  '

The latter seems very preferable, though perhaps it could be made more
concise like this?

sign=
test_have_prereq GPG && sign=-s

(But that's a minor issue.)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t4151 missing quotes

[Eric Sunshine]

On Mon, May 9, 2016 at 12:22 PM, Eric Sunshine  wrote:
> On Mon, May 9, 2016 at 12:09 PM, Armin Kunaschik
>  wrote:
>> *** t4151-am-abort.sh   Mon May  9 17:51:44 2016
>> --- t4151-am-abort.sh.orig  Fri Apr 29 23:37:00 2016
>> !   test 3 -eq "$(git ls-files -u | wc -l)" &&
>> !   test 3 -eq $(git ls-files -u | wc -l) &&
>
> Some comments:
>
> Quoting the output of 'wc -l' will break the tests on Mac OS X and BSD
> since the output contains leading whitespace which won't match the "3"
> on the other side of the '='.

This isn't quite accurate. Since the test is using '-eq' rather than
'=', the leading whitespace won't be a problem, but it can still be
problematic if you're somehow getting an empty result from the
pipeline:

% test 3 -eq "$(echo)"
-bash: test: : integer expression expected

> Your diff is backward, comparing 'current' against 'original', which
> makes it difficult to read. Reviewers on this list expect to see
> 'original' compared against 'current'.
>
> Use a unified format to make the diff easier to read; or just use
> git-diff or git-format patch, which is even simpler.
>
> It's not clear how the output of 'wc -l' could ever be the empty
> string. Perhaps git-ls-files is dying and causing the pipe to abort
> before 'wc -l' ever outputs anything? Without additional information
> about the problem you're experiencing, it's difficult to judge if this
> change is a good idea.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2

[Junio C Hamano]

Johannes Schindelin  writes:

> +# Apache 2.2 does not understand , so we use RewriteCond.
> +# And as RewriteCond unfortunately lacks "not equal" matching, we use this
> +# ugly trick to fail *unless* the two headers are present.
> +RewriteCond %{HTTP:x-magic-one} =abra
> +RewriteCond %{HTTP:x-magic-two} =cadabra
> +RewriteRule ^/smart_headers/.* - [L]
> +RewriteRule ^/smart_headers/.* - [F]

Clever.  Thanks.  Will queue.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t4151 missing quotes

[Eric Sunshine]

On Mon, May 9, 2016 at 12:09 PM, Armin Kunaschik
 wrote:
> skipping through some failed tests I found more (smaller) problems
> inside the test... when test arguments are empty they need to be
> quoted (quite a lot test in this sentence).
>
> Error is like
> t4151-am-abort.sh[5]: test: argument expected
>
> My patch:
>
> *** t4151-am-abort.sh   Mon May  9 17:51:44 2016
> --- t4151-am-abort.sh.orig  Fri Apr 29 23:37:00 2016
> ***
> *** 67,73 
>   test_expect_success 'am -3 --skip removes otherfile-4' '
> git reset --hard initial &&
> test_must_fail git am -3 0003-*.patch &&
> !   test 3 -eq "$(git ls-files -u | wc -l)" &&
> test 4 = "$(cat otherfile-4)" &&
> git am --skip &&
> test_cmp_rev initial HEAD &&
> --- 67,73 
>   test_expect_success 'am -3 --skip removes otherfile-4' '
> git reset --hard initial &&
> test_must_fail git am -3 0003-*.patch &&
> !   test 3 -eq $(git ls-files -u | wc -l) &&
> test 4 = "$(cat otherfile-4)" &&
> git am --skip &&
> test_cmp_rev initial HEAD &&
> ***

Some comments:

Quoting the output of 'wc -l' will break the tests on Mac OS X and BSD
since the output contains leading whitespace which won't match the "3"
on the other side of the '='.

Your diff is backward, comparing 'current' against 'original', which
makes it difficult to read. Reviewers on this list expect to see
'original' compared against 'current'.

Use a unified format to make the diff easier to read; or just use
git-diff or git-format patch, which is even simpler.

It's not clear how the output of 'wc -l' could ever be the empty
string. Perhaps git-ls-files is dying and causing the pipe to abort
before 'wc -l' ever outputs anything? Without additional information
about the problem you're experiencing, it's difficult to judge if this
change is a good idea.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFD/PATCH] submodule doc: describe where we can configure them

[Junio C Hamano]

Heiko Voigt  writes:

>> > - When upstream adds a new submodule, I have to do the same manual
>> >   work to change the options for that new submodule.
>> 
>> Because a new module is not automatically "init"ed by default?
>> 
>> Isn't "config only" vs "config with gitmodules fallback" orthogonal
>> to that issue?
>
> What do you mean with "orthogonal to that issue"? AFAICS a gitmodule
> fallback does not have that issue.
>
> Actually I would see it more like:
> .gitmodule is the default and .git/config a possibility to override.

The way I read Jonathan's "I have to do the same manual..." above is:

  Back when I cloned, the upstream had one submodule A.  I didn't like
  some aspect of the configuration for that submodule so I did a
  customization in [submodule "A"] section of .git/config for it.

  Now the upstream added another submodule B.  I want a tweak similar
  to what I did to A applied to this one, but that would mean I need
  to edit the entry in .git/config copied by "init" from .gitmodules.

I do not see how difference between ".git/config is the only source
of truth" or ".git/config overrides what is in .gitmodules" would
matter to the above scenario.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t6044 broken on pu

[Jeff King]

On Mon, May 09, 2016 at 12:02:45PM -0400, Eric Sunshine wrote:

> > I had that thought, too, but I think it would be an error to do so.
> > test_seq is supposed to be a replacement for "seq", which does not
> > understand non-numeric sequences.
> 
> Although, the comment block just above test_seq() in
> test-lib-functions.sh says otherwise:
> 
> Print a sequence of numbers or letters in increasing order.  This
> is similar to GNU seq(1), but the latter might not be available
> everywhere (and does not do letters).  It may be used like:
> 
> for i in $(test_seq 100)
> do
> for j in $(test_seq 10 20)
> do
> for k in $(test_seq a z)
> do
> echo $i-$j-$k
> done
> done
> done

Oh, indeed. I apparently even Acked that documentation once upon a time. :-/

Anyway, I double-checked my earlier "grep" and I do not think anybody is
using that functionality. So I think we'd be OK to change it as long as
we updated the documentation to match.

I don't really care either way whether it is replaced or not (at one
point there were some people really interested in NO_PERL not even using
one-liners in the test suite, but I am not one of them).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

t4151 missing quotes

[Armin Kunaschik]

Hi there,

skipping through some failed tests I found more (smaller) problems
inside the test... when test arguments are empty they need to be
quoted (quite a lot test in this sentence).

Error is like
t4151-am-abort.sh[5]: test: argument expected

My patch:

*** t4151-am-abort.sh   Mon May  9 17:51:44 2016
--- t4151-am-abort.sh.orig  Fri Apr 29 23:37:00 2016
***
*** 67,73 
  test_expect_success 'am -3 --skip removes otherfile-4' '
git reset --hard initial &&
test_must_fail git am -3 0003-*.patch &&
!   test 3 -eq "$(git ls-files -u | wc -l)" &&
test 4 = "$(cat otherfile-4)" &&
git am --skip &&
test_cmp_rev initial HEAD &&
--- 67,73 
  test_expect_success 'am -3 --skip removes otherfile-4' '
git reset --hard initial &&
test_must_fail git am -3 0003-*.patch &&
!   test 3 -eq $(git ls-files -u | wc -l) &&
test 4 = "$(cat otherfile-4)" &&
git am --skip &&
test_cmp_rev initial HEAD &&
***
*** 78,88 
  test_expect_success 'am -3 --abort removes otherfile-4' '
git reset --hard initial &&
test_must_fail git am -3 0003-*.patch &&
!   test 3 -eq "$(git ls-files -u | wc -l)" &&
test 4 = "$(cat otherfile-4)" &&
git am --abort &&
test_cmp_rev initial HEAD &&
!   test -z "$(git ls-files -u)" &&
test_path_is_missing otherfile-4
  '

--- 78,88 
  test_expect_success 'am -3 --abort removes otherfile-4' '
git reset --hard initial &&
test_must_fail git am -3 0003-*.patch &&
!   test 3 -eq $(git ls-files -u | wc -l) &&
test 4 = "$(cat otherfile-4)" &&
git am --abort &&
test_cmp_rev initial HEAD &&
!   test -z $(git ls-files -u) &&
test_path_is_missing otherfile-4
  '

***
*** 146,152 
git reset &&
rm -f otherfile-4 otherfile-2 file-1 file-2 &&
test_must_fail git am -3 initial.patch 0003-*.patch &&
!   test 3 -eq "$(git ls-files -u | wc -l)" &&
test 4 = "$(cat otherfile-4)" &&
git am --abort &&
test -z "$(git ls-files -u)" &&
--- 146,152 
git reset &&
rm -f otherfile-4 otherfile-2 file-1 file-2 &&
test_must_fail git am -3 initial.patch 0003-*.patch &&
!   test 3 -eq $(git ls-files -u | wc -l) &&
test 4 = "$(cat otherfile-4)" &&
git am --abort &&
test -z "$(git ls-files -u)" &&

Regards,
Armin
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t6392 broken on pu (Mac OS X)

[Jeff King]

On Sat, May 07, 2016 at 06:15:19PM +0200, Torsten Bögershausen wrote:

> These tests fail here under Mac OS,
> they pass under Linux:
> commit ff3d9b660a4b6e9d3eeb664ce1febe717adff737
> I haven't had a chance to dig further.

I assume you mean t6302. It looks like the difference is not Mac OS, but
rather that the GPG prerequisite is not fulfilled, so we are missing a
few of the tags.

Commit 618310a introduced a helper to munge the "expect" output. Using
that fixes some of the cases, but not test 34. That one is expecting
blank lines for tags, so test_prepare_expect doesn't know which lines
are related to GPG.

We could fix it by tweaking the test like this:

diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 7420e48..04042e1 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -343,29 +343,27 @@ test_expect_success 'improper usage of %(if), %(then), 
%(else) and %(end) atoms'
 '
 
 test_expect_success 'check %(if)...%(then)...%(end) atoms' '
-   git for-each-ref --format="%(if)%(authorname)%(then)%(authorname): 
%(refname)%(end)" >actual &&
-   cat >expect <<-\EOF &&
-   A U Thor: refs/heads/master
-   A U Thor: refs/heads/side
-   A U Thor: refs/odd/spot
-
-
-
-   A U Thor: refs/tags/foo1.10
-   A U Thor: refs/tags/foo1.3
-   A U Thor: refs/tags/foo1.6
-   A U Thor: refs/tags/four
-   A U Thor: refs/tags/one
-
-   A U Thor: refs/tags/three
-   A U Thor: refs/tags/two
+   git for-each-ref --format="%(refname):%(if)%(authorname)%(then) 
author=%(authorname)%(end)" >actual &&
+   test_prepare_expect >expect <<-\EOF &&
+   refs/heads/master: author=A U Thor
+   refs/heads/side: author=A U Thor
+   refs/odd/spot: author=A U Thor
+   refs/tags/annotated-tag:
+   refs/tags/doubly-annotated-tag:
+   refs/tags/foo1.10: author=A U Thor
+   refs/tags/foo1.3: author=A U Thor
+   refs/tags/foo1.6: author=A U Thor
+   refs/tags/four: author=A U Thor
+   refs/tags/one: author=A U Thor
+   refs/tags/three: author=A U Thor
+   refs/tags/two: author=A U Thor
EOF
test_cmp expect actual
 '
 
 test_expect_success 'check %(if)...%(then)...%(else)...%(end) atoms' '
git for-each-ref 
--format="%(if)%(authorname)%(then)%(authorname)%(else)No author%(end): 
%(refname)" >actual &&
-   cat >expect <<-\EOF &&
+   test_prepare_expect >expect <<-\EOF &&
A U Thor: refs/heads/master
A U Thor: refs/heads/side
A U Thor: refs/odd/spot
@@ -385,7 +383,7 @@ test_expect_success 'check 
%(if)...%(then)...%(else)...%(end) atoms' '
 '
 test_expect_success 'ignore spaces in %(if) atom usage' '
git for-each-ref --format="%(refname:short): %(if)%(HEAD)%(then)Head 
ref%(else)Not Head ref%(end)" >actual &&
-   cat >expect <<-\EOF &&
+   test_prepare_expect >expect <<-\EOF &&
master: Head ref
side: Not Head ref
odd/spot: Not Head ref


Though we'd perhaps want to tweak the subsequent tests to use the same
format, just to make things easier to read later.

However, I wonder if we could improve on the strategy in 618310a, and
simply create non-signed versions of the "signed" tags when GPG is not
available. That would make tests looking at the whole ref namespace
more consistent. And any tests which wanted to look specifically at the
signed attributes should be protected with the GPG prereq anyway (it
doesn't look like there are any currently, though).

I.e., something like:

diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 7420e48..a3df472 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -6,12 +6,8 @@ test_description='test for-each-refs usage of ref-filter APIs'
 . "$TEST_DIRECTORY"/lib-gpg.sh
 
 test_prepare_expect () {
-   if test_have_prereq GPG
-   then
-   cat
-   else
-   sed '/signed/d'
-   fi
+   # XXX this could now go away entirely, and just use cat in each test
+   cat
 }
 
 test_expect_success 'setup some history and refs' '
@@ -24,9 +20,12 @@ test_expect_success 'setup some history and refs' '
git tag -m "Annonated doubly" doubly-annotated-tag annotated-tag &&
if test_have_prereq GPG
then
-   git tag -s -m "A signed tag" signed-tag &&
-   git tag -s -m "Signed doubly" doubly-signed-tag signed-tag
+   sign=-s
+   else
+   sign=
fi &&
+   git tag $sign -m "A signed tag" signed-tag &&
+   git tag $sign -m "Signed doubly" doubly-signed-tag signed-tag &&
git checkout master &&
git update-ref refs/odd/spot master
 '

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Move test-* to t/helper/ subdirectory

[Junio C Hamano]

Duy Nguyen  writes:

> So among the options we have so far, which way should we go, or leave it as 
> is?

Thanks for reminding me.

I like that version you sent with "I may have rushed to judgment"
comment the most.  Perhaps I can just queue it with s/PATH/PROG/
fixup?

>
> On Tue, May 3, 2016 at 7:15 AM, Duy Nguyen  wrote:
>> On Tue, May 3, 2016 at 12:34 AM, Junio C Hamano  wrote:
>>> Duy Nguyen  writes:
>>>
 I may have rushed to judgement. wrap-for-bin.sh has always been the
 dependency for bin-wrappers/*. If we force that file to change, then
 bin-wrappers/* will be recreated when switching branches. So how about
 this?
>>>
>>> I do not think you are "force updating wrap-for-bin" in any way in
>>> the patch, though.  You are building it in such a way that it does
>>> not have to get updated within the revision that contains e6e7530
>>> (assuming that this will be queued directly on top it and merged to
>>> everywhere e6e7530 is contained).
>>
>> Yep.
>>
>>> The new case/esac looks somewhat bad (its knowing that where test-*
>>> lives, test-* is the only thing that is special, etc. troubles me at
>>> the same time that case/esac is funnily formated).
>>
>> We could just make some random changes in this file. That would have
>> the same effect.
>>
>>> Where does "@@PATH@@" come from and who rewrites it?  Is that a
>>> misspelt "@@PROG@@"?
>>
>> Yep. Should have run make distclean before testing :(
>> --
>> Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t6044 broken on pu

[Eric Sunshine]

On Mon, May 9, 2016 at 4:33 AM, Jeff King  wrote:
> On Mon, May 09, 2016 at 08:30:51AM +0200, demerphq wrote:
>> > -   perl -le 'print for $ARGV[0]..$ARGV[1]' -- "$@"
>> > +   test_seq_counter__=$1
>> > +   while test "$test_seq_counter__" -le $2
>> > +   do
>> > +   echo "$test_seq_counter__"
>> > +   test_seq_counter__=$((test_seq_counter__ + 1))
>> > +   done
>> >  }
>>
>> Is that perl snippet ever called with non-numeric output?
>>
>> perl -le 'print for $ARGV[0]..$ARGV[1]' -- A E
>> A
>> B
>> C
>> D
>> E
>
> I had that thought, too, but I think it would be an error to do so.
> test_seq is supposed to be a replacement for "seq", which does not
> understand non-numeric sequences.

Although, the comment block just above test_seq() in
test-lib-functions.sh says otherwise:

Print a sequence of numbers or letters in increasing order.  This
is similar to GNU seq(1), but the latter might not be available
everywhere (and does not do letters).  It may be used like:

for i in $(test_seq 100)
do
for j in $(test_seq 10 20)
do
for k in $(test_seq a z)
do
echo $i-$j-$k
done
done
done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v16 0/7] config commit verbose

[Junio C Hamano]

Jeff King  writes:

> I guess I do not really consider the template content to be the primary
> thing the command is doing. It is subjective, though. I don't feel
> strongly enough to keep discussing it if other people don't agree.

I just see the primary thing of what "commit -e" does is to help
users edit their log message (and view "-v" as giving more helping),
but I do agree with you that this is very subjective.

If we had these as either in broken-down form ("--show-diff",
"--show-diffstat", and "--show-untracked") or just a single
"--show-extra-info" option when we did the feature in the very
beginning, I do not think I'd feel that "--show-*" option(s) should
be renamed/redone to "--verbose".  So personally, my subjective
judgment is "'--verbose' and '--show-diff' would have been equally
valid, and it is OK to let whichever came first squat on the
feature."


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Pranit Bauva]

Hey Johannes,

On Mon, May 9, 2016 at 8:29 PM, Johannes Schindelin
 wrote:
> Hi Pranit,
>
> On Sun, 8 May 2016, Pranit Bauva wrote:
>
>> On Sun, May 8, 2016 at 12:34 PM, Johannes Schindelin
>>  wrote:
>> >
>> > On Fri, 6 May 2016, Pranit Bauva wrote:
>> >
>> >> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> >> index 3324229..d8de651 100644
>> >> --- a/builtin/bisect--helper.c
>> >> +++ b/builtin/bisect--helper.c
>> >> @@ -8,13 +8,17 @@ static const char * const git_bisect_helper_usage[] = {
>> >>   NULL
>> >>  };
>> >>
>> >> +enum subcommand {
>> >> + NEXT_ALL = 1
>> >> +};
>> >
>> > I still do not think that this enum needs to have file scope. Function
>> > scope is enough.
>>
>> In the very initial patch I made it in function scope. To which you
>> pointed out[1] that in all other examples but for one have file scope
>> so then I thought maybe that exception was a wrong example and I
>> should stick to the convention of putting it in file scope.
>
> Oh, sorry, I meant to imply that it is good as it is by saying "so this
> code is fine"...
>
> I was just surprised because I thought I remembered that some old C
> standard does not allow enums to be function scoped. But I was wrong.
>
>> But now I also realize that builtin/replace.c uses "cmdmode" instead of
>> "subcommand" so I am still wondering what would be the most appropriate?
>
> I think the replace.c code is really a good example. Function-scoped,
> using the "cmdmode" name that obviously corresponds to the OPT_CMDMODE
> name.

Sure. WIll do!

Regards,
Pranit Bauva
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2

[Johannes Schindelin]

Hi Peff,

On Mon, 9 May 2016, Jeff King wrote:

> On Mon, May 09, 2016 at 04:03:48PM +0200, Johannes Schindelin wrote:
> 
> > How about this:
> > 
> > As RewriteCond does not allow testing for *non*-matches, we simply
> > match the desired case first and let it pass by marking the
> > RewriteRule as '[L]' ("last rule, do not process any other
> > matching RewriteRules after this"), and then have another
> > RewriteRule that matches all other cases and lets them fail via
> > '[F]' ("fail").
> > 
> > Good enough?
> 
> Yep, I think that explains it. Thanks.

Okay, I already force-pushed my extra-http-header branch and the next
iteration will sport this paragraph.

Hopefully your patch to remove the -c ... sanitizing makes it to `master`
soon, then I can submit my next iteration.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 80/83] run-command: make dup_devnull() non static

[Johannes Schindelin]

Hi Duy,

On Sun, 8 May 2016, Duy Nguyen wrote:

> On Sun, May 8, 2016 at 1:33 PM, Johannes Schindelin
>  wrote:
> > The claim is that this libifies the procedure. But it makes the code
> > really nasty for use as a library: if this is run in a thread (and you
> > know that we are going to have to do this in the near future, for
> > performance reasons), it will completely mess up all the other threads
> > because it messes with the global file descriptors.
> 
> I vote one step at a time, leave multi-thread support for future.

Oh, but I never said that we have to do that now!

All I said was that using this dup() dance instead of truly libifying the
functions would slam the door almost shut for future multi-threading.
Which is a strong hint in my book that we should *not* do that dup()
dance, but fix our code by introducing a silent mode.

> There's a lot more shared state than file descriptors anyway, at least
> there are object db and index access and probably a couple of hidden
> static variables somewhere.

Sure. And do we change those shared states temporarily in our functions?
No, we don't. The object db is not made temporarily inaccessible while a
certain function runs. The index access is not temporarily disabled while
a certain function runs.

And this is what that dup() dance does: it disables *all* output, not only
from the current thread.

> And I'm not sure if multi-thread really helps here. Are we really
> CPU-bound? If object inflation causes that (wild guess), can we just
> inflate ahead in some separate process and pass the result back?

Again. I did *not* suggest to introduce multi-threading. I was making a
case for *avoiding* that ugly dup() to /dev/null and then dup() it back to
the original state. That would just ask for unintended side effects.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Johannes Schindelin]

Hi Pranit,

On Sun, 8 May 2016, Pranit Bauva wrote:

> On Sun, May 8, 2016 at 12:34 PM, Johannes Schindelin
>  wrote:
> >
> > On Fri, 6 May 2016, Pranit Bauva wrote:
> >
> >> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> >> index 3324229..d8de651 100644
> >> --- a/builtin/bisect--helper.c
> >> +++ b/builtin/bisect--helper.c
> >> @@ -8,13 +8,17 @@ static const char * const git_bisect_helper_usage[] = {
> >>   NULL
> >>  };
> >>
> >> +enum subcommand {
> >> + NEXT_ALL = 1
> >> +};
> >
> > I still do not think that this enum needs to have file scope. Function
> > scope is enough.
> 
> In the very initial patch I made it in function scope. To which you
> pointed out[1] that in all other examples but for one have file scope
> so then I thought maybe that exception was a wrong example and I
> should stick to the convention of putting it in file scope.

Oh, sorry, I meant to imply that it is good as it is by saying "so this
code is fine"...

I was just surprised because I thought I remembered that some old C
standard does not allow enums to be function scoped. But I was wrong.

> But now I also realize that builtin/replace.c uses "cmdmode" instead of
> "subcommand" so I am still wondering what would be the most appropriate?

I think the replace.c code is really a good example. Function-scoped,
using the "cmdmode" name that obviously corresponds to the OPT_CMDMODE
name.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v16 0/7] config commit verbose

[Jeff King]

On Sun, May 08, 2016 at 11:48:31AM -0700, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  writes:
> 
> > I.e. I really expect --verbose to be a more verbose version of the
> > primary thing a command is doing, which in the case of "commit
> > --amend" is giving me info I need to modify the commit.
> 
> That summarises what I wanted to say very well.  Thanks.

I guess I do not really consider the template content to be the primary
thing the command is doing. It is subjective, though. I don't feel
strongly enough to keep discussing it if other people don't agree.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2

[Jeff King]

On Mon, May 09, 2016 at 04:03:48PM +0200, Johannes Schindelin wrote:

> How about this:
> 
>   As RewriteCond does not allow testing for *non*-matches, we simply
>   match the desired case first and let it pass by marking the
>   RewriteRule as '[L]' ("last rule, do not process any other
>   matching RewriteRules after this"), and then have another
>   RewriteRule that matches all other cases and lets them fail via
>   '[F]' ("fail").
> 
> Good enough?

Yep, I think that explains it. Thanks.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2

[Johannes Schindelin]

Hi Peff,

On Mon, 9 May 2016, Jeff King wrote:

> On Mon, May 09, 2016 at 08:18:52AM +0200, Johannes Schindelin wrote:
> 
> > +# Apache 2.2 does not understand , so we use RewriteCond.
> > +# And as RewriteCond unfortunately lacks "not equal" matching, we use this
> > +# ugly trick to fail *unless* the two headers are present.
> > +RewriteCond %{HTTP:x-magic-one} =abra
> > +RewriteCond %{HTTP:x-magic-two} =cadabra
> > +RewriteRule ^/smart_headers/.* - [L]
> > +RewriteRule ^/smart_headers/.* - [F]
> > +
> 
> Thanks, this is the magic that eluded me earlier. I had to look up the
> flags, so for any observers in the same boat, this works because:
> 
>   - the '[L]' flag says "stop doing any more rewrite rules"; it triggers
> only when the RewriteConds above match
> 
>   - the '[F]' flag says "return 403 Forbidden"; it triggers always,
> because after a RewriteRule, all RewriteConds are reset
> 
> I'm sure that is all apparent to somebody who is familiar with Apache
> config, but I think that does not include most people on this project. I
> dunno if it is worth a comment here or in the commit message.

Oh, you're absolutely correct, I should have described this better. It
took me quite a couple of iterations to get it right, after all.

How about this:

As RewriteCond does not allow testing for *non*-matches, we simply
match the desired case first and let it pass by marking the
RewriteRule as '[L]' ("last rule, do not process any other
matching RewriteRules after this"), and then have another
RewriteRule that matches all other cases and lets them fail via
'[F]' ("fail").

Good enough?

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html