search

Unity-Technologies / qstat

New official qstat repository
Artistic License 2.0
121 stars 33 forks source link

Add json support, fix #10, ref XQF/xqf#12 #13

Closed illwieckz closed 7 years ago

illwieckz commented 9 years ago

Hi @stevenh, this PR imports patches from the @eXprojects's qstat-json fork.

This fork was based on a source tarball, so was amnesic to all the revision history, also, this fork included mistakenly some temporary build files.

I've rewritten by hand the two useful patches and I committed them with the original author (Steve Teuber <steve<ad>exprojects.org>) and the original author date for each commit.

After that I've modified the source to add json support to games that were added since the release tarball Steve used (like starmade, dirtybomb, farmsim and ksp) and fixed the ventrilo omission he did.

After that I renamed name_xfroms to xform_names and used xform_printf() instead of fprintf() in json code, like in e777674 (Refactored output splitting out xform processing into its own files improving various portions of it.)

So, some intermediate commits cannot build, but the whole PR builds. I think we can't do better than that without rewriting Steve's commits, which raises legal issues: it must be clear to know who have done what and when.

@skybon, you will probably be interested by this PR.

vorot93 commented 9 years ago

@illwieckz Nice work! Could you please rewrite the first two commits using @eXprojects current GitHub email? Thanks.

illwieckz commented 9 years ago

I used the email address @eXprojects used himself, see eXprojects/qstat-json/commit/1c56085.patch for example. Legally I think it's the better thing to do.

vorot93 commented 9 years ago

@illwieckz GitHub does profile linking using email. Since first and foremost license legal requirement is to make proper attribution I believe it is better to use @eXprojects' current email.

illwieckz commented 9 years ago

I think the only user email Github reveals is the commit one.

vorot93 commented 9 years ago

@illwieckz And that's the email those two commits should have too :-) Check out latest commits here

illwieckz commented 9 years ago

Well, the better is to ask @eXprojects himself if he wants I rewrite the commits with his Steve Teuber <steve.teuber<ad>idealo.de> author info.

illwieckz commented 9 years ago

I've emailed Steve to ask him which address he wants (he can also just add the former address to his GitHub profile to get profile linking on that commit).

illwieckz commented 9 years ago

so, the code now splits qstat.c to output-standard.c, output-xml.c and output-json.c (all with a .h file).

template.c was renamed to output-template.c and the missing output-template.h was created, and output-template.h now rely on qstat.h (before that, some parts were copypasted in template.c).

illwieckz commented 9 years ago

So there is more code rewritten and cleaned than added, it's more a cleaning effort than a code addition…

illwieckz commented 9 years ago

@stevenh, can you help me on these parts: https://github.com/multiplay/qstat/pull/13#discussion-diff-33666273 https://github.com/multiplay/qstat/pull/13#discussion-diff-33666396 https://github.com/multiplay/qstat/pull/13#discussion-diff-33667166

:smiley: ?

illwieckz commented 9 years ago

Ok, I've added the -nx/-nnx options to qstat usage print, and removed one of the two -nh indications. I also unified the uppercase/lowercase usage description, then I wrote a printf_opt function to align options properly, then I rearranged the usage display. Now it looks like that:

$ ./qstat -cfg qstat.cfg --help
Usage: ./qstat [options] [-default server-type] [-cfg file] [-f file] [host[:port]] ...
Where host is an IP address or host name

Configuration options:
    -cfg                 Read the extended types from given file not the default one
    -nocfg               Ignore qstat configuration loaded from any default location. Must be the first option on the command-line.

Game query options:
    -a2s                 Query Half-Life 2 new server
    -alienarenam         Query Alien Arena Master server
    -alienarenas         Query Alien Arena server
    -ams                 Query America's Army v2.x server
    -bfbc2               Query Battlefield Bad Company 2 server
    -bfs                 Query BFRIS server
    -cod2m               Query Call of Duty 2 Master server
    -cod2s               Query Call of Duty 2 server
    -cod4m               Query Call of Duty 4 Master server
    -cod4s               Query Call of Duty 4 server
    -codm                Query Call of Duty Master server
    -cods                Query Call of Duty server
    -crs                 Query Command and Conquer: Renegade server
    -crysis              Query Crysis server
    -cubes               Query Sauerbraten server
    -d3g                 Query Descent3 Gamespy Protocol server
    -d3m                 Query Descent3 Master (PXO) server
    -d3p                 Query Descent3 PXO protocol server
    -d3s                 Query Descent3 server
    -dirtybomb           Query DirtyBomb server
    -dm3m                Query Doom 3 Master server
    -dm3s                Query Doom 3 server
    -efm                 Query Star Trek: Elite Force server
    -efs                 Query Star Trek: Elite Force server
    -etlm                Query Enemy Territory: Legacy Master server
    -etls                Query Enemy Territory: Legacy server
    -etqws               Query QuakeWars server
    -eye                 Query All Seeing Eye Protocol server
    -farmsim             Query FarmingSimulator server
    -fcs                 Query FarCry server
    -fls                 Query Frontlines-Fuel of War server
    -gps                 Query Gamespy Protocol server
    -grs                 Query Ghost Recon server
    -gs2                 Query Gamespy V2 Protocol server
    -gs3                 Query Gamespy V3 Protocol server
    -gs4                 Query Gamespy V4 Protocol server
    -gsm                 Query Gamespy Master server
    -h2s                 Query Hexen II server
    -hazes               Query Haze Protocol server
    -hl2s                Query Half-Life 2 server
    -hla2s               Query Half-Life server
    -hla2sm              Query Steam Master server
    -hlm                 Query Half-Life Master server
    -hlqs                Query Half-Life server
    -hls                 Query Half-Life server
    -hrs                 Query Heretic II server
    -hwm                 Query HexenWorld Master server
    -hws                 Query HexenWorld server
    -iourtm              Query Urban Terror Master server
    -iourts              Query Urban Terror server
    -jk2m                Query Jedi Knight 2 server
    -jk2s                Query Jedi Knight 2 server
    -jk3m                Query Jedi Knight: Jedi Academy server
    -jk3s                Query Jedi Knight: Jedi Academy server
    -kps                 Query Kingpin server
    -ksp                 Query Kerbal Space Program server
    -maqs                Query Medal of Honor: Allied Assault (Q) server
    -mas                 Query Medal of Honor: Allied Assault server
    -mhs                 Query Medal of Honor: Allied Assault server
    -mumble              Query Mumble server
    -netp                Query NetPanzer server
    -netpm               Query NetPanzer Master server
    -nexuizm             Query Nexuiz Master server
    -nexuizs             Query Nexuiz server
    -openarenam          Query OpenArena Master server
    -openarenas          Query OpenArena server
    -ottdm               Query openTTD Master server
    -ottds               Query OpenTTD server
    -preym               Query Prey Master server
    -preys               Query PREY server
    -prs                 Query Pariah server
    -q2m                 Query Quake II Master server
    -q2s                 Query Quake II server
    -q3m                 Query Quake III Master server
    -q3rallym            Query Q3 Rally Master server
    -q3rallys            Query Q3 Rally server
    -q3s                 Query Quake III: Arena server
    -q4m                 Query Quake 4 Master server
    -q4s                 Query Quake 4 server
    -qs                  Query Quake server
    -qwm                 Query QuakeWorld Master server
    -qws                 Query QuakeWorld server
    -reactionm           Query Reaction Master server
    -reactions           Query Reaction server
    -rss                 Query Ravenshield server
    -rwm                 Query Return to Castle Wolfenstein Master server
    -rws                 Query Return to Castle Wolfenstein server
    -sas                 Query Savage server
    -sfs                 Query Soldier of Fortune server
    -sgs                 Query Shogo: Mobile Armor Division server
    -smokingunsm         Query Smokin' Guns Master server
    -smokingunss         Query Smokin' Guns server
    -sms                 Query Serious Sam server
    -sns                 Query Sin server
    -sof2m               Query SOF2 Master server
    -sof2m1.0            Query SOF2 Master (1.0) server
    -sof2s               Query Soldier of Fortune 2 server
    -starmade            Query StarMade server
    -stm                 Query Steam Master server
    -stma2s              Query Steam Master for A2S server
    -stmhl2              Query Steam Master for HL2 server
    -t2m                 Query Tribes 2 Master server
    -t2s                 Query Tribes 2 server
    -tbm                 Query Tribes Master server
    -tbs                 Query Tribes server
    -tee                 Query Teeworlds server
    -terraria            Query Terraria server
    -tm                  Query TrackMania server
    -tremfusionm         Query TremFusion Master server
    -tremfusions         Query TremFusion server
    -tremulousgppm       Query Tremulous GPP Master server
    -tremulousgpps       Query Tremulous GPP server
    -tremulousm          Query Tremulous Master server
    -tremulouss          Query Tremulous server
    -ts2                 Query Teamspeak 2 server
    -ts3                 Query Teamspeak 3 server
    -turtlearenam        Query Turtle Arena Master server
    -turtlearenas        Query Turtle Arena server
    -uns                 Query Unreal server
    -unvanquishedm       Query Unvanquished Master server
    -unvanquisheds       Query Unvanquished server
    -ut2004m             Query UT2004 Master server
    -ut2004s             Query UT2004 server
    -ut2s                Query Unreal Tournament 2003 server
    -ut3s                Query UT3 server
    -vent                Query Ventrilo server
    -warsowm             Query Warsow Master server
    -warsows             Query Warsow server
    -waws                Query Call of Duty World at War server
    -wics                Query World in Conflict server
    -woetm               Query Enemy Territory Master server
    -woets               Query Enemy Territory server
    -wolfs               Query Wolfenstein server
    -wopm                Query World Of Padman Master server
    -wops                Query World Of Padman server
    -xonoticm            Query Xonotic Master server
    -xonotics            Query Xonotic server
    -zeq2litem           Query ZEQ2 Lite Master server
    -zeq2lites           Query ZEQ2 Lite server
    -default             Set default server type

Server types: a2s alienarenam alienarenas ams bfbc2 bfs cod2m cod2s cod4m cod4s codm cods crs crysis cube2 d3g d3m d3p d3s dirtybomb dm3m dm3s efm efs etlm etls etqws eye farmsim fcs fls gps grs gs2 gs3 gs4 gsm h2s hazes hl2s hla2s hla2sm hlm hlqs hls hrs hwm hws iourtm iourts jk2m jk2s jk3m jk3s kps ksp maqs mas mhs mumble netp netpm nexuizm nexuizs openarenam openarenas ottdm ottds preym preys prs q2m q2s q3m q3rallym q3rallys q3s q4m q4s qs qwm qws reactionm reactions rss rwm rws sas sfs sgs smokingunsm smokingunss sms sns sof2m sof2m1.0 sof2s starmade stm stma2s stmhl2 t2m t2s tbm tbs tee terraria tm tremfusionm tremfusions tremulousgppm tremulousgpps tremulousm tremulouss ts2 ts3 turtlearenam turtlearenas uns unvanquishedm unvanquisheds ut2004m ut2004s ut2s ut3s ventrilo warsowm warsows waws wics woetm woets wolfs wopm wops xonoticm xonotics zeq2litem zeq2lites

Content options:
    -sort                Sort servers and/or players
    -u                   Only display servers that are up
    -nf                  Do not display full servers
    -ne                  Do not display empty servers
    -R                   Fetch and display server rules
    -P                   Fetch and display player info

Format options:
    -cn                  Display color names instead of numbers
    -ncn                 Display color numbers instead of names
    -hc                  Display colors in #rrggbb format
    -htmlmode            Convert <, >, and & to the equivalent HTML entities
    -htmlnames           Colorize Quake 3 and Tribes 2 player names using html font tags
    -nohtmlnames         Do not colorize Quake 3 and Tribes 2 player names even if $HTML is used in an output template.
    -carets              Display carets in Quake 3 player names
    -hpn                 Display player names in hex
    -hsn                 Display server names in hex
    -tc                  Display time in clock format (DhDDmDDs)
    -tsw                 Display time in stop-watch format (DD:DD:DD)
    -ts                  Display time in seconds
    -pa                  Display player address

Display options:
    -raw <delim>         Output in raw format using <delim> as delimiter
    -raw-arg             When used with -raw, always display the server address as it appeared in a file or on the command-line.
    -old                 Old style display
    -nh                  Do not display header
    -nx                  Enable name transformations, -nx is set by default
    -nnx                 Disable name transformations, -utf8 option implies -nnx
    -xml                 Output status data as an XML document
    -bom                 Output Byte-Order-Mark for XML output.
    -utf8                Use the UTF-8 character encoding for XML output
    -json                Output status data as an UTF-8 JSON document
    -Th,-Ts,-Tpt         Output templates: header, server and player
    -Tr,-Tt              Output templates: rule, and trailer
    -showgameport        Always display the game port in QStat output.
    -stripunprintable    Disable stripping of unprintable characters.
    -errors              Display errors
    -progress            Display progress meter on stderr (text only)

Output options:
    -of                  Output file
    -af                  Like -of, but append to the file

Query options:
    -f                   Read hosts from file
    -mdelim <delim>      For rules with multi values use <delim> as delimiter
    -retry               Number of retries, default is 3
    -interval            Interval between retries, default is 0.50 seconds
    -mi                  Interval between master server retries, default is 2.00 seconds
    -timeout             Total time in seconds before giving up
    -maxsim              Set maximum simultaneous queries
    -sendinterval        Set time in ms between sending packets, default 5
    -allowserverdups     Allow adding multiple servers with same ip:port (needed for ts2)
    -srcport <range>     Send packets from these network ports
    -srcip <IP>          Send packets using this IP address
    -H                   Resolve host names
    -Hcache              Host name cache file

Advanced options:
    -d                   Enable debug options. Specify multiple times to increase debug level.
    -dump                Write received raw packets to dumpNNN files which must not exist before
    -pkt <file>          Use file as server reply instead of quering the server. Works only with TF_SINGLE_QUERY servers
    -syncconnect         Process connect initialisation synchronously.
    -noportoffset        Dont use builtin status port offsets (assume query port was specified).

Help options:
    -h, --help           Print this help
    -protocols           Print the protocol list (json display format only)
    -v                   Print qstat version (can be displayed with -json format)

Sort keys:
  servers: p=by-ping, g=by-game, i=by-IP-address, h=by-hostname, n=by-#-players, l=by-list-order
  players: P=by-ping, F=by-frags, T=by-team, N=by-name

qstat version 2.15
illwieckz commented 9 years ago

OK, the only thing to fix is the protocol list, theres is already one in the usage() function.

illwieckz commented 9 years ago

@eXprojects, to query qstat version with json display, you must do that now:

qstat -json -v

to query protocols, now you must do that now:

qstat -json -protocols
illwieckz commented 9 years ago

@stevenh, OK this, PR is massive and it can be merged now, I hope you will be able to merge it soon, and I hope you will like all the improvements! :smiley:

illwieckz commented 9 years ago

The teeworld PR (#3) will be rebased after this PR merge, so, there is no reason to wait more.

stevenh commented 9 years ago

Sorry to be a pain but fixing style of "existing" code and splitting out other sections should be done in a separate PR so its easy to catch issues. As this stands currently its very hard to see what's new and what's just moved code.

Add to that unrelated other fixes e.g. https://github.com/illwieckz/qstat/commit/b1a54b6567ce3ead4563157d8ff38f7cfc7d8c94

illwieckz commented 9 years ago

Add to that unrelated other fixes e.g. illwieckz@b1a54b6

When you are doing things like this PR does, you must be sure there is no new warning introduced, and the better way is to have a warningless compile, so every warning is a new.

In fact, my first attempt of the commit illwieckz@c53e294 introduced some mistakes that appeared as warnings, I fixed the warnings in illwieckz@b1a54b6 to be able to fix the commit illwieckz@c53e294 .

You can't fix code if you keep mistakes.

illwieckz commented 9 years ago

As you can see in the history, I wrote the warning-cleaning commit 5 commit after the NULL pointer commit, it means because of this warning noise I've wrote 4 commits before seeing I've introduced a regression.

illwieckz commented 9 years ago

Sorry to be a pain but fixing style of "existing" code and splitting out other sections should be done in a separate PR so its easy to catch issues.

This is why I wrote different commits, after the merge, the PR will be nothing, catching issue is about commit.

Also, because of this work is huge, it would have been very difficult without cleaning some code to help to read the code.

The code can't be maintained if it's not readable.

stevenh commented 9 years ago

As a general rule each PR should be an individual commit, re-factoring / formatting code unrelated to the PR is a big no no.

Fixes made due to PR feedback should be squashed to avoid history bloat.

illwieckz commented 9 years ago

As this stands currently its very hard to see what's new and what's just moved code.

There is different commits for what is new, what is moved code, and what is spacing. Also, there is many standard git tools that allows every one to view diffs without spaces changes.

For example with commit https://github.com/illwieckz/qstat/commit/1d9d6c552cadb595eca46fdc4641c00ca9c3f7be that reindent qstat.cfg with about 3000 lines modified, the only not-spacing change is:

$ git show --ignore-all-space 1d9d6c552cadb595eca46fdc4641c00ca9c3f7be
commit 1d9d6c552cadb595eca46fdc4641c00ca9c3f7be
Author: Thomas Debesse <dev@illwieckz.net>
Date:   Sun Jul 5 08:30:31 2015 +0200

    reindent qstat.h, ref #14

diff --git a/qstat.h b/qstat.h
index 74da23d..4cf303b 100644
--- a/qstat.h
+++ b/qstat.h
@@ -877,7 +877,8 @@ char terraria_serverstatus[] = "GET /v2/server/status HTTP/1.1\x0d\x0a\x0d\x0a";
 /* SERVER BUILTIN TYPES */

-server_type builtin_types[] = {
+server_type builtin_types[] =
+{
        {
                /* QUAKE */
                Q_SERVER,

And in fact it's spacing change too, this is just the diff does not count new lines as space.

illwieckz commented 9 years ago

Well, this PR was just a

As a general rule each PR should be an individual commit

It's not true, a PR must not have only one commit, it must be coherent, if there is too much changes, they must be dispatched in different atomic commits to avoid jumbo commits.

This kind of commit is not acceptable: https://github.com/multiplay/qstat/commit/e777674c9d287af6ad1cd470237567e9b50f3652 it's from you, it's called "Refactored output splitting out xform processing into its own files improving various portions of it" but it's not only refactor output, it adds support for 3 games, changes packet handling and bump version. All these changes should have been splitted in different commits…

This PR is like the tee server PR, I've just wanted to fix some things that were wrong, but in the end I have rewritten all the whole file.

About this PR, All I wanted was to merge code from another guy, code I did not write myself.

You asked to rewrite legacy qstat code I never wrote and which is in VCS since more than 10 years or since the beginning.

You asked for reformatting code I never wrote, (like qstat.h code).

You asked to split the json display code, but the better way to achieve this is to display the whole display code, since it's the best way to do it properly with kinds .h files to not declare the same things multiple time.

formatting code unrelated to the PR is a big no no.

You asked to split code and to reformat code. The split can't be made cleanly without reformatting. The split can't be made cleanly without changing files unrelated to the PR. The split can't be made cleanly without reformatting - bis.

illwieckz commented 9 years ago

Fixes made due to PR feedback should be squashed to avoid history bloat.

Well, each commit have been squashed 20 time.

The only commits I never squashed are those ones that are not mine, because I can't legally do that.

All the commits are atomic (this is why this PR have multiple commits) except those ones that are not mine, because I can't legally change them.

All commits build, except those ones that are not mine, because I can't legally change them.

illwieckz commented 9 years ago

Well, each time I make a PR you ask for things that need to reformat and rewrite all, including legacy qstat code, and after that you said « it changes too much things, it's a big no-no ». What is the problem?

If you need 1 commit per PR, just merge the PR as-is when it comes with only one commit inside, do not ask to unrelated change that must not be squashed, and submit an issue to ask another PR for the other commits.

But I think you do not want 19 PR for 19 commits.

illwieckz commented 9 years ago

@eXprojects: do you allow me to squash your two commits named "Added json support" and "fixed exit code" ?

stevenh commented 9 years ago

As a general rule each PR should be an individual commit

It's not true, a PR must not have only one commit, it must be coherent, if there is too much changes, they must be dispatched in different atomic commits to avoid jumbo commits.

Each project / team has their own rules, this is one of ours, if a change is so big it would need multiple commits, its likely the change is too complex and should be split anyway. As I said its a general rule, not hard and fast.

The key is not to muddy the water with fixes required to allow a change to get committed. So if additional changes / corrections are made due to feedback then these should be squashed. I'm happy for the third party commits to remain as is.

This kind of commit is not acceptable: e777674 it's from you,...

What's in the past is just that.

You asked for reformatting code I never wrote, (like qstat.h code).

I asked that for the code you wanted to add, not reformat and split everything, sorry if that wasn't clear.

Its important moving forward that the changes are clear as this makes them much easier to review and test. In its current form splitting out all the raw methods etc makes it very hard to see what the actual functional changes are.

Hope that clarifies.

P.S. Keep up the good work :)

illwieckz commented 9 years ago

I asked that for the code you wanted to add, not reformat and split everything, sorry if that wasn't clear.

The point is the code I wanted to add was just copy pasting from legacy code, so changing this code is changing the legacy qstat too.

The best way to fix code you pointed out would be to refactor it to be able to fix it only once.

Since the refactoring is a too heavy change for this PR, I choose to fix code you pointed out and to fix other occurrence of this code to keep the code base coherent and to help a future refactoring to be doable.

About the raw and xml split, splitting the JSON code without splitting other display code is just the best way to fall in multiple #include hell, and it's the best way to merge ugly hacks and get an unmaintainable and incoherent code. The split should have been asked as another PR from the beginning.

I've just did what you asked, but the cleaner way, and the cleaner way needs more rewrite than the fast way.

If you do not like the cleaner way and prefer the fast-and-dirty way, why you do not merge the PR as-is and as fast as it comes, since you say the cleaning must be done in another PR ?

It seems you just do not measure the exorbitancy work you ask, and you do not notice that fixes you ask are mainly about legacy code. So, it's normal if the fixes asked just rewrite all the legacy code.

illwieckz commented 9 years ago

In its current form splitting out all the raw methods etc makes it very hard to see what the actual functional changes are.

The splitting just split the files, it does not change the functional code (except some #include and Makefile.am lines of course).

This is why, for example, I've not refactored both json_escape and xml_escape yet since refactoring is changing functionnal code. So, when I fix display_json.c because you ask it, I fix the exact same code in display_xml.c too to keep the code coherent. If I don't do that, who knows what is the good code and what is the dirty code if we keep both the good and the dirty code?

Also, if you ask to fix a spacing in one line of a 4000 line file, I will not just fix this line to be incoherent with the 3999 other lines, fixing a spacing in one line of a 4000 line file can't be done cleanly without fixing the other 3999 of the same file.

So, in end, the only way to do cleanly what you ask is to do it * 4000.

illwieckz commented 9 years ago

Another example, fixing cleanly the --json-protocols is not to rename this option to -jsonprotocols, but to allow to do -json -protocols the same way we already do -json -q3m, so, the clean way to fix one line is to change other lines.

Almost all the comments you made do not take into account the existing code. So, you do not expect the change you ask for.

So, you get what you ask for, but not what you expect.

illwieckz commented 9 years ago

I'm splitting the PR, but I will not do one PR per commit and potentially obligate me to rebase 20 branches if a potential change needs that.

illwieckz commented 9 years ago

So, you have now these PR:

I can't move #26 after #13 since it needs #23, #24, #25 to be achieved. I really needed all these splitting and cleaning effort to be able to write #26.

The json improvements you asked for needs these splitting and cleaning effort to be achieved first. They are just improvements to #13 which can be merged without #26 since it works without #26.

Rewriting one commit in on PR that another PR depends on will be very painfull (rebasing up to 5 PR).

illwieckz commented 9 years ago

Just for information, Github will not displays a clean N PR diff until you don't merge N-1 PR, it's normal, because Github does not know if a PR depends on another, so each PR recapitulates the diff from master.

illwieckz commented 9 years ago

Just one last thing about PR:

As a general rule each PR should be an individual commit Each project / team has their own rules

There is no PR in git, after a merge, a PR is nothing about a noisy additional commit in the worst case (like GitHub does), PR is just a social mechanism to say « hello, I have changes for you ».

A pull request is just the name GitHub gives to the process to request a pull from a foreign branch, some other online hosting platform call it a merge request for example, and in the traditional form, it's just a request for pull, like there is request for comments.

A pull request is nothing than requesting a pull from a foreign branch, git does not know what is a PR, and there is absolutely no rule in Git that said there must be only one commit per branch, because there is already two obvious rule: there is one commit per commit, and there is branches for multiples commits for changes that does not fit in one commit.

Git was not created to have only one branch per commit, which is insane to maintain, and a request for pull is about branch… If a PR were about commits and not branch, it were not called a pull request but a cherry-pick request…

Foreign commits are not pulled, they are cherry-picked, pull is for branches, branches is for multiple commit about the same effort, and jumbo commits that made too much things must be splitted, but branch with multiple commits designed for a unique purpose must not be splitted.

illwieckz commented 9 years ago

Just to help you:

stevenh commented 9 years ago

Making progress, but the issues highlighted by the review need to be fixed in this PR and not in separate PR's to ensure the code base is in good state no matter which revision is used.

Its great to use new PR's for other issues like general style cleanup and display split which effect other areas of the code but each new PR needs to pass review on its own prior to commit.

Currently your PR's are all dependent on each other instead of being based off separate branches, which make them impossible to review :(

By altering the structure of how they are committed it should be possible to all but eliminate these interdependencies.

From what I can determine we should see the following PR's:

  1. Eliminate unused vars (commits from PR #25)
  2. Add json support (combine PR #13, PR #26, PR #27)
  3. Style cleanup
  4. Split existing display code out to separate files.

Keep up the good work :)

illwieckz commented 9 years ago
  1. Eliminate unused vars (commits from PR #25)
  2. Add json support (combine PR #13, PR #26, PR #27)
  3. Style cleanup
  4. Split existing display code out to separate files.

I can't do that, it means redoing all the work from scratch, with risk to introduce new issues.

The work was done in the order I use because it was the safest way to do this job.

I can only do that:

  1. Add json support (+ fix utf8 etc. from PR #26), it means backports all the change by hand from #26 then rebase the next branch by hand (git will be able to rebase the others automatically but not this one).
  2. Split the display code.
  3. Begin to clean style - depend of the split
  4. Unset unused variables - depend of the cleaning
  5. Qstat help improvement PR #27 + args must exclude themselves from #26. This is not a JSON issue, it's just the current JSON effort spotted it, there was already a problem with xml and raw, it's better to keep this fix as another PR, it's not related to JSON processing, it's related to Qstat user interface.
      1. and 5. are not JSON-related, it's just some tasks motivated by JSON, they can be done in the order we want, they change nothing for qstat and for you. But it change many thing to me if we do not do that in the order above, because it means to me redoing all from scratch.
illwieckz commented 9 years ago

Ok, this was was painful but it's done, it needed to manually backports changes from #26 to #13, #23 and #24. No branch was able to rebase automatically, but I was able to use merge/cherry-pick to recreate each branch without editing files (except for the backports from #26).

Merging the unused var PR #25 before the json pr #13 is undoable, it means backporting by hand all the changes to all other PR since merge will be broken, editing plenty of files by hand each time to just redoing the job in each history. It's the better way to introduce new issues and it is very unsafe and painful. The unused var issue is in the code since years, it's not a problem at all if it's fixed after the json support addition.

Fixing unused var before or after introducing json support is the same to you (json does not depend on it): it was not fixed, it will be fixed, it's ok, it's just another PR unrelated that can be merged right after others PR unrelated.

But fixing unused var before or after introducing json support is not the same to me, it means editing manually each branch for each other PR. It's unsafe for no gain.

This is now the PR list:

13 Add json support

23 Split the display code

24 Begin to clean style, spacing, indentation (cleaning display code + qstat.cfg)

25 Unset unused variables

27 Qstat help improvement, also fix the user interface (some options must exclude themselves).

PR #26 is now unneeded since all commits were backported to #13 or merged to #27.

illwieckz commented 9 years ago

I've done some extra work with uncrustify to clean the source and I get very good results, so the PR #24 will just be pure noise in the history since I will submit a better one soon.

So, I closed the PR #24 and rebuilded the PR #25 and #27 to be able to merge them without it.

This is now the PR list:

13 Add json support

23 Split the display code

25 Unset unused variables

27 Qstat help improvement, also fix the user interface (some options must exclude themselves) and some query style.

z commented 8 years ago

What's the status on this?

illwieckz commented 8 years ago

Hello @stevenh, I just reedited all my PR to include the iterator limit to match malloc in protocol listing, I reminds you the merge order:

illwieckz commented 8 years ago

Hi @stevenh , is this PR ok for you now? I hope it will be merged soon.

stevenh commented 8 years ago

Still looks like a number issues already raised, haven't been addressed in this PR.

A secondary PR to fix them is not an option as this means we'll have bad code in the repo at the point this is merged, so I'm afraid I'm going to have to insist all issues are addressed before we can get this merged.

illwieckz commented 8 years ago

Hi,

Still looks like a number issues already raised, haven't been addressed in this PR.

Please list them, "looks like" is not an argument.

illwieckz commented 8 years ago

Hi, I fixed in this PR the temporary double hyphen --json options wrote by eXprojetcs that are deleted by myself in PR #27 dedicated to option/arg handling fixes which is blocked by the current PR.

illwieckz commented 8 years ago

Hi @stevenh , is there still something preventing the merge ?

stevenh commented 7 years ago

merged via #53