[ SmokeDetector | MS ] Bad keyword in body: Media Queries Not Working in WordPress by Jeffrey Johnson on stackoverflow.com

sd why

@DJMcMayhem [:41051297] Body - Position 1463-1479: Contact Us Today

fp- by DJMcMayhem

@quartata @tripleee that merge request from Eric?

has merge conflicts. I'll resolve them

and merge it. As soon as CI passes.

merge conflict resolved, CI is running again though.

so I'll go stab it once that's done.

Merged SmokeDetector #1218.

!!/pull

@ThomasWard CI build is still pending, wait until the build has finished and then pull again.

tosses pocket sand at @SmokeDetector

CI on b9c2088 succeeded.

!!/pull

@ThomasWard CI build is still pending, wait until the build has finished and then pull again.

LIES

CI COMPLETED

CI on ba3697f succeeded. Message contains 'autopull', pulling...

[ SmokeDetector ] SmokeDetector started at rev ba3697f (metasmoke: Merge pull request #1218 from Charcoal-SE/auto-blacklist-1510244780.952129) (running on angussidney/EC2)

Restart: API quota is 13164.

[ SmokeDetector | MS ] Repeating characters in answer: Write array to serial port by user39687 on arduino.SE

@ThomasWard can I get a review from you? gitmanager is kinda your thing too

tpu- by DJMcMayhem

@ThomasWard there are two separate rounds, one for each commit ... if you resolve a conflict there's a separate commit for that

but all good now?

@quartata you saw the failure? circleci.com/gh/Charcoal-SE/SmokeDetector/12725

[ SmokeDetector | MS ] Bad keyword in title: Human-Avian Hybrid wing span and muscle position by Dr Skelle Timaka on worldbuilding.SE

fp- by DJMcMayhem

@tripleee you're too slow, I already fixed it >:D

@quartata oooh (-:

yay look at all the green checkmarks

!!/test-a spam.sociat.com/watch-me

@quartata any way to tackle issue #776 if you are messing with git hooks anyway?

hmmm

I think unfortunately Github won't respect a merge driver

But maybe there's a webhook equivalent

github.com/isaacs/github/issues/487 :(

maybe don't bother if we are going to move these lists out of Git soon enough anyway

Setting it merge=union would be sufficient, but it doesn't look like GH will even do that

!!/blacklist-website classifiedadposter\.com

@tripleee Blacklisted classifiedadposter\.com

spam domain tracker only has one of the hits but the Halflife transcript has the other two

metasmoke.erwaysoftware.com/domains/15871

CI on cb5dea9 succeeded. Message contains 'autopull', pulling...

[ SmokeDetector ] SmokeDetector started at rev cb5dea9 (SmokeDetector: Auto blacklist of classifiedadposter\.com by tripleee --autopull) (running on angussidney/EC2)

Restart: API quota is 12919.

throws grenades at @quartata

you broke existing processes

what?

what did I do

check my reviews

i gave you three big comments and a "Go check the comments" statement

> This should be kept somewhere. We need to make sure we're actually on HEAD and not in a detached or reverted state. Which is incredibly important.

We want to be in a detached HEAD.

All I'm doing is checking out the latest remote commit

> As you refactor it, this will prevent in-place activation of changed things if one has code perms.

Honestly, this shouldn't happen before CI runs anyways

@quartata but it's been known to happen previously

it's one of the big issues we had hence we put the code in there to reset any local uncommitted changes.

that's what the reset is for to start with BEFORE we make revisions

Which doesn't work.

That's what the HEAD isn't at the tip error is all about, right?

i'm confused by your question

Dumb git stuff like that happens all the time, and it completely cripples our blacklisting until someone fixes it

I'm bypassing that by not touching the local master until autopull

@quartata what if a given instance of smokey is not in line with remote?

Precisely.

no i'm talking about with your case

I'd rather it succesfully blacklist, then fail to pull than not blacklist at all

i don't see where you address this in the code

which is what it does currently

because it wont' blacklist successfully if you are already in a detatched HEAD state

it'll fail git push

and you have the same problem

We're not pushing from master

the merge will fail

We're checking out the latest origin/master commit

committing on top of that

where?

@quartata I think Thomas is right, you can't push from detached

and pushing directly from that ref

@tripleee You absolutely can. I know this because I just did it 30 minutes ago while testing.

@quartata you can't do that safely

git push origin HEAD:master

@quartata that's unsafe

and exactly why I rejected

consider:

rev 2 is "foobarbaz"

rev 1 is "foobar"

rev 3 is rev1+"boom"

which means rev3 is foobarboom

git will see rev3 and ignore rev2

and fail to properly include rev2

How are these histories laid out?

so you're preventing rev2 from being included in the consideration points of the merge.

@quartata git commit histories.

Yeah, I know. What branches? Local or remote?

literally git log

@quartata rev3 is local, rev1 and 2 are remote.

So you're saying:

but rev3 was built on reverted head from rev 1

origin/master: 1 -> 2

master: 1 -> 2 -> 3

@quartata just serially, there is a race condition if you pull then push "in the blind" because if somebody else pushed between your pull and your push, those commits will be gone

no

@ThomasWard But it won't be.

The HEAD comes from git fetch origin master.

@quartata origin/master: 1->2; local/master: 1

It's the latest remote commits, completely untouched by anything local

new change on local = 1->3

We're fastforwarding the blacklist commit onto that

@ThomasWard But that wouldn't happen.

show me in your code where you prevent that

because I don't see that in your code.

and as a result I don't believe your code functions as you think it does. Unless you can show me absolute proof that it works as is, and show me that it's been shown to work in a forked repository.

The fetch happens here: github.com/Charcoal-SE/SmokeDetector/pull/1216/…

We switch from deploy to a detached HEAD, made from the latest remote commit: github.com/Charcoal-SE/SmokeDetector/pull/1216/…

"absolute proof" is a bit of a tall order but a demo script which produces this race condition and handles it successfully would be a start

We push from the detached HEAD: github.com/Charcoal-SE/SmokeDetector/pull/1216/…

@tripleee that's what i consider absolute proof.

it should be in the test suite really, only the current git manager is impossible to test

@quartata no, we push from a new branch based on master.

@ThomasWard Nope.

that's not a 'detatched head' that's a 'new branch'

It's done straight from the detached head.

push takes a refspec

HEAD:master

@quartata make a proof of concept script using this. Execute such script.

show it works

Sure. I've done it multiple times now. One second.

"multiple times now" is useless unless you show us your full test case.

OK, plain GH repo: github.com/quartata/test

and consider i'm in a pissed off mood having to have paid the FCC coordinators another $200 just to get the freaking radio application done, so I'm really not in the best mood right now.

I'm cloning from the initial commit: git clone https://github.com/quartata/test

forgive the harshness.

No, it's fine.

@tripleee I don't think @quartata knows what I mean by 'test case' and 'test script'

I'm going to put it together into one script for you to try

But I want to show you the full process, step by step

uncricks neck, and then sighs

@ThomasWard if it can be done manually at first, certainly it can be scripted and eventually turned into an actual test case in the code

Actually, first I guess I'll make deploy a remote branch

@tripleee bleh i'm tired, your job: yell at me if I seem to be too harsh :P

because you're here right now.

OK so I'm going to make a commit on master that adds something new

@ThomasWard just a bit of patience I think is all we need...

github.com/quartata/test/commit/…

Histories look like this now:

origin/master: 1 -> 2
master: 1

I'm going to make a local change now so that it looks like this:

origin/master: 1 -> 2
master: 1 -> 3

This is what you were envisioning, right?

For the initial state.

mhm

OK, so currently Github's master is 1 -> 2 but my local origin/master ref doesn't have 2.

So I'm going to do git fetch origin master

does your local change 2 touch the same file as the unmerged upstream commit 3?

Yes.

Local:

$ cat README.md
# test

asdf2

what's remote currently

this github.com/quartata/test/commit/…

# test

asdf1

I'm currently on local master, I'm going to execute git checkout origin/master

the link contains the diff, rev 1 had just the first line # test

> You are in 'detached HEAD' state.

I've made a commit, pushing from the detached HEAD

And now:

origin/master: 1 -> 2 -> 4
master: 1 -> 3

git push origin HEAD:master

Now, to be fair:

> Your branch and 'origin/master' have diverged

github.com/quartata/test/commit/… <- that's 4 I guess

But this time, it at least "blacklisted". Rather than failing completely on the merge

but asdf2 is lost ...?

that's my point

that's lost.

@quartata if the conflict is in the blacklist because something else is already blacklisted

you just destroyed it.

and it will be lost forever.

my case, exactly in point.

THEREFORE: reject.

But we don't commit blacklists to local master

If someone did that, it was a human error that should not be pushed

@quartata but consider this.

if we have a new branch

and we try and autopull merge that

but we ahve another item already handled and merged

we have merge conflicts from here to high heaven until everything is in sync again if we base things off a given master branch rev.

we still don't have a good idea of how concurrent commits to local might trample each other. The tread lock I put in should prevent that but I still don't have any good way to write a test case for that

that's the problem.

@tripleee and exactly why we need to keep that

@quartata what happens if you create a new branch with the original master, add in a commit that doesn't include the second newer upstream commit at the remote, in the new branch edit things, and try to merge

will it conflict?

what's git say? If it does conflict, then it's also a failure case

because it can't merge reliably code privs or not.

@quartata Pull in my PR without destroying it following your process

also consider we'll autopull so if the conflict happens there too then you've also just fubared all of Smokey.

the nested blanket except handlers also complicate things, where do you end up when something fails?

^ that

@quartata you'll note that there's now global pushback here because you don't handle any conflicts properly.

this is why we reset to upstream HEAD with Master before trying to create the new blacklist.

because we will have merge issues.

we only get merge issues currently if we have two blacklist requests which result in conflicts, or one gets merged in before the other by metasmoke and in turn is a race condition we can't solve regardless.

I'm all for rewriting this but I'm afraid we will probably need to refactor the current functionality to something we can actually test before we do anything else

@tripleee I agree.

@tripleee can you make a note about this discussion and such on the PR?

there are probably failure modes we could shake out pretty quickly if we could run a test suite locally without having a mess pushed to the production master

GH is lagged on me

I'll try to articulate something

just bookmark this discussion, and link to it and say "This is why we should reject, given this 'test case' which clobbers prior commits.

> Talked about stuff. A decision might have been made. I dunno. Maybe?

and then force people to read the discussion chain.

@Andy nailed it

lol

remind me, this is PR ... 1217?

1216

thanks, I was close (-:

@tripleee you were also review-requested

so if you do a "Needs changes" on it that's 2 rejects.

yeah, I guess I can use that

@ThomasWard Sorry I can't really talk more right now, I'll get back to you in about an hour

i'll be busy in an hour too :P

blah

meh, I created a competing bookmark chat.stackexchange.com/rooms/11540/conversation/…

LOL

conflict101

mine is bigger than yours

not sure that's a win

lol

MERGE CONFLICT!

BOOM!

DESTRUCTION!

THE END IS NIGH

shot

@quartata I hope you are not discouraged, this is a good effort

^ this

however there's cases that do need addressed before we accept a refactorization of this code.

we haven't even begun to talk about recovering in the event that somebody else pushes to master while you think you have the latest revision, Git obviously prevents you from pushing but then what?

the current implementation basically just goes into limbo, you can't do any git operations until it is sorted out locally

which is pesky but at least tractable

Someone is taken out back and subjected to hours of this

We could set merge=onion inside the git attributed for the blacklist/watch files, this changes the ways how git resolve the merge conflicts, and in turn can auto solve most of the conflicts we get with multiple insertion that happens when there are conflicting pull requests

learned my lesson, only check out Youtube links with curl

Barney&#39;s Pajama Party 2001

Hey, I didn't Rick Roll you. That should count for something.

@Ferrybig my Google skills bring me no obvious hits, do you have a pointer?

@tripleee git-scm.com/docs/gitattributes#gitattributes-union

thanks

oh "union", no wonder I only got steak recipes

> merge=onion

lol

!!/blame

@DJMcMayhem It's tripleee's fault.

foul!

I only now see that I misspelled it.. Oops

all good (-:

@tripleee perpetual explosivity.

[ SmokeDetector | MS ] Potentially bad keyword in body: Canon Printer Driver? by user374824 on meta.SE

tpu- by teward

@Andy expected rickroll, was unpleasantly surprised :(

I think Andy is a torture expert

@ArtOfCode no there's far more ruthless and vicious torture experts

you're looking at one of them

adjusts halo on devil horns slightly

@ThomasWard find me worse YouTube torture than that then :P

trades devil horns for shadowy mists of darkness around him

@ArtOfCode this for 10 hours

there's also the being strapped to a chair and forced to endure fifty hours of the numa numa song :P

then there's the PHYSICAL torture

Andy is pro youtube torturer.

@ThomasWard nah, not as bad

DOn't ask me how dark the evil is in my mind :P

it's mostly monotonal, you can tune it out really easily

:P

The skill of an older sibling :)

@ThomasWard Back for just a second, so I just wanted to say that I think this got blown way out of proportion. This was never intended to fix everything with Git (nor could it). It was supposed to fix one very particular problem: when there are local changes, blacklist fails when trying to merge and it's not blacklisted locally or remotely. All this does is add the entry right onto the latest remote blacklist, so even if it can't merge into local master it'll still be there on the remote

That's all I wanted. This isn't a regression.

@quartata we're more concerned about clobbering remote commits

you're right it's not.

but if FOR WHATEVER REASON remote had something your local didn't

you lose it with this.

or Git refuses to push, which is back to square one

mhm

well 10kers and owners can't suspend people lol

diamonds can.

Moderators can also edit or delete any message, as well as super-ping any user, right?

Can room owners (who aren't also moderators) unfreeze their rooms?

@EliahKagan no

don't think so

@EliahKagan nope

@JeremyBanks anonymize messages

Annotations

beeps

Accessing any gallery room

^ that should help

or rather

that answer.

!!/blacklist-keyword rejuvilane

@tripleee Blacklisted rejuvilane

CI on 07ac17f succeeded. Message contains 'autopull', pulling...

[ SmokeDetector ] SmokeDetector started at rev 07ac17f (SmokeDetector: Auto blacklist of rejuvilane by tripleee --autopull) (running on angussidney/EC2)

Restart: API quota is 11880.

@JeremyBanks talking in frozen/deleted rooms is another

also talking in rooms in timeout

@Mithrandir a room owner would have that permission though :)

making private rooms

[ SmokeDetector | MS ] Link at end of answer: JSON APIs in PHP, across the internet by Remy Houard on softwareengineering.SE

fp- by Glorfindel on Solved: How to make JS template out of html? [MS]

fp- by Glorfindel

[ SmokeDetector | MS ] Bad NS for domain in answer: WIll electric pressure washer work with low water pressure? by Ashlyn Roy on diy.SE

[ SmokeDetector | MS ] Blacklisted website in body: Connection timeout from Webserver to Proxyserver by aragonthebest on serverfault.com

sd why

@DJMcMayhem [:41054928] Body - Position 1147-1158: luminati.io

fp- by DJMcMayhem

@SmokeDetector f

@ThomasWard So you're saying if someone pushed a commit after the fetch?

@quartata yes, which could happen, but the fetch only fetches the refspec, it doesn't update what you have in your repository locally

Git would just refuse the push but I'll try it

@quartata and that's what we want to avoid.

because we could have that happen. Hence why we need to work within the existing workflow and iron out bugs and problem cases before we just refactor it

Wouldn't it also happen currently?

Like, after the HEAD reset

If you timed it right

We would need some kind of lock

maybe on MS

@quartata probably, but the chance of that is rare

with yours it's a lot more likely

and the only way we could have an MS lock is if we offload all the git stuff to MS to check if there's a diff

there's really no way around it in the long term but...

our current way fails to commit rather than just clobbers commits

we mitigate the risk a little with the HEAD reset

@ThomasWard How is it more likely? The window of opportunity is between the fetch and the push

It's maybe two seconds, tops

Also it's not going to clobber remote commits. I'm not force pushing

Hmm I wonder if Github lets you set a pre-push webhook

!!/blacklist-keyword (?:1\W*)?408\W*818\W*9369

@tripleee Blacklisted (?:1\W*)?408\W*818\W*9369

CI on 83bc40f succeeded. Message contains 'autopull', pulling...

Restart: API quota is 11162.

Commented on the PR

cc @tripleee

Honestly I feel like saying detached HEAD scared everyone because that's usually a bad thing

But really it's just an anonymous branch. I'm branching from origin/master instead of master, everything else is almost identical

I'm still scared of going live with this simply because it changes the model to one we are not yet familiar with. If only there was a way to test it without messing up production...

There is.

But it'll take me some time because I have to set up a new repo

Can't use the fork.

oh yeah, that's a workaround ... but a costly one

Testing the race condition will be the hardest. I can add a sleep to increase the window of opportunity

oops I made a bunch of typos

I'll fix them when I'm home, apologies

[ SmokeDetector | MS ] Blacklisted username: Multistore redirect from old.dk to new.dk by Fuck you on magento.SE

tpu- by DJMcMayhem

Hahahahahaha

o_O

naa- by rene

[ SmokeDetector | MS ] No whitespace in body: hekjnws,dxwns,adexs by Martil on stackoverflow.com

sd v

[ SmokeDetector ] SmokeDetector started in standby mode at rev 83bc40f (SmokeDetector: Auto blacklist of (?:1\W)?408\W*818\W*9369 by tripleee --autopull*) (running on Henders/EC2)

Just failing over to check mine is running correctly

Henders/EC2 received failover signal.

Restart: API quota is 19999.

!!/location

@Henders angussidney/EC2

@Henders Henders/EC2

!!/standby a

[ SmokeDetector ] SmokeDetector started in standby mode at rev 83bc40f (SmokeDetector: Auto blacklist of (?:1\W)?408\W*818\W*9369 by tripleee --autopull*) (running on angussidney/EC2)

!!/location

@Henders Henders/EC2

Yeah so when my instance died the other day with the 'head not at tip of branch etc..' was caused by the same thing as last time.

@ArtOfCode I just did the same thing as last time (see the message below that) and it fixed it again...

While MS was a bit wonky.

[ SmokeDetector | MS ] Bad keyword in answer: Is 'I want to thank somebody for having done something' a valid use of present perfect? by Jeff Bendum on ell.SE

@SmokeDetector f

[ SmokeDetector | MS ] Bad keyword in body, bad keyword in title, bad keyword with email in body, email in body, pattern-matching email in body: Do you need urgent loan? by Deo Christopher on money.SE

tpu- by NobodyNada