On Thu, 20 Mar 2008, Junio C Hamano wrote: > Daniel Barkalow <barkalow@xxxxxxxxxxxx> writes: > > > So we should call the same code to parse the string, have that code do no > > validation at all, and have the caller validate the return as appropriate. > > The parsing doesn't depend at all on whether it's for fetching or pushing. > > Obviously you did not read my patch before responding. > > While I would agree that removing the check from parsing and add necessary > checks to all callers would be another way to catch the same kind of > breakages, > > (1) it would be more code to change, and I do not see a clear advantage, > in that approach, especially given the discussion that lead to > $gmane/77413; Surely it's no more code to change all of the callers of parse_ref_spec to also call a function to validate it as appropriate than it is to change all of the callers of parse_ref_spec to call a different function instead. > (2) the error reporting by the callers that use the parsed result will > not be able to say "Invalid refspec '%s'" on the source string, > simply because the source string is not available to them anymore; That's easy enough to keep, and likely even worthwhile; it would probably be nice, for example, to have "git fetch origin typo/*:something/*" give an error when the pattern doesn't match anything, and that error would need the refspec string. > (3) worse yet, the older code even discarded part of the user input, so > the error reporting that uses the parsed src/dst pair may not even be > similar to the problematic input the user gave us to begin with, > making it harder for the user to spot what we did not like and > correct it. That was a bug that was fixed in the intermediate version. > In any case, don't you agree that the patch you responded to is much > easier to understand what we are (and we are not) checking than the > original code? No, I think it's much more complicated. It mixes the semantics of what an empty side means for a particular use of refspecs into the parsing of the string. At the very least, the checks should be outside of _internal() in the functions for particular uses. -Daniel *This .sig left intentionally blank* -- 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
