acmesh-official / acme.sh

A pure Unix shell script implementing ACME client protocol
https://acme.sh
GNU General Public License v3.0
39.32k stars 4.97k forks source link

Do not use spaces for indentation #4002

Open antichris opened 2 years ago

antichris commented 2 years ago

Spaces should not be used for indentation in a POSIX environment, as exemplified by POSIX specification of Here-Document (emphasis mine):

If the redirection operator is "<<-", all leading <tab> characters shall be stripped from input lines and the line containing the trailing delimiter.

The Open Group Base Specifications Issue 7, 2018 edition IEEE Std 1003.1-2017, § 2.7.4

Consequently, the following (somewhat contrived, yet perfectly valid) POSIX compliant code:

myfunc() {
    numLines <<-***
        my
        multi-line
        value
    ***
}
numLines() {
    lines=$(cat)
    total=$(printf %s\\n "$lines" | wc -l)
    digits=$(printf %.0f "$(printf 'l(%d)/l(10)+1\n' "$total" | bc -l)")
    n=0
    printf %s\\n "$lines" | while read -r line; do
        printf "%${digits}d: %s\n" "$((n=n+1))" "$line"
    done
}

completely falls apart when spaces are used for indentation:

myfunc() {
  numLines <<-***
    my
    multi-line
    value
  ***
}
numLines() {
  lines=$(cat)
  total=$(printf %s\\n "$lines" | wc -l)
  digits=$(printf %.0f "$(printf 'l(%d)/l(10)+1\n' "$total" | bc -l)")
  n=0
  printf %s\\n "$lines" | while read -r line; do
    printf "%${digits}d: %s\n" "$((n=n+1))" "$line"
  done
}

Also, any content that uses spaces for indentation renders the POSIX tabs utility perfectly useless in dealing with it.

There is no sane reason to force your favorite number of spaces per indentation on everyone else, when you can configure your text editors to display the tabs for you exactly the way you prefer, be it 2, 3, 4 or whatever the number of spaces you like. Especially when pushing your personal preference breaks valid, standard-compliant code.

LoganDark commented 2 years ago

The usage of spaces here also inflates the file size. I did a test and switching to tabs reduces the size of https://github.com/acmesh-official/acme.sh/blob/c8c1c09189cac5da52424a36eb0846f4da385fa6/acme.sh from 215541 bytes to (approximately) 202402 bytes. That's over 10KB.

Generally, hard tabs should always be used for indentation. Using spaces for indentation is a hack. Good for ASCII art, but not for code.

I put the tab-converted file here for reference: https://pastebin.com/7cr4U3ip

Neilpang commented 2 years ago

Tabs might be reasonable, but is there a hard reason that we HAVE TO replace all the indentions with Tabs? I mean are there any use cases that the current code can not work for you?

I would love to know that.

LoganDark commented 2 years ago

Tabs might be reasonable, but is there a hard reason that we HAVE TO replace all the indentions with Tabs? I mean are there any use cases that the current code can not work for you?

I would love to know that.

This reads quite aggressively. Do you have a strong opinion in favor of spaces? They are incorrect according to the POSIX specification, add file size and reduce readability of the script.

Strictly speaking, this does not compromise the advertised functionality of acme.sh.

Neilpang commented 2 years ago

Sorry, but I just wanted to know the real problem that prevented us from using spaces. For me, we have tested and run on a variety of OS and platforms for years, we have never encountered such a fatal Tabs/Spaces problem.

If there is a good enough reason, I'd love to make any kind of changes to make acme.sh better, otherwise I would prefer to keep it as is. This would be a big change to the source code, it will override git history of every line of code.

LoganDark commented 2 years ago

it will override git history of every line of code.

I think git supports the option to ignore whitespace, but not globally :( so you are right here. GitHub provides a nice UI for viewing blame prior to a change though.

antichris commented 2 years ago

@Neilpang

For me, we have tested and run on a variety of OS and platforms for years, we have never encountered such a fatal Tabs/Spaces problem.

See https://github.com/acmesh-official/acme.sh/issues/4002#issue-1185011336. It's a catch-22: the reason why no one tried using the indented Here-Document syntax is precisely because it would cause a "fatal Tabs/Spaces problem". It is also possible that the contributors are just not that familiar with shell scripting.

The 6% size overhead that could be shed, as @LoganDark pointed out, is also pretty significant.

This would be a big change to the source code, it will override git history of every line of code.

Not really an issue, since as of Git v2.23 you can use .git-blame-ignore-revs. Here's an article going into details. GitHub also supports it now.