Re: [ANNOUNCE] Git v2.9.1

[Duy Nguyen]

On Thu, Jul 14, 2016 at 9:38 AM, Lars Schneider
 wrote:
>
> On 13 Jul 2016, at 22:43, Junio C Hamano  wrote:
>
>> Junio C Hamano  writes:
>>
>>> It is somewhat disturbing that nobody seems to be regularly building
>>> on 32-bit platforms these days, which is the only reason I can think
>>> of why this was never reported until it hit a maintenance track.
>>> This should have been caught last week at f6a729f3 (Merge branch
>>> 'jk/tzoffset-fix', 2016-07-06) when the topic hit 'master' at the
>>> latest, and more preferrably it should have already been caught last
>>> month at 08ec8c5e (Merge branch 'jk/tzoffset-fix' into next,
>>> 2016-06-28).
>>>
>>> Those who care about 32-bit builds need to start building and
>>> testing 'next' and 'master' regularly, or similar breakages are
>>> bound to continue happening X-<.
>>>
>>> Volunteers?
>>
>> We might eventually see a volunteer or two but that hasn't happened
>> yet, at least in the past few days.
>>
>> Does Travis CI testing have an option to run our tests on some
>> 32-bit platforms?
>
> TravisCI does not support 32-bit platforms natively:
> https://github.com/travis-ci/travis-ci/issues/986#issuecomment-124141683
>
> However, there seems to be a way to enter a 32 bit Trusty chroot on
> 64 bit Travis via Docker:
> https://github.com/travis-ci/travis-ci/issues/5770
>
> @Duy:
> You mentioned that you compiled Git on Docker before ($gmane/297963).
> What do you think the chroot approach? Could that work? Would that
> be reliable?

"Docker chroot" is a weird term because they are not the same. If you
can launch a new docker process from travis-ci, I suppose you can use
a docker image with multilib support, then just run 32-bit binaries
and it should work (unless the host kernel is built to only support
64-bit).
-- 
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: Plugin mechanism(s) for Git?

[Duy Nguyen]

On Fri, Jul 15, 2016 at 11:28 PM, Eric Wong  wrote:
> Konstantin Khomoutov  wrote:
>> Ben Peart  wrote:
>> > > Lars Schneider wrote:
>> > >
>> > > We could dynamically load libraries but this would force us to
>> > > freeze the ABI as mentioned by Duy:
>> > > http://article.gmane.org/gmane.comp.version-control.git/298463
>> > >
>> >
>> > I wouldn’t be too quick to dismiss dynamically loaded libraries as
>> > there are some distinct advantages over the other patterns especially
>> > performance and simplicity.  I realize it requires us to version the
>> > ABI but there are established patterns to manage this.  It also isn’t
>> > that much different than us having to freeze or version the protocol
>> > for communicating with a remote-helper.
>
> (re-adding dropped CCs)
>
> The critical difference is protocols can be tested and debugged
> in a language/tool-independent way.

Not using dynamic libraries also makes it possible for other git
reimplementations to reuse the same plugins.
-- 
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: [PATCH] contrib/persistent-https: update ldflags syntax for Go 1.7+

[Jeff King]

[+cc Shawn, who participated in the original discussion, as I don't
  think Colby really works on git any more]

On Fri, Jul 15, 2016 at 01:44:14PM -0700, Parker Moore wrote:

> From: Parker Moore 
> 
> This fixes contrib/persistent-https builds for Go v1.7+ and is
> compatible with Go v1.5+.
> 
> Running `make all` in `contrib/persistent-https` results in a failure
> on Go 1.7 and above.
> 
> Specifically, the error is:
> 
> go build -o git-remote-persistent-https \
>-ldflags "-X main._BUILD_EMBED_LABEL 1468613136"
> # _/Users/parkr/github/git/contrib/persistent-https
> /usr/local/Cellar/go/1.7rc1/libexec/pkg/tool/darwin_amd64/link: -X
> flag requires argument of the form importpath.name=value
> make: *** [git-remote-persistent-https] Error 2
> 
> This `name=value` syntax for the -X flag was introduced in Go v1.5
> (released Aug 19, 2015):
> 
> - release notes: https://golang.org/doc/go1.5#link
> - commit: 
> https://github.com/golang/go/commit/12795c02f3d6fc54ece09a86e70aaa40a94d5131
> 
> In Go v1.7, support for the old syntax was removed:
> 
> - release notes: https://tip.golang.org/doc/go1.7#compiler
> - commit: 
> https://github.com/golang/go/commit/51b624e6a29b135ce0fadb22b678acf4998ff16f
> 
> This patch includes the `=` to fix builds with Go v1.7+.

With the disclaimer that I have very little experience with Go, this
seems like a good, well-explained change. My only question would be
whether people still use pre-v1.5 versions of Go, since it sounds like
this would adversely affect them if they do. (If it does, it seems the
Go people did not give a very good deprecation period for the syntax
change, if people are using both the pre-new-syntax and post-old-syntax
versions simultaneously). I'm not sure what the alternative is, beyond
perhaps checking the version of Go dynamically in the Makefile.

-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 v4 2/4] test-lib.sh: introduce and use $EMPTY_BLOB

[Nguyễn Thái Ngọc Duy]

Similar to $EMPTY_TREE this makes it easier to recognize this special
SHA-1 and change hash later.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 t/t1011-read-tree-sparse-checkout.sh |  8 
 t/t1700-split-index.sh   | 24 
 t/t3102-ls-tree-wildcards.sh |  8 
 t/t7011-skip-worktree-reading.sh | 12 +---
 t/t7012-skip-worktree-writing.sh | 10 --
 t/t7063-status-untracked-cache.sh|  6 +++---
 t/t7508-status.sh|  2 +-
 t/test-lib.sh|  3 ++-
 8 files changed, 35 insertions(+), 38 deletions(-)

diff --git a/t/t1011-read-tree-sparse-checkout.sh 
b/t/t1011-read-tree-sparse-checkout.sh
index 0c74bee..2563d18 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -15,11 +15,11 @@ test_description='sparse checkout tests
 . "$TEST_DIRECTORY"/lib-read-tree.sh
 
 test_expect_success 'setup' '
-   cat >expected <<-\EOF &&
+   cat >expected <<-EOF &&
100644 77f0ba1734ed79d12881f81b36ee134de6a3327b 0   init.t
-   100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0   sub/added
-   100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0   sub/addedtoo
-   100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0   subsub/added
+   100644 $EMPTY_BLOB 0sub/added
+   100644 $EMPTY_BLOB 0sub/addedtoo
+   100644 $EMPTY_BLOB 0subsub/added
EOF
cat >expected.swt <<-\EOF &&
H init.t
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 8aef49f..292a072 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -33,14 +33,14 @@ test_expect_success 'add one file' '
git update-index --add one &&
git ls-files --stage >ls-files.actual &&
cat >ls-files.expect expect ls-files.expect ls-files.expect ls-files.expect ls-files.expect expect expect ls-files.expect expect <<-EOF &&
+   100644 blob $EMPTY_BLOB a[a]/three
EOF
git ls-tree -r HEAD "a[a]" >actual &&
test_cmp expect actual
 '
 
 test_expect_success 'ls-tree outside prefix' '
-   cat >expect <<-\EOF &&
-   100644 blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391../a[a]/three
+   cat >expect <<-EOF &&
+   100644 blob $EMPTY_BLOB ../a[a]/three
EOF
( cd aa && git ls-tree -r HEAD "../a[a]"; ) >actual &&
test_cmp expect actual
diff --git a/t/t7011-skip-worktree-reading.sh b/t/t7011-skip-worktree-reading.sh
index 88d60c1..84f4145 100755
--- a/t/t7011-skip-worktree-reading.sh
+++ b/t/t7011-skip-worktree-reading.sh
@@ -23,17 +23,15 @@ S sub/1
 H sub/2
 EOF
 
-NULL_SHA1=e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
-
 setup_absent() {
test -f 1 && rm 1
git update-index --remove 1 &&
-   git update-index --add --cacheinfo 100644 $NULL_SHA1 1 &&
+   git update-index --add --cacheinfo 100644 $EMPTY_BLOB 1 &&
git update-index --skip-worktree 1
 }
 
 test_absent() {
-   echo "100644 $NULL_SHA1 0   1" > expected &&
+   echo "100644 $EMPTY_BLOB 0  1" > expected &&
git ls-files --stage 1 > result &&
test_cmp expected result &&
test ! -f 1
@@ -42,12 +40,12 @@ test_absent() {
 setup_dirty() {
git update-index --force-remove 1 &&
echo dirty > 1 &&
-   git update-index --add --cacheinfo 100644 $NULL_SHA1 1 &&
+   git update-index --add --cacheinfo 100644 $EMPTY_BLOB 1 &&
git update-index --skip-worktree 1
 }
 
 test_dirty() {
-   echo "100644 $NULL_SHA1 0   1" > expected &&
+   echo "100644 $EMPTY_BLOB 0  1" > expected &&
git ls-files --stage 1 > result &&
test_cmp expected result &&
echo dirty > expected
@@ -120,7 +118,7 @@ test_expect_success 'grep with skip-worktree file' '
test "$(git grep --no-ext-grep test)" = "1:test"
 '
 
-echo ":00 100644 $_z40 $NULL_SHA1 A1" > expected
+echo ":00 100644 $_z40 $EMPTY_BLOB A   1" > expected
 test_expect_success 'diff-index does not examine skip-worktree absent entries' 
'
setup_absent &&
git diff-index HEAD -- 1 > result &&
diff --git a/t/t7012-skip-worktree-writing.sh b/t/t7012-skip-worktree-writing.sh
index 9ceaa40..9d1abe5 100755
--- a/t/t7012-skip-worktree-writing.sh
+++ b/t/t7012-skip-worktree-writing.sh
@@ -53,17 +53,15 @@ test_expect_success 'read-tree removes worktree, dirty 
case' '
git update-index --no-skip-worktree added
 '
 
-NULL_SHA1=e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
-
 setup_absent() {
test -f 1 && rm 1
git update-index 

[PATCH v4 4/4] cache-tree: do not generate empty trees as a result of all i-t-a subentries

[Nguyễn Thái Ngọc Duy]

If a subdirectory contains nothing but i-t-a entries, we generate an
empty tree object and add it to its parent tree. Which is wrong. Such
a subdirectory should not be added.

Note that this has a cascading effect. If subdir 'a/b/c' contains
nothing but i-t-a entries, we ignore it. But then if 'a/b' contains
only (the non-existing) 'a/b/c', then we should ignore 'a/b' while
building 'a' too. And it goes all the way up to top directory.

Noticed-by: Junio C Hamano 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 cache-tree.c  | 10 +-
 t/t2203-add-intent.sh | 14 ++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/cache-tree.c b/cache-tree.c
index c2676e8..f28b1f4 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -325,6 +325,7 @@ static int update_one(struct cache_tree *it,
const unsigned char *sha1;
unsigned mode;
int expected_missing = 0;
+   int contains_ita = 0;
 
path = ce->name;
pathlen = ce_namelen(ce);
@@ -341,7 +342,8 @@ static int update_one(struct cache_tree *it,
i += sub->count;
sha1 = sub->cache_tree->sha1;
mode = S_IFDIR;
-   if (sub->cache_tree->entry_count < 0) {
+   contains_ita = sub->cache_tree->entry_count < 0;
+   if (contains_ita) {
to_invalidate = 1;
expected_missing = 1;
}
@@ -380,6 +382,12 @@ static int update_one(struct cache_tree *it,
continue;
}
 
+   /*
+* "sub" can be an empty tree if all subentries are i-t-a.
+*/
+   if (contains_ita && !hashcmp(sha1, EMPTY_TREE_SHA1_BIN))
+   continue;
+
strbuf_grow(, entlen + 100);
strbuf_addf(, "%o %.*s%c", mode, entlen, path + baselen, 
'\0');
strbuf_add(, sha1, 20);
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 24aed2e..8f22c43 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -99,5 +99,19 @@ test_expect_success 'cache-tree does not ignore dir that has 
i-t-a entries' '
)
 '
 
+test_expect_success 'cache-tree does skip dir that becomes empty' '
+   rm -fr ita-in-dir &&
+   git init ita-in-dir &&
+   (
+   cd ita-in-dir &&
+   mkdir -p 1/2/3 &&
+   echo 4 >1/2/3/4 &&
+   git add -N 1/2/3/4 &&
+   git write-tree >actual &&
+   echo $EMPTY_TREE >expected &&
+   test_cmp expected actual
+   )
+'
+
 test_done
 
-- 
2.9.1.566.gbd532d4

--
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 v4 0/4] cache-tree building fix in the presence of ita entries

[Nguyễn Thái Ngọc Duy]

v4 removes the leading underscore from _EMPTY_BLOB and _EMPTY_TREE and
updates 4/4 slightly like this.

diff --git a/cache-tree.c b/cache-tree.c
index 2d50640..f28b1f4 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -325,6 +325,7 @@ static int update_one(struct cache_tree *it,
const unsigned char *sha1;
unsigned mode;
int expected_missing = 0;
+   int contains_ita = 0;
 
path = ce->name;
pathlen = ce_namelen(ce);
@@ -341,7 +342,8 @@ static int update_one(struct cache_tree *it,
i += sub->count;
sha1 = sub->cache_tree->sha1;
mode = S_IFDIR;
-   if (sub->cache_tree->entry_count < 0) {
+   contains_ita = sub->cache_tree->entry_count < 0;
+   if (contains_ita) {
to_invalidate = 1;
expected_missing = 1;
}
@@ -381,10 +383,9 @@ static int update_one(struct cache_tree *it,
}
 
/*
-* "sub" can be an empty tree if subentries are i-t-a.
+* "sub" can be an empty tree if all subentries are i-t-a.
 */
-   if (sub && sub->cache_tree->entry_count < 0 &&
-   !hashcmp(sha1, EMPTY_TREE_SHA1_BIN))
+   if (contains_ita && !hashcmp(sha1, EMPTY_TREE_SHA1_BIN))
continue;
 
strbuf_grow(, entlen + 100);

Nguyễn Thái Ngọc Duy (4):
  test-lib.sh: introduce and use $EMPTY_TREE
  test-lib.sh: introduce and use $EMPTY_BLOB
  cache-tree.c: fix i-t-a entry skipping directory updates sometimes
  cache-tree: do not generate empty trees as a result of all i-t-a
subentries

 cache-tree.c | 14 +++---
 t/t-basic.sh |  2 +-
 t/t1011-read-tree-sparse-checkout.sh |  8 
 t/t1100-commit-tree-options.sh   |  2 +-
 t/t1700-split-index.sh   | 24 
 t/t2203-add-intent.sh| 31 +++
 t/t3102-ls-tree-wildcards.sh |  8 
 t/t4010-diff-pathspec.sh |  2 --
 t/t4054-diff-bogus-tree.sh   | 10 --
 t/t5504-fetch-receive-strict.sh  |  4 ++--
 t/t7011-skip-worktree-reading.sh | 12 +---
 t/t7012-skip-worktree-writing.sh | 10 --
 t/t7063-status-untracked-cache.sh|  6 +++---
 t/t7508-status.sh|  2 +-
 t/test-lib.sh|  5 -
 15 files changed, 87 insertions(+), 53 deletions(-)

-- 
2.9.1.566.gbd532d4

--
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 v4 1/4] test-lib.sh: introduce and use $EMPTY_TREE

[Nguyễn Thái Ngọc Duy]

This is a special SHA1. Let's keep it at one place, easier to replace
later when the hash change comes, easier to recognize.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 t/t-basic.sh|  2 +-
 t/t1100-commit-tree-options.sh  |  2 +-
 t/t4010-diff-pathspec.sh|  2 --
 t/t4054-diff-bogus-tree.sh  | 10 --
 t/t5504-fetch-receive-strict.sh |  4 ++--
 t/test-lib.sh   |  4 +++-
 6 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/t/t-basic.sh b/t/t-basic.sh
index 60811a3..1aa5093 100755
--- a/t/t-basic.sh
+++ b/t/t-basic.sh
@@ -834,7 +834,7 @@ test_expect_success 'git write-tree should be able to write 
an empty tree' '
 '
 
 test_expect_success 'validate object ID of a known tree' '
-   test "$tree" = 4b825dc642cb6eb9a060e54bf8d69288fbee4904
+   test "$tree" = $EMPTY_TREE
 '
 
 # Various types of objects
diff --git a/t/t1100-commit-tree-options.sh b/t/t1100-commit-tree-options.sh
index b7e9b4fc..ae66ba5 100755
--- a/t/t1100-commit-tree-options.sh
+++ b/t/t1100-commit-tree-options.sh
@@ -15,7 +15,7 @@ Also make sure that command line parser understands the normal
 . ./test-lib.sh
 
 cat >expected < 1117148400 +
 committer Committer Name  1117150200 +
 
diff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh
index 43c488b..35b35a8 100755
--- a/t/t4010-diff-pathspec.sh
+++ b/t/t4010-diff-pathspec.sh
@@ -78,8 +78,6 @@ test_expect_success 'diff-tree pathspec' '
test_cmp expected current
 '
 
-EMPTY_TREE=4b825dc642cb6eb9a060e54bf8d69288fbee4904
-
 test_expect_success 'diff-tree with wildcard shows dir also matches' '
git diff-tree --name-only $EMPTY_TREE $tree -- "f*" >result &&
echo file0 >expected &&
diff --git a/t/t4054-diff-bogus-tree.sh b/t/t4054-diff-bogus-tree.sh
index 1d6efab..18f42c5 100755
--- a/t/t4054-diff-bogus-tree.sh
+++ b/t/t4054-diff-bogus-tree.sh
@@ -3,8 +3,6 @@
 test_description='test diff with a bogus tree containing the null sha1'
 . ./test-lib.sh
 
-empty_tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904
-
 test_expect_success 'create bogus tree' '
bogus_tree=$(
printf "100644 fooQ" |
@@ -22,13 +20,13 @@ test_expect_success 'create tree with matching file' '
 
 test_expect_success 'raw diff shows null sha1 (addition)' '
echo ":00 100644 $_z40 $_z40 A  foo" >expect &&
-   git diff-tree $empty_tree $bogus_tree >actual &&
+   git diff-tree $EMPTY_TREE $bogus_tree >actual &&
test_cmp expect actual
 '
 
 test_expect_success 'raw diff shows null sha1 (removal)' '
echo ":100644 00 $_z40 $_z40 D  foo" >expect &&
-   git diff-tree $bogus_tree $empty_tree >actual &&
+   git diff-tree $bogus_tree $EMPTY_TREE >actual &&
test_cmp expect actual
 '
 
@@ -57,11 +55,11 @@ test_expect_success 'raw diff shows null sha1 (index)' '
 '
 
 test_expect_success 'patch fails due to bogus sha1 (addition)' '
-   test_must_fail git diff-tree -p $empty_tree $bogus_tree
+   test_must_fail git diff-tree -p $EMPTY_TREE $bogus_tree
 '
 
 test_expect_success 'patch fails due to bogus sha1 (removal)' '
-   test_must_fail git diff-tree -p $bogus_tree $empty_tree
+   test_must_fail git diff-tree -p $bogus_tree $EMPTY_TREE
 '
 
 test_expect_success 'patch fails due to bogus sha1 (modification)' '
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 44f3d5f..9b19cff 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -115,8 +115,8 @@ test_expect_success 'push with transfer.fsckobjects' '
test_cmp exp act
 '
 
-cat >bogus-commit <<\EOF
-tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904
+cat >bogus-commit < 1234567890 +
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0055ebb..85f4c6d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -162,6 +162,8 @@ _x40="$_x05$_x05$_x05$_x05$_x05$_x05$_x05$_x05"
 # Zero SHA-1
 _z40=
 
+EMPTY_TREE=4b825dc642cb6eb9a060e54bf8d69288fbee4904
+
 # Line feed
 LF='
 '
@@ -170,7 +172,7 @@ LF='
 # when case-folding filenames
 u200c=$(printf '\342\200\214')
 
-export _x05 _x40 _z40 LF u200c
+export _x05 _x40 _z40 LF u200c EMPTY_TREE
 
 # Each test should start with something like this, after copyright notices:
 #
-- 
2.9.1.566.gbd532d4

--
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 v4 3/4] cache-tree.c: fix i-t-a entry skipping directory updates sometimes

[Nguyễn Thái Ngọc Duy]

Commit 3cf773e (cache-tree: fix writing cache-tree when CE_REMOVE is
present - 2012-12-16) skips i-t-a entries when building trees objects
from the index. Unfortunately it may skip too much.

The code in question checks if an entry is an i-t-a one, then no tree
entry will be written. But it does not take into account that
directories can also be written with the same code. Suppose we have
this in the index.

a-file
subdir/file1
subdir/file2
subdir/file3
the-last-file

We write an entry for a-file as normal and move on to subdir/file1,
where we realize the entry name for this level is simply just
"subdir", write down an entry for "subdir" then jump three items ahead
to the-last-file.

That is what happens normally when the first file in subdir is not an
i-t-a entry. If subdir/file1 is an i-t-a, because of the broken
condition in this code, we still think "subdir" is an i-t-a file and
not writing "subdir" down and jump to the-last-file. The result tree
now only has two items: a-file and the-last-file. subdir should be
there too (even though it only records two sub-entries, file2 and
file3).

If the i-t-a entry is subdir/file2 or subdir/file3, this is not a
problem because we jump over them anyway. Which may explain why the
bug is hidden for nearly four years.

Fix it by making sure we only skip i-t-a entries when the entry in
question is actual an index entry, not a directory.

Reported-by: Yuri Kanivetsky 
Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Junio C Hamano 
---
 cache-tree.c  |  4 ++--
 t/t2203-add-intent.sh | 17 +
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/cache-tree.c b/cache-tree.c
index ddf0cc9..c2676e8 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -319,7 +319,7 @@ static int update_one(struct cache_tree *it,
i = 0;
while (i < entries) {
const struct cache_entry *ce = cache[i];
-   struct cache_tree_sub *sub;
+   struct cache_tree_sub *sub = NULL;
const char *path, *slash;
int pathlen, entlen;
const unsigned char *sha1;
@@ -375,7 +375,7 @@ static int update_one(struct cache_tree *it,
 * they are not part of generated trees. Invalidate up
 * to root to force cache-tree users to read elsewhere.
 */
-   if (ce_intent_to_add(ce)) {
+   if (!sub && ce_intent_to_add(ce)) {
to_invalidate = 1;
continue;
}
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 2a4a749..24aed2e 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -82,5 +82,22 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '
test_cmp expect actual
 '
 
+test_expect_success 'cache-tree does not ignore dir that has i-t-a entries' '
+   git init ita-in-dir &&
+   (
+   cd ita-in-dir &&
+   mkdir 2 &&
+   for f in 1 2/1 2/2 3
+   do
+   echo "$f" >"$f"
+   done &&
+   git add 1 2/2 3 &&
+   git add -N 2/1 &&
+   git commit -m committed &&
+   git ls-tree -r HEAD >actual &&
+   grep 2/2 actual
+   )
+'
+
 test_done
 
-- 
2.9.1.566.gbd532d4

--
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: obsolete index in wt_status_print after pre-commit hook runs

[Andrew Keller]

Am 15.07.2016 um 6:03 nachm. schrieb Junio C Hamano :
> Junio C Hamano  writes:
>> On Fri, Jul 15, 2016 at 1:30 PM, Andrew Keller  wrote:
>>> Am 15.07.2016 um 12:34 nachm. schrieb Andrew Keller :
>>> 
 I pulled out the source for version 2.9.1 and briefly skimmed how 
 run_commit and
 prepare_to_commit work.  It seems that Git already understands that a 
 pre-commit
 hook can change the index, and it rereads the index before running the
 prepare-commit-msg hook: 
 https://github.com/git/git/blob/v2.9.1/builtin/commit.c#L941-L951
>>> 
>>> Quick question: Why does Git reread the index after the pre-commit hook 
>>> runs?
>> 
>> Offhand I do not think of a good reason to do so; does something break
>> if you took it out?
> 
> Ahh, I misremembered.  2888605c (builtin-commit: fix partial-commit
> support, 2007-11-18) does consider the possibility that pre-commit
> may have modified the index contents after we take control back from
> that hook, so that is probably a good place to enumerate what got
> changed.  Getting the list before running the hook can give an
> out-of-date list, as you said.

Interesting.  So, the implication is that disallowing the pre-commit hook
to change the index may cause some problems (491 problems, if my run
of the tests was accurate).

Does that mean it would be desirable to update the index before the
commit message template is rendered?

Thanks,
 - Andrew Keller

--
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: obsolete index in wt_status_print after pre-commit hook runs

[Andrew Keller]

Am 15.07.2016 um 5:19 nachm. schrieb Junio C Hamano :
> 
> On Fri, Jul 15, 2016 at 1:30 PM, Andrew Keller  wrote:
>> Am 15.07.2016 um 12:34 nachm. schrieb Andrew Keller :
>> 
>>> I pulled out the source for version 2.9.1 and briefly skimmed how 
>>> run_commit and
>>> prepare_to_commit work.  It seems that Git already understands that a 
>>> pre-commit
>>> hook can change the index, and it rereads the index before running the
>>> prepare-commit-msg hook: 
>>> https://github.com/git/git/blob/v2.9.1/builtin/commit.c#L941-L951
>> 
>> Quick question: Why does Git reread the index after the pre-commit hook runs?
> 
> Offhand I do not think of a good reason to do so; does something break
> if you took it out?

According to only test failures, it seems that only the 
`update_main_cache_tree(0)` invocation
is needed to avoid a torrent of test failures (490 failures across 102 tests).  
Removing lines
946, 947, 949, and 950 do not cause test breakages (although my computer is not 
set up to
run all of the tests).

However, there seems to be an interaction between lines 946-947 and 
`update_main_cache_tree(0)`
on line 948: although lines 946-947 can be removed by themselves without test 
breakages,
when 946-948 are all disabled together (and, in turn, lines 949-950 never run), 
one additional test
failure is registered (t2203.5).

Thanks,
 - Andrew Keller

--
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 05/12] check_connected: relay errors to alternate descriptor

[Jeff King]

On Fri, Jul 15, 2016 at 11:19:23AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > +   /*
> > +* If non-zero, send error messages to this descriptor rather
> > +* than stderr. The descriptor is closed before check_connected
> > +* returns.
> > +*/
> > +   int err_fd;
> 
> Theoretically speaking it may be possible that a caller may want to
> write to fd#0 if it closed the standard input before creating the
> output channel for multiplexing into a sideband, but I think this
> design strikes a good balance between the theoretical correctness
> and usability.  Using err_fd == -1 as "no redirect" may allow the
> caller to redirect the errors to fd#0, but that forces normal users
> to explicitly set this field to -1.

Yep, I had all of those thoughts while writing it, but decided that "0"
was worth it to keep the initialization simple (though since we have
CHECK_CONNECTED_INIT, the "-1" would only have to appear there).

What decided me were two things:

  - directing to fd#0 really is theoretical; you'd have to close stdin,
and you shouldn't do that. You should redirect it from /dev/null
(and our sanitize_stdfds() makes sure that we're not surprised by
any callers)

  - "struct child_process" uses a similar zero-initialization

-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 04/12] check_everything_connected: use a struct with named options

[Jeff King]

On Fri, Jul 15, 2016 at 11:13:40AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > The number of variants of check_everything_connected has
> > grown over the years, so that the "real" function takes
> > several possibly-zero, possibly-NULL arguments. We hid the
> > complexity behind some wrapper functions, but this doesn't
> > scale well when we want to add new options.
> 
> I was kind of embarrassed to admit that I wasn't even aware that
> things got this bad, so I took a look at the history to realize that
> "grown over the years" above is a bit misleading statement.  It is
> not like many people over the years were doing something like this.
> There are only two commits that brought in this pattern with poor
> design taste.

Heh. It is easy to lose sight of such things when it is not your primary
goal. You should see how horrible the code was that I wrote in my
original iteration, before I went back and did this refactoring. I am
embarrassed to have even written it in a rough draft. ;)

-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 02/12] rev-list: add optional progress reporting

[Jeff King]

On Fri, Jul 15, 2016 at 11:00:52AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> > index b82bcc3..88d95a7 100644
> > --- a/builtin/rev-list.c
> > +++ b/builtin/rev-list.c
> > @@ -9,6 +9,7 @@
> >  #include "log-tree.h"
> >  #include "graph.h"
> >  #include "bisect.h"
> > +#include "progress.h"
> >  
> >  static const char rev_list_usage[] =
> >  "git rev-list [OPTION] ... [ -- paths... ]\n"
> > @@ -49,12 +50,17 @@ static const char rev_list_usage[] =
> >  "--bisect-all"
> >  ;
> >  
> > +struct progress *progress;
> > +unsigned progress_counter;
> 
> Are these supposed to be file-scope static?

Yep, they should be (I had originally made them part of the rev_info,
but then forgot to give them "static" when I pulled them out).

-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 v3 1/2] blame: Allow to blame paths freshly added to the index

[Mike Hommey]

When blaming files, changes in the work tree are taken into account
and displayed as being "Not Committed Yet".

However, when blaming a file that is not known to the current HEAD,
git blame fails with `no such path 'foo' in HEAD`, even when the file
was git add'ed.

This would seem uninteresting with the plain `git blame` case, which
it is, but it becomes useful when using copy detection, and the new file
was created from pieces already in HEAD, moved or copied from other
files.

Signed-off-by: Mike Hommey 
---
 builtin/blame.c   | 10 +-
 t/t8003-blame-corner-cases.sh | 45 +++
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 1e214bd..f297a7f 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2230,6 +2230,7 @@ static int git_blame_config(const char *var, const char 
*value, void *cb)
 static void verify_working_tree_path(struct commit *work_tree, const char 
*path)
 {
struct commit_list *parents;
+   int pos;
 
for (parents = work_tree->parents; parents; parents = parents->next) {
const unsigned char *commit_sha1 = 
parents->item->object.oid.hash;
@@ -2240,7 +2241,14 @@ static void verify_working_tree_path(struct commit 
*work_tree, const char *path)
sha1_object_info(blob_sha1, NULL) == OBJ_BLOB)
return;
}
-   die("no such path '%s' in HEAD", path);
+
+   pos = cache_name_pos(path, strlen(path));
+   if (pos >= 0)
+   ; /* path is in the index */
+   else if (!strcmp(active_cache[-1 - pos]->name, path))
+   ; /* path is in the index, unmerged */
+   else
+   die("no such path '%s' in HEAD", path);
 }
 
 static struct commit_list **append_parent(struct commit_list **tail, const 
unsigned char *sha1)
diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
index a9b266f..eab2e28 100755
--- a/t/t8003-blame-corner-cases.sh
+++ b/t/t8003-blame-corner-cases.sh
@@ -137,6 +137,51 @@ test_expect_success 'blame wholesale copy and more' '
 
 '
 
+test_expect_success 'blame wholesale copy and more in the index' '
+
+   cat >horse <<-\EOF &&
+   ABC
+   DEF
+   
+   
+   GHIJK
+   EOF
+   git add horse &&
+   test_when_finished "git rm -f horse" &&
+   git blame -f -C -C1 -- horse | sed -e "$pick_fc" >current &&
+   cat >expected <<-\EOF &&
+   mouse-Initial
+   mouse-Second
+   cow-Fifth
+   horse-Not
+   mouse-Third
+   EOF
+   test_cmp expected current
+
+'
+
+test_expect_success 'blame during cherry-pick with file rename conflict' '
+
+   test_when_finished "git reset --hard && git checkout master" &&
+   git checkout HEAD~3 &&
+   echo MOUSE >> mouse &&
+   git mv mouse rodent &&
+   git add rodent &&
+   GIT_AUTHOR_NAME=Rodent git commit -m "rodent" &&
+   git checkout --detach master &&
+   (git cherry-pick HEAD@{1} || test $? -eq 1) &&
+   git show HEAD@{1}:rodent > rodent &&
+   git add rodent &&
+   git blame -f -C -C1 rodent | sed -e "$pick_fc" >current &&
+   cat current &&
+   cat >expected <<-\EOF &&
+   mouse-Initial
+   mouse-Second
+   rodent-Not
+   EOF
+   test_cmp expected current
+'
+
 test_expect_success 'blame path that used to be a directory' '
mkdir path &&
echo A A A A A >path/file &&
-- 
2.9.1.277.gdbc86c9.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

[PATCH v3 2/2] t/t8003-blame-corner-cases.sh: Use here documents

[Mike Hommey]

Somehow, this test was using:

{
echo A
echo B
} > file

block to feed file contents. This changes those to the form most common
in git test scripts:

cat >file <<-\EOF
A
B
EOF

Signed-off-by: Mike Hommey 
---
 t/t8003-blame-corner-cases.sh | 34 +-
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
index eab2e28..e48370d 100755
--- a/t/t8003-blame-corner-cases.sh
+++ b/t/t8003-blame-corner-cases.sh
@@ -41,12 +41,12 @@ test_expect_success setup '
test_tick &&
GIT_AUTHOR_NAME=Fourth git commit -m Fourth &&
 
-   {
-   echo ABC
-   echo DEF
-   echo 
-   echo GHIJK
-   } >cow &&
+   cat >cow <<-\EOF &&
+   ABC
+   DEF
+   
+   GHIJK
+   EOF
git add cow &&
test_tick &&
GIT_AUTHOR_NAME=Fifth git commit -m Fifth
@@ -115,11 +115,11 @@ test_expect_success 'append with -C -C -C' '
 test_expect_success 'blame wholesale copy' '
 
git blame -f -C -C1 HEAD^ -- cow | sed -e "$pick_fc" >current &&
-   {
-   echo mouse-Initial
-   echo mouse-Second
-   echo mouse-Third
-   } >expected &&
+   cat >expected <<-\EOF &&
+   mouse-Initial
+   mouse-Second
+   mouse-Third
+   EOF
test_cmp expected current
 
 '
@@ -127,12 +127,12 @@ test_expect_success 'blame wholesale copy' '
 test_expect_success 'blame wholesale copy and more' '
 
git blame -f -C -C1 HEAD -- cow | sed -e "$pick_fc" >current &&
-   {
-   echo mouse-Initial
-   echo mouse-Second
-   echo cow-Fifth
-   echo mouse-Third
-   } >expected &&
+   cat >expected <<-\EOF &&
+   mouse-Initial
+   mouse-Second
+   cow-Fifth
+   mouse-Third
+   EOF
test_cmp expected current
 
 '
-- 
2.9.1.277.gdbc86c9.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] blame: Allow to blame paths freshly added to the index

[Mike Hommey]

> I suspect that this would be useful without copy detection.  If you
> "git mv fileA fileB" (optionally followed by "edit fileB"), fileB
> would not be in HEAD but you should be able to trace the lineage of
> the lines in it back through the renaming event, and this change
> also allows that use case, no?

It should, but that'd be copy/move detection, wouldn't it? :)

> > I suspect that the above change needs to be updated further if the
> > user wants to run "blame path" during a conflicted renaming merge,
> > i.e.
> >
> >  0. Before two histories diverged, there was old_path.
> >  1. Our side updated contents of that file and kept it at old_path.
> >  2. Their side updated contents of that file and moved it to path.
> >  3. "git merge" notcied the rename, created three stages at "path",
> > with the result of conflicted content-level merge in the working
> > tree at "path".
> >  4. The user edits "path" and resolves the conflict, but wants to
> > double check before running "git add path".
> >  5. "git blame path"
> >
> > Perhaps something like this should suffice:
> >
> > pos = cache_name_pos(path, strlen(path));
> > if (0 <= pos)
> > ; /* ok */
> > else if (!strcmp(active_cache[-1 - pos]->name), path)
> > ; /* ok -- just unmerged */
> > else
> > die("no such path in HEAD");
> 
> I do not think the "conflicted renaming merge" example would not be
> a problem in practice iff "git merge" was used, because the fake
> working tree commit would look at both our tree and their tree, and
> find "path" in theirs inside the loop above this "die".

More than that, it seems that's a case that already works without the
patch (it doesn't complain that "no such path in HEAD", although it
only identifies the "theirs" part of the merge conflict when blaming
the file straight out of the merge failure, but that'd be a separate
issue.

> But the user can be in the same conflicted rename situation with
> "git am -3" or cherry-pick, and in these cases there won't be extra
> parent commits for the fake work tree commit, hence the conclusion
> does not change.

Indeed, with cherry-pick, the "no such path in HEAD" error is happening
with the patch.

Mike
--
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: obsolete index in wt_status_print after pre-commit hook runs

[Junio C Hamano]

Junio C Hamano  writes:

> On Fri, Jul 15, 2016 at 1:30 PM, Andrew Keller  wrote:
>> Am 15.07.2016 um 12:34 nachm. schrieb Andrew Keller :
>>
>>> I pulled out the source for version 2.9.1 and briefly skimmed how 
>>> run_commit and
>>> prepare_to_commit work.  It seems that Git already understands that a 
>>> pre-commit
>>> hook can change the index, and it rereads the index before running the
>>> prepare-commit-msg hook: 
>>> https://github.com/git/git/blob/v2.9.1/builtin/commit.c#L941-L951
>>
>> Quick question: Why does Git reread the index after the pre-commit hook runs?
>
> Offhand I do not think of a good reason to do so; does something break
> if you took it out?

Ahh, I misremembered.  2888605c (builtin-commit: fix partial-commit
support, 2007-11-18) does consider the possibility that pre-commit
may have modified the index contents after we take control back from
that hook, so that is probably a good place to enumerate what got
changed.  Getting the list before running the hook can give an
out-of-date list, as you said.

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: Plugin mechanism(s) for Git?

[Eric Wong]

Konstantin Khomoutov  wrote:
> Ben Peart  wrote:
> > > Lars Schneider wrote:
> > > 
> > > We could dynamically load libraries but this would force us to
> > > freeze the ABI as mentioned by Duy:
> > > http://article.gmane.org/gmane.comp.version-control.git/298463
> > > 
> > 
> > I wouldn’t be too quick to dismiss dynamically loaded libraries as
> > there are some distinct advantages over the other patterns especially
> > performance and simplicity.  I realize it requires us to version the
> > ABI but there are established patterns to manage this.  It also isn’t
> > that much different than us having to freeze or version the protocol
> > for communicating with a remote-helper.

(re-adding dropped CCs)

The critical difference is protocols can be tested and debugged
in a language/tool-independent way.

> Using dynamically loaded libraries precludes or greatly complicates
> creation of plugins written in languages different from C (or C++ FWIW).

Agreed, and folks working on language bindings are often not
experts in or comfortable working in C (I speak for myself,
at least).

It's probably not a good use of git core developers time to
learn to deal with bindings and quirks for language XYZ, either
(as language XYZ likely breaks compatibility more than C,
POSIX or the git data model).

The SVN Perl XS bindings we use for git-svn have introduced
numerous incompatibilities and bugs over the years we've had to
deal with.  I doubt Perl bindings are a high priority for SVN
developers; and even checking their API headers reveals a huge
number of deprecated functions in their native C API.  I'm not
sure if it's a lack of foresight on their part or what, but I'm
happy git's data model and command-line interface has barely had
to deprecate or break compatibility in over a decade.

Protocols like "cat-file --batch" and fast-import using
pipes is great and I hope they're portable enough.  I would
welcome additions to avoid process spawning costs for things
like update-ref/rev-parse/etc...
--
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

[ANNOUNCE] Git v2.9.2

[Junio C Hamano]

The latest maintenance release Git v2.9.2 is now available at
the usual places.  This is only a fix to the test suite; there is
no change in the actual code produced for your daily use of Git.

The tarballs are found at:

https://www.kernel.org/pub/software/scm/git/

The following public repositories all have a copy of the 'v2.9.2'
tag and the 'maint' branch that the tag points at:

  url = https://kernel.googlesource.com/pub/scm/git/git
  url = git://repo.or.cz/alt-git.git
  url = git://git.sourceforge.jp/gitroot/git-core/git.git
  url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core
  url = https://github.com/gitster/git



Git v2.9.2 Release Notes


Fixes since v2.9.1
--

 * A fix merged to v2.9.1 had a few tests that are not meant to be
   run on platforms without 64-bit long, which caused unnecessary
   test failures on them because we didn't detect the platform and
   skip them.  These tests are now skipped on platforms that they
   are not applicable to.

No other change is included in this update.



Changes since v2.9.1 are as follows:

Jeff King (1):
  t0006: skip "far in the future" test when unsigned long is not long enough

Junio C Hamano (1):
  Git 2.9.2

--
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: obsolete index in wt_status_print after pre-commit hook runs

[Junio C Hamano]

On Fri, Jul 15, 2016 at 1:30 PM, Andrew Keller  wrote:
> Am 15.07.2016 um 12:34 nachm. schrieb Andrew Keller :
>
>> I pulled out the source for version 2.9.1 and briefly skimmed how run_commit 
>> and
>> prepare_to_commit work.  It seems that Git already understands that a 
>> pre-commit
>> hook can change the index, and it rereads the index before running the
>> prepare-commit-msg hook: 
>> https://github.com/git/git/blob/v2.9.1/builtin/commit.c#L941-L951
>
> Quick question: Why does Git reread the index after the pre-commit hook runs?

Offhand I do not think of a good reason to do so; does something break
if you took it out?
--
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] blame: Allow to blame paths freshly added to the index

[Junio C Hamano]

Junio C Hamano  writes:

> I suspect that the above change needs to be updated further if the
> user wants to run "blame path" during a conflicted renaming merge,
> i.e.
>
>  0. Before two histories diverged, there was old_path.
>  1. Our side updated contents of that file and kept it at old_path.
>  2. Their side updated contents of that file and moved it to path.
>  3. "git merge" notcied the rename, created three stages at "path",
> with the result of conflicted content-level merge in the working
> tree at "path".
>  4. The user edits "path" and resolves the conflict, but wants to
> double check before running "git add path".
>  5. "git blame path"
>
> Perhaps something like this should suffice:
>
> pos = cache_name_pos(path, strlen(path));
> if (0 <= pos)
>   ; /* ok */
> else if (!strcmp(active_cache[-1 - pos]->name), path)
> ; /* ok -- just unmerged */
> else
>   die("no such path in HEAD");

I do not think the "conflicted renaming merge" example would not be
a problem in practice iff "git merge" was used, because the fake
working tree commit would look at both our tree and their tree, and
find "path" in theirs inside the loop above this "die".

But the user can be in the same conflicted rename situation with
"git am -3" or cherry-pick, and in these cases there won't be extra
parent commits for the fake work tree commit, hence the conclusion
does not change.

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] blame: Allow to blame paths freshly added to the index

[Junio C Hamano]

Mike Hommey  writes:

> When blaming files, changes in the work tree are taken into account
> and displayed as being "Not Committed Yet".
>
> However, when blaming a file that is not known to the current HEAD,
> git blame fails with `no such path 'foo' in HEAD`, even when the file
> was git add'ed.
>
> This would seem uninteresting with the plain `git blame` case, which
> it is, but it becomes useful when using copy detection, and the new file
> was created from pieces already in HEAD, moved or copied from other
> files.

I suspect that this would be useful without copy detection.  If you
"git mv fileA fileB" (optionally followed by "edit fileB"), fileB
would not be in HEAD but you should be able to trace the lineage of
the lines in it back through the renaming event, and this change
also allows that use case, no?

> ---

Missing sign-off?

>  builtin/blame.c   |  4 +++-
>  t/t8003-blame-corner-cases.sh | 23 +++
>  2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 1e214bd..0858b18 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2240,7 +2240,9 @@ static void verify_working_tree_path(struct commit 
> *work_tree, const char *path)
>   sha1_object_info(blob_sha1, NULL) == OBJ_BLOB)
>   return;
>   }
> - die("no such path '%s' in HEAD", path);
> +
> + if (cache_name_pos(path, strlen(path)) < 0)
> + die("no such path '%s' in HEAD", path);
>  }

This is a surprisingly small change to bring a big difference at the
usage level.  I am sort-of surprised that the "fake working tree
commit" mechanism was already prepared to handle this, even though I
am responsible for the introduction of it at 1cfe7733 (git-blame: no
rev means start from the working tree file., 2007-01-30).

Having said that, do we act differently when the index is unmerged
at path in any way?  When path exists but is unmerged, you get
negative index from cache_name_pos(), and IIUC, "blaming of working
tree file" does not care if the index is unmerged.  It creates the
fake working tree file commit, pretends HEAD is its parent, and digs
the lineage from there.

I suspect that the above change needs to be updated further if the
user wants to run "blame path" during a conflicted renaming merge,
i.e.

 0. Before two histories diverged, there was old_path.
 1. Our side updated contents of that file and kept it at old_path.
 2. Their side updated contents of that file and moved it to path.
 3. "git merge" notcied the rename, created three stages at "path",
with the result of conflicted content-level merge in the working
tree at "path".
 4. The user edits "path" and resolves the conflict, but wants to
double check before running "git add path".
 5. "git blame path"

Perhaps something like this should suffice:

pos = cache_name_pos(path, strlen(path));
if (0 <= pos)
; /* ok */
else if (!strcmp(active_cache[-1 - pos]->name), path)
; /* ok -- just unmerged */
else
die("no such path in HEAD");

--
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] contrib/persistent-https: update ldflags syntax for Go 1.7+

[Parker Moore]

From: Parker Moore 

This fixes contrib/persistent-https builds for Go v1.7+ and is
compatible with Go v1.5+.

Running `make all` in `contrib/persistent-https` results in a failure
on Go 1.7 and above.

Specifically, the error is:

go build -o git-remote-persistent-https \
   -ldflags "-X main._BUILD_EMBED_LABEL 1468613136"
# _/Users/parkr/github/git/contrib/persistent-https
/usr/local/Cellar/go/1.7rc1/libexec/pkg/tool/darwin_amd64/link: -X
flag requires argument of the form importpath.name=value
make: *** [git-remote-persistent-https] Error 2

This `name=value` syntax for the -X flag was introduced in Go v1.5
(released Aug 19, 2015):

- release notes: https://golang.org/doc/go1.5#link
- commit: 
https://github.com/golang/go/commit/12795c02f3d6fc54ece09a86e70aaa40a94d5131

In Go v1.7, support for the old syntax was removed:

- release notes: https://tip.golang.org/doc/go1.7#compiler
- commit: 
https://github.com/golang/go/commit/51b624e6a29b135ce0fadb22b678acf4998ff16f

This patch includes the `=` to fix builds with Go v1.7+.
---
 contrib/persistent-https/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/persistent-https/Makefile
b/contrib/persistent-https/Makefile
index 92baa3be..033140b 100644
--- a/contrib/persistent-https/Makefile
+++ b/contrib/persistent-https/Makefile
@@ -26,7 +26,7 @@ git-remote-persistent-http: git-remote-persistent-https

 git-remote-persistent-https:
  go build -o git-remote-persistent-https \
- -ldflags "-X main._BUILD_EMBED_LABEL $(BUILD_LABEL)"
+ -ldflags "-X main._BUILD_EMBED_LABEL=$(BUILD_LABEL)"

 clean:
  rm -f git-remote-persistent-http* *.tar.gz
--
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] git-clean: remove fifo, devices, socket files

[Johannes Sixt]

Am 15.07.2016 um 09:46 schrieb Andrey Vagin:

On Thu, Jul 14, 2016 at 10:56 PM, Johannes Sixt  wrote:

IOW: These special files are invisible for Git unless it already knows the
names. The latter case is outside 'git clean's domain, and the former case
really means that special files in the working tree are left at the user's
discretion.


I understand your points, but I don't see any reasons to ignore these files.

What will be wrong if 'git status' will reports these files?
What will be wrong if 'git add' will returns an error instead of
skipping them silently?


I can buy that 'git add' reports an error for special files. (And I 
concur with Dscho that the behavior should otherwise remain unchanged.) 
But this is not what the commit message sells even if the patch changes 
the behavior of 'git add', too (I haven't tested the patch).


-- Hannes

--
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: obsolete index in wt_status_print after pre-commit hook runs

[Andrew Keller]

Am 15.07.2016 um 12:34 nachm. schrieb Andrew Keller :

> I pulled out the source for version 2.9.1 and briefly skimmed how run_commit 
> and
> prepare_to_commit work.  It seems that Git already understands that a 
> pre-commit
> hook can change the index, and it rereads the index before running the
> prepare-commit-msg hook: 
> https://github.com/git/git/blob/v2.9.1/builtin/commit.c#L941-L951

Quick question: Why does Git reread the index after the pre-commit hook runs?

Thanks,
 - Andrew Keller

--
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 12/12] receive-pack: send keepalives during quiet periods

[Junio C Hamano]

Jeff King  writes:

> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 1cba120..54f2cfb 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1626,6 +1626,7 @@ int cmd_index_pack(int argc, const char **argv, const 
> char *prefix)
>   struct pack_idx_option opts;
>   unsigned char pack_sha1[20];
>   unsigned foreign_nr = 1;/* zero is a "good" value, assume bad */
> + int report_end_of_input = 0;
>  
>   if (argc == 2 && !strcmp(argv[1], "-h"))
>   usage(index_pack_usage);
> @@ -1697,6 +1698,8 @@ int cmd_index_pack(int argc, const char **argv, const 
> char *prefix)
>   verbose = 1;
>   } else if (!strcmp(arg, "--show-resolving-progress")) {
>   show_resolving_progress = 1;
> + } else if (!strcmp(arg, "--report-end-of-input")) {
> + report_end_of_input = 1;
>   } else if (!strcmp(arg, "-o")) {
>   if (index_name || (i+1) >= argc)
>   usage(index_pack_usage);
> @@ -1754,6 +1757,8 @@ int cmd_index_pack(int argc, const char **argv, const 
> char *prefix)
>   obj_stat = xcalloc(st_add(nr_objects, 1), sizeof(struct 
> object_stat));
>   ofs_deltas = xcalloc(nr_objects, sizeof(struct ofs_delta_entry));
>   parse_pack_objects(pack_sha1);
> + if (report_end_of_input)
> + write_in_full(2, "\0", 1);
>   resolve_deltas();
>   conclude_pack(fix_thin_pack, curr_pack, pack_sha1);
>   free(ofs_deltas);

I naively thought "well, if we are teaching index-pack a new trick
anyway, why not make it do the keepalive?", but that would not work
because a keepalive is a 0-byte payload on band#1, i.e. "0005\1",
and index-pack is not aware of the sideband framing at all.

So I agree that a-nul-in-fd#2 is the best mechansim we can make it
work.

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

[StGit PATCH] contrib/vim: Fix filetype detection in VIM >=7.4

[Zane Bitter]

The command "setfiletype" will not override an existing filetype. This was
never a problem for me previously, but since upgrading from VIM 7.3 to 7.4
the filetype for StGit's files is explicitly set to "text", preventing the
stgit ftdetect plugin overriding it. Use "setlocal filetype=" instead to
ensure that we override any previously detected filetype.

Signed-off-by: Zane Bitter 
---
 contrib/vim/ftdetect/stg.vim |   11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/contrib/vim/ftdetect/stg.vim b/contrib/vim/ftdetect/stg.vim
index abd4d9f..adf46dd 100644
--- a/contrib/vim/ftdetect/stg.vim
+++ b/contrib/vim/ftdetect/stg.vim
@@ -6,23 +6,22 @@
 if has("autocmd")
 
   " Detect 'stg new' files
-  autocmd BufNewFile,BufRead .stgit-new.txt   setf stgnew
-  autocmd BufNewFile,BufRead .stgitmsg.txtsetf stgnew
+  autocmd BufNewFile,BufRead .stgit-new.txt   setlocal filetype=stgnew
+  autocmd BufNewFile,BufRead .stgitmsg.txtsetlocal filetype=stgnew
   " Ignore the modeline so we get type 'stgnew' instead of 'diff'
   autocmd BufNewFile,BufRead .stgitmsg.txtsetlocal nomodeline
 
   " Detect 'stg edit' files
-  autocmd BufNewFile,BufRead .stgit-edit.txt  setf stgedit
-  " Use set filetype instead of setfiletype to override detection as patch
+  autocmd BufNewFile,BufRead .stgit-edit.txt  setlocal filetype=stgedit
   autocmd BufNewFile,BufRead .stgit-edit.patchsetlocal filetype=stgedit
   autocmd BufNewFile,BufRead .stgit-edit.diff setlocal filetype=stgedit
   autocmd BufNewFile,BufRead .stgit-failed.patch  setlocal filetype=stgedit
 
   " Detect 'stg squash' files
-  autocmd BufNewFile,BufRead .stgit-squash.txtsetf stgsquash
+  autocmd BufNewFile,BufRead .stgit-squash.txtsetlocal filetype=stgsquash
 
   " Detect 'stg mail' files
-  autocmd BufNewFile,BufRead .stgitmail.txt   setf stgmail
+  autocmd BufNewFile,BufRead .stgitmail.txt   setlocal filetype=stgmail
 
 
   " A modeline in a diff belongs to the diffed file, so ignore it

--
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

[SOLVED] Re: Two consecutive clones of a remote produces different files

[Morten W. J.]

Hi Jeff.

Hmmm. You are the master of blind guessing - spot on.
Also, that is kind of embaressing to me. However, should your way cross mine, 
you hereby have a voucer for a drink after your choice as long as it is beer.

Thanks,
   Morten.

> Just a blind guess, but might you have two files with names that differ
> only in case, and on Windows one is overwriting the other because you
> have a case-insensitive filesystem?
> 
> Try:
> 
>   git ls-files -s | grep -i src/LogEventSubscriber.h
> 
> to see what is in the index (which is case-sensitive, and is the source
> from which git checks the files out into the working tree).
> 
> -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 05/12] check_connected: relay errors to alternate descriptor

[Junio C Hamano]

Jeff King  writes:

> + /*
> +  * If non-zero, send error messages to this descriptor rather
> +  * than stderr. The descriptor is closed before check_connected
> +  * returns.
> +  */
> + int err_fd;

Theoretically speaking it may be possible that a caller may want to
write to fd#0 if it closed the standard input before creating the
output channel for multiplexing into a sideband, but I think this
design strikes a good balance between the theoretical correctness
and usability.  Using err_fd == -1 as "no redirect" may allow the
caller to redirect the errors to fd#0, but that forces normal users
to explicitly set this field to -1.
--
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 04/12] check_everything_connected: use a struct with named options

[Junio C Hamano]

Jeff King  writes:

> The number of variants of check_everything_connected has
> grown over the years, so that the "real" function takes
> several possibly-zero, possibly-NULL arguments. We hid the
> complexity behind some wrapper functions, but this doesn't
> scale well when we want to add new options.

I was kind of embarrassed to admit that I wasn't even aware that
things got this bad, so I took a look at the history to realize that
"grown over the years" above is a bit misleading statement.  It is
not like many people over the years were doing something like this.
There are only two commits that brought in this pattern with poor
design taste.

> Instead, let's add a struct to hold all of the optional
> parameters.
> ...
>  connected.c| 39 +++

Yup, that is absolutely the right thing to do.

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 02/12] rev-list: add optional progress reporting

[Junio C Hamano]

Jeff King  writes:

> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index b82bcc3..88d95a7 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -9,6 +9,7 @@
>  #include "log-tree.h"
>  #include "graph.h"
>  #include "bisect.h"
> +#include "progress.h"
>  
>  static const char rev_list_usage[] =
>  "git rev-list [OPTION] ... [ -- paths... ]\n"
> @@ -49,12 +50,17 @@ static const char rev_list_usage[] =
>  "--bisect-all"
>  ;
>  
> +struct progress *progress;
> +unsigned progress_counter;

Are these supposed to be file-scope static?

--
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: obsolete index in wt_status_print after pre-commit hook runs

[Andrew Keller]

On 15.07.2016, at 1:28 nachm., Junio C Hamano  wrote:

> Earlier you said you are working on a patch series.  Since you have
> already looked at the codepath already, perhaps you may want to try
> a patch series to add the missing error-return instead, if you are
> interested?

Definitely interested — Sounds like a great learning experience.

Thanks,
 - Andrew Keller

--
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: obsolete index in wt_status_print after pre-commit hook runs

[Junio C Hamano]

Andrew Keller  writes:

> On 15.07.2016, at 1:02 nachm., Junio C Hamano  wrote:
>
>> Expected outcome is an error saying "do not modify the index inside
>> pre-commit hook", and a rejection.  It was meant as a verification
>> mechansim (hence it can be bypassed with --no-verify), not as a way
>> to make changes that the user didn't tell "git commit" to make.
>
> Ah!  Good to know, then.  I’ll rewrite my hook to behave more correctly.

No problem.

>> It is just the implementation that dates back to the old days were
>> too trusting that all users would behave (with its own definition of
>> "behaving well", which may or may not match your expectation), did
>> not anticipate that people would try to muck with the contents being
>> commited in the hook, and did not implement such verification.

Earlier you said you are working on a patch series.  Since you have
already looked at the codepath already, perhaps you may want to try
a patch series to add the missing error-return instead, if you are
interested?

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 12/12] receive-pack: send keepalives during quiet periods

[Stefan Beller]

On Fri, Jul 15, 2016 at 3:43 AM, Jeff King  wrote:

>
> Signed-off-by: Jeff King 

Read-entirely-by Stefan ;)
Thanks!

> @@ -319,10 +331,60 @@ static void rp_error(const char *err, ...)
>  static int copy_to_sideband(int in, int out, void *arg)
>  {
> char data[128];

While looking at this code, do you think it is feasible to increase the
size of data[] to 1024 ? (The largest that is possible when
side-band, but no side-band-64k is given).

> +   int keepalive_active = 0;
> +
> +   if (keepalive_in_sec <= 0)
> +   use_keepalive = KEEPALIVE_NEVER;
> +   if (use_keepalive == KEEPALIVE_ALWAYS)
> +   keepalive_active = 1;
> +
> while (1) {
> -   ssize_t sz = xread(in, data, sizeof(data));
> +   ssize_t sz;
> +
> +   if (keepalive_active) {
> +   struct pollfd pfd;
> +   int ret;
> +
> +   pfd.fd = in;
> +   pfd.events = POLLIN;
> +   ret = poll(, 1, 1000 * keepalive_in_sec);
> +
> +   if (ret < 0) {
> +   if (errno == EINTR)
> +   continue;
> +   else
> +   break;

The method was short and concise, this adds a lot of lines.
Remembering d751dd11 (2016-07-10, hoist out handle_nonblock
function for xread and xwrite), do you think it would be reasonable to
put the whole poll handling into a dedicated function, maybe even reuse the
that function?

if (keepalive_active) {
if (wrapper_around_poll(_in) < 0) // handles EINTR internally
break;
if (!data_in)
send_keep_alive();
}

I am not sure if that makes this function more legible, just food for thought.


> +   } else if (ret == 0) {
> +   /* no data; send a keepalive packet */
> +   static const char buf[] = "0005\1";

and the \1 is the first sideband. Why do we choose that sideband?

> +   write_or_die(1, buf, sizeof(buf) - 1);
> +   continue;
> +   } /* else there is actual data to read */

"If there is data to read, we need to break the while(1), to actually
read the data?"
I got confused and needed to go back and read the actual code again,
would it make sense to rather have a loop here?

while (1) {
   while(keepalive_active) {
if (wrapper_around_poll(_in) < 0) // handles EINTR internally
break;
if (!data_in)
send_keep_alive();
else
break;
}

   sz = xread(in, data, sizeof(data));
 if (sz <= 0)
 break;

turn_on_keepalive_on_NUL();
 }

> +   }
> +
> +   sz = xread(in, data, sizeof(data));
> if (sz <= 0)
> break;
> +
> +   if (use_keepalive == KEEPALIVE_AFTER_NUL && 
> !keepalive_active) {
> +   const char *p = memchr(data, '\0', sz);
> +   if (p) {
> +   /*
> +* The NUL tells us to start sending 
> keepalives. Make
> +* sure we send any other data we read along
> +* with it.
> +*/
> +   keepalive_active = 1;
> +   send_sideband(1, 2, data, p - data, 
> use_sideband);
> +   send_sideband(1, 2, p + 1, sz - (p - data + 
> 1), use_sideband);
> +   continue;

Oh, I see why the turn_on_keepalive_on_NUL doesn't work as well as I thought.
I wonder if we can use a better read function, that would stop reading at a NUL,
and return early instead?
--
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: obsolete index in wt_status_print after pre-commit hook runs

[Andrew Keller]

On 15.07.2016, at 1:02 nachm., Junio C Hamano  wrote:

> Expected outcome is an error saying "do not modify the index inside
> pre-commit hook", and a rejection.  It was meant as a verification
> mechansim (hence it can be bypassed with --no-verify), not as a way
> to make changes that the user didn't tell "git commit" to make.

Ah!  Good to know, then.  I’ll rewrite my hook to behave more correctly.

Thanks,
 - Andrew Keller

--
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 2/1] Verify that --cherry-pick avoids looking at full diffs

[Junio C Hamano]

Johannes Schindelin  writes:

> The entire point of the previous patch was to make sure that we look at
> abbreviated patch IDs (using the diff *headers* only, but avoiding
> to load the blobs into memory and diff them) first, and only look at
> full patch IDs when the abbreviated patch IDs were not for the
> --cherry-pick test.
>
> Let's make sure that we actually avoid looking at the full patch ID,
> simply by corrupting an object that is needed for the full patch ID, and
> then seeing that --cherry-pick still works.

I think "Avoid looking at" merely is the means to an end, and not
the goal by itself.  By not looking at them, you hopefully run
faster.

So I'd think a more useful addition under t/ would be to t/perf
somewhere, not "now you can rev-list --cherry-pick even inside a
corrupt repository, as long as corruption happens to be with blobs
and not the containing trees".

--
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: obsolete index in wt_status_print after pre-commit hook runs

[Junio C Hamano]

Andrew Keller  writes:

> I have observed an interesting scenario.  Here are example reproduction steps:
>
> 1. new repository
> 2. create new pre-commit hook that invokes `git mv one two`
> 3. touch one
> 4. git add one
> 5. git commit
>
> Expected outcome: In the commit message template, I expect to see
> “Changes to be committed: new file: two"

Expected outcome is an error saying "do not modify the index inside
pre-commit hook", and a rejection.  It was meant as a verification
mechansim (hence it can be bypassed with --no-verify), not as a way
to make changes that the user didn't tell "git commit" to make.

It is just the implementation that dates back to the old days were
too trusting that all users would behave (with its own definition of
"behaving well", which may or may not match your expectation), did
not anticipate that people would try to muck with the contents being
commited in the hook, and did not implement such verification.


--
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: Plugin mechanism(s) for Git?

[Konstantin Khomoutov]

On Fri, 15 Jul 2016 16:18:28 + (UTC)
Ben Peart  wrote:

> > Thanks for this great summary of the problem, Christian!
> > 
> > I think a generic plugin mechanism would be great but how would we
> > do it?
> 
> I’m also very glad to see the discussion about coming up with a good
> pattern for how git can interact with external code.  I had also
> noticed all the in-flight topics as I was searching for a good
> pattern to adopt.
> 
> > 
> > We could dynamically load libraries but this would force us to
> > freeze the ABI as mentioned by Duy:
> > http://article.gmane.org/gmane.comp.version-control.git/298463
> > 
> 
> I wouldn’t be too quick to dismiss dynamically loaded libraries as
> there are some distinct advantages over the other patterns especially
> performance and simplicity.  I realize it requires us to version the
> ABI but there are established patterns to manage this.  It also isn’t
> that much different than us having to freeze or version the protocol
> for communicating with a remote-helper.

Using dynamically loaded libraries precludes or greatly complicates
creation of plugins written in languages different from C (or C++ FWIW).

So that could be okay for some type of plugins (let's call them "core")
but not really for third-party / inhouse implementations.
--
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 v4 2/5] t5000: test tar files that overflow ustar headers

[Junio C Hamano]

Johannes Schindelin  writes:

> So what are your plans with 2.9.2? I ask because I do not want to engineer
> a 2.9.1 release just to see that 2.9.2 is out and having to spend the same
> amount of time for another release ;-)

Essentially unchanged from what I said in $gmane/299446 a few days
ago.  Merge only the t0006 workaround with lazy-prereq changes to
'maint', so that we can skip tests that are unrunnable on platforms
without 64-bit long to avoid unnecessary test failures, then tag
that to v2.9.2 soon enough so that you do not have to do two
releases in a row (i.e. skipping v2.9.1 saying "Git for Windows
skipped that one because it was not quite right; this release fixes
the issue" in your v2.9.2 announcement).

I couldn't do that while that t0006 fix was in flux.  Now I
hopefully can.







--
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

obsolete index in wt_status_print after pre-commit hook runs

[Andrew Keller]

Hi everyone,

I have observed an interesting scenario.  Here are example reproduction steps:

1. new repository
2. create new pre-commit hook that invokes `git mv one two`
3. touch one
4. git add one
5. git commit

Expected outcome: In the commit message template, I expect to see “Changes to 
be committed: new file: two"

Found outcome: In the commit message template, I see “Changes to be committed: 
new file: one"

This behavior seems to be reproducible in versions 2.9.1, 2.8.1, 2.0.0, and 
1.6.0.

Skip the next 3 paragraphs if you are in a hurry.

I pulled out the source for version 2.9.1 and briefly skimmed how run_commit and
prepare_to_commit work.  It seems that Git already understands that a pre-commit
hook can change the index, and it rereads the index before running the
prepare-commit-msg hook: 
https://github.com/git/git/blob/v2.9.1/builtin/commit.c#L941-L951

During the prepare-commit-msg hook, it seems that the index (according to Git
commands) is correct and up-to-date, but the textual message inside the commit
message template is out-of-date (it references the file `one` as a change to be
committed).

In builtin/commit.c, it seems that the commit message template is rendered
immediately after the pre-commit hook is ran, and immediately before the index 
is
reread.  If I move the small block of code that rereads the index up, to just 
after
the pre-commit hook is ran, the commit message template seems to be as I would
expect, both in .git/COMMIT_EDITMSG during the prepare-commit-msg hook and
in the editor for the commit message itself.

I am putting together a 2-patch series that includes a failing test, and then 
this
change (which fixes the test), but while I do that, I figure I may as well ping 
the
community to make sure that this behavior is not intentional.  I’d wager that 
this
change is for the better, but since this behavior has been around so long (I 
stopped
checking at 1.6.0), it doesn’t hurt to make sure.

Any comments, concerns, or advice?

Thanks,
 - Andrew Keller

Re: Plugin mechanism(s) for Git?

[Ben Peart]

Lars Schneider  gmail.com> writes:

> 
> Thanks for this great summary of the problem, Christian!
> 
> I think a generic plugin mechanism would be great but how would we do it?

I’m also very glad to see the discussion about coming up with a good pattern 
for 
how git can interact with external code.  I had also noticed all the in-flight 
topics as I was searching for a good pattern to adopt.

> 
> We could dynamically load libraries but this would force us to freeze 
> the ABI as mentioned by Duy:
> http://article.gmane.org/gmane.comp.version-control.git/298463
> 

I wouldn’t be too quick to dismiss dynamically loaded libraries as there are 
some distinct advantages over the other patterns especially performance and 
simplicity.  I realize it requires us to version the ABI but there are 
established patterns to manage this.  It also isn’t that much different than us 
having to freeze or version the protocol for communicating with a remote-helper.

[PATCH 2/1] Verify that --cherry-pick avoids looking at full diffs

[Johannes Schindelin]

The entire point of the previous patch was to make sure that we look at
abbreviated patch IDs (using the diff *headers* only, but avoiding
to load the blobs into memory and diff them) first, and only look at
full patch IDs when the abbreviated patch IDs were not for the
--cherry-pick test.

Let's make sure that we actually avoid looking at the full patch ID,
simply by corrupting an object that is needed for the full patch ID, and
then seeing that --cherry-pick still works.

Signed-off-by: Johannes Schindelin 
---
On Fri, 15 Jul 2016, Johannes Schindelin wrote:

> I also think that this patch series could use a test that
> verifies that we no longer generate unneeded diffs. Maybe by
> rebasing a commit on top of an commit touching a different file,
> after corrupting the blob of the latter one? Let me give it a
> try.

And here it is. I verified that this test passes with your patch
and fails without it.

 t/t6007-rev-list-cherry-pick-file.sh | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/t/t6007-rev-list-cherry-pick-file.sh 
b/t/t6007-rev-list-cherry-pick-file.sh
index 28d4f6b..a5f7c2a 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -207,4 +207,23 @@ test_expect_success '--count --left-right' '
test_cmp expect actual
 '
 
+remove_loose_object () {
+   sha1="$(git rev-parse "$1")" &&
+   remainder=${sha1#??} &&
+   firsttwo=${sha1%$remainder} &&
+   rm .git/objects/$firsttwo/$remainder
+}
+
+test_expect_success '--cherry-pick avoids looking at full diffs' '
+   git checkout -b shy-diff &&
+   test_commit dont-look-at-me &&
+   echo Hello >dont-look-at-me.t &&
+   test_tick &&
+   git commit -m tip dont-look-at-me.t &&
+   git checkout -b mainline HEAD^ &&
+   test_commit to-cherry-pick &&
+   remove_loose_object shy-diff^:dont-look-at-me.t &&
+   git rev-list --cherry-pick ...shy-diff
+'
+
 test_done
-- 
2.9.0.281.g286a8d9

--
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: Plugin mechanism(s) for Git?

[Ben Peart]

Jeff King  peff.net> writes:

> 
> On Fri, Jul 15, 2016 at 02:46:28PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> > On Fri, Jul 15, 2016 at 2:18 PM, Jeff King  peff.net> wrote:
> > > Some features, like the index-helper, aren't quite so easy. One reason
> > > is that its data needs to persist as a cache between multiple git
> > > invocations. In general, I think it would be nice to solve that by
> > > communicating via on-disk files, rather than a running daemon (just
> > > because it has fewer moving parts). But that's only half of it for
> > > index-helper. It needs to monitor inotify while git isn't running at
> > > all; so it really _does_ need some kind of long-running daemon.
> > 
> > This *may* have changed in the recent versions of the series, but I'm
> > fairly sure and for what it's worth to this discussion, that's not
> > what the index-helper does. It's there to keep the index file in
> > memory instead of reading it from disk.
> > 
> > It can *also* if you "git update-index --watchman" spawn a watchman
> > daemon in the background, which is the thing that'll be doing the
> > inotify calls and needs to stay persistent, the index-helper then
> > communicates with the watchman daemon "what changed since X?" to
> > compute a new index when requested.
> 
> Ah, yeah, you're right. Sorry for the confusion; I haven't actually
> followed the topic all that closely.
> 
> -Peff
> 


That's very close.  

Index-helper currently keeps the index alive in memory and shares it with 
git.exe via shm so it does need to persist as a daemon.  Otherwise, it will 
have 
to be run again and load the index from disk which pretty much defeats the 
purpose.  Currently, it times out after a prescribed time of not being used and 
shuts down to just to free up the resources.

The watchman daemon needs to _always_ be running so that it can monitor the 
working directory for changes and quickly provide the list of changed files and 
directories to git.  This currently happens via index-helper but could be 
separated as the logic to use the data from watchman already exists in git.  I 
have an RFC out that does exactly that.

Re: gc and repack ignore .git/*HEAD when checking reachability

[Duy Nguyen]

On Wed, Jul 13, 2016 at 8:59 PM, Johannes Schindelin
 wrote:
>> No.
>
> Oh? So you do not see that we are already heading into serious trouble
> ourselves?

I do see problems, that have solutions. But I have not yet seen that
we are heading to a dead end.

>> I prefer we have a single interface for dealing with _any_ worktree.
>
> I agree. So far, I did not see an interface, though, but the idea that we
> should somehow fake things so that there does not *have* to be an
> interface.

Calling it "fake" is a bit too strong. I'd call it an abstraction. We
have always had the one-repo/one-worktree relationship, now we're
breaking that assumption. Some operations work at repo level and
require a "repo view" while others work at worktree level. We do it in
a way that a program designed with these separate views can still work
correctly in the traditional one-worktree-one-repo configuration,
where both views are the same.

 - For accessing data in $GIT_DIR, you do not peek directly into it
any more. You use "git rev-parse --git-path" to retrieve a path in
$GIT_DIR (instead of doing `git rev-parse --git-dir`/some/path), then
you can do something about it. This is the equivalent of git_path() at
C level.

 - We have a set of rules to define what part of $GIT_DIR is shared
and what is not. When you add new stuff and follow this rule, it will
work in single or multiple worktree config. So far pretty much every
unknown thing is per-worktree. $GIT_DIR/common will be the shared
place for external scripts, soon.

 - It's the same thing for refs: we may reserve a portion of it for
per-worktree, and the rest is shared.

 - We provide means for one worktree to access data of any other
worktree if needed (e.g. $GIT_COMMON_DIR, or the new ref storage API)

 - Because the majority of operations is per-worktree, that has been
the default view so far when working in multiple worktrees. Repo-level
operations such as git-gc, rev-list --all, fsck... need to "switch
view". I missed this and this seemed to cause big problem for you. My
only excuse is, this is an experimental feature.

The idea of single config file with separate "worktree namespace" (eg.
core.worktree vs worktree..worktree) was shot down because it
would result in a lot of changes. And it's the same thing why we start
to split views this way (and make default view "per-worktree") instead
of rewriting every piece of code to deal with multiple worktrees
explicitly (by carrying the worktree id everywhere).

There are two loose ends that are on my mind, but I have not clear
ideas yet: the incorporation of ref namespace and odb alternates.
Imagine when you you submodules, all submodule repos (refs and odb)
can be stored in the top-level .git repo, separated by namespaces.
After all a repo is just a container, of a bunch of refs. This make it
much easier to peek into another submodule from a supermodule, and
makes it safer to "rm -r a-submodule" when you get mad at something.

I hope this shows that it is less of a fake thing.
-- 
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: Server-side preventing some files from being overwritten

[Thorsten Glaser]

On Fri, 15 Jul 2016, Ævar Arnfjörð Bjarmason wrote:

> I.e. it allows pushing a series which is a series of two commits which:
> 
>   1. Change the forbidden file(s)
>   2. Undo changes to the forbidden file(s)

That’s precisely allowed.

bye,
//mirabilos
-- 
tarent solutions GmbH
Rochusstraße 2-4, D-53123 Bonn • http://www.tarent.de/
Tel: +49 228 54881-393 • Fax: +49 228 54881-235
HRB 5168 (AG Bonn) • USt-ID (VAT): DE122264941
Geschäftsführer: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg
--
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] blame: Allow to blame paths freshly added to the index

[Johannes Schindelin]

Hi Mike,

On Fri, 15 Jul 2016, Mike Hommey wrote:

> diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
> index a9b266f..2812d7c 100755
> --- a/t/t8003-blame-corner-cases.sh
> +++ b/t/t8003-blame-corner-cases.sh
> @@ -41,12 +41,12 @@ test_expect_success setup '
>   test_tick &&
>   GIT_AUTHOR_NAME=Fourth git commit -m Fourth &&
>  
> - {
> - echo ABC
> - echo DEF
> - echo 
> - echo GHIJK
> - } >cow &&
> + cat >cow <<-\EOF &&
> + ABC
> + DEF
> + 
> + GHIJK
> + EOF
>   git add cow &&

Sorry, I did not realize that there was precedent for this awkward
paradigm in the same test script.

I like that you fix them, but I would prefer that to be done in a separate
patch (does not even need to be the same patch series).

Apart from that (i.e. apart from touching unrelated parts of the test
script), the patch looks good to me.

Thanks,
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 v4 2/5] t5000: test tar files that overflow ustar headers

[Johannes Schindelin]

Hi Junio,

On Thu, 14 Jul 2016, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Yeah, that is what I was trying to get at, but you stated it much more
> > clearly. LONG_IS_64BIT is good. I wonder if the "git version
> > --build-options" should be "sizeof-long", too. It's shorter, and
> > indicates our assumption that we are talking about all longs, not just
> > unsigned ones.
> 
> OK, I'll do a final reroll and then wait for Europeans ;-)

Don't let me stand in the way of progress.

I would have preferred two separate prereqs, of course, for the same
reason we use time_t and off_t: I find it better to describe what the
prereq is about, and I do not mean implementation details here.

But that does not matter all that much. We will have to address the issues
anyway, and at that point that wart will be removed and we will all go on
with our lives.

So what are your plans with 2.9.2? I ask because I do not want to engineer
a 2.9.1 release just to see that 2.9.2 is out and having to spend the same
amount of time for another release ;-)

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: Server-side preventing some files from being overwritten

[Ævar Arnfjörð Bjarmason]

On Thu, Jul 14, 2016 at 8:44 PM, Junio C Hamano  wrote:
> On Thu, Jul 14, 2016 at 11:27 AM, Junio C Hamano  wrote:
>> Thorsten Glaser  writes:
>>
>>>   if test x"0" != x"$(comm -23z \
>>>   <(git ls-tree -r -z "$old" "$subdir" | sort -z) \
>>>   <(git ls-tree -r -z "$new" "$subdir" | sort -z) | wc -c)"; then
>>>   echo >&2 'Untouchable files touched, commit rejected!'
>>>   exit 1
>>>   fi
>>
>> Can't this become simpler, e.g.
>>
>> if ! git diff-tree --quiet "$old" "$new" -- "$subdir"
>> then
>> echo >&2 "Ooh, $subdir is touched"
>> exit 1
>> fi
>
> Ehh, you need to tell diff-tree to recurse, i.e. "diff-tree -r".

Note that although this is literally what Thorsten is asking for, I
think it's worth noting for the list explicitly that all these
examples that do "diff $old..$new" will *not* prevent your repository
from having *commits* that touch those files, but they will prevent
you from having *pushes* where the end state is a net change in those
files.

I.e. it allows pushing a series which is a series of two commits which:

  1. Change the forbidden file(s)
  2. Undo changes to the forbidden file(s)

This *can* be critically important or not matter at all depending on
your use case, i.e. does it matter that disallowed and potentially
malicious changes come up in "git bisect", or will you ever be rolling
out anything but the latest tip of the branch you're testing in
production?

If the answer to either of those is "yes" you need something that does
a "git log --stat $old..$new" and parses out if *any* of the commits
make changes to those files.

See e.g. my https://github.com/avar/pre-receive-reject-binaries for
one example of that, although it rejects binaries you could easily
modify it to check the filename(s) instead.
--
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 v4 2/5] t5000: test tar files that overflow ustar headers

[Jeff King]

On Fri, Jul 15, 2016 at 03:37:32PM +0200, Torsten Bögershausen wrote:

> > So off_t is probably better. We do need to be careful, though, when
> > allocating objects. E.g., this:
> > 
> >   off_t size;
> >   struct git_istream *stream;
> >   void *buf;
> > 
> >   stream = open_istream(sha1, , , NULL);
> >   buf = xmalloc(size);
> >   while (1) {
> > /* read stream into buf */
> >   }
> > 
> > is a security hole when size_t is less than off_t (it gets truncated in
> > the call to xmalloc, which allocates too few bytes). This is a toy
> > example, obviously, but it's something to watch out for.
> > 
> That code is "illegal", it should be
>  buf = xmalloc(xsize_t(size));

Sure, I agree. The point is that it is easy to forget the extra
wrapper/check, and we should be aware of it. I don't think the compiler
will warn you (probably some static analyzers would, though).

> - Use the streaming interface to analyze if blobs are binary
>   (That is already on my list, the old "stream and early out"
>   from the olc 10/10, gmane/$293010 or so can be reused)

You might be interested in

  https://github.com/peff/git/commit/2fb07bc91f3ac6162c3dd5667d8167fc0bec6d99

I don't remember if it produced good results or not (ISTR that the cost
of setting up the streaming sometimes overwhelmed any benefit).

-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 v4 2/5] t5000: test tar files that overflow ustar headers

[Torsten Bögershausen]

On 07/15/2016 12:38 AM, Jeff King wrote:

On Thu, Jul 14, 2016 at 03:30:58PM -0700, Junio C Hamano wrote:


If we move to time_t everywhere, I think we'll need an extra
TIME_T_IS_64BIT, but we can cross that bridge when we come to it.

Likewise I think we'll need SIZE_T_IS_64BIT eventually (for real 32-bit
systems; LLP64 systems like Windows will then be able to run the test).


I guess I wrote essentially the same thing before refreshing my
Inbox.

I am a bit fuzzy between off_t and size_t; the former is for the
size of things you see on the filesystem, while the latter is for
you to give malloc(3).  I would have thought that off_t is the type
we would want at the end of the raw object header, denoting the size
of a blob object when deflated, which could be larger than the size
of a region of memory we can get from malloc(3), in which case we
would use the streaming interface.


Yeah, your understanding is right (s/deflated/inflated/). I agree that
off_t is probably a better size for blobs. Traditionally git assumed any
object could fit in memory. The streaming interface helps that somewhat,
but I think there are cases where we still must load a blob (e.g., if it
is stored as a delta). In theory that never happens because of
core.bigfilethreshold, but you may get a packfile from somebody with a
higher threshold than you.

I wouldn't be surprised if there are other cases that aren't smart
enough to use the streaming interface yet, but the solution there is to
make them smarter. :)

So off_t is probably better. We do need to be careful, though, when
allocating objects. E.g., this:

  off_t size;
  struct git_istream *stream;
  void *buf;

  stream = open_istream(sha1, , , NULL);
  buf = xmalloc(size);
  while (1) {
/* read stream into buf */
  }

is a security hole when size_t is less than off_t (it gets truncated in
the call to xmalloc, which allocates too few bytes). This is a toy
example, obviously, but it's something to watch out for.

-Peff

That code is "illegal", it should be
 buf = xmalloc(xsize_t(size));

And the transition from off_t into size_t
should always got via xsize_t():

static inline size_t xsize_t(off_t len)
{
if (len > (size_t) len)
die("Cannot handle files this big");
return (size_t)len;
}

There are some more things to be done, on the long run:
- convert "unsigned long" into either off_t of size_t in e.g. convert.c
- Use the streaming interface to analyze if blobs are binary
  (That is already on my list, the old "stream and early out"
  from the olc 10/10, gmane/$293010 or so can be reused)
--
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: Plugin mechanism(s) for Git?

[Jeff King]

On Fri, Jul 15, 2016 at 02:46:28PM +0200, Ævar Arnfjörð Bjarmason wrote:

> On Fri, Jul 15, 2016 at 2:18 PM, Jeff King  wrote:
> > Some features, like the index-helper, aren't quite so easy. One reason
> > is that its data needs to persist as a cache between multiple git
> > invocations. In general, I think it would be nice to solve that by
> > communicating via on-disk files, rather than a running daemon (just
> > because it has fewer moving parts). But that's only half of it for
> > index-helper. It needs to monitor inotify while git isn't running at
> > all; so it really _does_ need some kind of long-running daemon.
> 
> This *may* have changed in the recent versions of the series, but I'm
> fairly sure and for what it's worth to this discussion, that's not
> what the index-helper does. It's there to keep the index file in
> memory instead of reading it from disk.
> 
> It can *also* if you "git update-index --watchman" spawn a watchman
> daemon in the background, which is the thing that'll be doing the
> inotify calls and needs to stay persistent, the index-helper then
> communicates with the watchman daemon "what changed since X?" to
> compute a new index when requested.

Ah, yeah, you're right. Sorry for the confusion; I haven't actually
followed the topic all that closely.

-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] blame: Allow to blame paths freshly added to the index

[Mike Hommey]

When blaming files, changes in the work tree are taken into account
and displayed as being "Not Committed Yet".

However, when blaming a file that is not known to the current HEAD,
git blame fails with `no such path 'foo' in HEAD`, even when the file
was git add'ed.

This would seem uninteresting with the plain `git blame` case, which
it is, but it becomes useful when using copy detection, and the new file
was created from pieces already in HEAD, moved or copied from other
files.

Signed-off-by: Mike Hommey 
---
 builtin/blame.c   |  4 ++-
 t/t8003-blame-corner-cases.sh | 57 ++-
 2 files changed, 43 insertions(+), 18 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 1e214bd..0858b18 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2240,7 +2240,9 @@ static void verify_working_tree_path(struct commit 
*work_tree, const char *path)
sha1_object_info(blob_sha1, NULL) == OBJ_BLOB)
return;
}
-   die("no such path '%s' in HEAD", path);
+
+   if (cache_name_pos(path, strlen(path)) < 0)
+   die("no such path '%s' in HEAD", path);
 }
 
 static struct commit_list **append_parent(struct commit_list **tail, const 
unsigned char *sha1)
diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
index a9b266f..2812d7c 100755
--- a/t/t8003-blame-corner-cases.sh
+++ b/t/t8003-blame-corner-cases.sh
@@ -41,12 +41,12 @@ test_expect_success setup '
test_tick &&
GIT_AUTHOR_NAME=Fourth git commit -m Fourth &&
 
-   {
-   echo ABC
-   echo DEF
-   echo 
-   echo GHIJK
-   } >cow &&
+   cat >cow <<-\EOF &&
+   ABC
+   DEF
+   
+   GHIJK
+   EOF
git add cow &&
test_tick &&
GIT_AUTHOR_NAME=Fifth git commit -m Fifth
@@ -115,11 +115,11 @@ test_expect_success 'append with -C -C -C' '
 test_expect_success 'blame wholesale copy' '
 
git blame -f -C -C1 HEAD^ -- cow | sed -e "$pick_fc" >current &&
-   {
-   echo mouse-Initial
-   echo mouse-Second
-   echo mouse-Third
-   } >expected &&
+   cat >expected <<-\EOF &&
+   mouse-Initial
+   mouse-Second
+   mouse-Third
+   EOF
test_cmp expected current
 
 '
@@ -127,12 +127,35 @@ test_expect_success 'blame wholesale copy' '
 test_expect_success 'blame wholesale copy and more' '
 
git blame -f -C -C1 HEAD -- cow | sed -e "$pick_fc" >current &&
-   {
-   echo mouse-Initial
-   echo mouse-Second
-   echo cow-Fifth
-   echo mouse-Third
-   } >expected &&
+   cat >expected <<-\EOF &&
+   mouse-Initial
+   mouse-Second
+   cow-Fifth
+   mouse-Third
+   EOF
+   test_cmp expected current
+
+'
+
+test_expect_success 'blame wholesale copy and more in the index' '
+
+   cat >horse <<-\EOF &&
+   ABC
+   DEF
+   
+   
+   GHIJK
+   EOF
+   git add horse &&
+   test_when_finished "git rm -f horse" &&
+   git blame -f -C -C1 -- horse | sed -e "$pick_fc" >current &&
+   cat >expected <<-\EOF &&
+   mouse-Initial
+   mouse-Second
+   cow-Fifth
+   horse-Not
+   mouse-Third
+   EOF
test_cmp expected current
 
 '
-- 
2.9.1.276.geea30e8

--
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: Plugin mechanism(s) for Git?

[Ævar Arnfjörð Bjarmason]

On Fri, Jul 15, 2016 at 2:18 PM, Jeff King  wrote:
> Some features, like the index-helper, aren't quite so easy. One reason
> is that its data needs to persist as a cache between multiple git
> invocations. In general, I think it would be nice to solve that by
> communicating via on-disk files, rather than a running daemon (just
> because it has fewer moving parts). But that's only half of it for
> index-helper. It needs to monitor inotify while git isn't running at
> all; so it really _does_ need some kind of long-running daemon.

This *may* have changed in the recent versions of the series, but I'm
fairly sure and for what it's worth to this discussion, that's not
what the index-helper does. It's there to keep the index file in
memory instead of reading it from disk.

It can *also* if you "git update-index --watchman" spawn a watchman
daemon in the background, which is the thing that'll be doing the
inotify calls and needs to stay persistent, the index-helper then
communicates with the watchman daemon "what changed since X?" to
compute a new index when requested.

I.e. if you:

$ git config indexhelper.autorun true
$ git update-index --watchman

You'll end up with both:

$ ps ax|grep -e index-helper -e watchman|grep -v grep
61958 ?Ss 0:00 git-index-helper --detach --autorun
62813 ?Sl 0:04 watchman get-sockname

So for the purposes of your example the index-helper is one of those
things that doesn't "need" to persist as a demon, only the third party
watchman daemon does, but the index-helper speeds up reading the index
since it keeps it alive in memory.

All or some of the above may be wrong, Nguyen/David, any comments?
--
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] blame: Allow to blame paths freshly added to the index

[Mike Hommey]

On Fri, Jul 15, 2016 at 08:37:59AM -0400, Jeff King wrote:
> On Fri, Jul 15, 2016 at 09:32:45PM +0900, Mike Hommey wrote:
> 
> > > > +test_expect_success 'blame wholesale copy and more in the index' '
> > > > +
> > > > +   {
> > > > +   echo ABC
> > > > +   echo DEF
> > > > +   echo 
> > > > +   echo 
> > > > +   echo GHIJK
> > > > +   } >horse &&
> > > 
> > > A more common way to do this in our test scripts is by using here
> > > documents. However, in this case I would suggest
> > > 
> > >   test_write_lines ABC DEF   GHIJK >horse
> > 
> > I merely copied the pattern used in other places in the same test file.
> > Using test_write_lines or something else (what are "here documents"?)
> > would break consistency. I can also change the other similar blocks at
> > the same time, though, whichever you prefer.
> 
> A here document is this:
> 
>cat <<-\EOF
>ABC
>DEF
>
>
>GHIJK
>EOF
> 
> The "<<" starts the here-doc. The "-" tells the shell to strip leading
> tabs (so you can keep it indented with the rest of the code. The "\"
> tells the shell not to interpolate (not a big deal here, but great for
> more complicated input). The "EOF" tells it where to stop.

Oh, so that's what they are called! I've used them for 20 years
without knowing :) TIL.

Mike
--
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] blame: Allow to blame paths freshly added to the index

[Jeff King]

On Fri, Jul 15, 2016 at 09:32:45PM +0900, Mike Hommey wrote:

> > > +test_expect_success 'blame wholesale copy and more in the index' '
> > > +
> > > + {
> > > + echo ABC
> > > + echo DEF
> > > + echo 
> > > + echo 
> > > + echo GHIJK
> > > + } >horse &&
> > 
> > A more common way to do this in our test scripts is by using here
> > documents. However, in this case I would suggest
> > 
> > test_write_lines ABC DEF   GHIJK >horse
> 
> I merely copied the pattern used in other places in the same test file.
> Using test_write_lines or something else (what are "here documents"?)
> would break consistency. I can also change the other similar blocks at
> the same time, though, whichever you prefer.

A here document is this:

   cat <<-\EOF
   ABC
   DEF
   
   
   GHIJK
   EOF

The "<<" starts the here-doc. The "-" tells the shell to strip leading
tabs (so you can keep it indented with the rest of the code. The "\"
tells the shell not to interpolate (not a big deal here, but great for
more complicated input). The "EOF" tells it where to stop.

Matching surrounding style is always reasonable, though I do think this
particular file is a bit of an oddball. Most of our scripts use here
documents. Either is OK in this case, IMHO.

Personally I do not find test_write_lines particularly readable, but I
guess some people do, which is why it exists.

-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] blame: Allow to blame paths freshly added to the index

[Mike Hommey]

On Fri, Jul 15, 2016 at 12:45:15PM +0200, Johannes Schindelin wrote:
> Hi Mike,
> 
> On Fri, 15 Jul 2016, Mike Hommey wrote:
> 
> > When blaming files, changes in the work tree are taken into account
> > and displayed as being "Not Committed Yet".
> > 
> > However, when blaming a file that is not known to the current HEAD,
> > git blame fails with `no such path 'foo' in HEAD`, even when the file
> > was git add'ed.
> > 
> > This would seem uninteresting with the plain `git blame` case, which
> > it is, but it becomes useful when using copy detection, and the new file
> > was created from pieces already in HEAD, moved or copied from other
> > files.
> > ---
> 
> Well explained.
> 
> Please add your sign-off.

Facepalm, forgot to sign-off again.

> >  static struct commit_list **append_parent(struct commit_list **tail, const 
> > unsigned char *sha1)
> > diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
> > index a9b266f..a0a09e2 100755
> > --- a/t/t8003-blame-corner-cases.sh
> > +++ b/t/t8003-blame-corner-cases.sh
> > @@ -137,6 +137,29 @@ test_expect_success 'blame wholesale copy and more' '
> >  
> >  '
> >  
> > +test_expect_success 'blame wholesale copy and more in the index' '
> > +
> > +   {
> > +   echo ABC
> > +   echo DEF
> > +   echo 
> > +   echo 
> > +   echo GHIJK
> > +   } >horse &&
> 
> A more common way to do this in our test scripts is by using here
> documents. However, in this case I would suggest
> 
>   test_write_lines ABC DEF   GHIJK >horse

I merely copied the pattern used in other places in the same test file.
Using test_write_lines or something else (what are "here documents"?)
would break consistency. I can also change the other similar blocks at
the same time, though, whichever you prefer.

> instead. The equivalent applies to the 'expected' file below:
> 
> > +   git add horse &&
> > +   git blame -f -C -C1 -- horse | sed -e "$pick_fc" >current &&
> > +   {
> > +   echo mouse-Initial
> > +   echo mouse-Second
> > +   echo cow-Fifth
> > +   echo horse-Not
> > +   echo mouse-Third
> > +   } >expected &&
> > +   test_cmp expected current &&
> > +   git rm -f horse
> 
> Should this not be a
> 
>   test_when_finished "git rm -f horse"
> 
> at the beginning?

Indeed.

Thanks

Mike
--
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: Plugin mechanism(s) for Git?

[Jeff King]

On Fri, Jul 15, 2016 at 08:46:03AM +0200, Christian Couder wrote:

> One way to extend it for better performance is to require that the
> configured command should be able to deal with a number or a stream of
> files or objects (instead of just one objec/file) that are passed to
> it. It looks like that is what Lars wants for smudge/clean filters.
> 
> Another way is to have the external command run as a daemon, like what
> Duy and David implemented for the index-helper.

Where possible, I think we should avoid daemons. They introduce all
sorts of timings complexities and robustness issues (what happens when
the daemon isn't up? What happens when it hangs? Etc).

Junio mentioned elsewhere the way remote-helpers work, which is to have
a single program that is run once per git invocation, and that can serve
multiple requests interactively by speaking a specific protocol. I think
that's what you're getting at in the first paragraph I've quoted here,
and it's something that has worked reasonably well for us. I _do_ think
we've often not paid close attention to the protocol design, and it has
ended up biting us (there are some serious warts in the remote-helper
protocol, for instance).

I don't know if we would want to go so far as standardizing on something
like JSON for making RPC requests to any helpers. Probably the more
"git" thing would be to use something based around pkt-lines, but it's
a lot easier to find a JSON library for your helper program. :-/

For clean/smudge filters, that kind of model seems like it would work
well. Better still if it can actually accept requests asynchronously and
return them possibly out of order (so it can parallelize as it likes
under the hood).  I think that the external-odb stuff could run this way
pretty easily, too.

Though I'm not yet convinced that it wouldn't be sufficient to run each
request in its own program, but teach git to parallelize the invocations
and let multiple run at once. The problem often times is one of latency
in hitting the helper serially, not overall CPU time (and you'd need to
do this parallelizing anyway to make out-of-order requests of a single
program, so it seems like a useful first step anyway).


Some features, like the index-helper, aren't quite so easy. One reason
is that its data needs to persist as a cache between multiple git
invocations. In general, I think it would be nice to solve that by
communicating via on-disk files, rather than a running daemon (just
because it has fewer moving parts). But that's only half of it for
index-helper. It needs to monitor inotify while git isn't running at
all; so it really _does_ need some kind of long-running daemon.

> And a more integrated way is to require the external code to implement
> an API and to be compiled along with Git which looks like the approach
> taken by the ref backend work.

The nice thing about an API like this is that it can be very high
performance, and it's relatively easy to move data between the API and
the rest of Git. But I still don't think we've quite figured out how
backends are going to be compiled and linked into git. I'm not sure
anybody is really shooting for something like run-time loading of
modules. I think at this stage we're more likely to have a handful of
modules that are baked in at compile time.

That works OK for the refs code, which is mostly Git-related, and mostly
works synchronously; you ask it for a ref, it looks it up and returns
it. Something like Git-LFS seems much more complicated. Besides being
written in Go and having a bunch of extra library dependencies, it's
inherently network-oriented, and needs to handle being responsive on
multiple descriptors (especially if we try to do things in parallel).
That's a lot of complication to stuff into an API. It also has to make
policy decisions that shouldn't necessarily be part of git (like
managing the cache of objects).

> If people think that evolution is better than intelligent design, and
> want each current topic/work to just implement what is best for it,
> then that's ok for me. If on the other hand standardizing on some ways
> to interact with external processes could be helpful to avoid
> duplicating mechanisms/code in slightly different and incompatible
> ways, then I would be happy to discuss it in a thread that is not
> specific to one of the current work.

Those are all just my off-the-cuff thoughts. I reserve the right to
change my opinions above at any time. :)

I _do_ think each of the projects you've mentioned has their own needs,
so I don't think we'll find a one-size-fits-all solution.

-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: Multiple Keys in ssh-agent, fail to clone

[Benjamin Fritsch]

> On Jul 13, 2016, at 3:32 PM, Junio C Hamano  wrote:
> 
> Benjamin Fritsch  writes:
> 
>> I read the Changelog for 2.9 and couldn’t find any reference to changed key 
>> handling. Is there anything that I can add to the `git clone` command to get 
>> the old behavior?
> 
> I do not think this has much to do with the version of Git, unless
> you are getting an updated SSH client together with your new version
> of Git from whoever distributes it.

Thank you for the great feedback. I could confirm that this is a problem on 
Bitbucket’s end. Sorry for the confusion.
I managed to successfully clone with Git 2.9 and Git 2.8

Best,
ben--
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] Use path comparison for patch ids before the file content

[Johannes Schindelin]

Hi Kevin,

congratulations for your first patch on the Git mailing list!

On Thu, 14 Jul 2016, Kevin Willford wrote:

> When limiting the list in a revision walk using cherry pick, patch ids are
> calculated by producing the diff of the content of the files.  This would
> be more efficent by using a patch id looking at the paths that were
> changed in the commits and only if all the file changed are the same fall
> back to getting the content of the files in the commits to determine if
> the commits are the same.
> 
> This change uses a hashmap to store entries with a hash of the patch id
> calculated by just using the file paths.  The entries constist of the
> commit and the patch id calculated using file contents which is initially
> empty.  If there are commits that are found in the hashmap it means that
> the same files were changed in the commits and the file contents need to
> be checked in order to determine if the commits are truly the same.  The
> patch id that is calcuated based on the file contents is then stored in the
> hashmap entry if needed in later comparisons.  If the commits are determined 
> to be
> the same the cherry_flag is set on the commit being checked as well as the
> commit in the hashmap entry saving running though the original list of
> commits checking a seen flag.  This will speed up a rebase where the
> upstream has many changes but none of them have been pulled into the
> current branch.
> ---

How about pulling out the change that replaces patch-id's ad-hoc hashmap
with the struct hashmap solution?

It would become a two-part patch series, then.

BTW I tried my hand at a different commit message, maybe you want to
cherry-pick parts of it?

-- snip --
rebase: avoid computing unnecessary patch IDs

The `rebase` family of Git commands avoid applying patches that were
already integrated upstream. They do that by using the revision walking
option that computes the patch IDs of the two sides of the rebase
(local-only patches vs upstream-only ones) and skipping those local
patches whose patch ID matches one of the upstream ones.

In many cases, this causes unnecessary churn, as already the set of
paths touched by a given commit would suffice to determine that an
upstream patch has no local equivalent.

This hurts performance in particular when there are a lot of upstream
patches, and/or large ones.

Therefore, let's introduce the concept of a "header-only" patch ID,
compare those first, and only evaluate the "full" patch ID lazily.

Please note that in contrast to the "full" patch IDs, those "header-only"
patch IDs are prone to collide with one another, as adjacent commits
frequently touch the very same files. Hence we now have to be careful to
allow multiple hash entries with the same hash.
-- snap --

And as Junio pointed out, please add your Signed-off-by: lines (see
https://github.com/git/git/blob/v2.9.1/Documentation/SubmittingPatches#L239-L291
for an explanation).

And here are a couple of comments on the code (please read all the way to
the end, I cut the parts that I do not address):

> diff --git a/diff.c b/diff.c
> index fa78fc1..f38b663 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4449,7 +4449,7 @@ static void patch_id_consume(void *priv, char *line, 
> unsigned long len)
>  }
>  
>  /* returns 0 upon success, and writes result into sha1 */
> -static int diff_get_patch_id(struct diff_options *options, unsigned char 
> *sha1)
> +static int diff_get_patch_id(struct diff_options *options, unsigned char 
> *sha1, int use_path_only)

If we used `diff_header_only`, we could address Junio's concern about the
naming of this parameter.

> diff --git a/patch-ids.c b/patch-ids.c
> index a4d0016..f0262ce 100644
> --- a/patch-ids.c
> +++ b/patch-ids.c
> @@ -4,8 +4,9 @@
>  #include "sha1-lookup.h"
>  #include "patch-ids.h"
>  
> -int commit_patch_id(struct commit *commit, struct diff_options *options,
> - unsigned char *sha1)
> +
> +static int ll_commit_patch_id(struct commit *commit, struct diff_options 
> *options,

How about simply changing the signature of the commit_patch_id() function?
It's not like Git guarantees any kind of stable API of its libgit.a or
something.

> +int commit_patch_id(struct commit *commit, struct diff_options *options,
> + unsigned char *sha1)
>  {
> - return sha1_pos(id, table, nr, patch_id_access);
> + return ll_commit_patch_id(commit, options, sha1, 0);
>  }
>  
> -#define BUCKET_SIZE 190 /* 190 * 21 = 3990, with slop close enough to 4K */
> -struct patch_id_bucket {
> - struct patch_id_bucket *next;
> - int nr;
> - struct patch_id bucket[BUCKET_SIZE];
> -};

The idea of the original code was to get as close to 4kB for the initial
(and probably final) hashmap. I do not think we can, or have to, achieve
the same with struct hashmap. But we should use a larger initial size than
64 (maybe 128? I would actually go for 256, even if that roughly doubles
the initial hashmap size) by passing an explicit 

Re: [PATCH] git-clean: remove fifo, devices, socket files

[Johannes Schindelin]

Hi Andrey,

On Fri, 15 Jul 2016, Andrey Vagin wrote:

> What will be wrong if 'git status' will reports these [fifo/socket] files?

`git status` is intended to give you an idea what to commit next. And...

> What will be wrong if 'git add' will returns an error instead of
> skipping them silently?

... we *cannot* commit fifos or sockets. There is simply no representation
in Git's data model for them.

Having said that, I would welcome a patch that makes `git add` complain
about arguments that could not be added (and are not directories).

As to the patch in question, for the above-mentioned reasons, I think we
want to keep the existing behavior instead.

Ciao,
Johannes
--
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] blame: Allow to blame paths freshly added to the index

[Johannes Schindelin]

Hi Mike,

On Fri, 15 Jul 2016, Mike Hommey wrote:

> When blaming files, changes in the work tree are taken into account
> and displayed as being "Not Committed Yet".
> 
> However, when blaming a file that is not known to the current HEAD,
> git blame fails with `no such path 'foo' in HEAD`, even when the file
> was git add'ed.
> 
> This would seem uninteresting with the plain `git blame` case, which
> it is, but it becomes useful when using copy detection, and the new file
> was created from pieces already in HEAD, moved or copied from other
> files.
> ---

Well explained.

Please add your sign-off.

>  static struct commit_list **append_parent(struct commit_list **tail, const 
> unsigned char *sha1)
> diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
> index a9b266f..a0a09e2 100755
> --- a/t/t8003-blame-corner-cases.sh
> +++ b/t/t8003-blame-corner-cases.sh
> @@ -137,6 +137,29 @@ test_expect_success 'blame wholesale copy and more' '
>  
>  '
>  
> +test_expect_success 'blame wholesale copy and more in the index' '
> +
> + {
> + echo ABC
> + echo DEF
> + echo 
> + echo 
> + echo GHIJK
> + } >horse &&

A more common way to do this in our test scripts is by using here
documents. However, in this case I would suggest

test_write_lines ABC DEF   GHIJK >horse

instead. The equivalent applies to the 'expected' file below:

> + git add horse &&
> + git blame -f -C -C1 -- horse | sed -e "$pick_fc" >current &&
> + {
> + echo mouse-Initial
> + echo mouse-Second
> + echo cow-Fifth
> + echo horse-Not
> + echo mouse-Third
> + } >expected &&
> + test_cmp expected current &&
> + git rm -f horse

Should this not be a

test_when_finished "git rm -f horse"

at the beginning?

Otherwise it looks really good to me.

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

[PATCH 12/12] receive-pack: send keepalives during quiet periods

[Jeff King]

After a client has sent us the complete pack, we may spend
some time processing the data and running hooks. If the
client asked us to be quiet, receive-pack won't send any
progress data during the index-pack or connectivity-check
steps. And hooks may or may not produce their own progress
output. In these cases, the network connection is totally
silent from both ends.

Git itself doesn't care about this (it will wait forever),
but other parts of the system (e.g., firewalls,
load-balancers, etc) might hang up the connection. So we'd
like to send some sort of keepalive to let the network and
the client side know that we're still alive and processing.

We can use the same trick we did in 05e9515 (upload-pack:
send keepalive packets during pack computation, 2013-09-08).
Namely, we will send an empty sideband data packet every `N`
seconds that we do not relay any stderr data over the
sideband channel. As with 05e9515, this means that we won't
bother sending keepalives when there's actual progress data,
but will kick in when it has been disabled (or if there is a
lull in the progress data).

The concept is simple, but the details are subtle enough
that they need discussing here.

Before the client sends us the pack, we don't want to do any
keepalives. We'll have sent our ref advertisement, and we're
waiting for them to send us the pack (and tell us that they
support sidebands at all).

While we're receiving the pack from the client (or waiting
for it to start), there's no need for keepalives; it's up to
them to keep the connection active by sending data.
Moreover, it would be wrong for us to do so. When we are the
server in the smart-http protocol, we must treat our
connection as half-duplex. So any keepalives we send while
receiving the pack would potentially be buffered by the
webserver. Not only does this make them useless (since they
would not be delivered in a timely manner), but it could
actually cause a deadlock if we fill up the buffer with
keepalives. (It wouldn't be wrong to send keepalives in this
phase for a full-duplex connection like ssh; it's simply
pointless, as it is the client's responsibility to speak).

As soon as we've gotten all of the pack data, then the
client is waiting for us to speak, and we should start
keepalives immediately. From here until the end of the
connection, we send one any time we are not otherwise
sending data.

But there's a catch. Receive-pack doesn't know the moment
we've gotten all the data. It passes the descriptor to
index-pack, who reads all of the data, and then starts
resolving the deltas. We have to communicate that back.

To make this work, we instruct the sideband muxer to enable
keepalives in three phases:

  1. In the beginning, not at all.

  2. While reading from index-pack, wait for a signal
 indicating end-of-input, and then start them.

  3. Afterwards, always.

The signal from index-pack in phase 2 has to come over the
stderr channel which the muxer is reading. We can't use an
extra pipe because the portable run-command interface only
gives us stderr and stdout.

Stdout is already used to pass the .keep filename back to
receive-pack. We could also send a signal there, but then we
would find out about it in the main thread. And the
keepalive needs to be done by the async muxer thread (since
it's the one writing sideband data back to the client). And
we can't reliably signal the async thread from the main
thread, because the async code sometimes uses threads and
sometimes uses forked processes.

Therefore the signal must come over the stderr channel,
where it may be interspersed with other random
human-readable messages from index-pack. This patch makes
the signal a single NUL byte.  This is easy to parse, should
not appear in any normal stderr output, and we don't have to
worry about any timing issues (like seeing half the signal
bytes in one read(), and half in a subsequent one).

This is a bit ugly, but it's simple to code and should work
reliably.

Another option would be to stop using an async thread for
muxing entirely, and just poll() both stderr and stdout of
index-pack from the main thread. This would work for
index-pack (because we aren't doing anything useful in the
main thread while it runs anyway). But it would make the
connectivity check and the hook muxers much more
complicated, as they need to simultaneously feed the
sub-programs while reading their stderr.

The index-pack phase is the only one that needs this
signaling, so it could simply behave differently than the
other two. That would mean having two separate
implementations of copy_to_sideband (and the keepalive
code), though. And it still doesn't get rid of the
signaling; it just means we can write a nicer message like
"END_OF_INPUT" or something on stdout, since we don't have
to worry about separating it from the stderr cruft.

One final note: this signaling trick is only done with
index-pack, not with unpack-objects. There's no point in
doing it for the latter, because by 

[PATCH 10/12] receive-pack: relay connectivity errors to sideband

[Jeff King]

If the connectivity check encounters a problem when
receiving a push, the error output goes to receive-pack's
stderr, whose destination depends on the protocol used
(ssh tends to send it to the user, though without a "remote"
prefix; http will generally eat it in the server's error
log).

The information should consistently go back to the user, as
there is a reasonable chance their client is buggy and
generating a bad pack.

We can do so by muxing it over the sideband as we do with
other sub-process stderr.

Signed-off-by: Jeff King 
---
 builtin/receive-pack.c | 18 +-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index de322bc..d309109 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1317,9 +1317,12 @@ static void execute_commands(struct command *commands,
 const char *unpacker_error,
 struct shallow_info *si)
 {
+   struct check_connected_options opt = CHECK_CONNECTED_INIT;
struct command *cmd;
unsigned char sha1[20];
struct iterate_data data;
+   struct async muxer;
+   int err_fd = 0;
 
if (unpacker_error) {
for (cmd = commands; cmd; cmd = cmd->next)
@@ -1327,11 +1330,24 @@ static void execute_commands(struct command *commands,
return;
}
 
+   if (use_sideband) {
+   memset(, 0, sizeof(muxer));
+   muxer.proc = copy_to_sideband;
+   muxer.in = -1;
+   if (!start_async())
+   err_fd = muxer.in;
+   /* ...else, continue without relaying sideband */
+   }
+
data.cmds = commands;
data.si = si;
-   if (check_connected(iterate_receive_command_list, , NULL))
+   opt.err_fd = err_fd;
+   if (check_connected(iterate_receive_command_list, , ))
set_connectivity_errors(commands, si);
 
+   if (use_sideband)
+   finish_async();
+
reject_updates_to_hidden(commands);
 
if (run_receive_hook(commands, "pre-receive", 0)) {
-- 
2.9.1.434.g748be50

--
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 11/12] receive-pack: turn on connectivity progress

[Jeff King]

When we receive a large push, the server side of the
connection may spend a lot of time (30s or more for a full
push of linux.git) walking the object graph without
producing any output. Let's give the user some indication
that we're actually working.

Signed-off-by: Jeff King 
---
 builtin/receive-pack.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index d309109..7db1639 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1342,6 +1342,7 @@ static void execute_commands(struct command *commands,
data.cmds = commands;
data.si = si;
opt.err_fd = err_fd;
+   opt.progress = err_fd && !quiet;
if (check_connected(iterate_receive_command_list, , ))
set_connectivity_errors(commands, si);
 
-- 
2.9.1.434.g748be50

--
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 09/12] receive-pack: turn on index-pack resolving progress

[Jeff King]

When we receive a large push, the server side may have to
spend a lot of CPU processing the incoming packfile.

During the "receiving" phase, we are typically network
bound, and the client is writing its own progress to the
user. But during the delta resolution phase, we may spend
minutes (e.g., for a full push of linux.git) without
making any indication to the user that the connection has
not hung.

Let's ask index-pack to produce progress output for this
phase (unless the client asked us to be quiet, of course).

Signed-off-by: Jeff King 
---
 builtin/receive-pack.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index ce81920..de322bc 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1547,6 +1547,8 @@ static const char *unpack(int err_fd, struct shallow_info 
*si)
 (uintmax_t)getpid(),
 hostname);
 
+   if (!quiet && err_fd)
+   argv_array_push(, 
"--show-resolving-progress");
if (fsck_objects)
argv_array_pushf(, "--strict%s",
fsck_msg_types.buf);
-- 
2.9.1.434.g748be50

--
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 08/12] index-pack: add flag for showing delta-resolution progress

[Jeff King]

The index-pack command has two progress meters: one for
"receiving objects", and one for "resolving deltas". You get
neither by default, or both with "-v".

But for a push through receive-pack, we would want only the
"resolving deltas" phase, _not_ the "receiving objects"
progress. There are two reasons for this.

One is simply that existing clients are already printing
"writing objects" progress at the same time.  Arguably
"receiving" from the far end is more useful, because it
tells you what has actually gotten there, as opposed to what
might be stuck in a buffer somewhere between the client and
server. But that would require a protocol extension to tell
clients not to print their progress. Possible, but
complexity for little gain.

The second reason is much more important. In a full-duplex
connection like git-over-ssh, we can print progress while
the pack is incoming, and it will immediately get to the
client. But for a half-duplex connection like git-over-http,
we should not say anything until we have received the full
request.  Anything we write is subject to being stuck in a
buffer by the webserver.  Worse, we can end up in a deadlock
if that buffer fills up.

So our best bet is to avoid writing anything that isn't a
small fixed size until we've received the full pack.

Signed-off-by: Jeff King 
---
 builtin/index-pack.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index e8c71fc..1cba120 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -77,6 +77,7 @@ static int strict;
 static int do_fsck_object;
 static struct fsck_options fsck_options = FSCK_OPTIONS_STRICT;
 static int verbose;
+static int show_resolving_progress;
 static int show_stat;
 static int check_self_contained_and_connected;
 
@@ -1190,7 +1191,7 @@ static void resolve_deltas(void)
qsort(ref_deltas, nr_ref_deltas, sizeof(struct ref_delta_entry),
  compare_ref_delta_entry);
 
-   if (verbose)
+   if (verbose || show_resolving_progress)
progress = start_progress(_("Resolving deltas"),
  nr_ref_deltas + nr_ofs_deltas);
 
@@ -1694,6 +1695,8 @@ int cmd_index_pack(int argc, const char **argv, const 
char *prefix)
input_len = sizeof(*hdr);
} else if (!strcmp(arg, "-v")) {
verbose = 1;
+   } else if (!strcmp(arg, "--show-resolving-progress")) {
+   show_resolving_progress = 1;
} else if (!strcmp(arg, "-o")) {
if (index_name || (i+1) >= argc)
usage(index_pack_usage);
-- 
2.9.1.434.g748be50

--
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 07/12] clone: use a real progress meter for connectivity check

[Jeff King]

Because the initial connectivity check for a cloned
repository can be slow, 0781aa4 (clone: let the user know
when check_everything_connected is run, 2013-05-03) added a
"fake" progress meter; we simply say "Checking connectivity"
when it starts, and "done" at the end, with nothing between.

Since check_connected() now knows how to do a real progress
meter, we can drop our fake one and use that one instead.

Signed-off-by: Jeff King 
---
Obviously not related to the receive-pack bits, but we get this for
free because of the earlier refactoring.

 builtin/clone.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 32fe606..f044a8c 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -627,13 +627,10 @@ static void update_remote_refs(const struct ref *refs,
struct check_connected_options opt = CHECK_CONNECTED_INIT;
 
opt.transport = transport;
+   opt.progress = transport->progress;
 
-   if (transport->progress)
-   fprintf(stderr, _("Checking connectivity... "));
if (check_connected(iterate_ref_map, , ))
die(_("remote did not send all necessary objects"));
-   if (transport->progress)
-   fprintf(stderr, _("done.\n"));
}
 
if (refs) {
-- 
2.9.1.434.g748be50

--
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 05/12] check_connected: relay errors to alternate descriptor

[Jeff King]

Unless the "quiet" flag is given, check_connected sends any
errors to the stderr of the caller (because the child
rev-list inherits that descriptor). However, server-side
callers may want to send these over a sideband channel
instead.  Let's make that possible.

Signed-off-by: Jeff King 
---
 connected.c | 11 +--
 connected.h |  7 +++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/connected.c b/connected.c
index 2a51eac..5f5c8bd 100644
--- a/connected.c
+++ b/connected.c
@@ -31,8 +31,11 @@ int check_connected(sha1_iterate_fn fn, void *cb_data,
opt = 
transport = opt->transport;
 
-   if (fn(cb_data, sha1))
+   if (fn(cb_data, sha1)) {
+   if (opt->err_fd)
+   close(opt->err_fd);
return err;
+   }
 
if (transport && transport->smart_options &&
transport->smart_options->self_contained_and_connected &&
@@ -59,7 +62,11 @@ int check_connected(sha1_iterate_fn fn, void *cb_data,
rev_list.git_cmd = 1;
rev_list.in = -1;
rev_list.no_stdout = 1;
-   rev_list.no_stderr = opt->quiet;
+   if (opt->err_fd)
+   rev_list.err = opt->err_fd;
+   else
+   rev_list.no_stderr = opt->quiet;
+
if (start_command(_list))
return error(_("Could not run 'git rev-list'"));
 
diff --git a/connected.h b/connected.h
index 12594ef..5d88e26 100644
--- a/connected.h
+++ b/connected.h
@@ -23,6 +23,13 @@ struct check_connected_options {
 
/* Transport whose objects we are checking, if available. */
struct transport *transport;
+
+   /*
+* If non-zero, send error messages to this descriptor rather
+* than stderr. The descriptor is closed before check_connected
+* returns.
+*/
+   int err_fd;
 };
 
 #define CHECK_CONNECTED_INIT { 0 }
-- 
2.9.1.434.g748be50

--
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 06/12] check_connected: add progress flag

[Jeff King]

Connectivity checks have to traverse the entire object graph
in the worst case (e.g., a full clone or a full push). For
large repositories like linux.git, this can take 30-60
seconds, during which time git may produce little or no
output.

Let's add the option of showing progress, which is taken
care of by rev-list.

Signed-off-by: Jeff King 
---
 connected.c | 3 +++
 connected.h | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/connected.c b/connected.c
index 5f5c8bd..8e3e4b1 100644
--- a/connected.c
+++ b/connected.c
@@ -58,6 +58,9 @@ int check_connected(sha1_iterate_fn fn, void *cb_data,
argv_array_push(_list.args, "--not");
argv_array_push(_list.args, "--all");
argv_array_push(_list.args, "--quiet");
+   if (opt->progress)
+   argv_array_pushf(_list.args, "--progress=%s",
+_("Checking connectivity"));
 
rev_list.git_cmd = 1;
rev_list.in = -1;
diff --git a/connected.h b/connected.h
index 5d88e26..afa48cc 100644
--- a/connected.h
+++ b/connected.h
@@ -30,6 +30,9 @@ struct check_connected_options {
 * returns.
 */
int err_fd;
+
+   /* If non-zero, show progress as we traverse the objects. */
+   int progress;
 };
 
 #define CHECK_CONNECTED_INIT { 0 }
-- 
2.9.1.434.g748be50

--
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 04/12] check_everything_connected: use a struct with named options

[Jeff King]

The number of variants of check_everything_connected has
grown over the years, so that the "real" function takes
several possibly-zero, possibly-NULL arguments. We hid the
complexity behind some wrapper functions, but this doesn't
scale well when we want to add new options.

If we add more wrapper variants to handle the new options,
then we can get a combinatorial explosion when those options
might be used together (right now nobody wants to use both
"shallow" and "transport" together, so we get by with just a
few wrappers).

If instead we add new parameters to each function, each of
which can have a default value, then callers who want the
defaults end up with confusing invocations like:

  check_everything_connected(fn, 0, data, -1, 0, NULL);

where it is unclear which parameter is which (and every
caller needs updated when we add new options).

Instead, let's add a struct to hold all of the optional
parameters. This is a little more verbose for the callers
(who have to declare the struct and fill it in), but it
makes their code much easier to follow, because every option
is named as it is set (and unused options do not have to be
mentioned at all).

Note that we could also stick the iteration function and its
callback data into the option struct, too. But since those
are required for each call, by avoiding doing so, we can let
very simple callers just pass "NULL" for the options and not
worry about the struct at all.

While we're touching each site, let's also rename the
function to check_connected(). The existing name was quite
long, and not all of the wrappers even used the full name.

Signed-off-by: Jeff King 
---
The diffstat claims +2 lines, but 3 of those are documentation that
should have existed before but didn't. :)

But the real gain comes in the later patches. They were pretty nasty
before I went back and did this cleanup.

 builtin/clone.c|  7 +--
 builtin/fetch.c|  6 --
 builtin/receive-pack.c | 13 ++---
 connected.c| 39 +++
 connected.h| 27 +--
 5 files changed, 47 insertions(+), 45 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 31ea247..32fe606 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -624,10 +624,13 @@ static void update_remote_refs(const struct ref *refs,
const struct ref *rm = mapped_refs;
 
if (check_connectivity) {
+   struct check_connected_options opt = CHECK_CONNECTED_INIT;
+
+   opt.transport = transport;
+
if (transport->progress)
fprintf(stderr, _("Checking connectivity... "));
-   if (check_everything_connected_with_transport(iterate_ref_map,
- 0, , 
transport))
+   if (check_connected(iterate_ref_map, , ))
die(_("remote did not send all necessary objects"));
if (transport->progress)
fprintf(stderr, _("done.\n"));
diff --git a/builtin/fetch.c b/builtin/fetch.c
index f896aa1..3bf895f 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -615,7 +615,7 @@ static int store_updated_refs(const char *raw_url, const 
char *remote_name,
url = xstrdup("foreign");
 
rm = ref_map;
-   if (check_everything_connected(iterate_ref_map, 0, )) {
+   if (check_connected(iterate_ref_map, , NULL)) {
rc = error(_("%s did not send all necessary objects\n"), url);
goto abort;
}
@@ -751,6 +751,7 @@ static int store_updated_refs(const char *raw_url, const 
char *remote_name,
 static int quickfetch(struct ref *ref_map)
 {
struct ref *rm = ref_map;
+   struct check_connected_options opt = CHECK_CONNECTED_INIT;
 
/*
 * If we are deepening a shallow clone we already have these
@@ -761,7 +762,8 @@ static int quickfetch(struct ref *ref_map)
 */
if (depth)
return -1;
-   return check_everything_connected(iterate_ref_map, 1, );
+   opt.quiet = 1;
+   return check_connected(iterate_ref_map, , );
 }
 
 static int fetch_refs(struct transport *transport, struct ref *ref_map)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 15c323a..ce81920 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -737,7 +737,7 @@ static int update_shallow_ref(struct command *cmd, struct 
shallow_info *si)
 {
static struct lock_file shallow_lock;
struct sha1_array extra = SHA1_ARRAY_INIT;
-   const char *alt_file;
+   struct check_connected_options opt = CHECK_CONNECTED_INIT;
uint32_t mask = 1 << (cmd->index % 32);
int i;
 
@@ -749,9 +749,8 @@ static int update_shallow_ref(struct command *cmd, struct 
shallow_info *si)
!delayed_reachability_test(si, i))
sha1_array_append(, si->shallow->sha1[i]);

[PATCH 02/12] rev-list: add optional progress reporting

[Jeff King]

It's easy to ask rev-list to do a traversal that may take
many seconds (e.g., by calling "--objects --all"). In theory
you can monitor its progress by the output you get to
stdout, but this isn't always easy.

Some operations, like "--count", don't make any output until
the end.

And some callers, like check_everything_connected(), are
using it just for the error-checking of the traversal, and
throw away stdout entirely.

This patch adds a "--progress" option which can be used to
give some eye-candy for a user waiting for a long traversal.
This is just a rev-list option and not a regular traversal
option, because it needs cooperation from the callbacks in
builtin/rev-list.c to do the actual count.

Signed-off-by: Jeff King 
---
 Documentation/rev-list-options.txt |  4 
 builtin/rev-list.c | 17 +
 2 files changed, 21 insertions(+)

diff --git a/Documentation/rev-list-options.txt 
b/Documentation/rev-list-options.txt
index c5bd218..f39cb6d 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -274,6 +274,10 @@ ifdef::git-rev-list[]
Try to speed up the traversal using the pack bitmap index (if
one is available). Note that when traversing with `--objects`,
trees and blobs will not have their associated path printed.
+
+--progress=::
+   Show progress reports on stderr as objects are considered. The
+   `` text will be printed with each progress update.
 endif::git-rev-list[]
 
 --
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index b82bcc3..88d95a7 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -9,6 +9,7 @@
 #include "log-tree.h"
 #include "graph.h"
 #include "bisect.h"
+#include "progress.h"
 
 static const char rev_list_usage[] =
 "git rev-list [OPTION] ... [ -- paths... ]\n"
@@ -49,12 +50,17 @@ static const char rev_list_usage[] =
 "--bisect-all"
 ;
 
+struct progress *progress;
+unsigned progress_counter;
+
 static void finish_commit(struct commit *commit, void *data);
 static void show_commit(struct commit *commit, void *data)
 {
struct rev_list_info *info = data;
struct rev_info *revs = info->revs;
 
+   display_progress(progress, ++progress_counter);
+
if (info->flags & REV_LIST_QUIET) {
finish_commit(commit, data);
return;
@@ -190,6 +196,7 @@ static void show_object(struct object *obj, const char 
*name, void *cb_data)
 {
struct rev_list_info *info = cb_data;
finish_object(obj, name, cb_data);
+   display_progress(progress, ++progress_counter);
if (info->flags & REV_LIST_QUIET)
return;
show_object_with_name(stdout, obj, name);
@@ -276,6 +283,7 @@ int cmd_rev_list(int argc, const char **argv, const char 
*prefix)
int bisect_show_vars = 0;
int bisect_find_all = 0;
int use_bitmap_index = 0;
+   const char *show_progress = NULL;
 
git_config(git_default_config, NULL);
init_revisions(, prefix);
@@ -325,6 +333,10 @@ int cmd_rev_list(int argc, const char **argv, const char 
*prefix)
test_bitmap_walk();
return 0;
}
+   if (skip_prefix(arg, "--progress=", )) {
+   show_progress = arg;
+   continue;
+   }
usage(rev_list_usage);
 
}
@@ -355,6 +367,9 @@ int cmd_rev_list(int argc, const char **argv, const char 
*prefix)
if (bisect_list)
revs.limited = 1;
 
+   if (show_progress)
+   progress = start_progress_delay(show_progress, 0, 0, 2);
+
if (use_bitmap_index && !revs.prune) {
if (revs.count && !revs.left_right && !revs.cherry_mark) {
uint32_t commit_count;
@@ -392,6 +407,8 @@ int cmd_rev_list(int argc, const char **argv, const char 
*prefix)
 
traverse_commit_list(, show_commit, show_object, );
 
+   stop_progress();
+
if (revs.count) {
if (revs.left_right && revs.cherry_mark)
printf("%d\t%d\t%d\n", revs.count_left, 
revs.count_right, revs.count_same);
-- 
2.9.1.434.g748be50

--
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 03/12] check_everything_connected: convert to argv_array

[Jeff King]

This avoids the magic "9" array-size which we must avoid
overflowing, making further patches simpler.

Signed-off-by: Jeff King 
---
 connected.c | 21 +
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/connected.c b/connected.c
index 7560a31..a3bfc4e 100644
--- a/connected.c
+++ b/connected.c
@@ -26,10 +26,9 @@ static int check_everything_connected_real(sha1_iterate_fn 
fn,
   const char *shallow_file)
 {
struct child_process rev_list = CHILD_PROCESS_INIT;
-   const char *argv[9];
char commit[41];
unsigned char sha1[20];
-   int err = 0, ac = 0;
+   int err = 0;
struct packed_git *new_pack = NULL;
size_t base_len;
 
@@ -48,18 +47,16 @@ static int check_everything_connected_real(sha1_iterate_fn 
fn,
}
 
if (shallow_file) {
-   argv[ac++] = "--shallow-file";
-   argv[ac++] = shallow_file;
+   argv_array_push(_list.args, "--shallow-file");
+   argv_array_push(_list.args, shallow_file);
}
-   argv[ac++] = "rev-list";
-   argv[ac++] = "--objects";
-   argv[ac++] = "--stdin";
-   argv[ac++] = "--not";
-   argv[ac++] = "--all";
-   argv[ac++] = "--quiet";
-   argv[ac] = NULL;
+   argv_array_push(_list.args,"rev-list");
+   argv_array_push(_list.args, "--objects");
+   argv_array_push(_list.args, "--stdin");
+   argv_array_push(_list.args, "--not");
+   argv_array_push(_list.args, "--all");
+   argv_array_push(_list.args, "--quiet");
 
-   rev_list.argv = argv;
rev_list.git_cmd = 1;
rev_list.in = -1;
rev_list.no_stdout = 1;
-- 
2.9.1.434.g748be50

--
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 01/12] check_everything_connected: always pass --quiet to rev-list

[Jeff King]

The check_everything_connected function takes a "quiet"
parameter which does two things if non-zero:

  1. redirect rev-list's stderr to /dev/null to avoid
 showing errors to the user

  2. pass "--quiet" to rev-list

Item (1) is obviously useful. But item (2) is
surprisingly not. For rev-list, "--quiet" does not have
anything to do with chattiness on stderr; it tells rev-list
not to bother writing the list of traversed objects to
stdout, for efficiency.  And since we always redirect
rev-list's stdout to /dev/null in this function, there is no
point in asking it to ever write anything to stdout.

The efficiency gains are modest; a best-of-five run of "git
rev-list --objects --all" on linux.git dropped from 32.013s
to 30.502s when adding "--quiet". That's only about 5%, but
given how easy it is, it's worth doing.

Signed-off-by: Jeff King 
---
 connected.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/connected.c b/connected.c
index bf1b12e..7560a31 100644
--- a/connected.c
+++ b/connected.c
@@ -56,8 +56,7 @@ static int check_everything_connected_real(sha1_iterate_fn fn,
argv[ac++] = "--stdin";
argv[ac++] = "--not";
argv[ac++] = "--all";
-   if (quiet)
-   argv[ac++] = "--quiet";
+   argv[ac++] = "--quiet";
argv[ac] = NULL;
 
rev_list.argv = argv;
-- 
2.9.1.434.g748be50

--
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/12] push progress reporting and keepalives

[Jeff King]

If you push a large number of objects, the server side may have to chew
CPU for a long time processing the input (delta resolution, connectivity
check, and whatever any hooks choose to do). During this time, you get
no feedback that anything is happening, unless the hooks feel like
writing something to stderr.  For a repository the size of linux.git, a
full push from scratch can take several minutes.

This is annoying and confusing to the user, who wonders if the
connection has hung. But it can also cause problems on systems that have
other timeouts (e.g., firewalls dropping TCP sessions, or web proxies
dropping requests that produce no response within a certain time).

This patch series adds two new features:

 1. Progress reporting for the CPU-intensive parts of receive-pack.

 2. A keepalive mechanism similar to what we use in upload-pack
(basically sending zero-length packets on sideband 1 while the client
is waiting for us to speak).

Both are enabled for any client which speaks the sideband protocol.
Existing versions of git handle the new behavior just fine (the progress
reporting is easy, because they were expecting stderr messages anyway;
the keepalive works because the demuxer just relays zero bytes back to
send-pack).

I also tested with both JGit and libgit2 clients, and both seem to
handle the zero-length packets just fine.

There are unfortunately no automated tests, as it's hard to simulate the
effect. My manual testing involved inserting "sleep" statements into
index-pack (and hooks with manual sleeps), and then using "strace" to
confirm that the keepalives were sent.

  [01/12]: check_everything_connected: always pass --quiet to rev-list
  [02/12]: rev-list: add optional progress reporting
  [03/12]: check_everything_connected: convert to argv_array
  [04/12]: check_everything_connected: use a struct with named options
  [05/12]: check_connected: relay errors to alternate descriptor
  [06/12]: check_connected: add progress flag
  [07/12]: clone: use a real progress meter for connectivity check
  [08/12]: index-pack: add flag for showing delta-resolution progress
  [09/12]: receive-pack: turn on index-pack resolving progress
  [10/12]: receive-pack: relay connectivity errors to sideband
  [11/12]: receive-pack: turn on connectivity progress
  [12/12]: receive-pack: send keepalives during quiet periods

-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: Two consecutive clones of a remote produces different files

[Jeff King]

On Fri, Jul 15, 2016 at 10:45:47AM +0200, Morten W. J. wrote:

> I have a repository hosted on a GitLab server on my LAN and when I clone that 
> repository to a linux box and a windows box I get different files!
> 
> It is very hard to explain in words, so I have recorded my desktop while 
> reproducing it, which I can do consistently:
> 
> https://dl.dropboxusercontent.com/u/5234017/git-clone-produces-different-results.ogv
> 
> I have no ideas what is wrong or why it behaves the way it does, but I am 
> actually pretty scared now. The repository has been updated from as a subtree 
> from another repository, but that should not create such behavior?

Just a blind guess, but might you have two files with names that differ
only in case, and on Windows one is overwriting the other because you
have a case-insensitive filesystem?

Try:

  git ls-files -s | grep -i src/LogEventSubscriber.h

to see what is in the index (which is case-sensitive, and is the source
from which git checks the files out into the working tree).

-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

Two consecutive clones of a remote produces different files

[Morten W. J.]

Hi All.

I have experienced something weird today. 

I have a repository hosted on a GitLab server on my LAN and when I clone that 
repository to a linux box and a windows box I get different files!

It is very hard to explain in words, so I have recorded my desktop while 
reproducing it, which I can do consistently:

https://dl.dropboxusercontent.com/u/5234017/git-clone-produces-different-results.ogv

I have no ideas what is wrong or why it behaves the way it does, but I am 
actually pretty scared now. The repository has been updated from as a subtree 
from another repository, but that should not create such behavior?

Can anybody give me some hints on what may be wrong and how I might get 
identical clones again?


Cheers,
Morten
--
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: Plugin mechanism(s) for Git?

[Junio C Hamano]

Christian Couder  writes:

> If people think that evolution is better than intelligent design, and
> want each current topic/work to just implement what is best for it,
> then that's ok for me. If on the other hand standardizing on some ways
> to interact with external processes could be helpful to avoid
> duplicating mechanisms/code in slightly different and incompatible
> ways, then I would be happy to discuss it in a thread that is not
> specific to one of the current work.

You seem to have listed only in-flight topics, which may not be a
good starting point to think about the issues.  A common trait their
implementation share may not necessarily be a good thing (for one
thing, one trait they share is that none of them is battle tested
yet), so you cannot distill their commonality into something we
would want to have in the first place.

You would need to have existing practices that have worked well for
us in the mix.  A few examples that may help the discussion go
forward are the remote-helper framework and credential-helper.  They
both call out of Git so that external/third-party enhancement
implementations can do things that core-git alone cannot do
natively.


--
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: Plugin mechanism(s) for Git?

[Mike Hommey]

On Fri, Jul 15, 2016 at 08:46:03AM +0200, Christian Couder wrote:
> Hi,
> 
> It seems to me that there are many current topics/patch series in
> flight that are about making Git interact with external code/processes
> and that it could be interesting to step back a bit and see if we
> could find a common approach/mechanism for at least some of these
> current topics.
> 
> (This is also inspired by private discussions with AEvar, thanks to
> him, and by the fact that I am now also working for GitLab on the long
> run on external ODB for large file support.)
> 
> The current topics/work I can think of are:
> 
> - the ref backend work by Michael based on Ronnie Sahlberg's and others' work,
> - the smudge/clean filters work by Joey and Lars,
> - the watchman/index helper work by David, Duy and Ben,
> - the external ODB work by Peff and myself.

Relatedly, some future topics I'd like to have at some point and that
would require the same kind of hooking:

- committish mapping resolution.

It's not really clear what I mean with this, but here's the idea: You're
using a remote helper to talk to a non-git repository. Common examples
are svn, p4 and mercurial. I'm not sure how this would work for svn, or
what p4 revisions look like, but speaking as someone who clones
mercurial repositories with git (and working on one of the tools to do
so), there are many cases where I wish I could just do something like:

   git show hg::d4a5c8fbfc20cebcae60d1e073874d19fa47d831

where d4a5c8fbfc20cebcae60d1e073874d19fa47d831 is a mercurial changeset
id. And this is a simple example, but the idea is that it would work
anywhere we can use committishs and revranges. Ideally, it would also
work with abbreviated mercurial changesets e.g. hg::d4a5c8fbfc20


- providing information for --decorate

Here the idea would be the converse of the above, and would make e.g.
`git log --decorate` show hg::d4a5c8fbfc20cebcae60d1e073874d19fa47d831
or the abbreviated form hg::d4a5c8fbfc20 for the corresponding git
commit.


Cheers,

Mike
--
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] git-clean: remove fifo, devices, socket files

[Andrey Vagin]

On Thu, Jul 14, 2016 at 10:56 PM, Johannes Sixt  wrote:
> Am 15.07.2016 um 04:42 schrieb Andrey Vagin:
>>
>> Currently git-clean removes only links and files, but
>> there can be special files like fifo, sockets, devices.
>>
>> I think git-clean has to remove them too.
>
>
> I think that is not necessary. If you do
>
>   mkfifo fifo && sudo mknod zero c 1 5
>
> then 'git status' will not report them and 'git add' will not add them to a
> repository.
>
> Similarly, if you happen to have a special file under a name in your working
> tree where the repository has regular files, then 'git status' will report
> the names as modified, and 'git reset --hard' will replace the special files
> by the files recorded in the repository.
>
> IOW: These special files are invisible for Git unless it already knows the
> names. The latter case is outside 'git clean's domain, and the former case
> really means that special files in the working tree are left at the user's
> discretion.

I understand your points, but I don't see any reasons to ignore these files.

What will be wrong if 'git status' will reports these files?
What will be wrong if 'git add' will returns an error instead of
skipping them silently?

How it works now:
[avagin@laptop tmp]$ git add README
[avagin@laptop tmp]$ git add fifo && echo ok
ok
[avagin@laptop tmp]$ git commit -a -m "Add fifo file"
[master b04da32] Add fifo file
 1 file changed, 1 insertion(+)

How it works with this patch:

[avagin@laptop tmp]$ ../git/bin-wrappers/git add fifo && echo ok
error: fifo: can only add regular files, symbolic links or git-directories
fatal: adding files failed

I like more when 'git add' reports an error when it can't add a file.

The git-clean man page says that it removes untracked files from the
working tree. It doesn't specifies that there are only links and
regular files.

I won't insist on my point, it may be wrong. But I like when a
behavior is predictable. I didn't expect that 'git status' doesn't
show special files and 'git clean' doesn't remove them. I asked my
colleagues and the current behavior was not obvious for them too.

Thanks,
Andrew

>
> -- Hannes
>
--
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: Plugin mechanism(s) for Git?

[Lars Schneider]

> On 15 Jul 2016, at 08:46, Christian Couder  wrote:
> 
> [...]
> 
> The current topics/work I can think of are:
> 
> - the ref backend work by Michael based on Ronnie Sahlberg's and others' work,
> - the smudge/clean filters work by Joey and Lars,
> - the watchman/index helper work by David, Duy and Ben,
> - the external ODB work by Peff and myself.
> 
> They all have a need to talk to potentially different
> backends/external processes and this need is potentially not well
> satisfied by the usual mechanism that Git has traditionally used which
> is to have one or a few commands configured and to just launch the
> command(s) for each file or object, like what "git difftool" and "git
> mergetool" do.
> 
> [...]
> 
> If people think that evolution is better than intelligent design, and
> want each current topic/work to just implement what is best for it,
> then that's ok for me. If on the other hand standardizing on some ways
> to interact with external processes could be helpful to avoid
> duplicating mechanisms/code in slightly different and incompatible
> ways, then I would be happy to discuss it in a thread that is not
> specific to one of the current work.

Thanks for this great summary of the problem, Christian!

I think a generic plugin mechanism would be great but how would we do it?

We could dynamically load libraries but this would force us to freeze 
the ABI as mentioned by Duy:
http://article.gmane.org/gmane.comp.version-control.git/298463

Unix sockets are not really portable as mentioned by Dscho:
http://article.gmane.org/gmane.comp.version-control.git/298432

So far the pipe communication used by the clean/smudge filter seems
to be a good option. Maybe we could "standardize" some kind of protocol
over pipes?

I also like your ODB work and I think it is the right thing to do in
the long run. However, I only have a couple of hours a week to work 
on this and improving an existing solution seemed easier to me
(ODB would require bigger changes in Git and Git-LFS for instance).

Thanks,
Lars--
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

Plugin mechanism(s) for Git?

[Christian Couder]

Hi,

It seems to me that there are many current topics/patch series in
flight that are about making Git interact with external code/processes
and that it could be interesting to step back a bit and see if we
could find a common approach/mechanism for at least some of these
current topics.

(This is also inspired by private discussions with AEvar, thanks to
him, and by the fact that I am now also working for GitLab on the long
run on external ODB for large file support.)

The current topics/work I can think of are:

- the ref backend work by Michael based on Ronnie Sahlberg's and others' work,
- the smudge/clean filters work by Joey and Lars,
- the watchman/index helper work by David, Duy and Ben,
- the external ODB work by Peff and myself.

They all have a need to talk to potentially different
backends/external processes and this need is potentially not well
satisfied by the usual mechanism that Git has traditionally used which
is to have one or a few commands configured and to just launch the
command(s) for each file or object, like what "git difftool" and "git
mergetool" do.

One reason that the traditional mechanism might not work well is of
course performance. This traditional mechanism still is very
interesting because it is very easy to setup and experiment with.

One way to extend it for better performance is to require that the
configured command should be able to deal with a number or a stream of
files or objects (instead of just one objec/file) that are passed to
it. It looks like that is what Lars wants for smudge/clean filters.

Another way is to have the external command run as a daemon, like what
Duy and David implemented for the index-helper.

And a more integrated way is to require the external code to implement
an API and to be compiled along with Git which looks like the approach
taken by the ref backend work.

If people think that evolution is better than intelligent design, and
want each current topic/work to just implement what is best for it,
then that's ok for me. If on the other hand standardizing on some ways
to interact with external processes could be helpful to avoid
duplicating mechanisms/code in slightly different and incompatible
ways, then I would be happy to discuss it in a thread that is not
specific to one of the current work.

Thanks,
Christian.
--
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 3/3] fsck: optionally show more helpful info for broken links

[Eric Sunshine]

On Thu, Jul 14, 2016 at 11:30 AM, Johannes Schindelin
 wrote:
> When reporting broken links between commits/trees/blobs, it would be
> quite helpful at times if the user would be told how the object is
> supposed to be reachable.
>
> With the new --name-objects option, git-fsck will try to do exactly
> that: name the objects in a way that shows how they are reachable.
> [...]
> Signed-off-by: Johannes Schindelin 
> ---
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> @@ -576,6 +598,7 @@ static struct option fsck_opts[] = {
> OPT_BOOL(0, "lost-found", _lost_and_found,
> N_("write dangling objects in 
> .git/lost-found")),
> OPT_BOOL(0, "progress", _progress, N_("show progress")),
> +   OPT_BOOL(0, "name-objects", _objects, N_("show verbose names for 
> rechable objects")),

s/rechable/reachable/

> OPT_END(),
>  };
--
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