Comments on Phillip's review

> ret is -1 so we return the same value if unpack_trees() fails as do the
> checks at the top of the function do when they fail with "return
> error(...)". Therefore we cannot determine whether a failure of this
> function is due to unpack_trees() or not and so we wont know whether to
> autostash or not. You need to return a unique value here like -2 (or
> ideally a named constant)

> Good - now we only try to restore the stashed changes if we actually
> stashed. However we only restore the stashed changes if there was an
> error(). If there isn't an error we call update_refs_for_switch() before
> restoring them. It would be safer to restore them straight away in case
> that function ends up dying for any reason (though I think that's pretty
> unlikely)

> where the changes appear to be part of the advice message. Perhaps we
> should print a short (i.e. one sentance) message along the lines of
> 
>         The following paths have local changes
> 
> We should test what the user sees here as well.

> I find the bulleted list a bit odd, maybe
> 
>         You can either resolve the conflicts and then discard the stash
>         with "git stash drop", or, if you do not want to resolve them
>         now, run "git reset --hard" and apply the local changes later by
>         running "git stash pop"
> 
> would be better?

> > +
> > +     cat <<-EOF >expect &&
> > +     a
> > +     <<<<<<< simple
> > +     c
> > +     ||||||| main
> > +     b
> > +     c
> > +     d
> > +     =======
> > +     b
> > +     X
> > +     d
> > +     >>>>>>> local
> > +     e
> > +     EOF
> > +     test_cmp expect two
> > +'
> > +
> > +test_expect_success 'checkout -m respects merge.conflictStyle config' '
> 
> Looking at the existing tests, 'checkout with --merge, in diff3 -m
> style' and 'checkout --conflict=merge, overriding config' already test
> that we respect merge.conflictStyle and that --conflict overrides it so
> I don't see what new coverage this test adds.

> > +test_expect_success 'checkout -m skips stash when no conflict' '
> > +     git checkout -f main &&
> > +     git clean -f &&
> > +
> > +     fill 0 x y z >same &&
> > +     git stash list >stash-before &&
> > +     git checkout -m side >actual 2>&1 &&
> 
> file "same" is unchanged between branch "side" and "branch" main so we
> do not need to stash it.

> > +     test_grep ! "Created autostash" actual &&
> > +     git stash list >stash-after &&
> > +     test_cmp stash-before stash-after &&
> > +     fill 0 x y z >expect &&
> > +     test_cmp expect same
> 
> Even if we created an autostash this test would not pick it up as the
> stash is not written to refs/stash unless there are merge conflicts and
> we don't print "Created autostash" even when we do create an autostash.
> The same is true for "checkout -m -b skips stash with dirty tree" below.
> I don't see how we can check that a stash was not created without using
> GIT_TRACE to see if we run "git stash". Even that is fragile as we might
> start stashing without forking a separate process in future.

> > +test_expect_success 'checkout -m stashes on truly conflicting changes' '
> 
> This use of conflicting is rather confusing - what's the difference
> between a conflicting change and a truly conflicting change?
> 
> I think a single test is sufficient to check that we create a valid
> stash entry

> test_expect_success 'checkout -m which would overwrite untracked file' '
>         git checkout -f --detach main &&
>         test_commit another-file &&
>         git checkout HEAD^ &&
>         >another-file.t &&
>         test_must_fail git checkout -m @{-1} 2>err &&
>         test_grep "another-file.t.*overwritten" err
> '
> 
> which passes on master but fails with these patches applied. We need to
> make sure that we don't set "quiet" in unpack_tree_opts the second time
> we call merge_working_tree(). The test could be improved by adding some
> local changes.