Re: [BUG] two-way read-tree can write null sha1s into index

From: Jeff King <peff@peff.net>
Date: Jan 3, 2013, 20:23 UTC
Message-ID: <20130103202343.GA4632@sigill.intra.peff.net>
In-Reply-To: <7vehi2xote.fsf@alter.siamese.dyndns.org>

On Thu, Jan 03, 2013 at 07:34:53AM -0800, Junio C Hamano wrote:

Show 15 quoted lines

> > Good point; I was just thinking about the --reset case.
> >
> > With "-m", though, we could in theory carry over the unmerged entries
> > (again, assuming that "old" and "new" are the same; otherwise it is an
> > obvious reject). But those entries would be confused with any new
> > unmerged entries we create. It seems we already protect against this,
> > though: "read-tree -m" will not run at all if you have unmerged entries.
> >
> > Likewise, "checkout" seems to have similar protections.
> >
> > So I think it may be a non-issue.
> 
> Yeah.  Also earlier in the thread you mentioned three-way case, but
> I do not think we ever would want --reset with three trees, so I
> think that too is a non-issue for the same reason.

Yeah, agreed; we should always reject in the three-way case. I would worry more that it has a bug where something is _not_ rejected, and we end up putting a bogus null sha1 entry into the index (which is the actual problem with twoway_merge). IOW, if we have the bogus sha1 in the index (because we marked it with CE_CONFLICTED), and the two sides and the common ancestor are all the same, would we blindly carry through the bogus conflicted entry (which we would prefer, because it has the up-to-date stat information)?

Or are you suggesting that the three-way case should always be protected by checking that there are no unmerged entries before we start it? That seems sane to me, but I haven't confirmed that that is the case.

Show 10 quoted lines

> I would still feel safer if we expressed the expectation of
> the callee in the code, perhaps like this in the two-way case:
> 
> 	if (current->ce_flags & CE_CONFLICTED) {
>         	if (!o->reset) {
>                 	... either die or fail ...
> 		} else {
>                 	... your fix ...
> 		}
> 	}

Agreed. I looked at that, but it seemed like it was going to involve repeating a lot of the "are the two trees the same" logic. Let me see if I can refactor it to avoid that.

-Peff