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

Simple maintenance improvements #1167

Merged
merged 2 commits into from
Sep 18, 2021
Merged

Simple maintenance improvements #1167

merged 2 commits into from
Sep 18, 2021

Conversation

a1346054
Copy link
Contributor

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.

How familiar are you with the codebase?: 4

Contains whitespace fixes that make some editors deal with the content better.

yubiuser
yubiuser previously approved these changes Sep 16, 2021
@DL6ER
Copy link
Member

DL6ER commented Sep 16, 2021

make some editors deal with the content better

Can you elaborate on this? Just for the purpose of personal education.

@a1346054
Copy link
Contributor Author

a1346054 commented Sep 16, 2021

make some editors deal with the content better

Can you elaborate on this? Just for the purpose of personal education.

Some (arguably badly-behaving, though standards respecting) editors and other tools will not display the final line if it is not terminated by a newline, due to it not being posix compliant. For example, run the file through wc -l and if it doesn't contain the final newline, then wc -l will show a count that is off by one.

Similarly, if you wanted to append some text to the file, using shell redirection for example, it will end up with breakage because the first line of appended text will be put as a continuation of last "line" of old text instead of below it.

Other breakage I've seen is editors wrapping text at wrong points because they consider trailing whitespace to be part of the last word.

Last but not least, github and git itself complain about it.

@DL6ER DL6ER enabled auto-merge September 16, 2021 09:38
DL6ER
DL6ER previously approved these changes Sep 16, 2021
@DL6ER DL6ER disabled auto-merge September 16, 2021 09:39
@DL6ER
Copy link
Member

DL6ER commented Sep 16, 2021

Do you want to sign your commits? Otherwise, I can overwrite the repository-wide role to only accept GPG signed commits.

@a1346054
Copy link
Contributor Author

Thanks for the tip, I'll look into that (it was on my todo list anyway to investigate gpg signing commits). In the meantime, feel free to do whatever you wish with this PR, commit it in your own name without attribution if that helps, I really don't mind.

@DL6ER
Copy link
Member

DL6ER commented Sep 16, 2021

We're neither in a rush nor is this fixing any security related stuff. Hence, we have the time to wait until you are ready. Take your time.

@a1346054
Copy link
Contributor Author

I've done some testing and came up with an interesting question that I don't know the answer to.

  1. create a gpg key
  2. put it into github
  3. use it to sign commits
  4. observe the commit shows verified
  5. remove the gpg key from github
  6. observe the previously verified commit shows as unverified

Would it therefore cause any issues for pi-hole/FTL if I removed the gpg key at some point in the future, and my commits would show up as unverified?

@DL6ER
Copy link
Member

DL6ER commented Sep 17, 2021

No, not if your commits already hit master.

But why would you delete the key from GitHub? If you are worried about loosing it (because of a data catastrophe) you can do either:

  1. Limit the validity of the key to, say, one year, or
  2. Have an unlimited key and store it (password encrypted!) in several safe, physically separated locations. Including at least one "offline" version, i.e., a printed out version on paper you'd OCR of everything failed (storage device age).

Signed-off-by: a1346054 <[email protected]>
Signed-off-by: a1346054 <[email protected]>
@a1346054 a1346054 dismissed stale reviews from DL6ER and yubiuser via c13af08 September 17, 2021 15:37
@a1346054
Copy link
Contributor Author

I gpg-signed the commits now. Thank you for bringing this to my attention, I learned a lot.

@DL6ER DL6ER enabled auto-merge September 18, 2021 05:54
@DL6ER DL6ER merged commit dbff117 into pi-hole:development Sep 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants