Re: [PATCH] unpack-trees: do not fail reset because of unmerged skipped entry

3 messages, 2 authors, 2018-07-10 · open the first message on its own page

Re: [PATCH] unpack-trees: do not fail reset because of unmerged skipped entry

From: Junio C Hamano <hidden>
Date: 2018-06-15 19:58:46

Max Kirillov [off-list ref] writes:
After modify/delete merge conflict happens in a file skipped by sparse
checkout, "git reset --merge", which implements the "--abort" actions, and
"git reset --hard" fail with message "Entry * not uptodate. Cannot update
sparse checkout." The reason is that the entry is verified in
apply_sparse_checkout() for being up-to-date even when it has a conflict.
Checking conflicted entry for being up-to-date is not performed in other
cases. One obvious reason to not check it is that it is already modified
by inserting conflict marks.

Fix by not checking conflicted entries before performing reset.
Also, add test case which verifies the issue is fixed.
I do not know offhand if "reset --merge" should force succeeding in
such a case, but I agree that it is criminal to stop "reset --hard"
with "not uptodate", as the whole point of "hard reset" is to get
rid of the 'not up-to-date' modification.

I guess not may people make serious use of sparsely checked-out
working tree and that is why such a failure is reported this late
after the feature was introduced?
quoted hunk
+test_expect_success 'reset --hard works after the conflict' '
+	git reset --hard
+'
Do we want to verify the state after the 'hard' reset succeeds as
well?  Things like 

 - all paths in the HEAD and all paths in the index are identical;

 - paths that do exist in the working tree are all identical to HEAD
   version; and

 - paths that do not exist in the working tree are missing due to
   the sparse checkout setting (iow, it is a bug if a path that is
   outside the "sparse" setting is missing from the working tree).
quoted hunk
+test_expect_success 'setup: conflict back' '
+	! git merge theirs
+'
+
+test_expect_success 'Merge abort works after the conflict' '
+	git merge --abort
+'
Likewise here.

quoted hunk
+test_done
diff --git a/unpack-trees.c b/unpack-trees.c
index e73745051e..65ae0721a6 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -468,7 +468,7 @@ static int apply_sparse_checkout(struct index_state *istate,
 		 * also stat info may have lost after merged_entry() so calling
 		 * verify_uptodate() again may fail
 		 */
-		if (!(ce->ce_flags & CE_UPDATE) && verify_uptodate_sparse(ce, o))
+		if (!(ce->ce_flags & CE_UPDATE) && !(ce->ce_flags & CE_CONFLICTED) && verify_uptodate_sparse(ce, o))
 			return -1;
 		ce->ce_flags |= CE_WT_REMOVE;
 		ce->ce_flags &= ~CE_UPDATE;
Thanks.

Re: [PATCH] unpack-trees: do not fail reset because of unmerged skipped entry

From: Max Kirillov <hidden>
Date: 2018-06-16 08:22:49

I do not know offhand if "reset --merge" should force succeeding in
such a case, but I agree that it is criminal to stop "reset --hard"
with "not uptodate", as the whole point of "hard reset" is to get
rid of the 'not up-to-date' modification.

I originally had a fix just for "reset --hard". It was in
verify_uptodate_1(), to move check for "->reset" earlier. But then I
found that "merge --abort" does not use "reset --hard", but rather
--merge, so I fixed that. Because --merge should work also, shouldn't
it?

Actually, I think that fix in verify_uptodate_1() was right, I just
did not find what it affects, after the other fix

Re: [PATCH] unpack-trees: do not fail reset because of unmerged skipped entry

From: Max Kirillov <hidden>
Date: 2018-07-10 19:23:15

On Fri, Jun 15, 2018 at 12:58:40PM -0700, Junio C Hamano wrote:
Do we want to verify the state after the 'hard' reset succeeds as
well?  Things like 

 - all paths in the HEAD and all paths in the index are identical;

 - paths that do exist in the working tree are all identical to HEAD
   version; and

 - paths that do not exist in the working tree are missing due to
   the sparse checkout setting (iow, it is a bug if a path that is
   outside the "sparse" setting is missing from the working tree).
I implemented the additional check, it is a bit different
literally, but should be equivalent for this case
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help