On Tue, Jul 03, 2012 at 11:20:10AM -0700, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> writes: > > > However, if you feed a partial sha1 for which there are > > multiple matches, none of which satisfy the disambiguation hint, then we > > used to say "short SHA1 is ambiguous", and now we don't. > > In finish_object_disambiguation(), if the candidate hasn't been > checked, there are two cases: > > - It is the first and only object that match the prefix; or > - It replaced another object that matched the prefix but that > object did not satisfy ds->fn() callback. > > And the former case we set ds->candidate_ok to true without doing > anything else, while for the latter we check the candidate, which > may set ds->candidate_ok to false. Yeah, I think this is right. > At this point in the code, ds->candidate_ok can be false only if > this last-round check found that the candidate does not pass the > check, because the state after update_candidates() returns cannot > satisfy > > !ds->ambiguous && ds->candidate_exists && ds->candidate_checked > > and !ds->canidate_ok at the same time. > > Hence, when we execute this "return", we know we have seen more than > one object that match the prefix (and none of them satisfied ds->fn), > meaning that we should say "the short name is ambiguous", not "there > is no object that matches the prefix". Right. Per the comment just above your change, we are explicitly "ok" whether or not we meet the criteria in the hint function if we have seen only one. Which means the function is entirely about _disambiguating_. It is never about filtering. Which is a good thing, because it leaves the semantics of get_sha1 otherwise intact. > Please sanity check; I think it is just this one-liner, but I am > having hard time convincing myself that it can be that simple. It looks right to me (and certainly fixes the behavior I mentioned). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html