update-manager crashed with UnboundLocalError in show_diff(): local variable 'line_number' referenced before assignment

Status changed to 'Confirmed' because the bug affects multiple users.

This is actually a bug in aptdaemon.

There are two points in show_diff from aptdaemon/gtk3widgets.py where line_number is not defined:

        for line in difflib.unified_diff(from_lines, to_lines, lineterm=""):
            if line.startswith("@@"):
                match = re.match(self.REGEX_RANGE, line)
                if not match:
                    continue
                line_number = int(match.group("from_start"))
                if line_number > 1:
                    insert_tagged_text(iter, self.ELLIPSIS, "default")
            elif line.startswith("---") or line.startswith("+++"):
                continue
            elif line.startswith(" "):
                line_number += 1
                insert_tagged_text(iter, str(line_number), "number")
                insert_tagged_text(iter, line, "default")
            elif line.startswith("-"):
                line_number += 1
                insert_tagged_text(iter, str(line_number), "number")
                insert_tagged_text(iter, line, "remove")

This seems to be happening to quite a few Precise users as we can see at errors.ubuntu.com:

http://10.0.3.97/bucket/?id=%2Fusr%2Fbin%2Fupdate-manager%3AUnboundLocalError%3A_on_config_file_conflict%3Arun%3Ashow_diff

I suspect that it shouldn't enter those last two blocks unless line_number has been set properly yet, since diff hunks always start with @@.

The odd part is that I'm not sure how to get difflib to produce a diff that
doesn't start with the three lines:

--- blah
+++ blah
@@ blah

to trigger the bug. This is clearly unexpected in the code (hence the
UnboundLocalError in the first place) and it it definitely not covered in the
test suite. Hand crafting a call such as this does not trigger the bug:

$ PYTHONPATH=. python3
>>> from aptdaemon.gtk3widgets import DiffView
>>> dv = DiffView()
>>> dv.show_diff('/etc/passwd', '/etc/group')

Without a test, a fix is mostly just conjecture. I think the best thing to do is to set `line_number = 0` outside the loop. This might give you nonsense but it'll avoid a crash. I thought about setting it to None outside the loop, but then the += will still crash, just with a different one (perhaps we should force that and output some debugging to better see what's happening, but otoh is it worth it?)

I will attach a branch momentarily.

So, this is the loop http://bazaar.launchpad.net/~aptdaemon-developers/aptdaemon/main/view/head:/aptdaemon/gtk3widgets.py#L1114

        for line in difflib.unified_diff(from_lines, to_lines, lineterm=""):
            if line.startswith("@@"):
                match = re.match(self.REGEX_RANGE, line)
                if not match:
                    continue
                line_number = int(match.group("from_start"))
                if line_number > 1:
                    insert_tagged_text(iter, self.ELLIPSIS, "default")
            elif line.startswith("---") or line.startswith("+++"):
                continue

Note the 'if not match: continue' after the REGEX_RANGE lookup - and REGEX_RANGE is:
    REGEX_RANGE = "^@@ \-(?P<from_start>[0-9]+),(?P<from_context>[0-9]+) " \
                  "\+(?P<to_start>[0-9]+),(?P<to_context>[0-9]+) @@"

Now difflib.py has this for its output:
        first, last = group[0], group[-1]
        file1_range = _format_range_unified(first[1], last[2])
        file2_range = _format_range_unified(first[3], last[4])
        yield '@@ -{} +{} @@{}'.format(file1_range, file2_range, lineterm)

And if you look at _format_range_unified it has this:
    # Per the diff spec at http://www.unix.org/single_unix_specification/
    beginning = start + 1 # lines start numbering with one
    length = stop - start
    if length == 1:
        return '{}'.format(beginning)
    if not length:
        beginning -= 1 # empty ranges begin at line just before the range
    return '{},{}'.format(beginning, length)

So if the diff range is precisely 1 line long, the output will be
@@ -12 +12,13 @@
But the regex aptdaemon has expects x,y never just x.

So - bad regex, fix that and you should be good. May need to tweak the assignment code of course.

-----a.txt-----
one
-----a.txt-----

-----b.txt-----
onee
-----b.txt-----

diff those two files and you'll hit the bug. I'll add a test case.

This bug was fixed in the package aptdaemon - 1.0-0ubuntu1

---------------
aptdaemon (1.0-0ubuntu1) raring; urgency=low

  * New stable bug fix release 1.0:
    - Add PEP8 compliance and a corresponding test
    - Fix PackageKit tests if an http proxy is set (LP: #1050799)
    - Fix crash for Python GTK2 clients (LP: #1108552)
    - Fix crash if configuration file changes are only one line long -
      thanks to Barry Warsaw and Robert Collins (LP: #1120322)
    - Fix crash if the removal of a package valid candidate is simulated
    - Allow repository release names to include dots
    - Convert aptdaemon exceptions to PackageKit ones in the compat
      layer (currently only invalid input ant failed authorization are
      supported)
    - Store the default values in the PackageKit transaction interface
      statically typed
    - Fix setting the speed property in the PackageKit interface
    - Several grammar and typo fixes in user interface messages and
      documentation (LP: #1112492)
  * debian/patches:
    - fix-high-trust-pkcompat.diff: updated
    - apt-dbus-text-api.patch: updated
    - fix-gobject-io-add-watch.patch: Added. Always include the priority
      in a GLib.io_add_watch() call to not cause problems with the backwards
      compatibility

  [ Barry Warsaw ]
  * debian/control:
    - Add Build-Depend on gir1.2-gnomedesktop-3.0, gir1.2-vte-2.90, pep8,
      and xvfb.
    - Bump Standards-Version to 3.9.4.
  * debian/rules: Run the tests under xvfb-run to avoid core dumps.
  * debian/patches/pep8-fixes.patch: Fix PEP 8 violations.
  * debian/patches/lp1153725-temporary-workaround.patch: Temporarily
    disable two tests which fail during the build.
  * debian/rules:
    - Add the missing test directory and symlinks before running tests.
    - Clean up symlink directory in override_dh_auto_clean.
    - Sleep before the xvfb-run with a different Python version. Tear
      down of xvfb-run resources is apparently not immediate.
 -- Sebastian Heinlein <email address hidden> Mon, 11 Mar 2013 07:02:07 +0100

Hello Fjatle, or anyone else affected,

Accepted aptdaemon into precise-proposed. The package will build now and be available at http://launchpad.net/ubuntu/+source/aptdaemon/0.43+bzr805-0ubuntu9 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

I test aptdaemon version 0.43+bzr805-0ubuntu9 from precise-proposed and confirm that DiffView's show_diff now works.

The verification of this Stable Release Update has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regresssions.

This bug was fixed in the package aptdaemon - 0.43+bzr805-0ubuntu9

---------------
aptdaemon (0.43+bzr805-0ubuntu9) precise-proposed; urgency=low

  * debian/patches/fix-lp-1120322.patch: Resolve crash if configuration file
    changes are only one line long (LP: #1120322)
 -- Brian Murray <email address hidden> Wed, 20 Mar 2013 12:51:06 -0700