Ondrej Oprala wrote: > On 08/07/2012 05:37 PM, Bernhard Voelker wrote: >> On 08/07/2012 05:01 PM, Ondrej Oprala wrote: >>> Hi, I've renamed the variable to be more hinting of it's purpose >>> and --no-preserve=mode should now work properly with directories >>> as well. >>> Cheers, >>> Ondrej >> Thanks, that looks good ... maybe the directory case deserves to >> be added to the test. Minor nit: the change in copy_internal() is >> missing in the commit log. >> I'm sure that Padraig/Jim will add both when committing. >> >> BTW: Interestingly, the TODO entry mentioned "--no-preserve=X" >> to be buggy: >> >>> -cp --no-preserve=X should not attempt to preserve attribute X >>> - reported by Andreas Schwab >> but in fact, all the modes (t,o,l,c, and x) but "mode" >> have already been working. ;-) >> >> Have a nice day, >> Berny > Hi, I've amended the commit log and added a test for directories.
Thanks again for your work on this. Sorry it took so long for me to get back to it. I've looked over the patch and noticed a problem: What happens when we use --no-preserve=mode and --preserve=all together? I would not expect --no-preserve=mode to silently override a following --preserve=all. Rather, the latter should supersede the former. Hmm... the right thing appears to be what happens, in spite of conflicting settings. It looks like while the two members .preserve_mode and .explicit_no_preserve_mode can both be set (a contradiction), that doesn't cause an actual problem because whenever it happens, .preserve_mode is both correct and the member that is tested first in an if-else-if... cascade. So maybe all you need to do it to turn off .explicit_no_preserve_mode in the PRESERVE_ALL case? A test case addition to cover that case would be most welcome, but since I've waited so long to give feedback, it seems unfair to require that. > From d52f8990353dac04bff141ff31b6601ba5112a18 Mon Sep 17 00:00:00 2001 > From: Ondrej Oprala <[email protected]> > Date: Tue, 7 Aug 2012 16:56:52 +0200 > Subject: [PATCH] cp: Fix the --no-preserve=mode option
