Ben Peart <peartben@xxxxxxxxx> writes: [jc: it was a bit surprising that Eric covered all the bits I covered while we were writing without peeking each other's ;-)] >> This long list of special-case checks doesn't leave me too enthused, >> however, that aside, this approach seems backward. Rather than erring >> on the side of safety by falling back to the merging behavior, it errs >> in the other direction, ... > > I'm not thrilled with the long list either (the plethora of comments > probably makes it appear worse than it is) but I don't see how... If there were a simple and futureproof way to tell the option parsing loop to notice any feature other than "-b newbranch" was used, then such a whitelisting may be a viable way, but there is not, and the whitelist condition can become (over time---we are talking about futureproofing and not "a filter that happens to match today's feature set") just as complex as this blacklisting function is (e.g. feature A and feature B when used alone may be compatible with the optimization but not when used both), so if we were to use this optimization, I think this long list of special-case checks is the best we could do. >> if (!skip_worktree_merge(...)) >> ret = merge_working_tree(...); >> > > I personally agree, it was moved to its current location per feedback > the first time around. Perhaps with the addition of the config > setting it will be better received moved out to the caller. Sounds sensible. I am still not enthused by the configuration variable that is underdocumented. You already said that we know that this optimization does a wrong thing when sparse checkout is used---any other cases? I do not think of any myself, and if that is true, I am wondering if it makes more sense to "do we have sparse configuration?" as part of the blacklist condition. That way, the users do not have to set anything and they will get an optimization benefit from an obvious change to skip "read-tree -m HEAD HEAD" that ought to be a no-op. Thanks.
