Fix @sample to avoid "You may need to manually remove..." if file is already gone.

[mandree]

Summary

Currently the @sample rule for pkg-plist has a bug in that, upon deinstallation, it will print the message

You may need to manually remove ${target_file} if it is no longer needed.

even if the ${target_file} has already been removed by the user. This is confusing.

The attached patch checks that the real file still exists on mismatch and otherwise avoids printing the pointless message.

Test Plan

pkg install -IUfy mailman-2.1.30.txz
pkg delete -fy mailman

[manu]

Not sure you can count that as a bug as ${target_file} is somewhat part of the package and might not have been rm before the package.
But anyway, cmp -s existing_file non_existant file return 2, it will be better to use $? to avoid a useless call to test(1)

[mandree]

@manu cmp does not detail exit codes >1 beyond "An error occurred", so we cannot use that.
test is a shell built-in for sh, and the directory has also been accessed by cmp before when looking up the expected ${target_file}, so that stat(2) that the shell (or its test builtin) will run under-the-hood stat(2) will access the caches.

[mandree]

ping

[mat]

Could you use devel/arcanist, or at least generate a diff with full context like it does, with svn diff -x -U9999 or git diff -U9999.

[mandree]

same code, rediffed with "full" context - not really clear why it was needed for a one-line patch...

[mat]

In D24571#546282, @mandree wrote:

same code, rediffed with "full" context - not really clear why it was needed for a one-line patch...

To see what was around.
The ports tree is not a collection of one liners, it is a complex code suite.
Because of this we try very hard to never review "one line" without looking at the context surround it.
So to make sure the reviewer does not overlook something, we need more than the standard 3 lines of context.
(Which is why using arcanist is usually better, it does it for you.)

Keywords/sample.ucl

58–59

Can you make this a elif. We try not to use || or other operators and always use flow control structures.

[mandree]

replace test ! -e TEST || suite by elif test -e TEST ; then suite

[mat]

Keywords/sample.ucl

58

Can you use [ bla ] and not test to be consistent.

[mandree]

@mat, this is ridiculous. It was in the previous version as well and was not asked to be changed. This is syntactic sugar and makes not difference in content. The coding style is not published AFAICT.
I will not tolerate such a strategy of delaying by asking minimal steps with ridiculous effort.

Please list a full list of required changes alongside a conditional approval; then there will be one more iteration that catches all your concerns.

[bapt]

In D24571#546698, @mandree wrote:

@mat, this is ridiculous. It was in the previous version as well and was not asked to be changed. This is syntactic sugar and makes not difference in content. The coding style is not published AFAICT.
I will not tolerate such a strategy of delaying by asking minimal steps with ridiculous effort.

Please list a full list of required changes alongside a conditional approval; then there will be one more iteration that catches all your concerns.

@mat asked you to go from the form

else
  [ ...] || 
fi

to the form

elif [  ]; then

fi

and you moved to

elif test... ; then
fi

Consistency matters, and I don't think consistency is ridiculous.

[mandree]

bapt, I am willing to change that, but I am not willing to put up with obstructivism and take micro change steps with re-uploading everything for new review for each and every changed two bytes for a one-line change. This is out of balance, and the discussion is, too.

Do do I get a conditional approval to commit with the test -> [ ] change, or are we going to waste more time on a trivial fix?

[mat]

In D24571#546716, @mandree wrote:

bapt, I am willing to change that, but I am not willing to put up with obstructivism and take micro change steps with re-uploading everything for new review for each and every changed two bytes for a one-line change. This is out of balance, and the discussion is, too.

Do do I get a conditional approval to commit with the test -> [ ] change, or are we going to waste more time on a trivial fix?

All it takes to update a review is run arc diff --update Dxxxxx paths/to/files.

While it is true that there is no published coding style, it is not as if there were no other conditionals using if in this script, and that you were adding brand new things in here.

[mandree]

Thanks for the commit. arcanist being PHP infested finds no place on my computer, sorry. Feel free to recommend sane code with sane requisites.