Applying merge diffs leads to klocalizer AssertionError

[lolrepeatlol]

Description

• klocalizer will run into an AssertionError if passed a single merge commit.
• This AssertionError, as is, appears as a crash that is difficult for users with no knowledge of the existing codebase to understand. It provides no information immediately useful to the user about why the crash occurs.
• This issue occurs because merge commits tend to not include changes from other commits in git show, therefore leaving klocalizer with an essentially empty patch.
• Worthy to note: I initially believed this issue would manifest with commits that also only had non-C file changes, but interestingly, a different issue occurs in those situations, which I've filed separately in Enhancement: klocalizer should provide clearer error when patch has no C file changes #283.

Steps to reproduce

1. Clone the Linux kernel with git clone git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git and enter the directory.
2. Get a patch from the Linux kernel using git show {commit} > patch.diff. I initially encountered this issue with the commit 5635f189425e328097714c38341944fc40731f3d.
3. Check out the source code at the commit and create a kernel configuration file with a command like make defconfig.
4. Run klocalizer with klocalizer --repair .config -a x86_64 --include-mutex patch.diff.

What I expected to happen

1. I expected klocalizer to repair the kernel configuration file.
   (What should happen: klocalizer should tell me why it's unable to apply the patch and exit without crashing.)

What actually happened

1. klocalizer runs into an AssertionError:

(venv) alexei@turing:~/LinuxKernels/kmax_examination/linux$ klocalizer --repair .config -a x86_64 --include-mutex patch.diff --verbose
klocalizer, kmax 4.8
INFO: Diff file was given as input ("patch.diff"): assuming it was applied to the Linux source.
DEBUG: Computing the target lines from the patch file "patch.diff"
Traceback (most recent call last):
  File "/home/alexei/IDEProjects/PyCharmProjects/kmax/venv/bin/klocalizer", line 7, in <module>
    exec(compile(f.read(), __file__, 'exec'))
  File "/home/alexei/IDEProjects/PyCharmProjects/kmax/kmax/klocalizer", line 1740, in <module>
    klocalizerCLI()
  File "/home/alexei/IDEProjects/PyCharmProjects/kmax/kmax/klocalizer", line 529, in klocalizerCLI
    include_mutex_list = parse_units_args(include_mutex_arg)
  File "/home/alexei/IDEProjects/PyCharmProjects/kmax/kmax/klocalizer", line 513, in parse_units_args
    parsed_list.extend(list(get_patch_target_lines(a).items()))
  File "/home/alexei/IDEProjects/PyCharmProjects/kmax/kmax/klocalizer", line 376, in get_patch_target_lines
    r = patch.get_target_c_lines(patch_txt)
  File "/home/alexei/IDEProjects/PyCharmProjects/kmax/kmax/patch.py", line 250, in get_target_c_lines
    r = get_target_lines(patch_txt)
  File "/home/alexei/IDEProjects/PyCharmProjects/kmax/kmax/patch.py", line 207, in get_target_lines
    assert patch_summary
AssertionError
(venv) alexei@turing:~/LinuxKernels/kmax_examination/linux$

Fixing this issue

• To fix this issue, I propose that we:
  • Change the suggested usage for one commit from git show {commit} to git diff {commit}^ {commit}, which already aligns with what git diff does to large sets of commits and would make using merge commits with klocalizer significantly more useful.
  • Add logic to gracefully exit klocalizer when a patchfile with no file changes is provided.

[paulgazz]

Thanks @lolrepeatlol . Indeed we assumed no merge commits (and commits that had C content), which is probably why this wasn't identified before. Change to usage makes sense. Would there be a way to more gracefully fail rather than an assert?

[lolrepeatlol]

Thanks @lolrepeatlol . Indeed we assumed no merge commits (and commits that had C content), which is probably why this wasn't identified before. Change to usage makes sense. Would there be a way to more gracefully fail rather than an assert?

No problem @paulgazz -- I was thinking we could check if the processed diff was empty earlier on and print an error if so; this way, we can keep the assertion. I made a change to patch.py in summarize_patch() like this, which seems to work:

  # get first item from diff
  try:
    first_item = next(diff)
  except StopIteration:
    # stop if first item is empty
    print("ERROR: Patch file provided has no file changes. Exiting.")
    exit(1)

  diff_summaries = []
  diff = [first_item] + list(diff)  # place first_item back in the front

  for d in diff:

Here, we have patch.py check if first_item exists, and if not, we exit. Then, we turn diff into a list and set first_item as the front of the list.

• This works, but is kind of a workaround since diff is a generator, and here, we convert the generator into a list in order to get the first item. To my understanding, we could import itertools and then add first_item to the front by using diff = itertools.chain([first_item], diff) instead, assuming we wanted to keep it as a generator.
  • (Or, theoretically we could just remove the assertion in get_target_lines() and add a print statement + exit if the patch_summary list is empty, which would probably be simpler).
• patch.py has no print statements or loggers as it is currently written, so this wouldn't be consistent with the rest of its code. We could look into building on this by potentially returning an error code and then printing the error within klocalizer if you'd like.

That said, I'll take a bigger look at this next week, as I don't think I have the time to get around to a PR yet.