coreutils icon indicating copy to clipboard operation
coreutils copied to clipboard

install: fix bad target directory permissions

Open kylemanna opened this issue 2 years ago • 3 comments

  • Correct bug that set the last directory to the mode the file should have been set to.
  • Add unit test to verify correct functionality.
  • Fixes #4360

kylemanna avatar Feb 14 '23 06:02 kylemanna

This is ready to go, any feedback?

kylemanna avatar Feb 16 '23 01:02 kylemanna

The test looks good, I'm just not sure I understand the change. You now have a structure like this:

if b.create_leading {
    // ...
    if !b.create_leading && chmod(...).is_err() {
        // ...
    }
}

Where b is behind a reference, so isn't mutated. Doesn't that just make the boolean always false? And because the && operator short-circuits we never execute chmod, right? Or am I missing something?

tertsdiepraam avatar Feb 16 '23 09:02 tertsdiepraam

@kylemanna ping?

sylvestre avatar Feb 18 '23 09:02 sylvestre

@tertsdiepraam you're right, it doesn't make sense. Turns out that chmod never need to run in that section, perhaps it a bug from an earlier version of install?

I figure out that you don't need to chmod the leading components, but didn't think much of the surrounding if b.create_leading until you pointed it out.

I've updated the code to never chmod leading components and the unit test remains unchanged from my earlier code.

All tests pass and this makes more sense.

Not sure why that code was introduced, certainly doesn't seem that the code path was subject to a unit-test until now.

kylemanna avatar Feb 19 '23 04:02 kylemanna

Thanks!

kylemanna avatar Feb 19 '23 15:02 kylemanna