Disputed code from Commaster

[hakusaro]

As recently mentioned in #1460, Commaster has made the accusation that we stole specific code introduced in one of his pull requests without crediting him. I'm going to quote everything he said here:

Oh really, we are talking about giving credit? Well, that wasn't the case with #781 which were blatantly stolen in 16cf364 and 038f606. Shall we start fresh? You remove https://github.com/Pryaxis/TShock/blob/general-devel/TShockAPI/Commands.cs#L1288-L1334 and https://github.com/Pryaxis/TShock/blob/general-devel/TShockAPI/Commands.cs#L1389-L1423 permanently from the codebase and then we can talk about my contributions. But until then, keep stealing my work

After reviewing the diffs, I was able to see several similarities in how our code functions versus his. A good example of this is the eerie implementation of bool force = !args.Player.RealPlayer; which names the variable force and assigns it in a way that is atypical from the rest of the codebase. That being said, the ban code is somewhat of a fixed place system and it could be possible we've just accidentally written similar code to him.

Again, look at https://github.com/Pryaxis/TShock/pull/781/files#diff-72108b2e5b27cef0ef5199d364dafc54R1202 and https://github.com/Pryaxis/TShock/commit/038f6069397bcf73f23e8b1228de68f4ddd96542#diff-72108b2e5b27cef0ef5199d364dafc54R1163. Eerie similarity? Or just a coincidence?

Open source somewhat dictates that we should at least investigate this accusation at minimum, and perhaps look into rectifying it by either crediting Commaster or actually rebuilding this block of code/system.

Because this process is somewhat time consuming, I would like to open it up to the community to help identify similarities as well. If code looks eerily familiar across the PR and our codebase, it should be noted here and discussed.

If the issue devolves into a flame war it's going to get locked, though. Locking the issue would slow down the diff process and stifle debate, so I don't really want to do that.

[bartico6]

You would be correct, the way bool force is defined is in fact really odd. In fact, so odd that it shows up in only two places of the entire codebase - the two disputed snippets.
With that said... there's only so many ways you can implement banning an offline user (you have to fetch them from db, you have to check their known IPs and you have to check if the admin can ban said user).
Worst of all, there's only so many permutations of the instructions, sadly. And all of them will look "kind of" like @Commaster's code, if you see what I'm getting at.

And with THAT said the tShock ban system could really use some freshening up because it quite frankly sucks in the current state. This could be a good opportunity to do that. And don't point fingers at me, I'm sufficiently occupied as it is.

[hakusaro]

@bartico6 there is one other place that force is used -> in the arguments to kick or ban (in utils, I'm a bit sleepy). That's the only exception, however. I didn't investigate the time variable, but that one also seems to indicate weirdness too.

[bartico6]

It isn't defined as a NOT RealPlayer anywhere else, though. The "force" argument has been a thing in tShock for ages, I agree.

I in no way implied that the code is copied, by the way. I'm only saying that there's only so many ways you can do what @Commaster's code does and I'm afraid that this is such a generic function you can't really dispute it as copied. You can see in the git diff that some code was copy-pasted around and some looks based off of @Commaster's code but even with all that in place it could be a coincidence and unless @MarioE tells us we'll never know.

I think that coming to an agreement with @Commaster about this (either settling that this is too small to make such a fuss about, or just credit him like it's his pull request and drop the topic) is the best idea. Inspecting such a small snippet is a waste of time, especially that banning an offline user, as I said, can only be done in so many ways, especially when you compare to how tShock bans always worked (check player, check permission etc)

[hakusaro]

You can see in the git diff that some code was copy-pasted around

I think that the problem with this rationality is that tools like ReSharper can be used to effectively normalize things like spacing, indentation, etc.

I in no way implied that the code is copied, by the way.

Kinda hoping you either come to the conclusion that, in your opinion, it was or wasn't copied. I would much rather base results of this on what people think firmly than not. Which side do you lean to?

[bartico6]

Analysis time?

Self-explanatory. If the code was copied, the functionality would've been there. It's similar, but not the same.

Same as above, it's similar but not the same.

And this was just moved slightly.

Bonus points, I went ahead and googled the [somewhat characteristic] name of the feature. It would seem that it's mentioned around Russian boards (and even was an actual public plugin at some point!)

The plugin shown above was uploaded on 5th March 2014, the Pull Request was made on 20 April 2014.
Hasn't this happened multiple times that a tShock plugin was "consumed" and became part of tShock's functionality anyway?

In my opinion the code wasn't really copied, it could've at most been "based" off's Commaster's PR [or a plugin with the same name, as shown above].
Besides, it's a piece of code so small it's actually downright silly to even argue about it. Are you gonna fetch a user in a different way? Store IPs with a magic character as the separator instead of a JSON array? Are you gonna look for permissions of the banning user in another way?
No. This is the simplest way, and it's not even copied [or it's cleverly hidden]

[Commaster]

Well hello there.

I see quake1337 still thinks, that most of the time is spent on coding a feature. Well, I'm afraid I have to disappoint you. Most time is spent on figuring out the way. And, haha, once someone explains, how to do it, it feels obvious to everyone else.

You found a plugin with a similar name? And what do we see after downloading it? Oh, looky here, it's me. Someone downloaded the plugin while it was public and uploaded it to that shady website.

Obviously that feature was needed so much, that you can google a few "cries for help" on TShock forums after the plugin got removed. If "the way to do it" had been THAT OBVIOUS, someone (like MarioE) would have done it before me. But alas...

It's the very same case with the recent issue: Nobody could find the source of the problem so nobody COULD create a solution. My contribution (already) is in pointing out the source. And now that everyone knows it, they can start dishing out solutions left and right, "contributing".

I stand my point. Not the exact code matter, but the idea and the logic "how to do it". Everyone can change a few pixels here and there and start call it their own.

[Ijwu]

I do not see an argument refuting @bartico6's analysis.

@Commaster I respectfully request you help us see your point of view without a condescending tone. We're taking your claim seriously and I request you take us seriously as well. Is there something specifically which you'd like to dispute in @bartico6's analysis?

We aren't disputing your authorship of the plugin nor its presence on online forums. Please bring objectivity to this discussion.

[hakusaro]

@Commaster I honestly read your response twice and didn't understand what you were going for.

I asked in an earlier thread that you tell us whether or not you would prefer to be credited or have any potentially infringing work removed if we're able to conclude that you were copied. You haven't yet answered this question.

[Commaster]

As mentioned in https://github.com/Pryaxis/TShock/issues/1460#issuecomment-348192980 I would like the discussed functionality removed, now that it's 2017

[hakusaro]

Cool, as you can see, we’ve already rewritten this command and will move forward on it later. This issue will remain open until the other PR has been merged.

From now on, please consider this issue resolved. Further comments that have language and tone in an accusatory manner (e.g., “feel free to steal this fix too”) will be dealtwith under the handbook guidelines and as part of the TShock Code of Conduct.

[Commaster]

Can you still ban offline players/users/accounts in a similar fashion? Yes? Then it's not resolved.

Please be advised to remove said functionality in the nearest update.

[QuiCM]

Hi, I'm afraid that's not the way this works.

While you may have owned the IP for a specific implementation of an offline banning subsystem, you certainly don't own the IP for all offline banning subsystems.

We've attempted to be reasonable. I'm now marking this issue as resolved because it's obvious you have no intention of being mature.

[hakusaro]

@Commaster, no. Go away.