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

Change FILE_CHAR_BLOCKLIST to FILE_CHARS_DISALLOWED #19897

Merged
merged 1 commit into from Sep 6, 2020

Conversation

verretor
Copy link
Contributor

@verretor verretor commented Sep 6, 2020

Blocklist is ambiguous. It could mean a list of blocks.

Example: "blocknotify" in the same file refers to Bitcoin blocks.

@nopara73
Copy link

nopara73 commented Sep 6, 2020

image

@ghost
Copy link

ghost commented Sep 6, 2020

Blacklist is common nomenclature. Blocklist (being a fake word) is discriminatory to those whose first language is not English.

@maflcko
Copy link
Member

maflcko commented Sep 6, 2020

Clearly this is going to cause another pull request in n days to change it again. I really don't want to rehash this topic over and over, and I am certain that the other reviewers in this repo don't want to either. So instead of changing a variable name back and forth, it might be better to remove it completely.

It seems odd anyway to have three named constants where one would be sufficient and clearer.

@dstrukt
Copy link

dstrukt commented Sep 6, 2020

Was an unnecessary change in the first place.

Let’s change it back and be done with it.

@DubbleU
Copy link

DubbleU commented Sep 6, 2020

Injecting SJW politics into Bitcoin is an attack vector/slippery slope. Unless it was creating a problem why change it?

@KainerW
Copy link

KainerW commented Sep 6, 2020

Blacklist is easy to understand. I see no reason to change that cause of SJW reasons. That opens the door to endless word-changes.

@promag
Copy link
Member

promag commented Sep 6, 2020

It was suggested FILE_CHAR_EXCLUDE in #19227 (comment), nobody cared about a constructive comment, thank you for reviewing.

@initfortherekt
Copy link

initfortherekt commented Sep 6, 2020

ACK. "blocklist" is ambiguous, and the idea that political PR's engender more participation and review doesn't take into account the time and energy they waste.

@calkob
Copy link

calkob commented Sep 6, 2020

Whats a Blocklist?

is this it?
647010
647090
647080
647070
647060
647050

@StopAndDecrypt
Copy link

StopAndDecrypt commented Sep 6, 2020

ACK

The purpose of the words "white" and "black" are to convey the presence or absence of light visible to the human eye. They are the extremes, whereas "light" and "dark" are used to subjectively or contextually denominate some place in between. The purpose of these words are very similar to the purpose of the word "red", which conveys a specific sensation when observing a measurable frequency on the electromagnetic spectrum, except red is a color, and white & black are not.

Human skin is neither white or black, they are all shades of brown (and maybe some red after a hot day at the beach). While it's clear to me why humans have chosen to adopt these words and apply them to the human condition of outwardly observable genetic diversity, it's not quite clear to me why this adoption should then trickle back to the original meaning and purpose of the words themselves. Given that all humans are People-of-Color, it is impossible for the words white and black, without any stated context, to meaningfully apply to PoC (like myself, you, and everyone else).

On the subject of changing the words in consideration of the sensitive among us, where those sensitive individuals might apply their own context when no such context was provided by the use of the words: If the words blacklist and whitelist were to be changed to, for example, nightlist and daylist, this could potentially offend people who identify as nocturnal animals, or people who like to tan at the beach. They too would have to apply their own context to those words in order to become offended, but this is no different than those who take issue with the words white and black. Since many in the FOSS community are anonymous it's impossible to know if there are these kinds of (perfectly fine) people working on the project currently, or might otherwise be attracted to this project in the future.

Now, one might argue that one set of people is a known known, while the other set is a known unknown (I believe Nocturnal Furrys are inherently known unknowns), but I argue that it's simply not proper to prioritize the feelings of one set of contributors over another set, as that lays the groundwork of precedent for additional emotional/irrational changes in the code moving into the future. With that being said, I believe it to be much more important to focus on meaningful development and not get distracted by political signaling, or open the door for more political creep into the project.

@Bennettd77
Copy link

Keep social justice out of code. Code can be ambiguous enough at the best of times. ‘Blacklist’ is less confusing than ‘block list’ and we need to make sure we don’t extend technical debt into the future because of the stupidity around us right now.

@karozagorus
Copy link

I am a Bitcoiner, I have an English BA degree with linguistics focus, and I wish to protest the change from "Blacklisting" to "Blocklisting". It should be immediately reverted.

There is absolutely no racially charged meaning behind the blacklist word, the lexical usage of the term does not imply racially charged meanings or anything else that is perverted by those who wish to imply racial motives upon users of lexical terms, this is a politicized attempt at creating changes and softening the English language intentionally.

Therefore
ACK

@roadhater
Copy link

roadhater commented Sep 6, 2020

ACK

Critical race theory is too divisive to have even an inch in something as important as bitcoin. It's a Pandora's Box. Some will say changes like the original are merely attempts to be inclusive. That may be true, but it leaves open serious social attack vectors. Consider how megacorps and governments prevent effective organization against them.

@@ -18,7 +18,7 @@
# Windows disallow control characters (0-31) and /\?%:|"<>
FILE_CHAR_START = 32 if os.name == 'nt' else 1
FILE_CHAR_END = 128
FILE_CHAR_BLOCKLIST = '/\\?%*:|"<>' if os.name == 'nt' else '/'
FILE_CHAR_BLACKLIST = '/\\?%*:|"<>' if os.name == 'nt' else '/'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please end this pointless cycle and change it to `FILE_CHARS_DISALLOWED``

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to FILE_CHARS_DISALLOWED.

@lautaskriptaaja
Copy link

lautaskriptaaja commented Sep 6, 2020

It was suggested FILE_CHAR_EXCLUDE in #19227 (comment), nobody cared about a constructive comment, thank you for reviewing.

Even though I'm very much for this revert, in this particular case exclusion/inclusion (FILE_CHAR_EXCLUDE) seems to be as self-explanatory as the original blacklist/whitelist. "Blocklist" is a very horrible term to use in Bitcoin because it means a list of blocks.

Only change the variable names IF you can come up with as self-explanatory term as the original one. FILE_CHAR_EXCLUDE is one, FILE_CHAR_BLOCKLIST is not.^

EDIT: or @laanwj 's FILE_CHARS_DISALLOWED might be even better.

@promag
Copy link
Member

promag commented Sep 6, 2020

@calkob no, this is a block list:

  • image
  • image
  • image
  • image
  • image
  • image
  • image
  • image
  • image

@jmcorgan
Copy link
Contributor

jmcorgan commented Sep 6, 2020

Concept ACK

@alko89
Copy link

alko89 commented Sep 6, 2020

US culture war doesn't belong in a global software. The initial commit was pointless.

@repojohnray
Copy link

The SJW nonsense imported from the USA should be removed. This commit: #19227 had no place.

@promag
Copy link
Member

promag commented Sep 6, 2020

NACK, either

  • inline as @MarcoFalke suggested
  • @laanwj suggestion FILE_CHARS_DISALLOWED
  • my suggestion FILE_CHAR_EXCLUDE
  • anything else but revert, that will start another discussion...

@M-BTC
Copy link

M-BTC commented Sep 6, 2020

ACK

This is Bitcoin. Let's keep the SJW stuff on Facebook, please.

@karozagorus
Copy link

If you are such an SJW change it to FILE_CHAR_BANLIST, and don't pervert the English language.

@dooberdoober
Copy link

ACK
there have been zero issues with the term blacklist until a bunch of sally wanker WHITE folk all of a sudden read a book written by an actual racist person who is projecting her OWN racism, onto all others of her "skin color"

blacklist is not racist.
People who think the word blacklist is racist, are themselves, racist.

Copy link

@ashwin111 ashwin111 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FILE_CHAR_BLACKLIST is cleaner & less ambiguous than FILE_CHAR_BLOCKLIST in addition to being how it was originally defined. Unless there is a material reason for the change, we should avoid unnecessary variable renaming/refactoring.

@RobinLinus
Copy link

Injecting politics into Bitcoin is an attack vector.

@fiatjaf
Copy link

fiatjaf commented Sep 6, 2020

The argument according to which reverting this "will cause another PR down the road is bogus". It is the same as not returning a stolen item from a thief to the rightful owner "because it would cause the thief to try to steal it again in the future".

Also, the other PR was proposed by some anonymous random guy who just created a fake GitHub account and started opening SJW pull-requests in many repositories, it's not like it was the voice of a group or anything like that.

@AmishNick
Copy link

ACK

I thought it was unnecessary and bad precedent to begin with, and in the context of blockchain, blocklist is ambiguous af

@promag
Copy link
Member

promag commented Sep 6, 2020

FILE_CHAR_BLACKLIST is cleaner & less ambiguous than FILE_CHAR_BLOCKLIST in addition to being how it was originally defined.

I authored the original change 4e9efac and I feel sorry that a stupid test caused so much noise.

Also, the original PR was open from 29 May 2018 to 17 Fev 2020 ...

Copy link
Member

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 637d8bc.

The new variable reflects the goal stated in L18.

@lviggiano
Copy link

lviggiano commented Sep 6, 2020

@laanwj this was a very serious attack: political arguments should not get into bitcoin code: bitcoin is born to be censor resistant and neutral, and I feel this to be a big deal.

@Cobra-Bitcoin
Copy link

Two equally bad phenomena seem to be happening in this drama: the first was a random contributor coming out of nowhere, making a subtle change with a political bent, and exploiting the relative tendency of Bitcoin Core developers to want to avoid conflict to push through a change the community doesn't really agree with. The other thing that's happening here is the mob using this as a chance to rally around, the whole crazy "defenders of Bitcoin" bullshit that has existed ever since the scaling fork drama. If your only contributions in the Bitcoin community are essentially just shouting down others, then you aren't doing very much to help Bitcoin.

@verretor
Copy link
Contributor Author

verretor commented Sep 6, 2020

I knew very well that BLACKLIST was not going to be merged when I made my pull request.

I thought it was important that there was a discussion about it so that it can be avoided in the future.

@AmishNick
Copy link

AmishNick commented Sep 6, 2020

NACK

Why is it FILE_CHARS_DISALLOWED instead of FILE_CHAR_DISALLOWED?

@rockstardev
Copy link

rockstardev commented Sep 6, 2020

Props to you @verretor and @laanwj for trying to stay on the point. I'm pissed this even became an issue. But seems we must continuously tend to our USA SJWs and their need to virtue signal everyday and everywhere.

@luke-jr
Copy link
Member

luke-jr commented Sep 6, 2020

This isn't about blocking anything, so blocklist is technically wrong. "Blacklist" has actual ambiguity issues too.

What this list is doing, is listing characters to exclude from filenames, because the OS (or our libraries) are known to not support them in filenames.

I think FILE_CHAR_EXCLUDE is fine.

@verretor
Copy link
Contributor Author

verretor commented Sep 6, 2020

NACK

Why is it FILE_CHARS_DISALLOWED instead of FILE_CHAR_DISALLOWED?

Because multiple characters are disallowed.
FILE_CHAR_START and FILE_CHAR_END represent the range of possible characters.

@verretor
Copy link
Contributor Author

verretor commented Sep 6, 2020

This isn't about blocking anything, so blocklist is technically wrong. "Blacklist" has actual ambiguity issues too.

What this list is doing, is listing characters to exclude from filenames, because the OS (or our libraries) are known to not support them in filenames.

I think FILE_CHAR_EXCLUDE is fine.

@laanwj suggested FILE_CHARS_DISALLOWED.

@jonatack
Copy link
Contributor

jonatack commented Sep 6, 2020

Freaking ridiculous how few contributors coordinated their responses and then @MarcoFalke merged that PR in one day - #19227.

Let's see how long it'll take to revert it.

Friendly response: everything I've seen over the past year and a half here has been ad-hoc and voluntary. No one ever says which PRs to review, which issues to respond to, or which things to work on. Colleagues just ask if you have a few cycles available to review or re-review their work, which happens a lot (and is fine) but it wasn't the case for that PR. Or, someone might suggest an old PR that could be picked up if you ask. Everything happens in an ad-hoc, free manner, and that's how I like it and think it should be. There might be coordination, but other than the public weekly irc meetings, I haven't seen it. There is so much to do, the stack of PRs to review grows and grows, and so many important things to worry about, that a PR like that one is pretty much at the bottom of the list of things to allocate time and energy to. It's not surprising to me that it would be handled quickly to free up bandwidth for all the rest.

Coordination? It's actually kind of the opposite. Apart from discussion on GitHub, there is almost no direct feedback. You kind of wonder. How can I do better. It's a quiet, focused, set-your-own priorities-and-schedule sort of life. For me, anyway. Hope this helps. Cheers.

@AmishNick
Copy link

AmishNick commented Sep 6, 2020

@verretor Wouldn't that have also applied when it was blacklist and blocklist?

@verretor
Copy link
Contributor Author

verretor commented Sep 6, 2020

Props to you @verretor and @laanwj for trying to stay on the point. I'm pissed this even became an issue. But seems we must continuously tend to our USA SJWs and their fucking need to virtue signal everyday and everywhere.

Thank you. I hope there won't be a need for such debates in the future.

@verretor
Copy link
Contributor Author

verretor commented Sep 6, 2020

@verretor Wouldn't that have also applied when it was blacklist and blocklist?

Yes. The variable name wasn't appropriate. It should have been plural.

@eriknylund
Copy link
Contributor

I knew very well that BLACKLIST was not going to be merged when I made my pull request.

I thought it was important that there was a discussion about it so that it can be avoided in the future.

I appreciate your PR in that sense that it goes to show how you can:

  • start out from a proposed code change you think improves the code base
  • get good review and feedback from others
  • update the code where you see fit based on suggestions
  • update the title of the PR to reflect the change
  • have people agree or disagree with the changes
  • iterate until the PR can be merged or closed

I see a lot of new faces here and I hope some will stick around when this blows over. Every review is appreciated.

@lavie
Copy link

lavie commented Sep 6, 2020

so you are more afraid of a rogue variable rename than, say, a bug that would introduce a remote vulnerability in P2P, an inflation bug, or that would wipe your wallet ?
@laanwj

Is it your experience that most PRs in this project introduce harmful changes? If not, then why do you not appreciate the attention an objectively bad change that slipped through is getting, no matter how minor? Introducing ambiguity is objectively bad, and allowing code changes without technical merit for political reasons justifies future such changes "for consistency's sake". By accepting the revert you would effectively be removing unnecessary future work from your plate as a maintainer.

@AmishNick
Copy link

AmishNick commented Sep 6, 2020 via email

@verretor
Copy link
Contributor Author

verretor commented Sep 6, 2020

@AmishNick Yes. The variable name deserved to be changed since the beginning.

@lviggiano
Copy link

lviggiano commented Sep 6, 2020

@eriknylund

I see a lot of new faces here and I hope some will stick around when this blows over. Every review is appreciated.

After this, it will stick.

But I heartily suggest to fix it as FILE_CHARS_BLACKLIST. The technical point of ambiguity is definitely relevant, but the political argument is much more dangerous despite looking harmless.

@0xAnon101
Copy link

While we acknowledge it is a small change, Cisco Talos is moving to replace our use of the terms ‘blacklist’ and ‘whitelist’ with ‘block list’ and ‘allow list,’” according to the Cisco Talos team

Google had already made some headway on swapping “blocklist” in for “blacklist,” with efforts having begun as early as May 2018 to remove the user-facing instances of “blacklist” and “whitelist” in Chrome.

I bet the author read up the article about IT companies using fairer words, but using block-list in block-chain makes it somewhat ambiguous. Your FILE_CHARS_DISALLOWED is OK

@laanwj
Copy link
Member

laanwj commented Sep 6, 2020

Is it your experience that most PRs in this project introduce harmful changes?

no, but the point of review is to pay attention to when they might. PRs that introduce 'harmful changes' will look like completely harmless changes that have a mistake (usually not intentional) in their logic

variable names do not affect the functionality of the code at all: and this isn't even code that is shipped in the release ! It's only used for testing—for all intents and purposes, the change was harmless

a better question: where were all you so worried about details like variable names, when #19227 was openened? why do you gang up here almost three months later?

@kkoenen
Copy link

kkoenen commented Sep 6, 2020

Denylist.

@achow101
Copy link
Member

achow101 commented Sep 6, 2020

why do you gang up here almost three months later?

Because some asshole tweeted about #19227 3 months after it was opened.

@dergigi
Copy link

dergigi commented Sep 6, 2020

Concept ACK

@initfortherekt
Copy link

INVALID_FILE_CHARS ?

@verretor
Copy link
Contributor Author

verretor commented Sep 6, 2020

INVALID_FILE_CHARS ?

The other two variables are named FILE_CHAR_START and FILE_CHAR_END.

@0xAnon101
Copy link

While we acknowledge it is a small change, Cisco Talos is moving to replace our use of the terms ‘blacklist’ and ‘whitelist’ with ‘block list’ and ‘allow list,’” according to the Cisco Talos team

Google had already made some headway on swapping “blocklist” in for “blacklist,” with efforts having begun as early as May 2018 to remove the user-facing instances of “blacklist” and “whitelist” in Chrome.

I bet the author read up the article about IT companies using fairer words, but using block-list in block-chain makes it somewhat ambiguous. Your FILE_CHARS_DISALLOWED is OK

So, are we fixed on FILE_CHARS_DISALLOWED ? Who will make the final decision? Most of the community still disagrees with disallowed keyword.

@laanwj
Copy link
Member

laanwj commented Sep 6, 2020

I'm going to merge this when it has passed the CI checks.

@karozagorus
Copy link

karozagorus commented Sep 6, 2020

We need to reach compromises over time. Disallowed is an acceptable compromise.

@laanwj laanwj merged commit af8135e into bitcoin:master Sep 6, 2020
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet