lines-changed-only option does not work correctly

[turol]

lines-changed-only option does not work correctly and sometimes complains about lines that were not changed by the PR. For example this PR:
chocolate-doom/chocolate-doom#1480

Elicited comment "File opl/opl.c (lines 42, 56): Code does not conform to Custom style guidelines."
Line 42 was not changed. Line 56 wasn't either, but line 55 was.

https://github.com/chocolate-doom/chocolate-doom/runs/7307673916

Correct(ish) option appears to have been passed to clang-format: --lines=50:58 though the range should have been 53:55. Line 42 should definitely not have raised issues.

[2bndy5]

While I agree with you, I just want to point out that this might be clang-format's fault. Lets examine the complete object from the diff (starting at line 42):

 static opl_driver_t *drivers[] =
 {
 #if (defined(__i386__) || defined(__x86_64__)) && defined(HAVE_IOPERM)
     &opl_linux_driver,
 #endif
 #if defined(HAVE_LIBI386) || defined(HAVE_LIBAMD64)
     &opl_openbsd_driver,
 #endif
 #ifdef _WIN32
     &opl_win32_driver,
 #endif
+#ifndef DISABLE_SDL2MIXER
     &opl_sdl_driver,
+#endif // DISABLE_SDL2MIXER
     NULL
 };

So, the correct line numbers were passed to clang format, but it probably flagged line 42 as well because it was the start of the object definition.

Passing line ranges to clang-format doesn't necessarily mean it will only examine those lines. It has to examine the whole file and then filter out the warnings to only those related to the specified range.

---

I think we can have an extra check in the action src to ensure the lines mentioned are only in the diff. But this may be difficult if clang-format is nesting the warnings for diff'd lines within warnings for non-diff'd lines - its all parsed from clang-format's XML output.

[2bndy5]

Correct(ish) option appears to have been passed to clang-format: --lines=50:58 though the range should have been 53:55.

Actually, this is incorrect because the range is determined from the lines listed in the diff (which often includes unchanged lines at the beginning and end of the chunk). Your diff is in fact showing lines 50-58.

I am not going to change this because the feature is really more specific to examining the diff. The wording for the option lines-changed-only was chosen for simpler user understanding. What its really designed to do is more like diff-lines-only - its a bit late to change that option's name now though.

See the description for this option in the README: https://github.com/cpp-linter/cpp-linter-action#lines-changed-only

[turol]

Could you add a new option to restrict it more to only lines changed? The point was that we would want it to complain only about changed lines. The head maintainer didn't want to fix all the formatting at once.

[2bndy5]

TL:DR This isn't a simple request.

clang-format's --lines arg only accepts ranges.

 --lines=<string>           - <start line>:<end line> - format a range of
                              lines (both 1-based).
                              Multiple ranges can be formatted by specifying
                              several -lines arguments.

I think the intention of such an option should focus on additions only. Getting the line number for a added line in the diff is rather difficult because the diff doesn't actually have line number at the start of the lines in the diff. Meaning, line numbers have to be literally counted from the beginning of the diff, and subtracted lines in the diff will negatively offset the actual line number for added lines.

They literally write entire libraries just for parsing diffs, but none were intuitive enough to satisfy the needs for such an option (I already looked into it). This is why I settled for focusing on just what's shown in the diff.

[2bndy5]

BTW, that's a neat project. I haven't played doom since my win95 machine. Now, I just use the retroarch port when I'm feeling nostalgic. I'll admit that I'm much more interested in openMW (morrowind engine).

[2bndy5]

Assuming I can augment the feature to only pass added lines to clang-format, I think we can simply accepted a new value to the lines-changed-only option.

• If the option's value is a bool, then use diff entire chunks. <-- the current implementation
• If the option's value is "strict", then only use added lines. <-- the requested feature

Last time I tried this it got way too complex, so I'm going to try a much more brute force approach: Pass multiple ranges of a singular line (assuming I can get the correct line number for added lines in the diff).

[2bndy5]

It seems I already tried to filter the clang-format warnings.

cpp-linter-action/cpp_linter/thread_comments.py

Lines 131 to 136 in d24e622

	for scope in ranges:
	if fixed_line.line in range(scope[0], scope[1] + 1):
	in_range = True
	if not in_range:
	continue # line is out of scope for diff, so skip this fix

But this isn't actually used in production. It was meant to be a feature that actually puts the clang-format remedy inline with the diff. I abandoned it because we can't use multiple lines in the annotations (just 1 really long line).

[shenxianpeng]

The head maintainer didn't want to fix all the formatting at once.

My feeling is that using the clang-format command locally, it's easy to accidentally format the entire file.
If possible, perform a full format and then ignore this commit with git blame is a common practice, refer to llvm-project.

I personally want to use this method, but the Bitbucket used by my company has not followed up with the git blame option --ignore-rev. 😞

[2bndy5]

ok, I think I've got the strict additions feature implemented, but I need to test it on an actual CI event. I haven't yet added the extra filtering of warnings per only lines affected by commit.

So, far, the unit test looks promising...

[2bndy5]

I've extended the line filtering to the thread_comments feature (as well as the initially requested file_annotations feature). This will be solved when backlogged updates branch is merged into main.

BTW, @turol I used the commit from the chocolate-doom repo (referenced in this issue's OP) as a case for the new unit tests. I hope you don't mind.

[turol]

@2bndy5 I don't mind but the chocolate-doom code is GPLed so you might want to check with a lawyer if that causes a problem.

[2bndy5]

The code isn't actually being distributed with this repo, but the changed files (all 9 of them) from that commit are downloaded every time we run the unit tests. I'll check if this is suitable.

[2bndy5]

Looks like the license is focused on distribution, and anything related to copying the code seems to be subject to modifications.

1. We are not distributing the code as part of this repo. the tests do download some files from chocolate-doom, but they are explicitly gitignored. There will never be a copy of the files/code from chocolate-doom project in this repo.
2. We are not modifying the code, just analyzing it to make sure this issue is resolved.

In order to run the unit tests without hosting the code, I've copied the event_payload for the commit referenced in the OP of this thread. This event_payload.json does not contain any information that can be used maliciously. I have removed all info that isn't necessary to running the test (which doesn't require anything about users or user access). See 6a1f91d

If I've misinterpreted the GPL for chocolate-doom, then we may have to find a different sample to perform unit tests on.

[2bndy5]

After further consideration, I think the newer "strict" value should be the default (when lines-changed-only is 'true') and the old behavior should be used only if lines-changed-only is set to 'diff'. Given that the user intuition (in OP) assumed this would be the case, it might be seen as an improved default.