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

Jeff King [off-list ref] writes:

On Tue, Jan 01, 2013 at 02:24:46PM -0800, Junio C Hamano wrote:

quoted

Jeff King [off-list ref] writes:

quoted

So I think we need to update twoway_merge to recognize unmerged entries,
which gives us two options:

  1. Reject the merge.

  2. Throw away the current unmerged entry in favor of the "new" entry
     (when old and new are the same, of course; otherwise we would
     reject).

I think (2) is the right thing.

As "--reset" in "read-tree --reset -u A B" is a way to say "we know
we are based on A and we want to go to B no matter what", I agree we
should not reject the merge.

With -m instead of --reset, I am not sure what the right semantics
should be, though.

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.

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

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

quoted

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.

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