Hi Dscho,
On Thu, Jul 25, 2019 at 12:41 PM Johannes Schindelin
<Johannes.Schindelin@xxxxxx> wrote:
>
> Hi Elijah,
>
> On Thu, 25 Jul 2019, Elijah Newren wrote:
>
<snip>
> > ...And it was fixed again in commit
> > 160252f81626 ("git-merge-ours: make sure our index matches HEAD", 2005-11-03)
> > ...and it was fixed again in commit
> > 3ec62ad9ffba ("merge-octopus: abort if index does not match HEAD", 2016-04-09)
> > ...and again in commit
> > 65170c07d466 ("merge-recursive: avoid incorporating uncommitted changes in a merge", 2017-12-21)
> > ...and again in commit
> > eddd1a411d93 ("merge-recursive: enforce rule that index matches head before merging", 2018-06-30)
> >
> > ...with multiple testcases added to the testsuite that could be
> > enumerated in even more commits.
> >
> > Then, finally, in a patch in the same series as the last fix above, the
> > documentation about this requirement was fixed in commit 55f39cf7551b
> > ("merge: fix misleading pre-merge check documentation", 2018-06-30), and
> > we all lived happily ever after...
> >
> > </quick summary>
>
> Whoa. What a story.
I know, right?
> > diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
> > index 5b910e351e..a4bfd8fc51 100644
> > --- a/builtin/merge-recursive.c
> > +++ b/builtin/merge-recursive.c
> > @@ -1,3 +1,4 @@
> > +#include "cache.h"
> > #include "builtin.h"
> > #include "commit.h"
> > #include "tag.h"
> > @@ -63,6 +64,9 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
> > if (argc - i != 3) /* "--" "<head>" "<remote>" */
> > die(_("not handling anything other than two heads merge."));
> >
> > + if (repo_read_index_unmerged(the_repository))
> > + die_resolve_conflict("merge");
>
> For a moment I was unsure whether `_unmerged()` is the right thing to do
> here, as it specifically allows to read the index even when there are
> conflict stages. But I guess it does not matter too much here. I
> probably would have opted for `repo_read_index()` instead, though.
The names repo_read_index() and repo_read_index_unmerged() actually
seem slightly misleading to me; they seem to do the opposite of what
you'd think they do.
repo_read_index() reads in an index and allows unmerged entries and
returns istate->cache_nr.
repo_read_index_unmerged() calls repo_read_index(), then checks to see
if any of the entries are unmerged and returns whether or not any
unmerged entries were found.
So, the way to disallow conflict stages isn't to use
repo_read_index(), but to use repo_read_index_unmerged(), as I did.
Counter-intuitive, I know.
<snip>
> But of course, if there are uncommitted changes, this would write a tree
> different from HEAD, then reset the index to match HEAD, so indeed, this
> discard/read dance is necessary.
>
> So this hunk is good.
>
<snip>
>
> This is obviously a good change: it strengthens the test case by fixing
> a subtle bug.
>
> Thanks,
> Dscho
Thanks for taking a look!
