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.

Sign up for GitHub

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

Jump to bottom

MASSEMBLY-941 keep file permission in Reproducible mode #96

Merged
merged 5 commits into from
Nov 26, 2022
Merged

MASSEMBLY-941 keep file permission in Reproducible mode #96

merged 5 commits into from
Nov 26, 2022

Conversation

hboutemy
Copy link
Member

@michael-o
Copy link
Member

Use %n

@hboutemy
Copy link
Member Author

@michael-o what does "use %n" mean, please?

@pzygielo
Copy link
Contributor

@michael-o what does "use %n" mean, please?

In String.format instead of \n I presume...

@michael-o
Copy link
Member

@michael-o what does "use %n" mean, please?

In String.format instead of \n I presume...

Almost, it is replaced with the platform line separator.

@hboutemy
Copy link
Member Author

ok, but how does it relate to this PR?

In the current PR, Windows is a pain not as usual because it has specific newlines, but because it can't detect executable files on filesystem (100644 executable.txt) (notice that Unix is a pain because it has 2 representations: 100755 executable.txtand 100775 executable.txt)

@michael-o
Copy link
Member

ok, but how does it relate to this PR?

In the current PR, Windows is a pain not as usual because it has specific newlines, but because it can't detect executable files on filesystem (100644 executable.txt) (notice that Unix is a pain because it has 2 representations: 100755 executable.txtand 100775 executable.txt)

It is not related, I just noticed it. If you really just need a LF then a comment should tell about your intent.

@hboutemy
Copy link
Member Author

It is not related, I just noticed it. If you really just need a LF then a comment should tell about your intent.

AFAIK there is nothing related to LF in this PR, then I suppose I should just ignore that comment that looks to me just a mistake (no pun intended, it happens to me also to completely be out of context because I misread something): if you think there is one place in the PR where it applies, please precise the location

@michael-o
Copy link
Member

It is not related, I just noticed it. If you really just need a LF then a comment should tell about your intent.

AFAIK there is nothing related to LF in this PR, then I suppose I should just ignore that comment that looks to me just a mistake (no pun intended, it happens to me also to completely be out of context because I misread something): if you think there is one place in the PR where it applies, please precise the location

Agreed.

@hboutemy hboutemy merged commit f42194b into master Nov 26, 2022
@hboutemy hboutemy deleted the MASSEMBLY-941 branch November 26, 2022 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hboutemy @michael-o @pzygielo