On Thu, Aug 30, 2012 at 09:33:48AM -0700, Junio C Hamano wrote:

> Jeff King <p...@peff.net> writes:
> > The script originally comes from here:
> >
> >   http://thread.gmane.org/gmane.comp.version-control.git/33566/focus=33852
> >
> > and the discussion implies that the AUTHOR_DATEs were added to avoid a
> > race condition with the timestamps. But why would that ever have worked?
> I do not see how AUTHOR_DATE would affect anything there, either,
> other than to give reprodusible object names.  The test only sets
> committer-date upfront, so without setting author-date, you would
> still get different object names on commits.  Which suggests me that
> there may be something that tiebreaks based on object names?

Hmm. I wouldn't think so. The order should come from timestamps, with
ties broken by order of insertion, which in turn comes from traversal,
which depends only on parent order. We do check some raw sha1s in the
expected output, but it is only for blobs, not commits.  I guess it
could be some weirdness inside merge-recursive, though, and
not part of the merge-base computation.

So I really don't know how AUTHOR_DATE would change anything. And
indeed, after removing them it still passes on my machine.

However, I think I may understand why it fails if you tweak the
committer dates.

When my unstable-queue was used, I noticed that the merge bases returned
were the same (as you would expect), but that they came in a different
order. Which makes sense, as the order of multiple bases would not
necessarily be deterministic (they do not have an ancestry relationship,
or they would not be merge bases).

So the issue is that when you do a recursive merge with multiple bases,
the order in which you visit the recursive bases is going to impact the
exact conflicts you see. In theory, after the merge is done, you're
going to be at the same state, but the conflicts you see along the way
will be different. And it is this conflicted state that the test is
looking at.

The current test expects a particular order of merge bases based on the
same-second commit timestamps. There is no race condition because of the
setting of GIT_COMMITTER_DATE at the beginning (and _that_ is the real
thing that fixed the race conditions Dscho saw in the thread above; the
AUTHOR_DATE was just a red herring).

So the test is not broken or racy, which is good. It is just testing
something that is somewhat of an implementation detail. We could switch
it to use test_tick, and then adjust the expected output to look for the
expected conflict that git happens to generate in that case. But that is
no better than the current behavior.

But I'm not sure there is a way to test what it wants to test (that we
hit a conflict that involves one of the recursive merge bases) without
relying on the implementation detail. So I'm inclined to just leave it
in place.

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to