acme.sh icon indicating copy to clipboard operation
acme.sh copied to clipboard

Do not use spaces for indentation

Open antichris opened this issue 3 years ago • 6 comments

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.

antichris avatar Mar 29 '22 15:03 antichris

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

LoganDark avatar Apr 21 '22 08:04 LoganDark

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.

Neilpang avatar Apr 21 '22 08:04 Neilpang

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.

LoganDark avatar Apr 21 '22 08:04 LoganDark

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.

Neilpang avatar Apr 21 '22 09:04 Neilpang

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.

LoganDark avatar Apr 21 '22 09:04 LoganDark

@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.

antichris avatar Apr 21 '22 09:04 antichris