Jeff King <p...@peff.net> writes: > On Tue, Jan 07, 2014 at 11:38:15AM -0800, Junio C Hamano wrote: > >> >> > Alternatively, I guess "cat-file >> >> > --batch" could just turn off warn_ambiguous_refs itself. >> >> >> >> Sounds like a sensible way to go, perhaps on top of this change? >> > >> > The downside is that we would not warn about ambiguous refs anymore, >> > even if the user was expecting it to. I don't know if that matters much. >> >> That is true already with or without Brodie's change, isn't it? >> With warn_on_object_refname_ambiguity, "cat-file --batch" makes us >> ignore core.warnambigousrefs setting. If we redo 25fba78d >> (cat-file: disable object/refname ambiguity check for batch mode, >> 2013-07-12) to unconditionally disable warn_ambiguous_refs in >> "cat-file --batch" and get rid of warn_on_object_refname_ambiguity, >> the end result would be the same, no? > > No, I don't think the end effect is the same (or maybe we are not > talking about the same thing. :) ). > > There are two ambiguity situations: > > 1. Ambiguous non-fully-qualified refs (e.g., same tag and head name). > > 2. 40-hex sha1 object names which might also be unqualified ref names. > > Prior to 25ffba78d, cat-file checked both (like all the rest of git). > But checking (2) is very expensive,...
Ahh, of course. Sorry for forgetting about 1. > The two options I was musing over earlier today were (all on top of > Brodie's patch): > > a. Revert 25ffba78d. With Brodie's patch, core.warnAmbiguousRefs > disables _both_ warnings. So we default to safe-but-slow, but > people who care about performance can turn off ambiguity warnings. > The downside is that you have to know to turn it off manually (and > it's a global config flag, so you end up turning it off > _everywhere_, not just in big queries where it matters). Or "git -c core.warnambiguousrefs=false cat-file --batch", but I think a more important point is that it is no longer automatic for known-to-be-heavy operations, and I agree with you that it is a downside. > b. Revert 25ffba78d, but then on top of it just turn off > warn_ambiguous_refs unconditionally in "cat-file --batch-check". > The downside is that we drop the safety from (1). The upside is > that the code is a little simpler, as we drop the extra flag. > > And obviously: > > c. Just leave it at Brodie's patch with nothing else on top. > > My thinking in favor of (b) was basically "does anybody actually care > about ambiguous refs in this situation anyway?". If they do, then I > think (c) is my preferred choice. OK. I agree with that line of thinking. Let's take it one step at a time, i.e. do c. and also use warn_on_object_refname_ambiguity in "rev-list --stdin" first and leave the simplification (i.e. b.) for later. >> > I kind of feel in the --batch situation that it is somewhat useless (I >> > wonder if "rev-list --stdin" should turn it off, too). >> >> I think doing the same as "cat-file --batch" in "rev-list --stdin" >> makes sense. Both interfaces are designed to grok extended SHA-1s, >> and full 40-hex object names could be ambiguous and we are missing >> the warning for them. > > I'm not sure I understand what you are saying. We _do_ have the warning > for "rev-list --stdin" currently. We do _not_ have the warning for > "cat-file --batch", since my 25ffba78d. What I wanted to say was that we would be discarding the safety for "rev-list --stdin" with the same argument as we did for "cat-file --batch". If the argument for the earlier "cat-file --batch" were "this interface only takes raw 40-hex object names", then the situation would have been different, but that is not the case. > I was wondering if rev-list should go the same way as 25ffba78d, > for efficiency reasons (e.g., think piping to "rev-list --no-walk > --stdin"). Yes, and I was trying to agree with that, but apparently I failed ;-) -- 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
