Code review comment for ~sudipmuk/ubuntu/+source/zeitgeist:merge-lp2044300-noble

Tags ok, thanks for pushing those!

First pass:

$ git range-diff sudipmuk/old/debian..sudipmuk/logical/1.0.3-4ubuntu1 sudipmuk/new/debian..merge-lp2044300-noble
 1: f024545 = 1: b4d8957 d/p/add_datahub_autostart_delay.patch: Delay datahub autostart
 2: 77a362f = 2: 4b3347e d/p/nodisplay_autostart.patch: Don't show in Startup Applications
 3: a30b8dd ! 3: ca050ff d/p/pre_populator.patch: Pre-populate the log with some events
    @@ debian/patches/pre_populator.patch (new)
     +Description: Pre-populate the log with some events so that the dash isn't empty on first run.
     +
     +=== modified file 'src/Makefile.am'
    ++Bug-Ubuntu: https://launchpad.net/bugs/962265
    ++Forwarded: not-needed
     +---
     + datahub/Makefile.am | 1
     + datahub/pre-populator.vala | 117 +++++++++++++++++++++++++++++++++++++++++
 -: ------- > 4: 209e96a Drop d/p/add_datahub_autostart_delay.patch: no longer required
 -: ------- > 5: 66af12c Modify skip-failing-tests.patch: skip the test that still fails in Ubuntu as in the previous version
 -: ------- > 6: 23099ff merge-changelogs
 -: ------- > 7: 1fcd0ed reconstruct-changelog
 -: ------- > 8: c71981c update-maintainer
 -: ------- > 9: f2c0fec changelog

a) I see you added a bit of dep3 headers to d/p/pre_populator.patch

b) 209e96a (drop add_datahub_autostart_delay) is almost a revert of b4d8957 (add datahub_autostart_delay), but had to also touch nodisplay_autostart.patch because of context. In general, when dropping patches, you can just not apply it when rebasing on top of new/debian. No need to apply and then revert. Then in the range-diff output we would clearly see that one commit was dropped, and another one was added to nodisplay_autostart.patch to adjust for the changed context.

c) 66af12c Modify skip-failing-tests.patch: skip the test that still fails in Ubuntu as in the previous version

That is an "added change". It's delta we didn't have before, and that you are adding now. We like to call out to these in the changelog with a "Added changes" entry, like below:

  * Merge with Debian unstable. Remaining changes: (LP: #2044300)
    - Add nodisplay_autostart.patch: Don't show in Startup Applications
    - Add pre_populator.patch: Pre-populate the log with some events
      so that the Unity dash isn't empty on first run.
  * Added changes:
    - Modify skip-failing-tests.patch: skip the test that still fails
      in Ubuntu as in the previous version.
  * Dropped changes:
    - add_datahub_autostart_delay.patch, no longer required.

In the next merge, the "added changes" can be part of the "remaining changes", if it still makes sense to keep.

That being said, are you saying that in ubuntu we need to skip more tests than in debian? What is the reason again, different package versions? We like to know why, so that it's easier to re-evaluate the patch in the future, and see if we can stop disabling tests.