From: Junio C Hamano <gitster@pobox•com>
To: Paul Tan <pyokagan@gmail•com>
Cc: Johannes Schindelin <johannes.schindelin@gmx•de>,
Linus Torvalds <torvalds@linux-foundation•org>,
Stefan Beller <sbeller@google•com>,
Git Mailing List <git@vger•kernel.org>
Subject: Re: [PATCH] am --abort: merge ORIG_HEAD tree into index
Date: Mon, 17 Aug 2015 12:33:40 -0700 [thread overview]
Message-ID: <xmqqsi7hd817.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <20150817094819.GA10375@yoshi.chippynet.com> (Paul Tan's message of "Mon, 17 Aug 2015 17:48:19 +0800")
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
next prev parent reply other threads:[~2015-08-17 19:33 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-08-16 19:46 "git am --abort" screwing up index? Linus Torvalds
2015-08-16 23:33 ` Linus Torvalds
2015-08-17 8:01 ` Johannes Schindelin
2015-08-17 9:48 ` [PATCH] am --abort: merge ORIG_HEAD tree into index Paul Tan
2015-08-17 10:09 ` Johannes Schindelin
2015-08-17 14:54 ` Linus Torvalds
2015-08-17 19:33 ` Junio C Hamano [this message]
2015-08-19 8:22 ` [PATCH v2] am --skip/--abort: merge HEAD/ORIG_HEAD " Paul Tan
2015-08-19 17:55 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqsi7hd817.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=johannes.schindelin@gmx$(echo .)de \
--cc=pyokagan@gmail$(echo .)com \
--cc=sbeller@google$(echo .)com \
--cc=torvalds@linux-foundation$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox