[pacman-dev] [PATCH v2] Enable inverted patterns in NoExtract and NoUpgrade.

[Andrew Gregory]

On 06/18/13 at 04:51pm, Patrick Steinhardt wrote:
> First thanks for your feedback.
> 
> On Tue, Jun 18, 2013 at 09:22:54AM -0400, Andrew Gregory wrote:
> > On 06/18/13 at 09:14am, Patrick Steinhardt wrote:
> > > It is now possible to invert patterns in NoExtract and NoUpgrade.
> > > This feature allows users to whitelist certain files that were
> > > previously blacklisted by another entry.
> > > ---
> > >  doc/pacman.conf.5.txt | 10 ++++++++--
> > >  lib/libalpm/add.c     |  4 ++--
> > >  lib/libalpm/util.c    | 32 ++++++++++++++++++++++++++++++++
> > >  lib/libalpm/util.h    |  1 +
> > >  4 files changed, 43 insertions(+), 4 deletions(-)
> > > 
> > 
> > Looks good.  Fairly minor comments below.
> > 
> > > diff --git a/doc/pacman.conf.5.txt b/doc/pacman.conf.5.txt
> > > index 83a399e..e87f4c2 100644
> > > --- a/doc/pacman.conf.5.txt
> > > +++ b/doc/pacman.conf.5.txt
> > > @@ -128,7 +128,10 @@ Options
> > >  	'.pacnew' extension.
> > >  	These files refer to files in the package archive, so do not include the
> > >  	leading slash (the RootDir) when specifying them. Shell-style glob patterns
> > > -	are allowed.
> > > +	are allowed. It is possible to invert matches by prepending a file with
> > > +	an exclamation mark. Inverted files will result in previously blacklisted
> > > +	files to be whitelisted again. Subsequent matches will override previous
> > > +	ones.
> > 
> > s/to be/being/
> > 
> > Need to mention that a leading literal ! or \ needs to be escaped.
> 
> I'll change those.
> 
> > 
> > >  
> > >  *NoExtract =* file ...::
> > >  	All files listed with a `NoExtract` directive will never be extracted from
> > > @@ -138,7 +141,10 @@ Options
> > >  	from the 'apache' package.
> > >  	These files refer to files in the package archive, so do not include the
> > >  	leading slash (the RootDir) when specifying them. Shell-style glob patterns
> > > -	are allowed.
> > > +	are allowed. It is possible to invert matches by prepending a file with
> > > +	an exclamation mark. Inverted files will result in previously blacklisted
> > > +	files to be whitelisted again. Subsequent matches will override previous
> > > +	ones.
> > 
> > Same.
> > 
> > >  
> > >  *CleanMethod =* KeepInstalled &| KeepCurrent::
> > >  	If set to `KeepInstalled` (the default), the '-Sc' operation will clean
> > > diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c
> > > index c20e7c6..6e12f56 100644
> > > --- a/lib/libalpm/add.c
> > > +++ b/lib/libalpm/add.c
> > > @@ -183,7 +183,7 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive,
> > >  	}
> > >  
> > >  	/* if a file is in NoExtract then we never extract it */
> > > -	if(alpm_list_find(handle->noextract, entryname, _alpm_fnmatch)) {
> > > +	if(_alpm_fnmatch_patterns(handle->noextract, entryname) == 0) {
> > >  		_alpm_log(handle, ALPM_LOG_DEBUG, "%s is in NoExtract,"
> > >  				" skipping extraction of %s\n",
> > >  				entryname, filename);
> > > @@ -245,7 +245,7 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive,
> > >  		} else {
> > >  			/* case 3: */
> > >  			/* if file is in NoUpgrade, don't touch it */
> > > -			if(alpm_list_find(handle->noupgrade, entryname, _alpm_fnmatch)) {
> > > +			if(_alpm_fnmatch_patterns(handle->noupgrade, entryname) == 0) {
> > >  				notouch = 1;
> > >  			} else {
> > >  				alpm_backup_t *backup;
> > > diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c
> > > index f84fb66..1382889 100644
> > > --- a/lib/libalpm/util.c
> > > +++ b/lib/libalpm/util.c
> > > @@ -1249,6 +1249,38 @@ int _alpm_access(alpm_handle_t *handle, const char *dir, const char *file, int a
> > >  	return ret;
> > >  }
> > >  
> > > +/** Checks whether a string matches at least one shell wildcard pattern.
> > > + * Checks for matches with fnmatch. Matches are inverted by prepending
> > > + * patterns with an exclamation mark. Preceding exclamation marks may be
> > > + * escaped. Subsequent matches override previous ones.
> > > + * @param patterns patterns to match against
> > > + * @param string string to check against pattern
> > > + * @return 0 if string matches pattern, negative if they don't match and
> > > + * positive if the last match was inverted
> > > + */
> > > +int _alpm_fnmatch_patterns(alpm_list_t *patterns, const char *string)
> > > +{
> > > +	int result = -1;
> > > +	alpm_list_t *i;
> > > +	char *pattern;
> > > +	short inverted;
> > 
> > pattern and inverted should be moved to inside the loop.
> 
> I'd normally agree with that, but I adapted to the code style in
> util.c where variables used only inside a loop are still declared
> outside of it.
> 
> > 
> > > +
> > > +	for(i = patterns; i; i = i->next) {
> > > +		pattern = i->data;
> > > +
> > > +		inverted = pattern[0] == '!';
> > > +		if(inverted || pattern[0] == '\\') {
> > > +			pattern++;
> > > +		}
> > > +
> > > +		if(_alpm_fnmatch(pattern, string) == 0) {
> > > +			result = inverted;
> > 
> > If we just loop through the list backwards (or reverse the list) we
> > could return after the first match instead of saving the result and
> > continuing the loop.
> 
> Indeed, you're right with that one. Even though I didn't think
> about it and it certainly is a good optimisation I don't want to
> include it as there are some problems with that.
> 
> First option would be to reverse the list. It might either be
> reversed on NoExtract parsing, resulting in conventions that may
> introduce bugs more easily, or on each call of
> _alpm_fnmatch_patterns which would certainly kill all benefit by
> reverse-iterating.
> 
> Second option is using the prev links of alpm_list, zut by
> default, they've got an infinite loop when using the prev links.
> alpm_list_reverse handles this by backing up head's prev and
> restoring it later. But that's not a nice solution which should
> be used here, I guess.
> 
> So if I didn't forget an easier method I'm against that
> optimisation. I guess it won't matter much anyway, as NoExtract
> or NoUpgrade lists likely will not be that huge anyway.
> 

alpm_list_previous will stop at the beginning of the list.

> > 
> > > +		}
> > > +	}
> > > +
> > > +	return result;
> > > +}
> > > +
> > >  /** Checks whether a string matches a shell wildcard pattern.
> > >   * Wrapper around fnmatch.
> > >   * @param pattern pattern to match against
> > > diff --git a/lib/libalpm/util.h b/lib/libalpm/util.h
> > > index 56031f3..24b7c22 100644
> > > --- a/lib/libalpm/util.h
> > > +++ b/lib/libalpm/util.h
> > > @@ -140,6 +140,7 @@ alpm_time_t _alpm_parsedate(const char *line);
> > >  int _alpm_raw_cmp(const char *first, const char *second);
> > >  int _alpm_raw_ncmp(const char *first, const char *second, size_t max);
> > >  int _alpm_access(alpm_handle_t *handle, const char *dir, const char *file, int amode);
> > > +int _alpm_fnmatch_patterns(alpm_list_t *patterns, const char *string);
> > >  int _alpm_fnmatch(const void *pattern, const void *string);
> > >  
> > >  #ifndef HAVE_STRSEP
> > > -- 
> > > 1.8.3.1
> > > 
> > 
> > apg
> >