{"thread":{"id":"27782","subject":"reproducible unexpected behavior for 'git reset'","startedAt":"2011-07-10T22:30:31Z","lastAt":"2011-07-19T21:28:01Z","messageCount":9,"participants":["John Nowak","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"171039","messageId":"75BBC138-2BD7-41F9-9D84-CEFF23CDC235@johnnowak.com","threadId":"27782","inReplyTo":null,"subject":"reproducible unexpected behavior for 'git reset'","fromName":"John Nowak","fromEmail":"john@johnnowak.com","sentAt":"2011-07-10T22:30:31Z","receivedAt":"2011-07-10T22:30:31Z","isPatch":false,"sender":{"key":"john@johnnowak.com","avatar":null},"body":"(Apologies if you receive this twice. I sent the first copy before confirming my list subscription and I'm not sure if it went through.)\n\nI am able to reproduce a scenario where, after a 'commit' and a 'stash pop' that results in a merge conflict, I need to 'reset' a file twice in order to get the index back to HEAD. It appears that the first 'reset' sets the index to the merge base version instead of HEAD which was, for me, rather unexpected. I encountered this on 1.7.4 but others have reproduced it on the latest master. If this behavior is documented, I cannot find it.\n\nA transcript to reproduce this oddity is below; note how the file is partially staged after the first 'reset' and unstaged after the second:\n\n---\n\n$ git init\nInitialized empty Git repository in /Users/jn/test/.git/\n\n$ echo \"a\" > foo\n\n$ git add foo\n\n$ git commit -a -m \"Initial\"\n[master (root-commit) 5214837] Initial\n1 files changed, 1 insertions(+), 0 deletions(-)\ncreate mode 100644 foo\n\n$ echo \"b\" >> foo\n\n$ git stash\nSaved working directory and index state WIP on master: 5214837 Initial\nHEAD is now at 5214837 Initial\n\n$ echo \"c\" >> foo\n\n$ git add foo\n\n$ git commit -a -m \"Added c\"\n[master 69eef48] Added c\n1 files changed, 1 insertions(+), 0 deletions(-)\n\n$ git stash pop\nAuto-merging foo\nCONFLICT (content): Merge conflict in foo\n\n$ git status\n# On branch master\n# Unmerged paths:\n#   (use \"git reset HEAD <file>...\" to unstage)\n#   (use \"git add/rm <file>...\" as appropriate to mark resolution)\n#\n#\tboth modified:      foo\n#\nno changes added to commit (use \"git add\" and/or \"git commit -a\")\n\n$ git reset foo\nUnstaged changes after reset:\nM\tfoo\n\n$ git status\n# On branch master\n# Changes to be committed:\n#   (use \"git reset HEAD <file>...\" to unstage)\n#\n#\tmodified:   foo\n#\n# Changes not staged for commit:\n#   (use \"git add <file>...\" to update what will be committed)\n#   (use \"git checkout -- <file>...\" to discard changes in working directory)\n#\n#\tmodified:   foo\n#\n\n$ git reset foo\nUnstaged changes after reset:\nM\tfoo\n\n$ git status\n# On branch master\n# Changes not staged for commit:\n#   (use \"git add <file>...\" to update what will be committed)\n#   (use \"git checkout -- <file>...\" to discard changes in working directory)\n#\n#\tmodified:   foo\n#\nno changes added to commit (use \"git add\" and/or \"git commit -a\")\n"},{"id":"171112","messageId":"7vwrfoa3fg.fsf@alter.siamese.dyndns.org","threadId":"27782","inReplyTo":"75BBC138-2BD7-41F9-9D84-CEFF23CDC235@johnnowak.com","subject":"Re: reproducible unexpected behavior for 'git reset'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-11T21:54:43Z","receivedAt":"2011-07-11T21:54:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Nowak <john@johnnowak.com> writes:\n\n> I am able to reproduce a scenario where, after a 'commit' and a 'stash\n> pop' that results in a merge conflict, I need to 'reset' a file twice in\n> order to get the index back to HEAD.\n\nThanks, you found a bug in \"git reset [<commit>] -- path\" codepath, it\nseems, when dealing with an unmerged path.\n"},{"id":"171115","messageId":"7voc10a1a2.fsf@alter.siamese.dyndns.org","threadId":"27782","inReplyTo":"7vwrfoa3fg.fsf@alter.siamese.dyndns.org","subject":"Re: reproducible unexpected behavior for 'git reset'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-11T22:41:09Z","receivedAt":"2011-07-11T22:41:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> I am able to reproduce a scenario where, after a 'commit' and a 'stash\n>> pop' that results in a merge conflict, I need to 'reset' a file twice in\n>> order to get the index back to HEAD.\n>\n> Thanks, you found a bug in \"git reset [<commit>] -- path\" codepath, it\n> seems, when dealing with an unmerged path.\n\nThis patch is probably a wrong way to fix this issue; I have this\nsuspicion that the original code for \"reset [<commit>] -- paths...\"\ncodepath is correctly designed to deal with unmerged index, and it would\nprobably need a deeper surgery.\n\n-- >8 --\nSubject: [PATCH] reset [<commit>] paths...: do not mishandle unmerged paths\n\nBy reading the index with unmerged content before running diff-index\nwith the tree-ish we are reading from, read_from_tree() function ended\nup stuffing the object name from a wrong stage to the resulting index.\n\nNoticed by John Nowak.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n builtin/reset.c  |    2 +-\n t/t7102-reset.sh |   15 +++++++++++++++\n 2 files changed, 16 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 98bca04..09c8d2a 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -202,7 +202,7 @@ static int read_from_tree(const char *prefix, const char **argv,\n \n \tindex_fd = hold_locked_index(lock, 1);\n \tindex_was_discarded = 0;\n-\tread_cache();\n+\tread_cache_unmerged();\n \tif (do_diff_cache(tree_sha1, &opt))\n \t\treturn 1;\n \tdiffcore_std(&opt);\ndiff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\nindex f1cfc9a..1784412 100755\n--- a/t/t7102-reset.sh\n+++ b/t/t7102-reset.sh\n@@ -429,6 +429,21 @@ test_expect_success '--mixed refreshes the index' '\n \ttest_i18ncmp expect output\n '\n \n+test_expect_success 'resetting specific path that is unmerged' '\n+\tF1=$(git rev-parse HEAD:file1) &&\n+\tF2=$(git rev-parse HEAD:file2) &&\n+\tF3=$(git rev-parse HEAD:secondfile) &&\n+\t{\n+\t\techo \"000000 $_z40 0\tfile2\" &&\n+\t\techo \"100644 $F1 1\tfile2\" &&\n+\t\techo \"100644 $F2 2\tfile2\" &&\n+\t\techo \"100644 $F3 3\tfile2\"\n+\t} | git update-index --index-info &&\n+\tgit ls-files -u &&\n+\ttest_must_fail git reset HEAD file2 &&\n+\tgit diff-index --exit-code --cached HEAD\n+'\n+\n test_expect_success 'disambiguation (1)' '\n \n \tgit reset --hard &&\n"},{"id":"171307","messageId":"7vzkkh2yfu.fsf@alter.siamese.dyndns.org","threadId":"27782","inReplyTo":"7voc10a1a2.fsf@alter.siamese.dyndns.org","subject":"Re: reproducible unexpected behavior for 'git reset'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-14T06:01:09Z","receivedAt":"2011-07-14T06:01:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> This patch is probably a wrong way to fix this issue...\n\nYes, it was a wrong way. Here is the hopefully right one ;-)\n\nFunny thing is that this is a rather ancient bug, dating back to d1f2d7e\n(Make run_diff_index() use unpack_trees(), not read_tree(), 2008-01-19)\nand survived a fix for that commit at 29796c6 (diff-index: report unmerged\nnew entries, 2009-08-04). The reason it survived is probably because\npeople care about the fact that the path is unmerged, but not the exact\nobject name on the LHS of the diff (which comes from the tree).\n\n-- >8 --\nSubject: [PATCH] reset [<commit>] paths...: do not mishandle unmerged paths\n\nBecause \"diff --cached HEAD\" showed an incorrect blob object name on the\nLHS of the diff, we ended up updating the index entry with bogus value,\nnot what we read from the tree.\n\nNoticed by John Nowak.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/reset.c  |    2 +-\n diff-lib.c       |    3 ++-\n t/t7102-reset.sh |   15 +++++++++++++++\n 3 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 98bca04..777e7c6 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -162,7 +162,7 @@ static void update_index_from_diff(struct diff_queue_struct *q,\n \n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filespec *one = q->queue[i]->one;\n-\t\tif (one->mode) {\n+\t\tif (one->mode && !is_null_sha1(one->sha1)) {\n \t\t\tstruct cache_entry *ce;\n \t\t\tce = make_cache_entry(one->mode, one->sha1, one->path,\n \t\t\t\t0, 0);\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 9c29293..fd61acb 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -379,7 +379,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n \tif (cached && idx && ce_stage(idx)) {\n \t\tstruct diff_filepair *pair;\n \t\tpair = diff_unmerge(&revs->diffopt, idx->name);\n-\t\tfill_filespec(pair->one, idx->sha1, idx->ce_mode);\n+\t\tif (tree)\n+\t\t\tfill_filespec(pair->one, tree->sha1, tree->ce_mode);\n \t\treturn;\n \t}\n \ndiff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\nindex f1cfc9a..b096dc8 100755\n--- a/t/t7102-reset.sh\n+++ b/t/t7102-reset.sh\n@@ -429,6 +429,21 @@ test_expect_success '--mixed refreshes the index' '\n \ttest_i18ncmp expect output\n '\n \n+test_expect_success 'resetting specific path that is unmerged' '\n+\tgit rm --cached file2 &&\n+\tF1=$(git rev-parse HEAD:file1) &&\n+\tF2=$(git rev-parse HEAD:file2) &&\n+\tF3=$(git rev-parse HEAD:secondfile) &&\n+\t{\n+\t\techo \"100644 $F1 1\tfile2\" &&\n+\t\techo \"100644 $F2 2\tfile2\" &&\n+\t\techo \"100644 $F3 3\tfile2\"\n+\t} | git update-index --index-info &&\n+\tgit ls-files -u &&\n+\ttest_must_fail git reset HEAD file2 &&\n+\tgit diff-index --exit-code --cached HEAD\n+'\n+\n test_expect_success 'disambiguation (1)' '\n \n \tgit reset --hard &&\n-- \n1.7.6.178.g55272\n"},{"id":"171688","messageId":"1311099186-16482-1-git-send-email-gitster@pobox.com","threadId":"27782","inReplyTo":"7vzkkh2yfu.fsf@alter.siamese.dyndns.org","subject":"[PATCH 0/2] refactoring diff-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-19T18:13:04Z","receivedAt":"2011-07-19T18:13:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"While I was looking at a rather ancient bug in \"git reset\" reported by\nJohn Nowak recently, I noticed that we can reduce duplicated code in the\ndiff_index() codepath.\n\nJunio C Hamano (2):\n  diff-lib: simplify do_diff_cache()\n  diff-lib: refactor run_diff_index() and do_diff_cache()\n\n diff-lib.c |   71 ++++++++++++++++-------------------------------------------\n 1 files changed, 19 insertions(+), 52 deletions(-)\n\n-- \n1.7.6.178.g55272\n"},{"id":"171690","messageId":"1311099186-16482-2-git-send-email-gitster@pobox.com","threadId":"27782","inReplyTo":"1311099186-16482-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/2] diff-lib: simplify do_diff_cache()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-19T18:13:05Z","receivedAt":"2011-07-19T18:13:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Since 34110cd (Make 'unpack_trees()' have a separate source and\ndestination index, 2008-03-06), we can run unpack_trees() without munging\nthe index at all, but do_diff_cache() tried ever so carefully to work\naround the old behaviour of the function.\n\nWe can just tell unpack_trees() not to touch the original index and there\nis no need to clean-up whatever the previous round has done.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n diff-lib.c |   26 +-------------------------\n 1 files changed, 1 insertions(+), 25 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex fd61acb..b5bb58d 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -480,33 +480,9 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)\n {\n \tstruct tree *tree;\n \tstruct rev_info revs;\n-\tint i;\n-\tstruct cache_entry **dst;\n-\tstruct cache_entry *last = NULL;\n \tstruct unpack_trees_options opts;\n \tstruct tree_desc t;\n \n-\t/*\n-\t * This is used by git-blame to run diff-cache internally;\n-\t * it potentially needs to repeatedly run this, so we will\n-\t * start by removing the higher order entries the last round\n-\t * left behind.\n-\t */\n-\tdst = active_cache;\n-\tfor (i = 0; i < active_nr; i++) {\n-\t\tstruct cache_entry *ce = active_cache[i];\n-\t\tif (ce_stage(ce)) {\n-\t\t\tif (last && !strcmp(ce->name, last->name))\n-\t\t\t\tcontinue;\n-\t\t\tcache_tree_invalidate_path(active_cache_tree,\n-\t\t\t\t\t\t   ce->name);\n-\t\t\tlast = ce;\n-\t\t\tce->ce_flags |= CE_REMOVE;\n-\t\t}\n-\t\t*dst++ = ce;\n-\t}\n-\tactive_nr = dst - active_cache;\n-\n \tinit_revisions(&revs, NULL);\n \tinit_pathspec(&revs.prune_data, opt->pathspec.raw);\n \ttree = parse_tree_indirect(tree_sha1);\n@@ -521,7 +497,7 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)\n \topts.fn = oneway_diff;\n \topts.unpack_data = &revs;\n \topts.src_index = &the_index;\n-\topts.dst_index = &the_index;\n+\topts.dst_index = NULL;\n \n \tinit_tree_desc(&t, tree->buffer, tree->size);\n \tif (unpack_trees(1, &t, &opts))\n-- \n1.7.6.178.g55272\n"},{"id":"171689","messageId":"1311099186-16482-3-git-send-email-gitster@pobox.com","threadId":"27782","inReplyTo":"1311099186-16482-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/2] diff-lib: refactor run_diff_index() and do_diff_cache()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-19T18:13:06Z","receivedAt":"2011-07-19T18:13:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The latter is meant to be an API for internal callers that want to inspect\nthe resulting diff-queue, while the former is an implementation of \"git\ndiff-index\" command. Extract the common logic into a single helper\nfunction and make them thin wrappers around it.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n diff-lib.c |   47 +++++++++++++++++++----------------------------\n 1 files changed, 19 insertions(+), 28 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex b5bb58d..c762655 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -440,20 +440,19 @@ static int oneway_diff(struct cache_entry **src, struct unpack_trees_options *o)\n \treturn 0;\n }\n \n-int run_diff_index(struct rev_info *revs, int cached)\n+static int diff_cache(struct rev_info *revs,\n+\t\t      const unsigned char *tree_sha1,\n+\t\t      const char *tree_name,\n+\t\t      int cached)\n {\n-\tstruct object *ent;\n \tstruct tree *tree;\n-\tconst char *tree_name;\n-\tstruct unpack_trees_options opts;\n \tstruct tree_desc t;\n+\tstruct unpack_trees_options opts;\n \n-\tent = revs->pending.objects[0].item;\n-\ttree_name = revs->pending.objects[0].name;\n-\ttree = parse_tree_indirect(ent->sha1);\n+\ttree = parse_tree_indirect(tree_sha1);\n \tif (!tree)\n-\t\treturn error(\"bad tree object %s\", tree_name);\n-\n+\t\treturn error(\"bad tree object %s\",\n+\t\t\t     tree_name ? tree_name : sha1_to_hex(tree_sha1));\n \tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = 1;\n \topts.index_only = cached;\n@@ -466,7 +465,15 @@ int run_diff_index(struct rev_info *revs, int cached)\n \topts.dst_index = NULL;\n \n \tinit_tree_desc(&t, tree->buffer, tree->size);\n-\tif (unpack_trees(1, &t, &opts))\n+\treturn unpack_trees(1, &t, &opts);\n+}\n+\n+int run_diff_index(struct rev_info *revs, int cached)\n+{\n+\tstruct object_array_entry *ent;\n+\n+\tent = revs->pending.objects;\n+\tif (diff_cache(revs, ent->item->sha1, ent->name, cached))\n \t\texit(128);\n \n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"c/\", cached ? \"i/\" : \"w/\");\n@@ -478,29 +485,13 @@ int run_diff_index(struct rev_info *revs, int cached)\n \n int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)\n {\n-\tstruct tree *tree;\n \tstruct rev_info revs;\n-\tstruct unpack_trees_options opts;\n-\tstruct tree_desc t;\n \n \tinit_revisions(&revs, NULL);\n \tinit_pathspec(&revs.prune_data, opt->pathspec.raw);\n-\ttree = parse_tree_indirect(tree_sha1);\n-\tif (!tree)\n-\t\tdie(\"bad tree object %s\", sha1_to_hex(tree_sha1));\n+\trevs.diffopt = *opt;\n \n-\tmemset(&opts, 0, sizeof(opts));\n-\topts.head_idx = 1;\n-\topts.index_only = 1;\n-\topts.diff_index_cached = !DIFF_OPT_TST(opt, FIND_COPIES_HARDER);\n-\topts.merge = 1;\n-\topts.fn = oneway_diff;\n-\topts.unpack_data = &revs;\n-\topts.src_index = &the_index;\n-\topts.dst_index = NULL;\n-\n-\tinit_tree_desc(&t, tree->buffer, tree->size);\n-\tif (unpack_trees(1, &t, &opts))\n+\tif (diff_cache(&revs, tree_sha1, NULL, 1))\n \t\texit(128);\n \treturn 0;\n }\n-- \n1.7.6.178.g55272\n"},{"id":"171700","messageId":"20110719201400.GA2938@sigill.intra.peff.net","threadId":"27782","inReplyTo":"1311099186-16482-2-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 1/2] diff-lib: simplify do_diff_cache()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-07-19T20:14:00Z","receivedAt":"2011-07-19T20:14:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 19, 2011 at 11:13:05AM -0700, Junio C Hamano wrote:\n\n> Since 34110cd (Make 'unpack_trees()' have a separate source and\n> destination index, 2008-03-06), we can run unpack_trees() without munging\n> the index at all, but do_diff_cache() tried ever so carefully to work\n> around the old behaviour of the function.\n> \n> We can just tell unpack_trees() not to touch the original index and there\n> is no need to clean-up whatever the previous round has done.\n\nCan we drop the calls to discard_cache in reset.c now?\n\n$ git grep -B1 discard_cache builtin/reset.c\nbuiltin/reset.c-        /* do_diff_cache() mangled the index */\nbuiltin/reset.c:        discard_cache();\n--\nbuiltin/reset.c-                /* The index is still clobbered from do_diff_cache() */\nbuiltin/reset.c:                discard_cache();\n\n-Peff\n"},{"id":"171706","messageId":"7vei1m7yfy.fsf@alter.siamese.dyndns.org","threadId":"27782","inReplyTo":"20110719201400.GA2938@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] diff-lib: simplify do_diff_cache()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-19T21:28:01Z","receivedAt":"2011-07-19T21:28:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Jul 19, 2011 at 11:13:05AM -0700, Junio C Hamano wrote:\n>\n>> Since 34110cd (Make 'unpack_trees()' have a separate source and\n>> destination index, 2008-03-06), we can run unpack_trees() without munging\n>> the index at all, but do_diff_cache() tried ever so carefully to work\n>> around the old behaviour of the function.\n>> \n>> We can just tell unpack_trees() not to touch the original index and there\n>> is no need to clean-up whatever the previous round has done.\n>\n> Can we drop the calls to discard_cache in reset.c now?\n\nPossibly, along with \"index-was-discarded\" flag bit.\n\n> $ git grep -B1 discard_cache builtin/reset.c\n> builtin/reset.c-        /* do_diff_cache() mangled the index */\n> builtin/reset.c:        discard_cache();\n> --\n> builtin/reset.c-                /* The index is still clobbered from do_diff_cache() */\n> builtin/reset.c:                discard_cache();\n>\n> -Peff\n"}]}