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. 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. -- 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