Comments on Phillip's review

[Harald Nordgren <haraldnordgren@gmail.com>]

> 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)

I hope I understand correctly, the code becomes easier this way, so that's
nice!

> As I said last time we should not be calling apply_autostash() if we
> have not created an autostash. We should also not discard and re-read
> the index if we haven't stashed. I do think we'd be better restoring the
> stashed changes in a single place as I said above.

Makes sense.

> 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.

Add that message.

Do you mean to test the full output? I'm not against it at all, but that
seems to be going against the convention of the other tests in this file.
But it would be a more robust test.

> I'm not sure we need to say "local changes" twice here

👍

> 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?

Much cleaner, thanks!

> we already have tests for --conflict=diff3 and
> --conflict=merge I'm not sure this test adds much.

Deleted.

> > +
> > +     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.

Deleted.

> > +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.

Deleted.

> > +     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.

Deleted.

> I don't think the two tests above add any extra coverage when we have
> the one below so they can be deleted. Our test suite is slow enough
> already - we only need one test to fail for any given issue.

Deleted.

> > +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

Updated.

> 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.

Tricky to get right, the test if very good to have! I rewrote the logic now
to make this test pass, I hope it looks better now.


Harald