Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Native patch: handling file permission properly #10970

Closed
wants to merge 2 commits into from

Conversation

@meteorcloudy
Copy link
Member

meteorcloudy commented Mar 16, 2020

  1. If no file permission is specified in the patch file, preserve the
    original file permission.

  2. If file permission is specified, then set it as it is.

Fixes #10913

RELNOTES: Native patch can handle file permission properly

1. If no file permission is specified in the patch file, preserve the
original file permission.

2. If file permission is specified, then set it as it is.

Fixes #10913

RELNOTES: Native patch can handle file permission properly
@googlebot googlebot added the cla: yes label Mar 16, 2020
@meteorcloudy meteorcloudy requested a review from irengrig Mar 16, 2020
@meteorcloudy

This comment has been minimized.

Copy link
Member Author

meteorcloudy commented Mar 16, 2020

/cc @irengrig Can you help review this change?

@meteorcloudy meteorcloudy requested a review from philwo Mar 18, 2020
@meteorcloudy

This comment has been minimized.

Copy link
Member Author

meteorcloudy commented Mar 18, 2020

/cc @philwo Can you help review this change?

@meteorcloudy meteorcloudy removed the request for review from irengrig Mar 18, 2020
@meteorcloudy meteorcloudy self-assigned this Mar 18, 2020
@@ -229,8 +237,35 @@ private static void writeFile(Path file, List<String> content) throws IOExceptio
FileSystemUtils.writeLinesAs(file, StandardCharsets.UTF_8, content);
}

private static boolean getReadPermission(int permission) {
// Parse read permission from posix file permission notation
return permission / 4 % 2 == 1;

This comment has been minimized.

Copy link
@philwo

philwo Mar 18, 2020

Member

We can use bit-operators here to test for the bits in the permission bitmask :)

Executable is 0b001 = 1
Writable is 0b010 = 2
Readable is 0b100 = 4

So we can replace this with:

return permission & 4;

This comment has been minimized.

Copy link
@meteorcloudy

meteorcloudy Mar 19, 2020

Author Member

Thanks, I'll use

return (permission & 4) == 4

private static boolean getWritePermission(int permission) {
// Parse write permission from posix file permission notation
return permission / 2 % 2 == 1;

This comment has been minimized.

Copy link
@philwo

philwo Mar 18, 2020

Member

Similarly:

return permission & 2;

This comment has been minimized.

Copy link
@meteorcloudy

meteorcloudy Mar 19, 2020

Author Member

Done.


private static boolean getExecutablePermission(int permission) {
// Parse executable permission from posix file permission notation
return permission % 2 == 1;

This comment has been minimized.

Copy link
@philwo

philwo Mar 18, 2020

Member

Similarly:

return permission & 1;

This comment has been minimized.

Copy link
@meteorcloudy

meteorcloudy Mar 19, 2020

Author Member

Done

@@ -378,7 +408,7 @@ public void testMissingBothOldAndNewFile() throws IOException {
Path patchFile =
scratch.file(
"/root/patchfile",
"diff --git a/foo.cc b/foo.cc",
"diff --git a/ b/",

This comment has been minimized.

Copy link
@philwo

philwo Mar 18, 2020

Member

How is this change related?

This comment has been minimized.

Copy link
@meteorcloudy

meteorcloudy Mar 19, 2020

Author Member

So this test case is for testing an error message is thrown when no input and output file name is specified. Before, we only parse file names from --- and +++ lines. But now we also parses file names from diff --git line, therefore we should also remove them here to keep the test passing.

@@ -245,6 +280,10 @@ private static void applyPatchToFile(
oldContent = new ArrayList<>();
} else {
oldContent = readFile(inputFile);
// Preserve old file permission if no explicit permission is set.
if (filePermission == -1) {

This comment has been minimized.

Copy link
@philwo

philwo Mar 18, 2020

Member

If you want to use some more exotic Java data types, you could use Optional<BitSet> for the filePermission type (and BitSet in general, rather than an int, to store the permissions). Then you could use an enum as the keys for the BitSet (e.g. EXECUTABLE = 0, WRITABLE = 1, READABLE = 2).

But using -1 as the indicator for "no explicit permissions" is also fine, if you prefer it. ;)

This comment has been minimized.

Copy link
@meteorcloudy

meteorcloudy Mar 19, 2020

Author Member

I tried to use BitSet, but found converting a character (eg. 7) to a BitSet also requires some steps. The code overall didn't look much cleaner, so I'll keep using -1 and bit operations ;)

@philwo
philwo approved these changes Mar 19, 2020
Copy link
Member

philwo left a comment

Thanks!

@meteorcloudy

This comment has been minimized.

Copy link
Member Author

meteorcloudy commented Mar 19, 2020

Thanks for the review!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.