[WIP] save permissions in dvc files

[dudarev]

This is work in progress for #4578

Modification of tests/func/test_add.py:test_add to output mode parameter in .dvc file.
Implementation of this test.
Other tests are not validated and not addressed yet.

• ❗ I have followed the Contributing to DVC checklist.

• 📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

[shcheklein]

@dudarev could you please summarize what are the next steps here?

[dudarev]

@shcheklein the next steps could be:

• make all tests pass
• figure out how to test mode change on checkout (test on Windows either should be skipped or set only allowed permission byte)
• implement chmod on checkout
• fix all tests if they will be broken on the previous step
  --- tasks below this line probably can be a separate ticket ---
• think of various error handling scenarios
• implement error handling

[shcheklein]

@dudarev sounds about right to me! I guess we can move forward with this and other folks will review and chime in along the way!

[efiop]

Is full mode intentional? Or did you only mean to include permission bits?

[dudarev]

I'm leaning now more towards only saving user executable bit, so we could have a boolean parameter:

is_user_executable: false/true

[efiop]

@dudarev Hi! Thanks for the PR! 🚀 A few comments:

1. Could you please rename it to reflect the contents?
2. Please add a reference for the ticket, so github could track it.
3. Please install pre-commit hooks as noted in https://dvc.org/doc/user-guide/contributing/core . Currently linter checks fail.

Thanks! 🙂

[efiop]

mode is not really used by state, so we shouldn't include it here (we can probably do that in Output.save with self.tree.stat(self.path_info).st_mode. Also mode is not really related to HashInfo so it shoudn't be.a part of it, unless there are good reasons to include it. 🙂

[dudarev]

Yes, it makes sense not to include mode as part of hash. Should content of output file looks like:

outs:
- md5: acbd18db4cc2f85cedef654fccc4a4d8
  size: 3
  path: foo
- is_user_executable: false

So there will be two separate outputs. Will it affect anything that uses those files?

Could you provide a link to a place where outs are set in the code? I'm having trouble finding it quickly.

[efiop]

@dudarev It should be more like:

outs:
- md5: acbd18db4cc2f85cedef654fccc4a4d8
  size: 3
  path: foo
  is_user_executable: false

Since that relates to the same output. Maybe you are confused about the way that entry is generated, see

dvc/dvc/output/base.py

Line 299 in f09c27e

def dumpd(self):

. We could just add ret[self.PARAM_ISEXEC] = self.isexec there and parse it on the roundtrip similar to other fields.

Could you provide a link to a place where outs are set in the code? I'm having trouble finding it quickly.

Sure!

dvc/dvc/output/base.py

Line 289 in f09c27e

self.hash_info = self.get_hash()

[dudarev]

@efiop Thank you for your feedback!

1. I renamed the PR to better reflect its content.
2. Also added the reference to the ticket in the description.
3. I installed pre-commit hook when working on the previous PR but this time commited from IDE that did not use it. I'll be more mindful about commit from git cli in the following PRs.

You have a very good point about storing only user execution permission and not having mode in HashInfo. I asked a few questions to your comments above that could streamline the development. Please have a look.