[Bug 519395] cleanup patch: use better buildroot, nicer find coomands

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=519395





--- Comment #3 from Stepan Kasal <skasal at redhat.com>  2009-08-26 11:48:44 EDT ---
(In reply to comment #2)
> May-be you should explain which issues you are trying to solve. I don't see
> them.

Well, I have to admit that I'm not trying to solve any real problem, I was just
replacing things I did not like with "nicer" patterns.  I wanted to make the
cpanspec-generated code as educative as possible, because it gets replicated
many times.

Thank you, Ralf, for your comment, you made me to think more about the changes.
Rationale follows.

> I agree with your proposal on adopting the new buildroot,

Great.  Even FPG mention that this is the best of the allowed alternatives.

> * Your proposal adds further external commands (pipe, xargs), where using
> find's internal -exec is more efficient.

Generally speaking, xargs is often more efficient than -exec, as it calls the
command only once for many arguments.  That's why I prefer it.
But in this case, you are right: the rm command is called ony a few times, if
ever, so it is more appropriate here.

Anyway, there is a way to combine both advantages: we can use
      find ... -exec rm -f {} +
The plus terminator in find is defined by POSIX.

> * -size 0 complies to POSIX, -empty doesn't.

Right.

> * Your proposal is sensitive to white space quoting, due to using xargs

Right.  To fix it, I'd have to use "-print0|xargs -0", another non-POSIX
feature.

> (The original construct also is, due to missing quotes)

Right, should be fixed.  I think that the values in @MACROS should contain
quotes and then would be used unquoted.

To sum up, you convinced me to back out my changes to first two instances of
"find", except for perhaps changing the -exec delimiter to +.

But I think the situation is different with the last instance of find:

   find $RPM_BUILD_ROOT -depth -type d -exec rmdir {} 2>/dev/null \;

Note that the stderr redirection does not apply to rmdir, but to the whole find
command.  Consequently, any error output from find is discarded.

At the very least, we should make this clear by reordering the arguments:

   find $RPM_BUILD_ROOT -depth -type d -exec rmdir {} \; 2>/dev/null

But I should it would be better to overcome this limitation by using the
non-POSIX predicate -empty (-size 0 does not work for directories).
That means I still support my original proposal.

I'll post an updated patch tomorrow.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.