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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 17, 2015, 19:33 UTC
Message-ID: <xmqqsi7hd817.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20150817094819.GA10375@yoshi.chippynet.com>

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.

Show 15 quoted lines

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

Show 31 quoted lines

> +
> +/**
>   * 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.

Show 21 quoted lines

> 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