[Bug default/19082] New: Make abipkgdiff recognize suppression specifications to apply to comparisons

[Dodji Seketeli]

Hi!

Thank you for the nice patch, it's really appreciated!

Please see my comments below.

> --- a/tools/abipkgdiff.cc

[...]

> +static vector<string> suppression_paths;

Rather than putting this here, I'd rather declare a (global) variable of
type (called, e.g, prog_options) pointer to the options type, right
before the main() and initialize that pointer in main(), right after
we've parsed the command line options.  That pointer to the options
would then be globally accessible to the walker functions, if need be.

I believe that could have some interesting consequences, which I talk
about below.

[...]

>  /// Abstract the result of comparing two packages.
>  ///
>  /// This contains the the paths of the set of added binaries, removed
> @@ -653,9 +660,9 @@ extract_package(const package& package)
>  ///
>  /// @param stat the stat struct of the file.
>  static int
> -file_tree_walker_callback_fn(const char *fpath,
> -			     const struct stat *,
> -			     int /*flag*/)
> +file_tree_walker_elf_callback_fn(const char *fpath,
> +				 const struct stat *,
> +				 int /*flag*/)

[...]

> +static int
> +file_tree_walker_suppr_callback_fn(const char *fpath,
> +				   const struct stat *,
> +				   int /*flag*/)
> +{

I would call the two functions above first_package_tree_walker_callback_fn and
second_package_tree_walker_callback_fn, because I think that really is
the intent of having two walker functions.  One is to walk the content
of the first package, and the other is to walk the content of the second
package.

In the second walking function:

> +  struct stat s;
> +  lstat(fpath, &s);
> +
> +  if (!S_ISLNK(s.st_mode))
> +    {
> +      if (guess_file_type(fpath) == abigail::tools_utils::FILE_TYPE_ELF)
> +	elf_file_paths.push_back(fpath);

> +      else if (string_ends_with(fpath, ".abignore"))
> +	suppression_paths.push_back(fpath);

... I would conditionalize the action of recognizing the suppression
specification here to prog_options->abignore

So that it would read something like:

    if (prog_options->abignore
        && string_ends_with(fpath, ".abignore"))
      prog_options->suppression_paths.push_back(fpath);

> +    }
> +  return 0;
> +}

[...]

@@ -696,8 +727,8 @@ set_diff_context_from_opts(diff_context_sptr ctxt,
      | abigail::comparison::HARMLESS_SYMBOL_ALIAS_CHANGE_CATEORY);
 
>    suppressions_type supprs;
> -  for (vector<string>::const_iterator i = opts.suppression_paths.begin();
> -       i != opts.suppression_paths.end();
> +  for (vector<string>::const_iterator i = suppression_paths.begin();
> +       i != suppression_paths.end();
>         ++i)

This change would then become unnecessary.

[...]
 
> @@ -947,8 +979,14 @@ prepare_packages(package&	first_package,
>  		 package&	second_package,
>  		 const options& opts)
>  {
> -  if (!extract_package_and_map_its_content(first_package, opts)
> -      || !extract_package_and_map_its_content(second_package, opts))
> +  if (!extract_package_and_map_its_content(first_package, opts,
> +					   file_tree_walker_elf_callback_fn)
> +  /// We go through the files of the newer (second) pkg to look for suppression
> +  /// specifications, matching the "*.abignore" name pattern.
> +      || !extract_package_and_map_its_content(second_package, opts,
> +					      opts.abignore ?
> +					      file_tree_walker_suppr_callback_fn
> +					      : file_tree_walker_elf_callback_fn))

Here, the change would then look like:

> +  if (!extract_package_and_map_its_content(first_package, opts,
> +					   first_package_tree_walker_callback_fn)
> +      || !extract_package_and_map_its_content(second_package, opts,
> +					      second_package_tree_walker_callback_fn))

which I think is simpler because there is no condition any more.  The
condition happens at a lower logical level, where we really need to act
depending on that condition: take the suppression specification into
account, if the user asked for it.

[...]

> @@ -1142,13 +1180,15 @@ parse_command_line(int argc, char* argv[], options& opts)
>  	opts.fail_if_no_debug_info = true;
>        else if (!strcmp(argv[i], "--verbose"))
>  	verbose = true;
> +      else if (!strcmp(argv[i], "--no-abignore"))
> +	opts.abignore = false;
>        else if (!strcmp(argv[i], "--suppressions")
>  	       || !strcmp(argv[i], "--suppr"))
>  	{
>  	  int j = i + 1;
>  	  if (j >= argc)
>  	    return false;
> -	  opts.suppression_paths.push_back(argv[j]);
> +	  suppression_paths.push_back(argv[j]);

This change would then be unnecessary.

What do you think?

Cheers,

-- 
		Dodji