*Re: [PATCH] unpack-trees: do not fail reset because of unmerged skipped entry
2018-06-15 4:42 [PATCH] unpack-trees: do not fail reset because of unmerged skipped entry Max Kirillov
@ 2018-06-15 19:58 ` Junio C Hamano
2018-06-16 8:22 ` Max Kirillov
2018-07-10 19:23 ` Max Kirillov
2018-06-16 5:14 ` Duy Nguyen
2018-07-10 19:17 ` [PATCH v2] " Max Kirillov
2 siblings, 2 replies; 9+ messages in thread
From: Junio C Hamano @ 2018-06-15 19:58 UTC (permalink / raw)
To: Max Kirillov; +Cc: git, Nguyễn Thái Ngọc Duy
Max Kirillov <max@max630.net> 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?
> +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).
> +test_expect_success 'setup: conflict back' '
> + ! git merge theirs
> +'
> +
> +test_expect_success 'Merge abort works after the conflict' '
> + git merge --abort
> +'
Likewise here.
> +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.
^permalinkrawreply [flat|nested] 9+ messages in thread

*Re: [PATCH] unpack-trees: do not fail reset because of unmerged skipped entry
2018-06-15 19:58 ` Junio C Hamano@ 2018-06-16 8:22 ` Max Kirillov
2018-07-10 19:23 ` Max Kirillov1 sibling, 0 replies; 9+ messages in thread
From: Max Kirillov @ 2018-06-16 8:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Nguyễn Thái Ngọc Duy
> 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
^permalinkrawreply [flat|nested] 9+ messages in thread

*Re: [PATCH] unpack-trees: do not fail reset because of unmerged skipped entry
2018-06-15 19:58 ` Junio C Hamano
2018-06-16 8:22 ` Max Kirillov@ 2018-07-10 19:23 ` Max Kirillov1 sibling, 0 replies; 9+ messages in thread
From: Max Kirillov @ 2018-07-10 19:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Max Kirillov, git, Nguyễn Thái Ngọc Duy
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
^permalinkrawreply [flat|nested] 9+ messages in thread