Date: Mon, 14 May 2007 17:29:38 +0200 From: Alexander Leidinger <Alexander@Leidinger.net> To: Dag-Erling =?utf-8?b?U23Dg8K4cmdyYXY=?= <des@des.no> Cc: cvs-src@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src Makefile.inc1 Message-ID: <20070514172938.5v4yi78kisokk84s@webmail.leidinger.net> In-Reply-To: <86wszbqxbz.fsf@dwp.des.no> References: <200705131815.l4DIFop9026874@repoman.freebsd.org> <20070514154040.0lofzrgm8kskgog4@webmail.leidinger.net> <86wszbqxbz.fsf@dwp.des.no>
next in thread | previous in thread | raw e-mail | index | archive | help
Quoting Dag-Erling Sm=C3=83=C2=B8rgrav <des@des.no> (from Mon, 14 May 2007 =
=20
16:44:16 +0200):
> Alexander Leidinger <Alexander@Leidinger.net> writes:
>> Dag-Erling Smorgrav <des@FreeBSD.org> writes:
>> > Log:
>> > Greatly speed up {check,delete}-old* by replacing make loops =20
>> 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 =20
speedup. I didn't expect the for-loop in make to be that slow compared =20
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 =20
changed to tell that it will remove schg, in case this flag is set. As =20
you did the change and I don't have time to commit such a change when =20
I'm back at home (the place where I can make commits), would you =20
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 =20
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 =20
chflags if it was able to delete the file. So the bug is not that =20
chflags prints the message on a FS which does not supports flags, the =20
bug is that the chflags is done when the user doesn't want to remove =20
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 =20
necessary anymore. This may be cosmetics to you, but I did ask myself =20
"Why does des use DESTDIR here?". Sorry that I used "bug", I should =20
have used "overlooked" or something like this. Removing the =20
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 =20
chflags command.
Please fix it the right way, and not by using a workaround.
Bye,
Alexander.
--=20
A hermit is a deserter from the army of humanity.
http://www.Leidinger.net Alexander @ Leidinger.net: PGP ID =3D B0063FE7
http://www.FreeBSD.org netchild @ FreeBSD.org : PGP ID =3D 72077137
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070514172938.5v4yi78kisokk84s>