commit 0761fd5ce4a7a34fa5b776c0bcfdc3b6e18d8697
author Dave Borowitz <dborowitz@google.com> Sun Dec 13 21:59:04 2015 -0500
committer Dave Borowitz <dborowitz@google.com> Thu Dec 17 13:25:02 2015 -0500
tree 40f84b97eac6a5d632a6390f0fad5e682b603058
parent 6ba79765808c1c106d141b98eacf6ef287e666e6

MergeOp: Disallow multiple submit types on a single branch

Submit type rules allow different changes to the same branch to have
different submit types. A main use case for this functionality is
allowing branch-specific rather than project-specific submit types.

However, in theory this also allows a single batch of commits to mix
submit types. MergeOp currently handles this situation by splitting
up open changes on a branch by submit type and running submit types
on each subset in arbitrary order (based on HashMap iteration).

My thesis is that this behavior produces nondeterministic results
that, even if we can justify them as "sane", are likely to be
surprising and/or confusing to the user, and that we are better off
failing fast rather than trying to support this scenario.

In the past, there was a distinct reason for this behavior, which was
that there might be (through no fault of a user) changes in the
submitted state with various submit types. Spinning through the submit
type list and making progress, while perhaps confusing, was probably
better than not making progress at all. But now that the submitted
state is gone, the only way in which multiple changes can be submitted
at once is within a single batch (including parents or by topic), so
this reasoning does not really exist any more.

For one example of confusing behavior, say we have two changes A<-B
based on the branch tip 0, where A is Merge Always and B is Merge If
Necessary. If MergeOp chooses to run Merge Always first, the resulting
history will be:

 0----Ma--Mab
  \-A-/   /
     \-B-/

If, however, MergeOp chooses to run Merge If Necessary first, the
merge sorter will choose the fast-forward resolution for B, resulting
in:

 0--A--B

When Merge Always runs, it will find that A is already merged and do
nothing.

For another example, consider three changes A<-B<-C, where A and C
are Cherry-Pick and B is Merge If Necessary. If MergeOp chooses to
run Cherry-Pick first, it cherry-picks A' and C':

 0--A'--C'

Then merging B fails since it now depends on an out-of-date patch set
of A.

If MergeOp chooses Merge If Necessary first, then B gets chosen as a
fast-forward and C gets cherry-picked on top:

 0--A--B--C'

It is not at all obvious that any one of these solutions is what the
user expects to get, to say nothing of more complicated cases.

Note that I am only about 75% sure of what actually happens in these
scenarios; I might be completely wrong. That just goes to show how
weird this behavior is.

Enforce during validateChangeList that only a single submit type is
present on each branch. This also eliminates one level of looping in
the main integrateIntoHistory logic.

Another possible solution would be in the case of mixed submit types
to run the entire process one change at a time in topological order.
This at least might be easier to reason about, although it would still
not always succeed, for example if a Merge Always change follows a
Cherry-Pick change. But it would introduce considerably more
complexity to rework the loops in MergeOp, all for the questionable
benefit of making it easier for users to get into a confused
situation. Better to just not let them do it at all.

Change-Id: I0cec2a7e3e3625fedbdd621b0c6eca6c4100f232