On Sat, Feb 11, 2012 at 08:12:54AM +0100, Michael Haggerty wrote: >On 02/11/2012 03:16 AM, Tom Grennan wrote: >> diff --git a/refs.h b/refs.h >> index 00ba1e2..13015ba 100644 >> --- a/refs.h >> +++ b/refs.h >> @@ -152,4 +152,12 @@ int update_ref(const char *action, const char *refname, >> const unsigned char *sha1, const unsigned char *oldval, >> int flags, enum action_on_err onerr); >> >> +/** >> + * Returns: >> + * 1 with NULL patterns >> + * 0 if refname fnmatch()es any ! prefaced pattern >> + * 1 if refname fnmatch()es any pattern >> + */ >> +extern int refname_match_patterns(const char **patterns, const char *refname); >> + >> #endif /* REFS_H */ > >This comment is unclear and incomplete. > >1. What does "NULL patterns" mean? Your code fails if patterns==NULL, >so I guess you mean "1 if there are no patterns in the list". > >2. Since the three conditions are not mutually exclusive, you should say >how they are connected. I believe that you want something like "A >otherwise B otherwise C". > >3. You haven't specified what happens if refname matches neither a >!-prefixed pattern nor a non-!-prefixed pattern. Does this behavior >depend on which types of patterns were present in the list? > >I see that you have described the behavior more completely in the commit >message for patch 2/4, but the commit message is not enough: this >behavior should be described precisely in both code comments (when the >function is defined) and in the user documentation (when the >functionality is added to a command). Yes, I didn't explicitly state that the precedence is the order written and in correctly described the first case. How about? /** * Returns in highest to lowest precedence: * 1 with an empty patterns list * 0 if refname fnmatch()es any ^ prefaced pattern * 1 if refname fnmatch()es any other pattern * 0 otherwise */ Thanks, 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