Re: "git am --abort" screwing up index?

2015-08-17 Thread Johannes Schindelin
Hi Linus,

On 2015-08-17 01:33, Linus Torvalds wrote:
> On Sun, Aug 16, 2015 at 12:46 PM, Linus Torvalds
>  wrote:
>>
>> Maybe it has always done this, and I just haven't noticed (I usually
>> _just_ do the "git reset --hard" thing, don't ask me why I wanted to
>> be doubly sure this time). But maybe it's an effect of the new
>> built-in "am".
> 
> I bisected this. It's definitely used to work, and the regression is
> from the new built-in am.

This patch is a reproducer:

-- snipsnap --
>From 5323f1c309ad40721e2e19fa9c6ce5ad52d98271 Mon Sep 17 00:00:00 2001
From: Johannes Schindelin 
Date: Mon, 17 Aug 2015 09:37:39 +0200
Subject: [PATCH] t4151: demonstrate that builtin am corrupts index' stat data

Reported by Linus Torvalds.

Signed-off-by: Johannes Schindelin 
---
 t/t4151-am-abort.sh | 12 
 1 file changed, 12 insertions(+)

diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
index 05bdc3e..bf2e6f4 100755
--- a/t/t4151-am-abort.sh
+++ b/t/t4151-am-abort.sh
@@ -168,4 +168,16 @@ test_expect_success 'am --abort on unborn branch will keep 
local commits intact'
test_cmp expect actual
 '
 
+test_expect_failure 'am --abort leaves index stat info alone' '
+   git checkout -f --orphan stat-info &&
+   git reset &&
+   test_commit should-be-untouched &&
+   test-chmtime =0 should-be-untouched.t &&
+   git update-index --refresh &&
+   git diff-files --exit-code --quiet &&
+   test_must_fail git am 0001-*.patch &&
+   git am --abort &&
+   git diff-files --exit-code --quiet
+'
+
 test_done
-- 
2.3.1.windows.1.9.g8c01ab4


--
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: "git am --abort" screwing up index?

2015-08-16 Thread Linus Torvalds
On Sun, Aug 16, 2015 at 12:46 PM, Linus Torvalds
 wrote:
>
> Maybe it has always done this, and I just haven't noticed (I usually
> _just_ do the "git reset --hard" thing, don't ask me why I wanted to
> be doubly sure this time). But maybe it's an effect of the new
> built-in "am".

I bisected this. It's definitely used to work, and the regression is
from the new built-in am. But I cannot bisect into that branch
'pt/am-builtin', because "git am" doesn't actually work in the middle
of that branch.

So I've verified that commit c1e5ca90dba8 ("Merge branch
'es/worktree-add'") is good, and that commit 7aa2da616208 ("Merge
branch 'pt/am-builtin'") is bad, but I cannot pinpoint the exact
commit where "git am --abort" starts breaking the index.

But I assume it's simply that initial implementation of "--abort" in
commit 33388a71d23e ("builtin-am: implement --abort") that already
ends up rewriting the index from scratch without applying the old stat
data.

The test-case is pretty simple: just force a "git am" failure, then do
"git am --abort", and then you can check whether the index stat()
information is valid in various ways. For the kernel, doing a "git
reset --hard" makes it obvious because the reset will force all files
to be written out (since the index stat information doesn't match the
current tree). But you can do it by just counting system calls for a
"git diff" too. On the git tree, for example, when the index has
matching stat information, you get something like

  [torvalds@i7 git]$ strace -cf git diff
  ..
0.040.25   126 4 open
  ..

ie you only actually ended up with 26 open() system calls. When the
index is not in sync with the stat information, "git diff" will have
to open each file to see what the actual contents are, and you get

  [torvalds@i7 git]$ strace -cf git diff
  ...
0.300.70   0  5987   302 open
  ...

so now it opened about 6k files instead (and for the kernel, that
number will be much larger, of course).

I _think_ it's because git-am (in "clean_index()") uses read_tree(),
while it probably should use "unpack_trees" with opts.update and
opts.reset set (like reset_index() does in builtin/reset.h).

I have to go off do my weekly -rc now, and probably won't get to
debugging this much further. Adding Stefan to the cc, since he helped
with that "--abort" implementation.

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