Code review comment for ~paelzer/ubuntu/+source/chrony:merge-cosmic-3.3-1

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

> Looks good!
>
> One question from an upload perspective:
>
> 2d3ebbe: is a policy-rc.d interaction required? I'm not sure so I'll defer to
> you or someone like xnox.

Interesting - This is no new change, I can kick a discussion with xnox separate to this but would not block the upload here for now. If it turns out we need a modification later on that can be done individually.

To kick that off properly could you elaborate a few sentences on the policy-rc.d interaction you'd have in mind outside of this MP e.g. in a mail to me?

> Minor typo, while you're fixing typos in README.container described inline.

Yeah while changing it anyway let change that as well. Commit added.

> From a workflow perspective:
>
> 9eec4ae->43b8bf9 seems to be transferred correctly but I can't find how it is
> or ever has been documented in debian/changelog. Should it perhaps now be
> squashed into cb49079? If yes, then I wouldn't expect it to be documented
> separately in the changelog for this upload (since it was part of the logical
> delta previously), but I would expect it to have been documented when the
> change was made in the past, unless it was done at the same time as the
> introduction of the original change to d/links.

It was done at the same time when introducing the d/links for this at all.
So I had a commit adding and a commit improving this since the other package changed
But it was released just with the combined content - therefore no "fixup" changelog entry.
The combined changelog entry was a subsection of the networkd-dispatcher:
       - d/links: link dispatcher script to networkd-dispatcher events routable
         and off
I missed the squashing when making the Delta logical.
I did that now (force push) to next time have it combined the right way in the old history.

> Commit message for d4ebf92 appears inaccurate now, but the changelog message
> is correct. The upload is correct, but from a workflow POV it should be fixed
> by rebasing this branch to avoid carrying the inaccuracy forwards.

Ack, the update I did to changelog is now integrated in the commit message.

> IMHO, instead of:
>
> Forwarded: no (backport)
> Origin: upstream, ...
>
> The correct form in dep3 is:
>
> Origin: backport, ...
>
> "The field is really required only if the patch is vendor specific..." so I
> don't think a "Forwarded" entry is needed at all in the case of a backport.

Yes, my dep3 templates are ever improving - I have 3 now and adapted my template as I agree.
I also modified the commit to match.
I'm not going to rewrite all my old patches I've done last year, but from now on I make a difference between backport from upstream git and backport from upstream discussion (which was how the forwarded came in - to carry a link or such).

« Back to merge proposal