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

Junio C Hamano <gitster@pobox.com> writes:

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.