"git am --abort" screwing up index?

"git am --abort" screwing up index?

[Linus Torvalds]

So I just noticed while applying a patch with "git am" when I had a
dirty tree, and I ended up getting a failure and starting over:

   [torvalds@i7 linux]$ git am --abort
   [torvalds@i7 linux]$ git reset --hard
   Checking out files: 100% (50794/50794), done.0794)
   HEAD is now at 1efdb5f0a924 Merge tag 'scsi-fixes' of
git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi

and the thing I reacted to is that the "git reset --hard" re-checked
out all the files.

That implies that "git am --abort" ended up leaving the index in a bad
state, presumably it re-did the index entirely from HEAD, without
filling it in with the stat() details from the old index.

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'm about to go out and don't have time to debug this any further
right now, but I'll try to get back to it later. I thought I'd send
out this email in case it makes Paul goes "ahh, yes.. obvious"

Not a big deal - things *work* fine. But forcing checking out every
file obviously also means that subsequent builds end up being slowed
down etc.,.

                  Linus

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

[Linus Torvalds]

On Sun, Aug 16, 2015 at 12:46 PM, Linus Torvalds
<torvalds@linux-foundation.org> 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.04    0.000025           1        26         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.30    0.000070           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

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

[Johannes Schindelin]

Hi Linus,

On 2015-08-17 01:33, Linus Torvalds wrote:
> On Sun, Aug 16, 2015 at 12:46 PM, Linus Torvalds
> <torvalds@linux-foundation.org> 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 <johannes.schindelin@gmx.de>
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 <johannes.schindelin@gmx.de>
---
 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

[PATCH] am --abort: merge ORIG_HEAD tree into index

[Paul Tan]

On Mon, Aug 17, 2015 at 10:01:29AM +0200, Johannes Schindelin wrote:
> Hi Linus,
> 
> On 2015-08-17 01:33, Linus Torvalds wrote:
> > On Sun, Aug 16, 2015 at 12:46 PM, Linus Torvalds
> > <torvalds@linux-foundation.org> 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:

Thanks Johannes for the test, and sorry all for the oversight. ><

It's true that we need to merge the ORIG_HEAD tree into the index
instead of overwriting it. Patch below.

Regards,
Paul

-- >8 --
Subject: [PATCH] am --abort: merge ORIG_HEAD tree into index

After running "git am --abort", and then running "git reset --hard",
files that were not modified would still be re-checked out.

This is because clean_index() in builtin/am.c mistakenly called the
read_tree() function, which overwrites all entries in the index,
including the stat info.

Fix this by using unpack_trees() instead to merge the tree into the
index, so that the stat info from the index is kept.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/am.c        | 49 ++++++++++++++++++++++++++++++++++++-------------
 t/t4151-am-abort.sh | 12 ++++++++++++
 2 files changed, 48 insertions(+), 13 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 1399c8d..6aaa85d 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1940,15 +1940,48 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
 }
 
 /**
+ * Merges a tree into the index. The index's stat info will take precedence
+ * over the merged tree's. Returns 0 on success, -1 on failure.
+ */
+static int merge_tree(struct tree *tree)
+{
+	struct lock_file *lock_file;
+	struct unpack_trees_options opts;
+	struct tree_desc t[2];
+
+	if (parse_tree(tree))
+		return -1;
+
+	lock_file = xcalloc(1, sizeof(struct lock_file));
+	hold_locked_index(lock_file, 1);
+
+	memset(&opts, 0, sizeof(opts));
+	opts.head_idx = 1;
+	opts.src_index = &the_index;
+	opts.dst_index = &the_index;
+	opts.merge = 1;
+	opts.fn = oneway_merge;
+	init_tree_desc(&t[0], tree->buffer, tree->size);
+
+	if (unpack_trees(1, t, &opts)) {
+		rollback_lock_file(lock_file);
+		return -1;
+	}
+
+	if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
+		die(_("unable to write new index file"));
+
+	return 0;
+}
+
+/**
  * Clean the index without touching entries that are not modified between
  * `head` and `remote`.
  */
 static int clean_index(const unsigned char *head, const unsigned char *remote)
 {
-	struct lock_file *lock_file;
 	struct tree *head_tree, *remote_tree, *index_tree;
 	unsigned char index[GIT_SHA1_RAWSZ];
-	struct pathspec pathspec;
 
 	head_tree = parse_tree_indirect(head);
 	if (!head_tree)
@@ -1973,18 +2006,8 @@ static int clean_index(const unsigned char *head, const unsigned char *remote)
 	if (fast_forward_to(index_tree, remote_tree, 0))
 		return -1;
 
-	memset(&pathspec, 0, sizeof(pathspec));
-
-	lock_file = xcalloc(1, sizeof(struct lock_file));
-	hold_locked_index(lock_file, 1);
-
-	if (read_tree(remote_tree, 0, &pathspec)) {
-		rollback_lock_file(lock_file);
+	if (merge_tree(remote_tree))
 		return -1;
-	}
-
-	if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
-		die(_("unable to write new index file"));
 
 	remove_branch_state();
 
diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
index 05bdc3e..9c3bbd1 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_success '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.5.0.331.g11c07ce

Re: [PATCH] am --abort: merge ORIG_HEAD tree into index

[Johannes Schindelin]

Hi Paul,

On 2015-08-17 11:48, Paul Tan wrote:

> It's true that we need to merge the ORIG_HEAD tree into the index
> instead of overwriting it. Patch below.

Thanks for your impressive, very responsive work!
Dscho

Re: [PATCH] am --abort: merge ORIG_HEAD tree into index

[Linus Torvalds]

On Mon, Aug 17, 2015 at 2:48 AM, Paul Tan <pyokagan@gmail.com> wrote:
>
> It's true that we need to merge the ORIG_HEAD tree into the index
> instead of overwriting it. Patch below.

Seems to work for me. Thanks,

                     Linus

Re: [PATCH] am --abort: merge ORIG_HEAD tree into index

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

The codepath in the original looks like this:

        head_tree=$(git rev-parse --verify -q HEAD || echo $empty_tree) &&
==>     git read-tree --reset -u $head_tree $head_tree &&
        index_tree=$(git write-tree) &&
        orig_head=$(git rev-parse --verify -q ORIG_HEAD || echo $empty_tree) &&
==>     git read-tree -m -u $index_tree $orig_head
        if git rev-parse --verify -q ORIG_HEAD >/dev/null 2>&1
        then
                git reset ORIG_HEAD
        else
                git read-tree $empty_tree
                curr_branch=$(git symbolic-ref HEAD 2>/dev/null) &&
                git update-ref -d $curr_branch
        fi

Your am_abort() implements the above fairly faithfully up to the
point where it computes orig_head.  Your clean_index() function that
is called from there roughly corresponds to the "read-tree --reset -u"
to reset the index to the HEAD's tree and then "read-tree -m -u" to
go to ORIG_HEAD from $index_tree.

> diff --git a/builtin/am.c b/builtin/am.c
> index 1399c8d..6aaa85d 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1940,15 +1940,48 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
>  }
>  
>  /**
> + * Merges a tree into the index. The index's stat info will take precedence
> + * over the merged tree's. Returns 0 on success, -1 on failure.
> + */
> +static int merge_tree(struct tree *tree)
> +{
> +...
> +}

This looks more like "git reset ORIG_HEAD" in the original above ;-)

> +
> +/**
>   * Clean the index without touching entries that are not modified between
>   * `head` and `remote`.
>   */
>  static int clean_index(const unsigned char *head, const unsigned char *remote)
>  {
> -	struct lock_file *lock_file;
>  	struct tree *head_tree, *remote_tree, *index_tree;
>  	unsigned char index[GIT_SHA1_RAWSZ];
> -	struct pathspec pathspec;
>  
>  	head_tree = parse_tree_indirect(head);
>  	if (!head_tree)
> @@ -1973,18 +2006,8 @@ static int clean_index(const unsigned char *head, const unsigned char *remote)
>  	if (fast_forward_to(index_tree, remote_tree, 0))
>  		return -1;
>  
> -	memset(&pathspec, 0, sizeof(pathspec));
> -
> -	lock_file = xcalloc(1, sizeof(struct lock_file));
> -	hold_locked_index(lock_file, 1);
> -
> -	if (read_tree(remote_tree, 0, &pathspec)) {
> -		rollback_lock_file(lock_file);
> +	if (merge_tree(remote_tree))
>  		return -1;
> -	}
> -
> -	if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
> -		die(_("unable to write new index file"));

And by getting rid of the call to "one-tree from scratch" form or
read_tree(), we can lose quite a lot of code from here.  Good ;-)

Note that "am skip" codepath also calls clean_index(), so this patch
would affect it.

Have you checked how this change affects that codepath?  To put it
differently, does "am skip" have the same issue without this fix?
If so, I wonder if we can have a test for that, too?

Thanks.

> diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
> index 05bdc3e..9c3bbd1 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_success '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

[PATCH v2] am --skip/--abort: merge HEAD/ORIG_HEAD tree into index

[Paul Tan]

On Mon, Aug 17, 2015 at 12:33:40PM -0700, Junio C Hamano wrote:
> Have you checked how this change affects that codepath?  To put it
> differently, does "am skip" have the same issue without this fix?

Hmm, I adopted Dscho's test to run "git am --skip" and it did not fail.
I think it's because am_skip() calls am_run(), which calls
refresh_cache(), so the resulting index will have the updated stat info.
However, there should still be a performance penalty because
refresh_cache() would have to scan all files for changes.

> If so, I wonder if we can have a test for that, too?

So yeah, we should have a test for that too.

(In addition, I fixed a small mistake with the "struct tree_desc" array
size.)

Thanks,
Paul

-- >8 --
Subject: [PATCH v2] am --skip/--abort: merge HEAD/ORIG_HEAD tree into index

After running "git am --abort", and then running "git reset --hard",
files that were not modified would still be re-checked out.

This is because clean_index() in builtin/am.c mistakenly called the
read_tree() function, which overwrites all entries in the index,
including the stat info.

"git am --skip" did not seem to have this issue because am_skip() called
am_run(), which called refresh_cache() to update the stat info. However,
there's still a performance penalty as the lack of stat info meant that
refresh_cache() would have to scan all files for changes.

Fix this by using unpack_trees() instead to merge the tree into the
index, so that the stat info from the index is kept.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/am.c        | 49 ++++++++++++++++++++++++++++++++++++-------------
 t/t4151-am-abort.sh | 24 ++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 13 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 1399c8d..3e7e66f 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1940,15 +1940,48 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
 }
 
 /**
+ * Merges a tree into the index. The index's stat info will take precedence
+ * over the merged tree's. Returns 0 on success, -1 on failure.
+ */
+static int merge_tree(struct tree *tree)
+{
+	struct lock_file *lock_file;
+	struct unpack_trees_options opts;
+	struct tree_desc t[1];
+
+	if (parse_tree(tree))
+		return -1;
+
+	lock_file = xcalloc(1, sizeof(struct lock_file));
+	hold_locked_index(lock_file, 1);
+
+	memset(&opts, 0, sizeof(opts));
+	opts.head_idx = 1;
+	opts.src_index = &the_index;
+	opts.dst_index = &the_index;
+	opts.merge = 1;
+	opts.fn = oneway_merge;
+	init_tree_desc(&t[0], tree->buffer, tree->size);
+
+	if (unpack_trees(1, t, &opts)) {
+		rollback_lock_file(lock_file);
+		return -1;
+	}
+
+	if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
+		die(_("unable to write new index file"));
+
+	return 0;
+}
+
+/**
  * Clean the index without touching entries that are not modified between
  * `head` and `remote`.
  */
 static int clean_index(const unsigned char *head, const unsigned char *remote)
 {
-	struct lock_file *lock_file;
 	struct tree *head_tree, *remote_tree, *index_tree;
 	unsigned char index[GIT_SHA1_RAWSZ];
-	struct pathspec pathspec;
 
 	head_tree = parse_tree_indirect(head);
 	if (!head_tree)
@@ -1973,18 +2006,8 @@ static int clean_index(const unsigned char *head, const unsigned char *remote)
 	if (fast_forward_to(index_tree, remote_tree, 0))
 		return -1;
 
-	memset(&pathspec, 0, sizeof(pathspec));
-
-	lock_file = xcalloc(1, sizeof(struct lock_file));
-	hold_locked_index(lock_file, 1);
-
-	if (read_tree(remote_tree, 0, &pathspec)) {
-		rollback_lock_file(lock_file);
+	if (merge_tree(remote_tree))
 		return -1;
-	}
-
-	if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
-		die(_("unable to write new index file"));
 
 	remove_branch_state();
 
diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
index 05bdc3e..ea5ace9 100755
--- a/t/t4151-am-abort.sh
+++ b/t/t4151-am-abort.sh
@@ -168,4 +168,28 @@ test_expect_success 'am --abort on unborn branch will keep local commits intact'
 	test_cmp expect actual
 '
 
+test_expect_success 'am --skip leaves index stat info alone' '
+	git checkout -f --orphan skip-stat-info &&
+	git reset &&
+	test_commit skip-should-be-untouched &&
+	test-chmtime =0 skip-should-be-untouched.t &&
+	git update-index --refresh &&
+	git diff-files --exit-code --quiet &&
+	test_must_fail git am 0001-*.patch &&
+	git am --skip &&
+	git diff-files --exit-code --quiet
+'
+
+test_expect_success 'am --abort leaves index stat info alone' '
+	git checkout -f --orphan abort-stat-info &&
+	git reset &&
+	test_commit abort-should-be-untouched &&
+	test-chmtime =0 abort-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.5.0

Re: [PATCH v2] am --skip/--abort: merge HEAD/ORIG_HEAD tree into index

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> (In addition, I fixed a small mistake with the "struct tree_desc" array
> size.)

Yeah, one thing I forgot to mention was that I suspect this kind of
"oneway merge" already in other places in the code, and a future
mini-project might be to inspect them all and try to see if their
commonalities can be taken advantage of.  If we had a small helper
function to do the "do the equivalent of 'git reset $TREE'", this
miscounting wouldn't have happened.

The change looks sensible.  Thanks.