Re: [RFC/PATCH] tag: make list exclude !<pattern>

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 11, 2012, 03:06 UTC
Message-ID: <7vaa4qnk4u.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1328926618-17167-1-git-send-email-tmgrennan@gmail.com>

Tom Grennan <tmgrennan@gmail.com> writes:

Show 18 quoted lines

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