Paul Tan <pyokagan@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html