On Fri, Feb 10, 2012 at 11:50:06PM -0800, Junio C Hamano wrote: >Junio C Hamano <gitster@xxxxxxxxx> writes: > >> ... Same >> for 1c, which I highly suspect will introduce regression without >> looking at the code (for-each-ref is prefix-match only), ... OK I'll study this further and run through t6300-for-each-ref.sh I see it has a bunch of errors. I think there are similar issues using match_pattern() with show-branch and ls-remote. Thanks, >This part needs correction. for-each-ref matches the command line >arguments differently from branch --list and tag --list in two important >ways. > > (1) It allows (not "only" which was a mistake in my earlier message) > prefix matching, e.g. "for-each-ref refs/heads/", in addition to > fnmatch(); and > > (2) The fnmatch() call is made with FNM_PATHMAME, which "branch --list" > and "tag --list" does not use. > >Strictly speaking, therefore, if you make all three commands to use the >same matching logic, there is no way to avoid regression. If you choose >to use fnmatch() without FNM_PATHNAME, then for-each-ref suddenly starts >matching wildcards across name hierarchy boundary '/' for a pattern that >does not match today, e.g. "git for-each-ref 'refs/heads/*'" was a good >way to grab only the integration branches while excluding individual topic >branches such as refs/heads/tg/tag-points-at, but this technique can no >longer be used for such a purpose, which is an unpleasant regression. > >I personally think that it was an annoying UI mistake that we let branch >and tag call fnmatch without FNM_PATHNAME, but we cannot fix it lightly, >either. People who use hierchical branch names (e.g. maint-1.0/$topic, >maint-2.0/$topic, and feature-2.0/$topic) may already be used to list all >the topics on the maintenance tracks with "branch --list 'maint*'", and we >need to keep "branch --list" and "tag --list" working as they expect. > >One possible way forward (now I am talking about a longer term solution) >would be to introduce > > refname_match_pattern(const char *refname, > const char **pattern, > unsigned flags); > >where flags can tell the implementation if FNM_PATHNAME should be used, >and if prefix matching should be attempted, so that the three commands >share the single same matching function while still retaining their >current behaviour in the initial round. Inside the implementation, we >would use good old fnmatch(), with or without FNM_PATHNAME, depending on >the flags the caller passes. > >In a future versions, we may want to have "branch/tag --list" also ask for >FNM_PATHNAME (this *is* a backward incompatible change, so it needs to be >performed across major version boundary, with backward compatibility >configurations, deprecation warnings and whole nine yards). Under the new >match function, today's "branch --list 'maint*'" needs to be spelled as >"branch --list 'maint*/*'" or something. The prefix matching is probably >safer to enable by default without causing big regression hassle if we >limit the prefix match to only patterns that end with an explicit slash, >as users already *know* today's "branch --list tg/" would not match >anything (because the pattern does not even match a brahch 'tg', so it is >unlikely they are using it and expecting only 'tg' to match), which means >that is an unlikely input we can safely give new meaning to match anything >under tg/ hierarchy. > -- 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