Minor and non-Minor PR merging

[mumbel]

This really really comes from the best intentions, and understandably if the answer is we cannot do much different... but I think worth starting a discussion.

I really do(try to) understand the issues with pulling in data into your space (generic ISSO concerns, data migration, which way the data goes, constrained by data movement, CDROMs((lol) vs USB, when/if air-gapped) and just generally man hours to do this. Then you have to consider the developer time for actual reviews for pull requests, issues, and managing time/effort/merging the pull after the fact and weigh that against your team's goals/issues/constraints.

There are several "1-liner" PRs that could add significant QoL results for users, some have been open for months to a year plus. Is there anything that can be done to help these efforts on 'our' side of things... more PR comments, more PR reviews, more "approvals", and to a lesser degree more at'ing of devs to grab attention?

This does get a bit frustrating when it appears to be very simple fixes getting ignored and overlooked (from 'our' POV, just due to limited contact/communication), either just missing the issue and/or PR by the relevant dev or just not getting the time allotted to manage getting it pulled into your branch(es).

[ryanmkurtz]

Thank you for considering some of the issues we face when interacting with the GitHub community, and some of the extra technical hurdles we have to clear for even the simplest looking PR. While it's not nearly as bad as some of the things you mentioned (like CD-ROMS), we can't just click the big green Merge button on a PR because it would screw up our internal "primary" repo. GitHub is just a mirror of that that we push to, typically after we see the nightly tests passing (we try not to push out severely broken commits to GitHub). So, while we do have some annoyances relating to this, it's not a good excuse for why a good PR would slip through the cracks.

I could go more into some of the other things that slow us down, and why seemingly simple PR's with big impact might fall through the cracks, but instead I will just say that we are aware of the issue, and we are actively working on ways to improve (i.e., we are currently planning new ticket manage workflows).

In the meantime, it would be helpful if you could list out some of the PR's you are talking about in this discussion. It is likely that some of them have just fallen through the cracks, but it is possible that we haven't accepted others for a good reason and just did a poor job of communicating that back to the contributor. Either way, it could be valuable to this discussion to have some examples to refer to.

[astrelsky]

No 3.5 inch floppy disks?

[mumbel]

@ryanmkurtz I went through each PR and looked for small diffs, presumably straight forward fixes/changes or not, but easy to tell due to size that could "quickly" get addressed. Then a few more looked small but stuck out for whatever reason so I set them aside, and then a couple larger ones, but they're self isolated and should be easy to review. Also maybe 2 that could be closed (1903, 1909)

I guess its hard to put value on each one or things I was specifically thinking of to prioritize. And yes you're absolutely right that some of the burden is on the contributor to keep up with it, but others just received little to no attention/communication. I have a few in this list and some that touch my contributed code, so obviously I think about them more, but not saying I'd rank any higher. Is this kind of what you were suggesting or is this too much of a fire hose

Small diff
1437 +1/-0 adds to skel
1574 +5/-1 fixes iso9660 npe
1640 +4/-4 read from correct space
1667 +1/-0 documentation fix
1835 +62/-0 fix if preproc handling (+3/-0, rest is is test file)
1844 +2/-2 pe fix
1845 +2/-0 transaction handling
1905 +25/-25 handle int values instead of short in NE
1928 +6/-3 handle assignment in pragma C parser
1942 +3/-3 print RIP/EIP instead of immediate
2041 +6/-1 allow for comparable clangline
2089 +33/-0 adds some octeon to mips
2156 +25/-0 adds script helper to get data type manager
2203 +5/-0 bugfix missing switch case
2221 +8/-2 dyld importing
2244 +4/-0 ignore vscode
2277 +12/-4 z80 bugfix
2364 +3/-1 better syscall naming (slightly out of date)
2494 +1/-1 fixes elf DT_PREINIT_ARRAY
2511 +3/-0 export C func defs
2590 +4/-4 fix default riscv size in cspec
2633 +3/-0 __cdecl16far DX/AX join
2662 +1/-1 null ptr usage in createfunctioncmd
2695 +4/-6 flow override, potetnially significant rammifications, but small enough to at least comment on
2969 +12/-0 decompiler xml race
3026 +17/-1 get_pc_thunk.di
3069 +6/-0 equate symbol fix
3094 +1/-1 unicode string mgmt
3106 +7/-16 pic17 analyzer
3109 +0/-3 remove invalid pdb servers
3156 +2/-1 fix mac locale
3157 +6/-0 elf image base address option
3211 +2/-0 AARCH64 IDA-PRO external_name
3300 +1/-1 slgh compile bugfix
3342 +9/-0 pic18 postinc/dec fix
3349 +1/-1 getfileattributes
3408 +1/-1 arm setstackmode
3418 +4/-4 arm vcvt rounding
3481 +1/-1 filteredmemorystate
3534 +162/-1 correct export file validation (+1/-1 + tests)
3546 +18/-0 aarch64 elf reloc
3630 +20/-19 msvc byte/std::byte (+1/-0 w/o WS)
3734 +15/-4 sleigh space size check
3760 +1/-1 pdb range match
3761 +1/-1 length check
3777 +2/-2 display counter of by 1
3785 +1/-0 count inc in debug decompiler output
3903 +26/-0 getOriginalBytes
3982 +1/-1 messagedigest.isequal (needs fixing)
4003 +0/-0 (file rename) riscv patterns filename
4019 +1/-1 add whitespace around is token
4095 +1/-1 .bss dont need file offset validation
4100 +1/-1 vt string equals
4107 +9/-1 last modified time
4161 +1/-1 NPE check
4181 +5/-0 noreturn in wdk
4192 +1/-0 x86 pattern
4193 +4/-4 mips jump pattern
4196 +1/-1 comspec
4211 +2/-2 url documentation
4213 +2/-17 selectFieldNumber fix

Slightly larger or stuck out as more involved
3953 +8/-0 propagate bool
3262 +38/-50 sleigh allow for more operands
2290 +51/-47 objc struct generation
2932 +50/-0 dylib symtab parsing
3001 +57/-16 respect array size for string printing
3816 +50/-29 riscv reloc
4001 +50/-10 codeblockreferenceiterator

Larger but self isolated
3634 +208/-0 mips 32 eabi
1449 +441/-3 adds pattern and ELF to tricore

[ryanmkurtz]

Thanks for going through all of these. They (and really all) PRs should be revisited and either closed, accepted, or commented on in some way or another.

I've personally looked at some of these and can comment on why I haven't accepted them yet:

• 1844: I'm scared of what it would break. I've already tweaked this code to work with some other sketchy PE cases and I'd have to dig those samples up to make sure they still work. It's a tiny change, but it requires a lot of testing time. Since no one else was having issues with it, it seemed safer to leave it be for now. Ultimately the burden of testing is on us.
• 2244: The solution is incomplete, and we don't officially support VSCode, but I wanted to accept it. However, we decided that it's annoying to keep having to add these exceptions one by one so I started to go down a more generic path. That technique eventually didn't pan out, and by that time, the PR was already kind of old and I moved on to some other things. This is an example of one we should really just not support/close.
• 3156: No context provided. We should ask for more info.
• 3777: It's probably a good fix, but its impact is so low that it's hard to prioritize accepting it over working on other things.
• 2932: I'm in a Mach-O mode now, so this is one I plan on incorporating in one way or another (I don't believe the implantation is optimal design).

I could go on, but I think the take away here is we should give back more insight into the comments of the PR to provide better transparency. While some PRs have certainly fallen through the cracks and probably haven't been properly evaluated yet, others have been considered, but that has not been reflected in the comments or labels of the ticket.

[mumbel]

@ryanmkurtz thanks for the time and taking a look (didn't mean to make weekend work for you, did almost wait to post until Monday, so please ignore til you're on the clock). Yeah, I should have read through a little bit more on each to see how much they could break or not backwards compatible and should have left out more or set more tiny ones aside in that 2nd category. Some were just blatantly bugs (does that become a "is this a a feature now?" or "what fallout does this open up?" conversation too often) or could be merged, and maybe my lists should have broken those out. I did leave a few tiny ones out of this where it was obviously not great. And yeah, priorities do come into play so not fair for me to just say that the way I did (that could "quickly" get addressed).

Not to be super biased, but issues like 2590 (12/20) and 4003 (I did get a laugh when I saw my mistake) stick out to me, extremely easy fixes and significant RE value add. Ones like these, while I understand the time/effort that is taken away from current work, seem insignificant outside looking in. We have no way of weighing impact to your customers, but certainly if its worth reporting the issue and a PR, its not insignificant to someone... and I understand why we don't know/aren't told. One question I guess that I'm thinking of now, do you prioritize bugs like these differently or any others when the fix can be applied to the unzipped release (these 2 could simply be fixed by hand w/o any sort of outside tools, though I guess a typical user wouldn't know to).

Generic comments from nsa are very helpful, even if it is comments like "We cannot support this due testing/env/lic/etc..." or "thank you, but we cannot prioritize this, but it is now a known issue to pull". Just acknowledgment in general I suppose whether that be in favor or against or neutral (as in 3156, until more info is provided, maybe who knows). Labels do address that concern to a degree and were super helpful when you started using those.

(also some PRs are super recent, and I just included small ones for completeness, not really saying they belong here though for my initial concern)

[ryanmkurtz]

It looks like 2590 probably fell through the cracks. It's hard to say with 4002...it is somewhat new. We tend to operate more on a release timeline, so I would be more concerned about the PR if we did a new release and it wasn't included. I wouldn't be afraid to bump a ticket though if you suspect it was forgotten...at the very least it shows people care about it and at least for me the new notification is very helpful. I'm hoping our new ticket workflows that we will soon be implementing will make most of this unnecessary tho.

One question I guess that I'm thinking of now, do you prioritize bugs like these differently or any others when the fix can be applied to the unzipped release

Not in terms of accepting PRs. The only time that might come into play is determining how quickly we need to build and release an official patch release. If the bug is critical but there is a temporary workaround (like hand editing a sleigh file), the new release can probably wait until Monday vs working the weekend...that sort of thing.