threads / patch / 40146

patch, 3 partsam: do not corrupt the index stat state

Subject: [PATCH 2/3] am: do not corrupt the index stat state

## tl;dr

5 messages between Aug 21, 2015 and Aug 21, 2015. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

David Aguilar· Aug 21, 2015, 08:02 UTC · lore

[PATCH 1/3] t4151: demonstrate that builtin am corrupts index' stat data

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Reported by Linus Torvalds.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t4151-am-abort.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Show changes to t/t4151-am-abort.sh +12 −0

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.5.0.403.gd17121e

David Aguilar· Aug 21, 2015, 08:02 UTC · re: David Aguilar · lore

Reported-by: Linus Torvalds
Signed-off-by: David Aguilar <davvid@gmail.com>
---
 builtin/am.c        | 14 +++++++++++++-
 t/t4151-am-abort.sh |  2 +-
 2 files changed, 14 insertions(+), 2 deletions(-)

Show changes to 2 files +14 −2

builtin/am.c, t/t4151-am-abort.sh

diff --git a/builtin/am.c b/builtin/am.c
index 1399c8d..9db1b34 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1949,6 +1949,8 @@ static int clean_index(const unsigned char *head, const unsigned char *remote)
 	struct tree *head_tree, *remote_tree, *index_tree;
 	unsigned char index[GIT_SHA1_RAWSZ];
 	struct pathspec pathspec;
+	struct tree_desc desc;
+	struct unpack_trees_options opts;
 
 	head_tree = parse_tree_indirect(head);
 	if (!head_tree)
@@ -1975,10 +1977,20 @@ static int clean_index(const unsigned char *head, const unsigned char *remote)
 
 	memset(&pathspec, 0, sizeof(pathspec));
 
+	memset(&opts, 0, sizeof(opts));
+	opts.fn = oneway_merge;
+	opts.pathspec = &pathspec;
+	opts.src_index = &the_index;
+	opts.dst_index = &the_index;
+	opts.head_idx = 1;
+	opts.merge = 1;
+	opts.reset = 1;
+	init_tree_desc(&desc, remote_tree->buffer, remote_tree->size);
+
 	lock_file = xcalloc(1, sizeof(struct lock_file));
 	hold_locked_index(lock_file, 1);
 
-	if (read_tree(remote_tree, 0, &pathspec)) {
+	if (unpack_trees(1, &desc, &opts)) {
 		rollback_lock_file(lock_file);
 		return -1;
 	}
diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
index bf2e6f4..9c3bbd1 100755
--- a/t/t4151-am-abort.sh
+++ b/t/t4151-am-abort.sh
@@ -168,7 +168,7 @@ 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' '
+test_expect_success 'am --abort leaves index stat info alone' '
 	git checkout -f --orphan stat-info &&
 	git reset &&
 	test_commit should-be-untouched &&

-- 
2.5.0.403.gd17121e

Junio C Hamano· Aug 21, 2015, 16:37 UTC · re: David Aguilar · lore

Re: [PATCH 2/3] am: do not corrupt the index stat state

David Aguilar <davvid@gmail.com> writes:

Show 21 quoted lines

> Reported-by: Linus Torvalds
> Signed-off-by: David Aguilar <davvid@gmail.com>
> @@ -1975,10 +1977,20 @@ static int clean_index(const unsigned char *head, const unsigned char *remote)
>  
>  	memset(&pathspec, 0, sizeof(pathspec));
>  
> +	memset(&opts, 0, sizeof(opts));
> +	opts.fn = oneway_merge;
> +	opts.pathspec = &pathspec;
> +	opts.src_index = &the_index;
> +	opts.dst_index = &the_index;
> +	opts.head_idx = 1;
> +	opts.merge = 1;
> +	opts.reset = 1;
> +	init_tree_desc(&desc, remote_tree->buffer, remote_tree->size);
> +
>  	lock_file = xcalloc(1, sizeof(struct lock_file));
>  	hold_locked_index(lock_file, 1);
>  
> -	if (read_tree(remote_tree, 0, &pathspec)) {
> +	if (unpack_trees(1, &desc, &opts)) {

I think the same fix was done in 3ecc7040 (am --skip/--abort: merge HEAD/ORIG_HEAD tree into index, 2015-08-19).

David Aguilar· Aug 21, 2015, 08:02 UTC · re: David Aguilar · lore

[PATCH 3/3] am: rename "struct tree_desc t" to "desc" for readability

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 builtin/am.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Show changes to builtin/am.c +4 −4

diff --git a/builtin/am.c b/builtin/am.c
index 9db1b34..7e6860d 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1907,7 +1907,7 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
 {
 	struct lock_file *lock_file;
 	struct unpack_trees_options opts;
-	struct tree_desc t[2];
+	struct tree_desc desc[2];
 
 	if (parse_tree(head) || parse_tree(remote))
 		return -1;
@@ -1925,10 +1925,10 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
 	opts.merge = 1;
 	opts.reset = reset;
 	opts.fn = twoway_merge;
-	init_tree_desc(&t[0], head->buffer, head->size);
-	init_tree_desc(&t[1], remote->buffer, remote->size);
+	init_tree_desc(&desc[0], head->buffer, head->size);
+	init_tree_desc(&desc[1], remote->buffer, remote->size);
 
-	if (unpack_trees(2, t, &opts)) {
+	if (unpack_trees(2, desc, &opts)) {
 		rollback_lock_file(lock_file);
 		return -1;
 	}

-- 
2.5.0.403.gd17121e

Junio C Hamano· Aug 21, 2015, 16:51 UTC · re: David Aguilar · lore

Re: [PATCH 3/3] am: rename "struct tree_desc t" to "desc" for readability

David Aguilar <davvid@gmail.com> writes:

> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
>  builtin/am.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Naming the tree_desc parameter given to unpack_trees() as "t" is (unfortunately) an established convention, just like we often use "i" and "j" in for() loop control.

Descriptive names are nicer, especially on the calling sites, but we have quite a many hits from

    $ git grep unpack_trees\( \*.c

so perhaps do this as a clean-up patch after dust settles for all the existing callers in archive, clone, commit, merge, read-tree, reset, diff-lib, and merge-recursive?