cvs commit: src Makefile.inc1

[Alexander Leidinger]

Quoting Dag-Erling Smørgrav <des at des.no> (from Mon, 14 May 2007  
16:44:16 +0200):

> Alexander Leidinger <Alexander at Leidinger.net> writes:
>> Dag-Erling Smorgrav <des at FreeBSD.org> writes:
>> >   Log:
>> >   Greatly speed up {check,delete}-old* by replacing make loops   
>> with sh loops.
>> How much faster?
>
> A *lot* faster.  Makefile loops are extremely slow.  The speedup was so
> great that I didn't even bother measuring it.

Thanks for the short evaluation in the other mail. It's a very nice  
speedup. I didn't expect the for-loop in make to be that slow compared  
to a shell for-loop.

>> Some review:
>>
>> Why did you remove the echo?
>
> which echo?  about schg?  it was completely bogus.

Yes. I don't think so, please explain.

> feel free to re-add it, but only if you also add logic to check whether
> the file actually has the schg flag set before running chflags.

In the current code it doesn't make sense, and the wording can be  
changed to tell that it will remove schg, in case this flag is set. As  
you did the change and I don't have time to commit such a change when  
I'm back at home (the place where I can make commits), would you  
please add the echo again?

>> Why do you redirect stderr of chflags to /dev/null?
>
> because chflags will complain if your /usr is on a file system which
> does not support flags, such as NFS or ZFS.

But it also removes any other failure message. A more sensible  
approach would be to replace the redirection with a "grep -v".

>> Why did you change the removal logic?
>
> because it was broken; it would always ask twice about files which you
> chose not to delete.

Yes, that's the bug I talked about. But it also did not try the  
chflags if it was able to delete the file. So the bug is not that  
chflags prints the message on a FS which does not supports flags, the  
bug is that the chflags is done when the user doesn't want to remove  
the file.

See below.

>> The way it was before:
>>  - rm (without -i if requested)
>>  - if rm fails do a chflags and rm again (bug: if user says no in the
>> interactive mode, the chlags is done regardless)
>>  - tell the user about the stuff we do (removing flags)
>>  - it aborts on a failure of the second rm (AFAIR)
>>
>> The way it is now:
>>  - for every file do a chflags without notifying the user, don't tell
>> about problems
>>  - rm (without -i if requested)
>>  - bug(?): ${DESTDIR} in the rm line
>
> no, this is intentional, and in fact the original code had it too.

The original code hat it before, but you CD into DESTDIR, so it is not  
necessary anymore. This may be cosmetics to you, but I did ask myself  
"Why does des use DESTDIR here?". Sorry that I used "bug", I should  
have used "overlooked" or something like this. Removing the  
unnecessary DESTDIR helps other people understand this better.

>>  - doesn't abort on rm failure
>
> that was not intentional...  I forgot to remove the || true.
>
>>  - has the same bug that it chflags even if the user doesn't want to
>> rm the file
>
> yes, but at least it doesn't ask twice.

That's the bug you should fix, and not the part where you quiten the  
chflags command.

Please fix it the right way, and not by using a workaround.

Bye,
Alexander.

-- 
A hermit is a deserter from the army of humanity.

http://www.Leidinger.net    Alexander @ Leidinger.net: PGP ID = B0063FE7
http://www.FreeBSD.org       netchild @ FreeBSD.org  : PGP ID = 72077137