On Fri, Feb 10, 2012 at 07:06:57PM -0800, Junio C Hamano wrote: >Tom Grennan <tmgrennan@xxxxxxxxx> writes: > >>>If we pursue this, it may be best to first add match_patterns() to ./refs.[ch] >>>then incrementally modify these builtin commands to use it. >> >> The following series implements !<pattern> with: git-tag, git-branch, and >> git-for-each-ref. >> >> This still requires Documentation and unit test updates but I think these are >> close to functionally complete. >> >>>>About the '!' for exclusion, maybe it's better to move from fnmatch() >>>>as matching machinery to pathspec. Then when git learns negative >>>>pathspec [1], we have this feature for free. >>>> >>>>[1] http://thread.gmane.org/gmane.comp.version-control.git/189645/focus=190072 >> >> After looking at this some more, I don't understand the value of replacing >> libc:fnmatch(). Or are you just referring to '--exclude' instead of >> [!]<pattern> argument parsing? > >I have not formed a firm opinion on Nguyen's idea to reuse pathspec >matching infrastructure for this purpose, so I wouldn't comment on that >part. It certainly looks attractive, as it allows users to learn one and >only one extended matching syntax, but at the same time, it has a risk to >mislead people to think that the namespace for refs is similar to that of >the filesystem paths, which I see as a mild downside. > >In any case, I do not like the structure of this series. If it followed >our usual pattern, it would consist of patches in this order: > > - Patch 1 would extract match_pattern() from builtin/tag.c and introduce > the new helper function refname_match_patterns() to refs.c. It updates > the call sites of match_pattern() in builtin/tag.c, match_patterns() in > builtin/branch.c, and the implementation of grab_single_ref() in > builtin/for-each-ref.c with a call to the new helper function. > > This step can and probably should be done as three sub-steps. 1a would > move builtin/tag.c::match_pattern() to refs.::refname_match_patterns(), > 1b would use the new helper in builtin/branch.c and 1c would do the > same for builtin/for-each-ref.c. > > It is important that this patch does so without introducing any new > functionality to the new function over the old one. When done this way, > there is no risk of introducing new bugs at 1a because it is purely a > code movement and renaming; 1b could introduce a bug that changes > semantics for bulitin/branch.c if its match_patterns() does things > differently from match_pattern() lifted from builtin/tag.c, and if it > is found out to be buggy, we can discard 1b without discarding 1a. Same > for 1c, which I highly suspect will introduce regression without > looking at the code (for-each-ref is prefix-match only), that can > safely be discarded. > > This is to make it easier to ensure that the update does not introduce > new bugs. > > - Patch 2 would then add the new functionality to the new helper. It > would also adjust the documentation of the three end user facing > commands to describe the fallout coming from this change, and adds new > tests to make sure future changes will not break this new > functionality. > >That is, first refactor and clean-up without adding anything new, and then >build new stuff on solidified ground. Nuts! I have the cart before the horse. I'll try to rearrange the series as suggested by tomorrow. Thanks. >Do we allow a refname whose pathname component begins with '!', by the >way? If we do, how does a user look for a tag whose name is "!xyzzy"? >"Naming your tag !xyzzy used to be allowed but it is now forbidden after >this patch" is not an acceptable answer---it is called a regression. If >the negation operator were "^" or something that we explicitly forbid from >a refname, we wouldn't have such a problem. Cool, as I recall, v7 or earlier /bin/sh also used "^" to preface exclusion patterns. Another option is the bash extglob syntax of !(pattern) although I'd prefer "^" b/c one wouldn't have to quote it with bash: $ git branch --list ^pu -- TomG -- 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