Hi Andreas,

Thanks for looking into this.

On Fri, 8 Dec 2023 at 21:41, Andreas Hasenack
<email address hidden> wrote:
>
> 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

Yes, Utkarsh asked me to add the dep3 headers, and that bug seems to
the reason of the patch but tbh, I am not entirely sure as there is no
other information in the patch or changelog.

>
> 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.

Yes, that's what I did initially. But if you see
https://github.com/canonical/ubuntu-maintainers-handbook/blob/main/PackageMerging.md#split-commits
the step mentioned splitting the old changes in logincal changes. And
https://github.com/canonical/ubuntu-maintainers-handbook/blob/main/PackageMerging.md#purpose-of-logical-tag
explains why the logincal change has been done. To quote that "The
logical tag is the cleanest possible representation of a previous
Ubuntu delta".  And, so when I rebase those logical changes on top of
new/debian those commits will be added. So, unless I modify the commit
while adding the logical changes, I think there is no other way to
drop the patch. And modifying while making the logical change means I
am not splitting the old delta properly. But please correct me if I am
wrong.

>
> 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.

I tried to look into this over the weekend but I could not figure out
what exactly is causing the monitor test to fail. I think I will try
to bisect Debian and find out what has fixed the monitor test in
Debian.


-- 
Regards
Sudip